Как улучшить этот класс методов многократного использования?

Question

У меня есть класс, содержащий методы для заполнения DropDowns, возврата DataSet, Scalar или просто выполнения запроса. В одном из моих старых постов в StackOverflow я представил код ошибки того же класса. Основываясь на советах авторов, я улучшил код и хочу узнать, подходит ли этот класс для использования в среде с высоким уровнем одновременности:

public sealed class reuse
{
    public void FillDropDownList(string Query, DropDownList DropDownName)
    {
        using (TransactionScope transactionScope = new TransactionScope())
        {
            using (SqlConnection con = new SqlConnection(ConfigurationManager.ConnectionStrings["MyDbConnection"].ConnectionString.ToString()))
            {
                SqlDataReader dr;
                try
                {
                    if (DropDownName.Items.Count > 0)
                        DropDownName.Items.Clear();

                    SqlCommand cmd = new SqlCommand(Query, con);
                    dr = cmd.ExecuteReader();

                    while (dr.Read())
                        DropDownName.Items.Add(dr[0].ToString());

                    dr.Close();
                }
                catch (Exception ex)
                {
                    CustomErrorHandler.GetScript(HttpContext.Current.Response,ex.Message.ToString());
                }
            }
        }
    }
}

Я хочу знать, нужно ли удалять объекты Command и DataReader, или они тоже автоматически удаляются с помощью USING?

Answer 1

Команда / считыватель: они будут уничтожаться путем «использования», но только если вы используете «использование» для них, что вам следует.

Критицизмы:

• вы ужасно смешиваете пользовательский интерфейс и доступ к данным - обработка исключений, в частности, не дает указания вызывающему коду (хотя лично я бы тоже держал код управления отдельно) и предполагает, что вызывающая сторона всегда хочет такой подход на основе сценариев для меня, если этот код не работает, все очень плохо: пусть это исключение всплывает вверх!)
• нет механизма для правильных параметров; Поэтому я подозреваю, что вы объединяете строки для выполнения запроса - потенциальный (но очень реальный) риск внедрения SQL
• Вы упомянули высококонкурентный; если так, то я ожидаю увидеть здесь какое-то участие в кэше
• по причинам обслуживания кода, я бы переместил весь код "создать соединение" в центральную точку - "СУХОЙ" и т. Д .; Я не ожидал бы, что отдельный метод, подобный этому, будет заниматься такими деталями, как, например, откуда взята строка подключения

Честно говоря, я бы просто использовал здесь dapper и избежал всех этих проблем:

using(var connection = Config.OpenConnection()) {
     return connection.Query<string>(tsql, args).ToString();
}

(и позволить вызывающей стороне перебирать список, или использовать AddRange, или привязку данных, что угодно)

Answer 2

В целом согласен с ответом Марка, но у меня есть некоторые другие комментарии и другой угол.Надеюсь, мой ответ будет вам полезен.

Во-первых, нет ничего плохого в использовании статических классов и методов в параллельной среде, если нет необходимости в какой-либо информации о состоянии и данных.поделилсяВ вашем случае, заполняя DropDownList, это прекрасно, потому что вам нужен только список строк, и как только это будет сделано, вы можете забыть все о том, как вы его получили.Также нет помех между параллельными вызовами статического метода, если они не обращаются к статическим полям.Статические методы распространены в .NET Framework и они безопасны для потоков.

В моем примере ниже я использую одно статическое поле - log4net logger.Он по-прежнему ориентирован на многопотоковое исполнение, поскольку не несет никакого состояния и является просто точкой перехода к библиотеке log4net, которая сама по себе является поточно-ориентированной.Рекомендуем хотя бы взглянуть на log4net - отличная регистрация lib.

Это может быть небезопасно, если вы попытаетесь заполнить один и тот же выпадающий список из двух потоков, но тогда будет также небезопасно, если этот класс не будет статичным.Убедитесь, что выпадающие списки заполнены из одного (основного) потока.

Вернуться к вашему коду. Смешивание пользовательского интерфейса и извлечения данных не является хорошей практикой , поскольку это делает код гораздо менее поддерживаемым и менее стабильным.Разделите эти два. Dapper библиотека может быть хорошим способом упростить вещи.Я не использовал его сам, поэтому все, что я могу сказать, это то, что он выглядит очень удобно и эффективно.Если вы хотите / должны узнать, как все работает, не используйте его.По крайней мере, на первый взгляд.

Наличие непараметризованного запроса в одной строке потенциально подвержено атакам с использованием SQL-инъекций, но если этот запрос не построен на основе какого-либо прямого пользовательского ввода, он должен быть безопасным.Конечно, вы всегда можете принять параметризацию, чтобы быть уверенным.

Обработка исключений с использованием

CustomErrorHandler.GetScript(HttpContext.Current.Response, ex.Message.ToString());

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

Если вы выполняете только одно чтение БД, явной транзакции не требуется. Что касается объекта соединения, он не должен быть статичным в любой ситуации и создаваться по требованию. В этом нет снижения производительности, поскольку .NET сохраняет пул готовых к использованию соединений и перезапускает те, которые были утилизированы.'.

Я считаю, что пример всегда лучше, чем просто объяснения, поэтому вот как я бы перестроил ваш код.

public static class reuse
{
    static public readonly log4net.ILog log = log4net.LogManager.GetLogger("GeneralLog");

    public static void FillDropDownList(string query, string[] parms,  DropDownList dropDown)
    {
        dropDown.Items.Clear();
        dropDown.DataSource = GetData(query, parms);
        dropDown.DataBind();
    }

    private static IEnumerable<string> GetData(string query, string[] parms)
    {
        using (SqlConnection con = new SqlConnection(GetConnString()))
        {
            try
            {
                List<string> result = new List<string>();
                SqlCommand cmd = new SqlCommand(query, con);
                cmd.Parameters.AddRange(parms);
                SqlDataReader dr = cmd.ExecuteReader();
                if (dr.VisibleFieldCount > 0)
                {
                    while (dr.Read())
                        result.Add(dr[0].ToString());
                }
                dr.Close();
                return result;
            }
            catch (Exception ex)
            {
                log.Error("Exception in GetData()", ex);
                throw;
            }
        }
    }

    private static string GetConnString()
    {
        return ConfigurationManager.ConnectionStrings["MyDbConnection"].ConnectionString.ToString(CultureInfo.InvariantCulture);
    }
}