Насколько оборонительно я должен программировать?
Я работал с небольшой подпрограммой, которая используется для создания подключения к базе данных:
До
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...
В документации говорится, что вызов ConnectionStrings вызывает исключение ConfigurationErrorException, если он не может получить коллекцию. В этом случае я ничего не могу сделать, чтобы обработать это исключение, поэтому я его отпущу.
Следующая часть - это фактическая индексация ConnectionStrings для поиска connectionName:
...ConnectionStrings[connectionName];
В этом случае в документации ConnectionStrings говорится, что свойство вернет значение NULL, если не удалось найти имя подключения. я могу проверить это и выдать исключение, чтобы кто-то поднял трубку, указав недопустимое имя_соединения:
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();
Метод Open в DbConnection является абстрактным, поэтому любой провайдер должен исходить из DbConnection, чтобы решить, какие исключения они выбрасывают. В абстрактной документации по методам Open также нет указаний на то, что я могу ожидать в случае ошибки. Если при подключении произошла ошибка, я знаю, что не могу с ней справиться - мне придется дать ей всплыть, чтобы вызывающий пользователь мог показать какой-то пользовательский интерфейс и позволить им повторить попытку.
После
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 минут поиска документации. В конце я поймал один случай, когда метод может вернуть ноль. Но на практике все, что я сделал, это преобразовал исключение нарушения прав доступа (если я пытаюсь вызвать методы по нулевой ссылке) в InvalidArgumentException.
я также поймал два возможных случая, когда могут быть нулевые возвращаемые объекты; но опять же я обменял только одно исключение на другое.
С положительной стороны, он поймал две проблемы и объяснил, что произошло в сообщении об исключении, а не плохие вещи, происходящие в будущем (то есть доллар останавливается здесь)
Но стоит ли это того? Это перебор? Это защитное программирование пошло не так?
14 ответов
Ручная проверка конфигурации и создание исключения не лучше, чем просто позволить платформе генерировать исключение, если конфигурация отсутствует. Вы просто дублируете проверки предварительных условий, которые в любом случае происходят внутри методов фреймворка, и это делает ваш код многословным без какой-либо выгоды. (На самом деле вы можете удалять информацию, выбрасывая все как базовый класс Exception. Исключения, создаваемые платформой, обычно более специфичны.)
Изменить: Этот ответ кажется несколько спорным, поэтому немного сложнее: защитное программирование означает "готовиться к неожиданному" (или "быть параноиком"), и один из способов сделать это - выполнить множество предварительных проверок. Во многих случаях это хорошая практика, однако, как и во всех других практиках, затраты следует сопоставлять с выгодами.
Например, он не дает никакой выгоды, чтобы выдать исключение "Не удалось получить соединение с фабрики", поскольку в нем ничего не говорится о том, почему поставщик не может быть получен - и следующая строка в любом случае вызовет исключение, если поставщик является нулевым. Таким образом, стоимость проверки предусловия (во время разработки и сложности кода) не оправдана.
С другой стороны, проверка для проверки наличия конфигурации строки подключения может быть оправдана, поскольку исключение может помочь разработчику сообщить, как решить проблему. Нулевое исключение, которое вы получите в следующей строке, в любом случае не сообщает имя отсутствующей строки соединения, поэтому ваша проверка предусловий действительно дает некоторое значение. Если ваш код является частью компонента, например, значение достаточно велико, так как пользователь компонента может не знать, какие конфигурации требует компонент.
Другая интерпретация защитного программирования состоит в том, что вы должны не просто обнаруживать ошибки, вы также должны пытаться восстанавливаться после любой ошибки или исключения, которые могут возникнуть. Я не верю, что это хорошая идея в целом.
По сути, вы должны обрабатывать только те исключения, с которыми вы можете что- то сделать. Исключения, которые вы не можете восстановить в любом случае, должны быть просто переданы в обработчик верхнего уровня. В веб-приложении обработчик верхнего уровня, вероятно, просто показывает общую страницу с ошибкой. Но в большинстве случаев делать особо нечего, если база данных отключена или отсутствует какая-либо важная конфигурация.
В некоторых случаях такой вид защитного программирования имеет смысл, если вы принимаете пользовательский ввод, и этот ввод может привести к ошибкам. Если, например, пользователь предоставляет URL-адрес в качестве входных данных, и приложение пытается извлечь что-то из этого URL-адреса, то очень важно убедиться, что URL-адрес выглядит правильно, и обработать все исключения, которые могут возникнуть в результате запроса. Это позволяет вам предоставить ценную обратную связь с пользователем.
Ну, это зависит от того, кто ваша аудитория.
Если вы пишете библиотечный код, который, как вы ожидаете, будет использоваться многими другими людьми, которые не будут говорить с вами о том, как его использовать, то это не излишне. Они оценят ваши усилия.
(Тем не менее, если вы делаете это, я предлагаю вам определить лучшие исключения, чем просто System.Exception, чтобы было проще для людей, которые хотят перехватить некоторые из ваших исключений, но не другие.)
Но если вы просто собираетесь использовать его самостоятельно (или вы и ваш собеседник), то, очевидно, это излишне, и, вероятно, в конечном итоге причинит вам боль, сделав код менее читабельным.
Хотел бы я, чтобы моя команда могла так кодировать. Большинство людей даже не получают смысла оборонительного программирования. Лучшее, что они могут сделать - это обернуть весь метод в оператор try catch и разрешить обработку всех исключений общим блоком исключений!
Снимаю шляпу перед тобой, Ян. Я могу понять вашу дилемму. Я сам прошел через то же самое. Но то, что вы сделали, вероятно, помогло разработчику несколько часов избивать клавиатуру.
Помните, что когда вы используете API.net Framework, что вы ожидаете от него? Что кажется естественным? Сделайте то же самое с вашим кодом.
Я знаю, что это занимает время. Но тогда качество стоит денег.
PS: вам действительно не нужно обрабатывать все ошибки и создавать пользовательские исключения. Помните, что ваш метод будет использоваться только другими разработчиками. Они должны уметь сами определять общие рамки исключений. Это не стоит хлопот.
Ваш "до" пример отличается четкостью и краткостью.
Если что-то не так, в конце концов, структура будет создавать исключение. Если вы ничего не можете сделать с этим исключением, вы можете позволить ему распространяться вверх по стеку вызовов.
Однако бывают случаи, когда глубоко внутри структуры возникает исключение, которое действительно не проливает свет на реальную проблему. Если ваша проблема заключается в том, что у вас нет действительной строки подключения, но инфраструктура выдает исключение, например "недопустимое использование нулевого значения", то иногда лучше перехватить исключение и повторно выдать его с более значимым сообщением.
Я часто проверяю нулевые объекты, так как мне нужен фактический объект для работы, и если объект пуст, то выбрасываемое исключение будет, по меньшей мере, косым. Но я проверяю только нулевые объекты, если знаю, что именно это и произойдет. Некоторые объектные фабрики не возвращают нулевые объекты; вместо этого они выдают исключение, и проверка на ноль будет бесполезна в этих случаях.
Я не думаю, что написал бы какую-либо логику проверки нулевой ссылки - по крайней мере, не так, как вы это сделали.
Мои программы, которые получают параметры конфигурации из файла конфигурации приложения, проверяют все эти параметры при запуске. Обычно я создаю статический класс, содержащий параметры и ссылки на свойства этого класса (а не ConfigurationManager) в другом месте приложения. Для этого есть две причины.
Во-первых, если приложение не настроено должным образом, оно не будет работать. Я бы лучше знал это в тот момент, когда программа считывает файл конфигурации, чем когда-нибудь в будущем, когда я попытаюсь создать соединение с базой данных.
Во-вторых, проверка правильности конфигурации не должна касаться объектов, которые зависят от конфигурации. Нет смысла заставлять себя вставлять проверки повсюду в коде, если вы уже выполнили эти проверки заранее. (Конечно, есть исключения из этого - например, долго работающие приложения, в которых вам нужно иметь возможность изменять конфигурацию во время работы программы и отражать эти изменения в поведении программы; в таких программах вам нужно перейти к ConfigurationManager всякий раз, когда вам нужна настройка.)
Я бы не стал проверять нулевую ссылку на GetFactory а также CreateConnection звонки тоже. Как бы вы написали контрольный пример для реализации этого кода? Вы не можете, потому что вы не знаете, как заставить эти методы возвращать нуль - вы даже не знаете, что можно заставить эти методы возвращать ноль. Итак, вы заменили одну проблему - ваша программа может бросить NullReferenceException в условиях, которые вы не понимаете, с другой, более важной: при тех же самых загадочных условиях ваша программа будет запускать код, который вы не тестировали.
В целом, специфичные для базы данных исключения должны быть перехвачены и выброшены как нечто более общее, например (гипотетическое) DataAccessFailure исключение. Код высокого уровня в большинстве случаев не должен знать, что вы читаете данные из базы данных.
Еще одна причина быстрого перехвата этих ошибок заключается в том, что они часто включают в свои сообщения некоторые сведения о базе данных, например "Нет такой таблицы: ACCOUNTS_BLOCKED" или "Недопустимый ключ пользователя: 234234". Если это распространяется на конечного пользователя, это плохо по нескольким причинам:
- Смешение.
- Потенциальное нарушение безопасности.
- Смущает имидж вашей компании (представьте, что клиент читает сообщение об ошибке с сырой грамматикой).
Ваша документация по методу отсутствует.;-)
Каждый метод имеет определенные входные и выходные параметры и определенное поведение в результате. В вашем случае что-то вроде: "Возвращает действительное открытое соединение в случае успеха, иначе возвращает ноль (или выбрасывает XXXException, как вам нравится). Имея это в виду, вы можете теперь решить, насколько защитным вы должны программировать.
Если ваш метод должен раскрыть подробную информацию о том, почему и что не удалось, то сделайте это так, как вы, и проверьте и поймайте все и вся и верните соответствующую информацию.
Но, если вас просто интересует открытое DBConnection или просто null (или какое-то пользовательское исключение) в случае сбоя, тогда просто оберните все в try/catch и верните null (или некоторое исключение) в случае ошибки, а также объект в другом месте.
Поэтому я бы сказал, что это зависит от поведения метода и ожидаемого результата.
Мое правило:
не поймать, если сообщение сгенерированного исключения относится к вызывающей стороне.
Таким образом, NullReferenceException не имеет соответствующего сообщения, я бы проверил, является ли оно нулевым, и сгенерировал бы исключение с лучшим сообщением. ConfigurationErrorException имеет значение, поэтому я не улавливаю его.
Единственное исключение - если "контракт" GetConnection не извлекает строку подключения обязательно в файле конфигурации.
Если это так, то GetConnection ДОЛЖЕН иметь договор с пользовательским исключением, в котором говорится, что соединение не может быть восстановлено, тогда вы можете заключить исключение ConfigurationErrorException в свое пользовательское исключение.
Другое решение состоит в том, чтобы указать, что GetConnection не может выдать (но может вернуть ноль), затем вы добавляете "ExceptionHandler" в ваш класс.
Почему бы не разделить метод, который вы используете после того, как вы добавили все защитное программирование? У вас есть несколько отдельных логических блоков, которые требуют отдельных методов. Зачем? Потому что тогда вы инкапсулируете логику, которая принадлежит друг другу, и ваш полученный публичный метод просто правильно соединяет эти блоки.
Примерно так (отредактировано в 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;
}
PS: это не полный рефакторинг, только первые два блока.
На мой взгляд, ваш "после" образец не очень оборонительный. Потому что защитой будет проверка частей под вашим контролем, что будет аргументом connectionName (проверьте на нулевое или пустое значение и создайте исключение ArgumentNullException).
Измененная версия не добавляет особой ценности, пока приложение имеет AppDomain.UnexpectedException обработчик, который сбрасывает exception.InnerException цепочка и все трассировки стека в файл журнала какого-либо вида (или даже лучше, захватывает мини-дамп), а затем вызвать Environment.FailFast,
Исходя из этой информации, будет достаточно просто определить, что пошло не так, без необходимости усложнять код метода дополнительной проверкой ошибок.
Обратите внимание, что предпочтительно обрабатывать AppDomain.UnexpectedException и позвонить Environment.FailFast вместо того, чтобы иметь на высшем уровне try/catch (Exception x) потому что с последним исходная причина проблемы, вероятно, будет скрыта дальнейшими исключениями.
Это потому, что если вы поймаете исключение, любой открытый finally блоки будут выполняться и, вероятно, будут генерировать больше исключений, которые будут скрывать исходное исключение (или, что еще хуже, они будут удалять файлы при попытке вернуть какое-либо состояние, возможно, неправильные файлы, возможно, даже важные файлы). Вы никогда не должны ловить исключение, которое указывает на недопустимое состояние программы, которое вы не знаете, как обрабатывать, даже на верхнем уровне main функция try/catch блок. обращение AppDomain.UnexpectedException и звонит Environment.FailFast это другой (и более желательный) чайник рыбы, потому что он останавливается finally блокирует запуск, и если вы пытаетесь остановить свою программу и зарегистрировать некоторую полезную информацию, не нанося дополнительного вреда, вы определенно не хотите запускать свою finally блоки.
Я бы закодировал это в точности как твоя первая попытка.
Однако пользователь этой функции будет защищать объект соединения с помощью блока USING.
Я действительно не люблю переводить исключения, как это делают другие ваши версии, поскольку очень трудно выяснить, почему это произошло (база данных отключена? У вас нет разрешения на чтение файла конфигурации и т. Д.?).
Не забудьте проверить OutOfMemoryExceptions... вы знаете, это может произойти.
Изменения Иана выглядят разумными для меня.
Если я использую систему и использую ее ненадлежащим образом, я хочу получить максимум информации о неправильном использовании. Например, если я забуду вставить некоторые значения в конфигурацию перед вызовом метода, я хочу InvalidOperationException с сообщением, подробно описывающим мою ошибку, а не KeyNotFoundException / NullReferenceException.
Это все о контексте ИМО. Я видел несколько довольно непроходимых сообщений об исключениях в свое время, но в других случаях исключение по умолчанию, исходящее из фреймворка, совершенно нормально.
В целом, я думаю, что следует проявлять осторожность, особенно когда вы пишете что-то, что интенсивно используется другими людьми или обычно глубоко в графе вызовов, где ошибку сложнее диагностировать.
Я всегда стараюсь помнить, что, как разработчик кода или системы, я в лучшем положении для диагностики сбоев, чем тот, кто его просто использует. Иногда бывает так, что несколько строк проверки кода + пользовательское сообщение об исключении могут в совокупности сэкономить часы отладки (а также упростить вашу собственную жизнь, так как вас не тянет за чужой компьютер для отладки их проблемы).