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

Ответ 1

Мои два цента...

• Как комментарии Бобби, используйте использование на каждом одноразовом объекте
• Я бы не открывал таблицу и не перебирал все записи, используя фильтр, если возможно, чтобы получить значения
• Если это вообще невозможно, избегайте использования строк в строке, так как есть только два варианта, которые вы могли бы сделать, если else с помощью string.Compare с нечувствительным к регистру, поэтому вам не нужно делать верхний каждый раз.
• Проверяйте нулевые значения перед их чтением, чтобы избежать необработанных исключений.
• Если вам нужно много раз открывать такие типы соединений в своем коде, вы можете использовать метод factory, чтобы дать вам соединение.
• Я бы избегал использовать ключевое слово "var", когда вы уже знаете, какой объект вы создаете. Вы обычно используете рефакторинг для повышения четкости кода.

Ответ 2

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

Здесь код с некоторыми изменениями в нем:

• Нет вызова ToUpper(). (ToUpper и ToLower может быть evil)
• кэширование значения 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.");

Ответ 3

Этот код фактически выполняет две разные вещи:

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

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

Ответ 4

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

Ответ 5

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

Ответ 6

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

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

внутри блока использования.

Ответ 7

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

sqlConn.Open();

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

   // perform read settings logic here

}

Ответ 8

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