Вопрос о рефакторинге C#

В коде есть небольшие неэффективности (много выделений строк и ненужных поисков).

Вот код с некоторыми изменениями:

• Нет вызова ToUpper(). (ToUpper и ToLower могут быть злыми )
• кэширование значения DataReader
• отсутствие вызовов ToString
• удалено создание экземпляра DataTable (не используется)

Получившийся код выглядит так:

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

Предположительно, в таблице настроек есть и другие полезные значения. Я бы предложил считывать все значения в словарь, который хранится в приложении. Затем искать значения по мере необходимости. Дополнительные затраты на захват всех записей, а не только этих двух, ничтожны по сравнению с тем, чтобы позже установить другое соединение и повторно выполнить тот же запрос.

Этот код на самом деле выполняет две разные функции:

1) Get the database version

2) Get the application version

Единственное общее - это метод хранения данных

Я бы предложил для рефакторинга три метода:

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

Это обеспечивает разделение между получением data и фактически что-то с ними делать, что всегда является целью. Было бы рекомендовано использовать кеширование, чтобы не выполнять запрос дважды, и я бы предложил, возможно, иметь метод, который выполнял бы как вызовы версии базы данных, так и версии приложения за один шаг.

Я бы переписал запрос так, чтобы он возвращал одну запись с двумя столбцами. Это позволило бы избавиться от условий внутри оператора using(). Как сказал Гейб, я бы добавил

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

внутри блока using.

используйте if вместо переключателя менее чем для 5 случаев, это быстрее.

Если вы хотите стремиться к устойчивости и расширению по мере появления большего количества настроек, настройте шаблон стратегии, сохраняя каждую из стратегий для работы с конкретным параметром в словаре со связанным значением ключа, чтобы вытащить правильную стратегию для замены оператор переключения.

Одно из предложений, которое я бы добавил, это выполнить проверку, чтобы убедиться, что соединение с базой данных было установлено перед выполнением дальнейших команд.

sqlConn.Open();

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

   // perform read settings logic here

}