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

Question

У меня есть класс, содержащий методы для заполнения DropDowns, возврата DataSet, возврата Scalar или просто excute запроса. В одном из моих older posts в 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 объектов также или они тоже будут автоматически утилизироваться с ИСПОЛЬЗОВАНИЕМ?

Answer 1

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

Критицизмы:

• вы смешиваете пользовательский интерфейс и доступ к данным чудовищно - манипуляционный, в частности, исключение не дает никаких указаний на вызывающем коде (хотя лично я бы держать контрольный код отдельно тоже), и предполагается, что caller всегда хочет, чтобы этот сценарий был подход (для меня, если этот код не работает, все очень неправильно: пусть это исключение пузырь вверх!)
• нет механизма для правильных параметров; мое подозрение тогда заключается в том, что вы объединяете строки, чтобы сделать запрос - потенциальный (но очень реальный) риск SQL-инъекции.
• Вы упоминаете высокий уровень совпадения; если это так, я бы ожидал увидеть участие в кэше здесь
• для улучшения кода, я бы переместил код «создать соединение» в центральную точку - «DRY» и т. д .; Я не ожидал бы отдельный метод, как это относиться к себе с деталями, как, где соединение-нить происходит от

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

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

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

Answer 2

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

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

В моем примере ниже я использую одно статическое поле - 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); 
    } 
}