Declaring Unnecessary Variables for Readability in C#

cclean codeobject-oriented-design

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.

Best Answer

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