Насколько оборонительно мне следует программировать?[закрыто]

https://stackoverflow.com/questions/1053215

20-08-2019
|

Вопрос

я работал с небольшой процедурой, которая используется для создания соединения с базой данных:

До

public DbConnection GetConnection(String connectionName)
{
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);
   DbConnection conn = factory.CreateConnection();
   conn.ConnectionString = cs.ConnectionString;
   conn.Open();

   return conn;
}

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

Например:

ConfigurationManager.ConnectionStrings...

А документация говорит, что звонит Строки подключения бросает КонфигурацияErrorException если он не смог получить коллекцию.В этом случае я ничего не могу сделать, чтобы обработать это исключение, поэтому я его оставлю.

Следующая часть — фактическое индексирование Строки подключения найти название соединения:

...ConnectionStrings[connectionName];

В этом случае Документация ConnectionStrings говорит, что имущество вернется нулевой если имя соединения не найдено.я могу проверить, происходит ли это, и выдать исключение, чтобы позволить кому-то сообщить, что он дал неверное имя соединения:

ConnectionStringSettings cs= 
      ConfigurationManager.ConnectionStrings[connectionName];
if (cs == null)
   throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

я повторяю то же упражнение с:

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);

А GetFactory метод не имеет документации о том, что произойдет, если фабрика для указанного ProviderName не удалось найти.Это не документально подтверждено, чтобы вернуться null, но я все еще могу защищаться, и проверять для нуля:

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);
if (factory == null) 
   throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

Далее идет построение объекта DbConnection:

DbConnection conn = factory.CreateConnection()

И снова документация не говорит, что произойдет, если не удастся создать соединение, но опять же я могу проверить возвращаемый объект с нулевым значением:

DbConnection conn = factory.CreateConnection()
if (conn == null) 
   throw new Exception.Create("Connection factory did not return a connection object");

Далее устанавливается свойство объекта Connection:

conn.ConnectionString = cs.ConnectionString;

В документах не говорится, что произойдет, если не удастся установить строку подключения.Выдает ли это исключение?Оно игнорирует это?Как и в большинстве случаев, если при попытке установить ConnectionString соединения произошла ошибка, я ничего не могу сделать, чтобы ее исправить.Так что я ничего не буду делать.

И наконец, открываем соединение с базой данных:

conn.Open();

А Открытый метод DbConnection является абстрактным, поэтому поставщик, являющийся потомком DbConnection, должен решить, какие исключения он выдает.В документации по абстрактным открытым методам также нет указаний о том, что можно ожидать в случае ошибки.Если при подключении произошла ошибка, я знаю, что не смогу с ней справиться — мне придется позволить ей всплыть, чтобы вызывающий абонент мог показать пользователю некоторый пользовательский интерфейс, и позволить ему повторить попытку.

После

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
    if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

Краткое содержание

Таким образом, моя четырехстрочная функция превратилась в 12 строк и потребовала 5 минут поиска документации.В конце концов я поймал один случай, когда методу разрешено возвращать значение null.Но на практике все, что я сделал, это преобразовал исключение нарушения прав доступа (если я пытаюсь вызвать методы по нулевой ссылке) в ИнвалидАргументИсключение.

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

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

Но стоит ли оно того?Это излишество?Неужели эта защитная программа пошла наперекосяк?

Решение

Проверка конфигурации вручную и выдача исключения не лучше, чем просто позволить платформе выдать исключение, если конфигурация отсутствует.Вы просто дублируете проверки предварительных условий, которые в любом случае происходят внутри методов платформы, и это делает ваш код многословным, но это не приносит никакой пользы.(На самом деле вы можете быть удаление информацию, выбрасывая все как базовый класс Exception.Исключения, создаваемые платформой, обычно более специфичны.)

Редактировать:Этот ответ кажется несколько спорным, поэтому немного уточнений:Защитное программирование означает «готовиться к неожиданностям» (или «быть параноиком»), и один из способов сделать это — выполнить множество предварительных проверок.Во многих случаях это хорошая практика, однако, как и в любой другой практике, затраты следует сопоставлять с выгодами.

Например, выдача исключения «Не удалось получить соединение с завода» не дает никакой пользы, поскольку в нем ничего не говорится о почему поставщик не может быть получен - и уже следующая строка все равно выдаст исключение, если поставщик имеет значение null.Таким образом, затраты на проверку предварительных условий (с точки зрения времени разработки и сложности кода) не оправданы.

С другой стороны, проверка наличия конфигурации строки подключения. может быть оправданным, поскольку исключение может помочь разработчику подсказать, как решить проблему.Исключение null, которое вы получите в следующей строке, в любом случае не сообщает имя отсутствующей строки подключения, поэтому ваша проверка предварительных условий действительно дает некоторую ценность.Например, если ваш код является частью компонента, значение довольно велико, поскольку пользователь компонента может не знать, какие конфигурации требуются компоненту.

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

По сути, вам следует обрабатывать только те исключения, которые вы можете делать Кое-что о.Исключения, из которых вы все равно не сможете восстановиться, должны просто передаваться вверх обработчику верхнего уровня.В веб-приложении обработчик верхнего уровня, вероятно, просто показывает общую страницу ошибки.Но в большинстве случаев делать особо нечего, если база данных находится в автономном режиме или отсутствует какая-то важная конфигурация.

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

Другие советы

Ну, это зависит от того, кто ваша аудитория.

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

(Тем не менее, если вы это делаете, я предлагаю вам определить лучшие исключения, чем просто System.Exception, чтобы облегчить людям, которые хотят перехватывать некоторые из ваших исключений, но не другие.)

Но если вы собираетесь использовать его самостоятельно (или вы и ваш приятель), то, очевидно, это излишне и, вероятно, в конечном итоге навредит вам, сделав ваш код менее читабельным.

Мне бы хотелось, чтобы моя команда писала такой код.Большинство людей даже не понимают сути защитного программирования.Лучшее, что они могут сделать, — это обернуть весь метод в оператор try catch и позволить всем исключениям обрабатываться общим блоком исключений!

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

Помните: когда вы используете API .net framework, чего вы от него ожидаете?Что кажется естественным?Сделайте то же самое со своим кодом.

Я знаю, что это требует времени.Но тогда качество обходится дорого.

ПС:Вам действительно не нужно обрабатывать все ошибки и создавать собственное исключение.Помните, что ваш метод будет использоваться только другими разработчиками.Они должны быть в состоянии самостоятельно выявлять общие исключения в рамках платформы.ЭТО не стоит усилий.

Ваш пример «до» отличается ясностью и краткостью.

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

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

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

Я не думаю, что стал бы писать какую-либо логику проверки нулевых ссылок - по крайней мере, не так, как вы это сделали.

Мои программы, которые получают параметры конфигурации из файла конфигурации приложения, проверяют все эти параметры при запуске.Обычно я создаю статический класс, содержащий настройки и ссылающийся на свойства этого класса (а не на ConfigurationManager) в другом месте приложения.Для этого есть две причины.

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

Во-вторых, проверка правильности конфигурации на самом деле не должна быть заботой объектов, которые полагаются на конфигурацию.Нет смысла заставлять себя вставлять проверки повсюду в коде, если вы уже выполнили эти проверки заранее.(Конечно, из этого есть исключения - например, долго работающие приложения, где вам нужно иметь возможность изменять конфигурацию во время работы программы и отражать эти изменения в поведении программы;в таких программах вам нужно будет перейти к ConfigurationManager всякий раз, когда вам нужна настройка.)

Я бы не стал проверять нулевые ссылки на GetFactory и CreateConnection звонки тоже.Как бы вы написали тестовый пример для проверки этого кода?Вы не можете, потому что не знаете, как заставить эти методы возвращать ноль — вы даже не знаете, что это так. возможный чтобы эти методы возвращали ноль.Итак, вы решили одну проблему: ваша программа может выдать NullReferenceException при условиях, которые вам не понятны - при другом, более существенном:в тех же загадочных условиях ваша программа будет запускать код, который вы не тестировали.

Мое практическое правило:

Не поймайте, если сообщение об исключении брошенных имеет отношение к вызывающему абоненту.

Таким образом, NullReferenceException не имеет соответствующего сообщения, я бы проверил, имеет ли оно значение NULL, и выдал бы исключение с лучшим сообщением.ConfigurationErrorException имеет значение, поэтому я его не уловил.

Единственным исключением является случай, когда «контракт» GetConnection не обязательно извлекает строку подключения из файла конфигурации.

Если это так, GetConnection ДОЛЖЕН иметь контракт с пользовательским исключением, в котором говорится, что соединение не может быть получено, тогда вы можете обернуть ConfigurationErrorException в свое собственное исключение.

Другое решение — указать, что GetConnection не может выдавать исключение (но может возвращать значение null), а затем вы добавляете «ExceptionHandler» в свой класс.

Документация по вашему методу отсутствует.;-)

Каждый метод имеет определенные входные и выходные параметры и определенное результирующее поведение.В вашем случае что-то вроде:«В случае успеха возвращает действительное открытое соединение, в противном случае возвращает значение null (или выдает исключение XXXException, как вам нравится).Помня об этом поведении, теперь вы можете решить, какую защиту вам следует запрограммировать.

Если ваш метод должен предоставлять подробную информацию о том, почему и что не удалось, тогда сделайте это так, как вы, и проверяйте, перехватывайте все и вся и возвращайте соответствующую информацию.
Но если вас просто интересует открытое соединение DBConnection или просто нулевой (или какое-то пользовательское исключение) в случае сбоя, затем просто оберните все в try/catch и верните нулевой (или какое-то исключение) в случае ошибки и объект еще.

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

В общем, исключения, специфичные для базы данных, должны перехватываться и повторно выбрасываться как нечто более общее, например (гипотетическое) DataAccessFailure исключение.Коду более высокого уровня в большинстве случаев не обязательно знать, что вы читаете данные из базы данных.

Еще одна причина быстро перехватывать эти ошибки заключается в том, что они часто включают в свои сообщения некоторые сведения о базе данных, например «Нет такой таблицы:ACCOUNTS_BLOCKED» или «Ключ пользователя недействителен:234234".Если это распространится на конечного пользователя, это плохо по нескольким причинам:

Сбивает с толку.
Потенциальное нарушение безопасности.
Стыдно для имиджа вашей компании (представьте, что клиент читает сообщение об ошибке с грубой грамматикой).

Я бы закодировал это точно так же, как ваша первая попытка.

Однако пользователь этой функции защитит объект соединения с помощью блока USING.

Мне действительно не нравится переводить исключения, как это делают другие ваши версии, поскольку очень сложно выяснить, почему он сломался (База данных не работает?У вас нет разрешения на чтение файла конфигурации и т. д.?).

Исправленная версия не добавляет особой ценности, пока приложение имеет AppDomain.UnexpectedException обработчик, который выгружает exception.InnerException цепочку и все трассировки стека в какой-нибудь файл журнала (или, что еще лучше, записывает минидамп), а затем вызывает Environment.FailFast.

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

Обратите внимание, что предпочтительнее обрабатывать AppDomain.UnexpectedException и позвони Environment.FailFast вместо того, чтобы иметь высший уровень try/catch (Exception x) потому что в последнем случае первоначальная причина проблемы, вероятно, будет скрыта дальнейшими исключениями.

Это связано с тем, что если вы поймаете исключение, любое открытое finally блоки будут выполняться и, вероятно, генерируют больше исключений, которые скроют исходное исключение (или, что еще хуже, они удалят файлы в попытке вернуть некоторое состояние, возможно, неправильные файлы, возможно, даже важные файлы).Никогда не следует ловить исключение, указывающее на недопустимое состояние программы, с которым вы не знаете, как обработать, даже на верхнем уровне. main функция try/catch блокировать.Умение обращаться AppDomain.UnexpectedException и звоню Environment.FailFast это другой (и более желательный) котел с рыбой, потому что он останавливается finally блокирует запуск, и если вы пытаетесь остановить свою программу и записать некоторую полезную информацию, не нанося при этом дальнейшего ущерба, вам определенно не следует запускать свою программу. finally блоки.

Не забудьте проверить OutOfMemoryExceptions...ты знаешь это мощь случаться.

Изменения Иэна кажутся мне разумными.

Если я использую систему и использую ее неправильно, мне нужна максимальная информация о неправильном использовании.Например.если я забуду вставить некоторые значения в конфигурацию перед вызовом метода, мне нужно исключение InvalidOperationException с сообщением с подробным описанием моей ошибки, а не исключение KeyNotFoundException/NullReferenceException.

Все дело в контексте, ИМХО.В свое время я видел несколько довольно непонятных сообщений об исключениях, но в других случаях исключение по умолчанию, исходящее из платформы, вполне нормально.

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

Я всегда стараюсь помнить, что как разработчик фрагмента кода или системы я могу лучше диагностировать сбои, чем тот, кто просто его использует.Иногда бывает так, что несколько строк проверочного кода + специальное сообщение об исключении могут в совокупности сэкономить часы времени отладки (а также облегчить вашу собственную жизнь, поскольку вас не будут тянуть к чужой машине для отладки их проблемы).

На мой взгляд, ваш образец «после» не является оборонительным.Потому что защита будет заключаться в проверке частей, находящихся под вашим контролем, что будет аргументом. connectionName (проверьте значение NULL или пустое и создайте исключение ArgumentNullException).

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

Что-то вроде этого (отредактировано в редакторе SO, поэтому синтаксис/компилятор не проверяет.Может не скомпилироваться ;-))

private string GetConnectionString(String connectionName)
{

   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
   if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");
   return cs;
}

private DbProviderFactory GetFactory(String ProviderName)
{
   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+ProviderName+"\"");
   return factory;
}

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs = GetConnectionString(connectionName);

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = GetFactory(cs.ProviderName);


   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

ПС:Это не полный рефакторинг, сделаны только первые два блока.

Лицензировано под: CC-BY-SA с атрибуция

Не связан с StackOverflow