A C# Refactoring Question...

Question

I came accross the following code today and I didn't like it. It's fairly obvious what it's doing but I'll add a little explanation here anyway:

Basically it reads all the settings for an app from the DB and the iterates through all of them looking for the DB Version and the APP Version then sets some variables to the values in the DB (to be used later).

I looked at it and thought it was a bit ugly - I don't like switch statements and I hate things that carry on iterating through a list once they're finished. So I decided to refactor it.

My question to all of you is how would you refactor it? Or do you think it even needs refactoring at all?

Here's the code:

        using (var sqlConnection = new SqlConnection(Lfepa.Itrs.Framework.Configuration.ConnectionString))
        {
            sqlConnection.Open();

            var dataTable = new DataTable("Settings");

            var selectCommand = new SqlCommand(Lfepa.Itrs.Data.Database.Commands.dbo.SettingsSelAll, sqlConnection);
            var reader = selectCommand.ExecuteReader();
            while (reader.Read())
            {
                switch (reader[SettingKeyColumnName].ToString().ToUpper())
                {
                    case DatabaseVersionKey:
                        DatabaseVersion = new Version(reader[SettingValueColumneName].ToString());
                        break;
                    case ApplicationVersionKey: 
                        ApplicationVersion = new Version(reader[SettingValueColumneName].ToString());
                        break;
                    default:
                        break;
                }
            }

            if (DatabaseVersion == null)
                throw new ApplicationException("Colud not load Database Version Setting from the database.");
            if (ApplicationVersion == null)
                throw new ApplicationException("Colud not load Application Version Setting from the database.");
        }

Answer 1

My two cents...

1. As Bobby comments, use using on every disposable object
2. I would avoid opening a table and iterate through all the records, use a filter if possible to obtain the values
3. If not possible at all, avoid using switch on strings, as there are only two options you could do an if else with a string.Compare with the case insensitive option, so you don't have to make an upper each time.
4. Check for null values before reading them to avoid unhandled exceptions
5. If you have to open that kind of connections many times in your code you may use a factory method to give you the connection.
6. I would avoid using "var" keyword when you already know what kind of object you are creating. You usually refactor to enhance code legibility.

Answer 2

use an if instead of a switch for less than 5 cases, it's faster.

Answer 3

There are minor inefficiencies in the code (a lot of string allocations and unnecessary lookups).

Here's the code with some changes in it:

• No ToUpper() call. (ToUpper and ToLower can be evil )
• caching DataReader value
• No ToString calls
• removed DataTable instance creation (not used)

The resulting code looks like this:

using (var sqlConnection = new SqlConnection(Lfepa.Itrs.Framework.Configuration.ConnectionString))
{
    sqlConnection.Open();

    using(var selectCommand = new SqlCommand(Lfepa.Itrs.Data.Database.Commands.dbo.SettingsSelAll, sqlConnection))
    {
        using (var reader = selectCommand.ExecuteReader())
        {
            while (reader.Read())
            {
                string val = reader.GetString(reader.GetOrdinal(SettingKeyColumnName));
                if ( string.IsNullOrEmpty(val) )
                    continue;
                if ( val.Equals(DatabaseVersionKey, StringComparison.OrdinalIgnoreCase))
                    DatabaseVersion = new Version(val);
                else if (val.Equals(ApplicationVersionKey, StringComparison.OrdinalIgnoreCase))
                    ApplicationVersion = new Version(val);
            }
        }
    }
}

if (DatabaseVersion == null)
    throw new ApplicationException("Could not load Database Version Setting from the database.");
if (ApplicationVersion == null)
    throw new ApplicationException("Could not load Application Version Setting from the database.");

Answer 4

I would rewrite the query so that it returns a single record with two columns. That would get rid of the conditionals inside the using() statement. As Gabe said, I'd add

if (DatabaseVersion != null && ApplicationVersion != null) break;

inside the using block.

Answer 5

One suggestion I would add is to perform a check to make sure the connection to the database was established before performing any more commands.

sqlConn.Open();

if (sqlConn.State == ConnectionState.Open)
{

   // perform read settings logic here

}

Answer 6

This code actually does two different things:

1) Get the database version

2) Get the application version

The only commonality is the data storage method

I would suggest refactoring to have three methods:

// Get the entire dataset from the database using the SettingsSelAll command.
private DataTable GetVersionData() {...}
// Parse a version from the dataset.
private Version GetVersion(string versionName, DataTable dataSet) { ... }

public Version GetVersion(string versionName)
{
    DataTable table = GetVersionData();
    return GetVersion(versionName, table);
}

This enforces seperation between getting the data and actually doing stuff with it, which is always something to aim for. A form of caching would be recommended to avoid doing the query twice, and I would suggest maybe having method that did both the database and application version calls in one step.

Answer 7

If you want to aim for sustainability and expansion as more settings come through, set up a strategy pattern storing each of the strategies for dealing with the particular setting in a dictionary with an associated Key value to pull the correct strategy out to replace the switch statement.

Answer 8

Presumably, there are other useful values in the settings table. I would suggest reading all of the values into a dictionary that is held by the application. Then look up the values as needed. The added expense of grabbing all of the records instead of just these two is trivial compared to making another connection later and re-executing the same query.