Declaring unnecessary variables for readability

Question

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.

Answer 1

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:

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);
    }
}

Answer 2

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.

Answer 3

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.

Answer 4

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

Answer 5

Declaring unnecessary variables makes the code less readable, not more readable. By having a variable, as someone reading the code, from the point of use, you then have to find the point at which the variable is initialized and also ensure that it has not been mutated before its use in order to know what its value will be when passed to its target.