Which code is better

Question

1.00/5 (1 vote)

See more:

hi guys,

Can any one tell which code is better:

1.

C#

public void Ftpfunction(List<string> quoteNos, DataTable dt, string Templatename)
        {
            var row = GlobalVariables.dtglobal.Select("templatename='" + Templatename + "'");
            string path = row[0]["UPSLabelsPathuid"].ToString();            string username = row[0]["UPSLabelsPathuid"].ToString();
            string password = row[0]["UPSLabelsPathpwd"].ToString();

            var ftpfiles = FtpDownload.GetFiles(path, username, password);
            FtpDownload ftpClient = new FtpDownload(path, username, password);
            string desImage = string.Empty;
            foreach (var quote in quoteNos.Where(x => ftpfiles.Contains(x)))
            {
                desImage = System.AppDomain.CurrentDomain.BaseDirectory + @"\image\" + quote + ".jpg";
                if (!File.Exists(desImage))
                    ftpClient.download(quote + ".jpg", desImage);
            }
        }

2:

C#

public void Ftpfunction(List<string> quoteNos, DataTable dt, string Templatename)
        {
            string _sqlWhere = "templatename='" + Templatename + "'";
            DataTable _newDataTable = GlobalVariables.dtglobal.Select(_sqlWhere).CopyToDataTable();

            FtpDownload ftpClient = new FtpDownload(_newDataTable.Rows[0]["UPSLabelsPath"].ToString(), _newDataTable.Rows[0]["UPSLabelsPathuid"].ToString(), _newDataTable.Rows[0]["UPSLabelsPathpwd"].ToString());
            string desImage = string.Empty;

            var ftpfiles = FtpDownload.GetFiles(_newDataTable.Rows[0]["UPSLabelsPath"].ToString(), _newDataTable.Rows[0]["UPSLabelsPathuid"].ToString(), _newDataTable.Rows[0]["UPSLabelsPathpwd"].ToString());

            foreach (var quote in quoteNos.Where(x => ftpfiles.Contains(x)))
            {
                desImage = System.AppDomain.CurrentDomain.BaseDirectory + @"\image\" + quote + ".jpg";
                if (!File.Exists(desImage))
                    ftpClient.download(quote + ".jpg", desImage);
            }
        }

Posted 19-Feb-15 20:31pm

tanishtaman

Add a Solution

Comments

TheRealSteveJudge 20-Feb-15 2:40am

The real question is: Is any of these codes good?
On the other side it is more important that the code does what
it is supposed to do and that it is readable.
Your code is not readable.
e.g. what does "desImage" mean?
You should use self-explanatory names.
If you will have a look at your code in e.g. 1 year again
you will have no clue what it really does.

tanishtaman 20-Feb-15 3:13am

I m trying to make application centralized by removing repetitive code. But some one in our team spoils everything. In eg. 1 it is only a thing which i m trying to do but making a class and using that class every where. hope u understand.

Wendelius 20-Feb-15 2:41am

Better in which way?

tanishtaman 20-Feb-15 2:58am

in way or understanding and doing changes quicker. According to me, using code like "string _sqlWhere = "templatename='" + Templatename + "'";
DataTable _newDataTable = GlobalVariables.dtglobal.Select(_sqlWhere).CopyToDataTable();" and _newDataTable.Rows[0]["string"] in every part of the application is bad habit. Please do reply guys about your opinion.

2 solutions

Solution 2

As long as both do what is required "better" becomes largely subjective. However the first version is easier on the eye and easier to understand at first glance

Note though...
- the System on System.AppDomain is redundant
- declaration of variables is inconsistent var row... but string path (etc)
- desImage = string.Empty; is not actually required (the value is not used)
- desImage could be declared within the foreach loop
- As TheRealSteveJudge pointed out some of your variable naming could be improved - it is not clear what desImage would be for example.
- Formatting in version 2 would make it easier to read e.g. rather than one very long line introduce linefeeds

C#

var ftpfiles = FtpDownload.GetFiles(_newDataTable.Rows[0]["UPSLabelsPath"].ToString(),
                                  _newDataTable.Rows[0]["UPSLabelsPathuid"].ToString(),
                                  _newDataTable.Rows[0]["UPSLabelsPathpwd"].ToString());

The main advantage for me that version 1 has over version 2 is the ease of being able to introduce some validation on the datarow content (which I presume has happened outside this function) - I would probably be doing some null checking etc before diving in to use the data

Posted 19-Feb-15 23:03pm

CHill60

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

manak chand · Accepted Answer · 2015-02-19T20:43:00

Solution 1

The first code is looking clean and better.. It is good as for maintenance wise, as coder has to change value for one time only and not multiple times..

Posted 19-Feb-15 20:43pm

manak chand

Comments

manak chand 20-Feb-15 3:07am

Please do not down vote if you are accepting the solution and pls give some rating as well.. thanks...

tanishtaman 20-Feb-15 3:14am

happy :)