1

I know some languages (C comes to mind) its best to declare as few variables as possible. In a language with memory management like C# is it better practice to continue with this philosophy or should I be declaring unnecessary variables if it improvese readability?

Ex: Declaring unnecessary database and server variables

public static DataTable CreateMonthlyPerformanceSummaryTable(int perfSumId, DateTime date, bool startDate)
{
    DataTable monthlyPerformanceTable = new DataTable();
    string dataBase = PerformanceDBDataObject.DBName;
    string server = PerformanceDBDataObject.Server;

    using (SqlConnection conn = SQL.ConnectToDB(dataBase, server, 30))
    {
        if (startDate == true)
        {
            SelectPerformanceSummaryUsingStartDate(date, monthlyPerformanceTable, conn);
        }
        else
        {
            SelectPerformanceSummaryUsingEndDate(date, monthlyPerformanceTable, conn);
        }
    }
    return monthlyPerformanceTable;            
}

Or: Simply plugging in Values into ConnectToDB

public static DataTable CreateMonthlyPerformanceSummaryTable(int perfSumId, DateTime date, bool startDate)
{
    DataTable monthlyPerformanceTable = new DataTable();

    using (SqlConnection conn = SQL.ConnectToDB(PerformanceDBDataObject.DBName, PerformanceDBDataObject.Server, 30))
    {
        if (startDate == true)
        {
            SelectPerformanceSummaryUsingStartDate(date, monthlyPerformanceTable, conn);
        }
        else
        {
            SelectPerformanceSummaryUsingEndDate(date, monthlyPerformanceTable, conn);
        }
    }
    return monthlyPerformanceTable;            
}

Personally I think the first option is better because the shorter line length allows for improved readability, but I am wondering if this is bad practice.

  • 4
    if you want to simplify something you can start from using if (startDate) instead of if (startDate == true) :) – Engineert Mar 7 '19 at 19:54
  • 5
    I would strongly question whether avoiding variables in C is a good idea. C compilers are generally good at optimizing out locals. – Andrew Mar 7 '19 at 20:59
  • 10
    "I know some languages (C comes to mind) its best to declare as few variables as possible" - honestly, this sounds like a very unfounded, superstitous believe to me, do you have any references for this strange recommendation? Who told you this? – Doc Brown Mar 7 '19 at 21:18
  • 7
    For readability's sake, startDate is a terrible name for a Boolean variable. If I randomly saw startDate somewhere in a block of code and did not see the type declaration, Boolean would definitely not be the type I'd infer. – Matt Mar 7 '19 at 21:58
  • 2
    "I know some languages (C comes to mind) its best to declare as few variables as possible" - That's just wrong, and you can easily see why, because the compiler will typically produce the exact same assembly, at least when you provide the usual optimisation flags. Please compare: godbolt.org/z/0H3RKj & godbolt.org/z/9hPdie – Christian Hackl Mar 8 '19 at 9:26
5

We can speculate on whether those extra variable affect performance. But it's far better to find out, rather than speculating. So use tools available to you, such as Roslyn and profilers to determine it. For example, compare the resultant assembler for your first example, compared with your second version.

Regarding the other part of your question: readability. The problem with readability is that folk all too easily focus on the minutia, such as extra variables, whitespace etc and in the process miss the elephant in the room. Readability is just one component of code quality. And as Thom Holwerda put it, quality can be expressed as being inversely proportional to the number of WTFs/minute:

Code quality as WTFs/minute

And the I see two glaring quality issues with your code that make me go "WTF!".

Firstly, you have a flag parameter that, depending on its state, causes your method to do one of two things. Using a boolean in this way is a well documented code smell, eg here, here, here etc. That boolean is telling me, and should be telling you, that you need two methods here as you are doing two completely different things.

Secondly, and oh so much worse: PerformanceDBDataObject.DBName and PerformanceDBDataObject.Server. The method is static and these aren't passed in via the methods parameters. Simply put, this means they are global variables. Global variables, except in very limited edge cases, are as about as poor quality as our code can get.

By fixing those two issues, your original question becomes academic as we now have those items named in the parameter lists, no local variables needed. And then, rather than create monthlyPerformanceTable, pass it to other methods for them to modify and return it, let those other methods create and return a DataTable. And we end up with:

public static DataTable CreateMonthlyPerformanceSummaryTableUsingStartDate(
        DateTime startDate,
        string database,
        string server)
{
    using (var conn = SQL.ConnectToDB(database, server, 30))
    {
        return SelectPerformanceSummaryUsingStartDate(startDate, conn);         
    }
}

public static DataTable CreateMonthlyPerformanceSummaryTableUsingEndDate(
        DateTime endDate,
        string database,
        string server)
{
    using (var conn = SQL.ConnectToDB(database, server, 30))
    {
        return SelectPerformanceSummaryUsingEndDate(endDate, conn);
    }
}
| improve this answer | |
  • Thank you for this valuable insight, I think that comic is in my addition of The Pragmatic Programmer, so maybe I need to give it a review read. The function was quickly split in two after my first round of refactoring. One question I have concerning global variables; where should I hard code my Server and Database strings? My database dataObject does seem like a bit of a hack, but I have learned my lesson about hard coding DB and server names into my methods. – Mwspencer Mar 8 '19 at 16:10
5

This should not be an unnecessary variable question. This should be a white space question.

public static DataTable CreateMonthlyPerformanceSummaryTable
(
        int perfSumId, 
        DateTime date, 
        bool isStartDate
) 
{
    DataTable monthlyPerformanceTable = new DataTable();

    using 
    (
        SqlConnection conn = SQL.ConnectToDB
        (
            PerformanceDBDataObject.DBName, 
            PerformanceDBDataObject.Server, 
            30
        )
    )        
    {
        if (isStartDate)
        {
            SelectPerformanceSummaryUsingStartDate(
                date, 
                monthlyPerformanceTable, 
                conn)
            ;
        }
        else
        {
            SelectPerformanceSummaryUsingEndDate(
                date, 
                monthlyPerformanceTable, 
                conn)
            ;
        }
    }
    return monthlyPerformanceTable;            
}

Why? Because a names one and only job is to be descriptive, no matter how long that makes it. Never shorten a name for the sake of a one liner. Find a better way to lay out your code.

| improve this answer | |
4

The cost of additional variables is small, so unless this is happening inside a performance-critical loop that needs to be tightly optimized, I'd always go for readability. Note that in C# most things are reference types, so you're not allocating/copying more than a pointer unless it's a large struct.

The example you give is interesting, because if this wasn't in a using statement I wouldn't consider the temporary variables necessary; the fields they come from are already equally as descriptive.

SQL.ConnectToDB(
    PerformanceDBDataObject.DBName, 
    PerformanceDBDataObject.Server, 
    30) // If anything, this is the one that could use a name

But given that multi-line using looks rather ugly, I'd say they're an improvement in this case.

| improve this answer | |
  • Great thank you, I am trying use C practices with a grain of salt as I learn, knowing that many things may not apply high level languages. – Mwspencer Mar 7 '19 at 19:45
  • This may not matter in this case but when you have gobs of stuff happening in one line, it can also be problematic for debugging. The line numbers in stacktraces can point you right at the problem when there's only one statement. – JimmyJames Mar 7 '19 at 20:46
1

Readability goes first, unless it noticeably hurts performance. (Simply because 80% of our time we are reading the code)

Another approach to your particular sample.
All required data for connection in one class PerformanceDBDataObject, so we can "hide" implementation details of how to create a connection behind a method

public SqlConnection ConnectTo(PerformanceDBDataObject dbData)
{
    return SQL.ConnectToDB(dbData.DBName, dbData.Server, 30);    
}

Look now it's probably can fit into one line and consumer's code will look much more readable without extra implementation details.

public static DataTable CreateMonthlyPerformanceSummaryTable(
    int perfSumId, 
    DateTime date, 
    bool startDate)
{
    var monthlyPerformanceTable = new DataTable();
    using (var conn = ConnectTo(PerformanceDBDataObject))
    {
        if (startDate == true)
        {
            SelectPerformanceSummaryUsingStartDate(date, monthlyPerformanceTable, conn);
        }
        else
        {
            SelectPerformanceSummaryUsingEndDate(date, monthlyPerformanceTable, conn);
        }
    }

    return monthlyPerformanceTable;            
}

Not sure are PerformanceDBDataObject and SQL properties or static classes, sample above assumes PerformanceDBDataObject is property of current class.

In case both are static classes, you can create one static method to create a connection

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.