A C# Refactoring Question

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.");

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.

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.

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

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.

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

}

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.