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

1. Не допускается использование конструкций

try
{
…
}
catch
{
…
}

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

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

Примечание: фильтр исключений вида catch(Exception ex) или catch не является обработкой ожидаемого исключения!

3. При реализации собственного типа исключений обязательно реализовывать ВСЕ конструкторы родительского исключения.

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

5. Думать, когда использовать исключения, а когда нет. Например, при делении на ноль и выходе за границу массива. Т.е. если при выполнении операции, где возможно деление на 0 нужно смотреть какова вероятность того, что аргумент может быть 0 — если часто, то лучше проверить аргумент на равенство 0, иначе лучше использовать проверку на DivideByZeroException.

6. Запрещается использовать код следующего вида:

int SomeMethod(SomeData data)
{
try
{
…
    return 0;
}
catch
{
…
return -1;// or return false; or etc.
}
}

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

7. Там где это возможно, вместо блоков try{}finally{} использовать using(){}
Так, например, следующий код может не освобождать ресурсы, как это ожидалось:

FileStream stream1 = new FileStream(…);
FileStream stream2 = new FileStream(…); // 1
try
{
…
}
finally
{
stream1.Dispose(); // 2
stream2.Dispose();
}

В данном коде две ошибки:
а) При генерации исключения при создании stream2 – stream1 не будет закрыт, до сборки мусора.
б) При генерации исключения при закрытии stream1 – stream2 не будет закрыт, до сборки мусора.
Вместо этого было правильнее написать:

using(FileStream stream1 = new FileStream(…))
{
using(FileStream stream2 = new FileStream(…))
{
…
}
}

8. Все пользовательский исключения должны быть унаследованы от ApplicationException а не от Exception (Просто хороший стиль). Так же должен обязательно быть определен атрибут [Serializable].

9. Конструкторы типов-исключений не должны генерировать исключений.

10. При реализации фильтров исключений более «специализированные» исключения должны обрабатываться раньше, т.е. например, если есть исключение

class MyException1: Exception 
{
}

class MyException2: MyException1
{
}

То фильтр для них должен выглядеть следующим образом:

try
{
}catch(MyException2)
{
}
catch(MyException1)
{
}
catch(Exception)
{
}

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

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

13. Никогда не генерируйте исключения в методах Dispose и Close, даже если объект уже закрыт или очищен.

14. Если вы обрабатываете исключение, то верните приложение в согласованное состояние (т.е. восстановить значение переменных, смещение в потоках и.т.п)

15. Не перехватывайте специальные исключения OutOfMemoryException, StackOverflowException, ThreadAbortException и ExecutionEngineException или перехватывайте их только для регистрации. Если перехватываете какое-либо из этих исключений его нужно сгенерировать повторно.

Здравствуйте, Александер Малафеев, Вы писали:

АМ>Недавно мне дали разбираться с багами в одной програмулине и, читая код понял что народ который это писал совершенно не умеет работать с исключениями. Вот я решил написать несколько общих правил, было бы интересно послушать мнения, может я в чем неправ или кто нибуть подскажет что я еще забыл.
<skipped>

Поправочка по пункту 7. Лучше писать не:

using(FileStream stream1 = new FileStream(:))
{
    using(FileStream stream2 = new FileStream(:))
    {
        :
    }
}

а:

using(FileStream stream1 = new FileStream(:))
using(FileStream stream2 = new FileStream(:))
{
    :
}

Просто покрасивше будет. Рекомендация от MS, кажется...

Здравствуйте, Александер Малафеев, Вы писали:

АМ>2. Следует четко разделять логику и представление приложения. Все исключения сгенерированные в логическом слое должны передаваться наружу в исходном виде, либо обернуты в другое исключение, при этом исходное исключение обязательно должно передаваться в конструктор вновь генерируемого исключения.

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

АМ>10. При реализации фильтров исключений более «специализированные» исключения должны обрабатываться раньше, т.е. например, если есть исключение

По-другому и не скомпилируется.

Public — методы не должны бросать NullReferenceException и т.п. из-за неверных входных параметров. Вместо этого все входные параметры должны проходить валидацию.

Не следует писать try/catch/finally если в блоке catch есть только оператор throw. Лучше написать просто try/finally.

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

Здравствуйте, ForestLabs, Вы писали:

FL>Поправочка по пункту 7. Лучше писать не:

FL>

FL>using(FileStream stream1 = new FileStream(:))
FL>{
FL>    using(FileStream stream2 = new FileStream(:))
FL>    {
FL>        :
FL>    }
FL>}
FL>

FL>а:

FL>

FL>using(FileStream stream1 = new FileStream(:))
FL>using(FileStream stream2 = new FileStream(:))
FL>{
FL>    :
FL>}
FL>

FL>Просто покрасивше будет. Рекомендация от MS, кажется...
Тогда уж:

using(FileStream stream1 = new FileStream(...),
                 stream2 = new FileStream(...))
{
    ...
}

Здравствуйте, Александер Малафеев, Вы писали:

Можно еще ориетироваться на рекомендации от Microsoft.
Design Guidelines for Exceptions

Еще Exception Handling Best Practices in .NET

АМ>8. Все пользовательский исключения должны быть унаследованы от ApplicationException а не от Exception (Просто хороший стиль). Так же должен обязательно быть определен атрибут [Serializable].

В частности там Microsoft они рекомендует наследоваться от Exception (эта рекомендация недавно изменилась). FxCop показывает issue, если наследоваться от ApplicationException.

Здравствуйте, Александер Малафеев, Вы писали:

АМ>1. Не допускается использование конструкций

АМ>try {
АМ>  …
АМ>} catch {
АМ>  …
АМ>}

АМ>При таком фильтре блок catch должен содержать throw; или документировано, почему его нет.

+1, а то некоторые считают что в принципе так делать нельзя.

АМ>3. При реализации собственного типа исключений обязательно реализовывать ВСЕ конструкторы родительского исключения.

Пока не согласен. Можно узнать причины такого требования?

АМ>13. Никогда не генерируйте исключения в методах Dispose и Close, даже если объект уже закрыт или очищен.

А надо-ли перехватывать возможные исключения, которые могут возникнуть "внутри" этих методов? То есть оправдано ли использование перехвата исключение в этих методах? Если да, то в каком виде? try\catch? try\finally? Если try\catch, то какой должен быть фильтр?

АМ>14. Если вы обрабатываете исключение, то верните приложение в согласованное состояние (т.е. восстановить значение переменных, смещение в потоках и.т.п)

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

АМ>15. Не перехватывайте специальные исключения OutOfMemoryException, StackOverflowException, ThreadAbortException и ExecutionEngineException или перехватывайте их только для регистрации. Если перехватываете какое-либо из этих исключений его нужно сгенерировать повторно.

А разве все из них перехватываются?

Здравствуйте, Pavel Chikulaev, Вы писали:

FL>>Поправочка по пункту 7. Лучше писать не:

FL>>using(FileStream stream1 = new FileStream(:))
FL>>{
FL>>    using(FileStream stream2 = new FileStream(:))
FL>>    {
FL>>        :
FL>>    }
FL>>}

FL>>а:

FL>>using(FileStream stream1 = new FileStream(:))
FL>>using(FileStream stream2 = new FileStream(:))
FL>>{
FL>>    :
FL>>}

FL>>Просто покрасивше будет. Рекомендация от MS, кажется...
PC>Тогда уж:

PC>using(FileStream stream1 = new FileStream(...),
PC>                 stream2 = new FileStream(...))
PC>{
PC>    ...
PC>}

Ага, а потом помни, что если тип переменных один, то и юзинг один, а вот если тип разный, то и юзинги разные
Во-вторых, последний вариант заморочнее рефакторить, в отличии от второго

Здравствуйте, _FRED_, Вы писали:

_FR>Здравствуйте, Александер Малафеев, Вы писали:

АМ>>1. Не допускается использование конструкций
_FR>

АМ>>try {
АМ>>  …
АМ>>} catch {
АМ>>  …
АМ>>}
_FR>

АМ>>При таком фильтре блок catch должен содержать throw; или документировано, почему его нет.

_FR>+1, а то некоторые считают что в принципе так делать нельзя.

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


АМ>>3. При реализации собственного типа исключений обязательно реализовывать ВСЕ конструкторы родительского исключения.

_FR>Пока не согласен. Можно узнать причины такого требования?

Ну здесь немного погорячился имелось в виду реализовать все конструкторы класса Exception, т.е. те что нужны для сериализации, для скрытия исключения.
Но с другой стороны пока не придумал ни один случай, когда при грамотно cпроектированной иерархии типов исключений потребывалось бы скрыть конструктор, единственно, наверное, можно как нибуть так:

class MyException:Exception
{
   // какие -то конструкторы и методы
   public MyException(SomeType st)
   {
   }
}

class MyExceptionDrv:MyException
{
   // какие -то конструкторы и методы
   
   public MyException(SomeType st, AdditionType at):base(st)

   {
   }
}

Но и в этом случае я использую конструкт родительского типа.

Ну а вообще грамотнее было бы переписать этот пункт, как нибуть так:
При реализации собственного типа исключений обязательно реализовывать следующие конструкторы:
1. Конструктор по умолчанию.
2. Конструктор принимающий два параметра: SerializationInfo и StreamingContext
3. Конструктор, который принимает одним из параметров Exception и устанавливает свойство InnerException


АМ>>13. Никогда не генерируйте исключения в методах Dispose и Close, даже если объект уже закрыт или очищен.

_FR>А надо-ли перехватывать возможные исключения, которые могут возникнуть "внутри" этих методов? То есть оправдано ли использование перехвата исключение в этих методах? Если да, то в каком виде? try\catch? try\finally? Если try\catch, то какой должен быть фильтр?

Я считаю что перехват оправдан. И фильтр в идеале(если бы во фреймворке не было бы косяков) должен быть таким:

try
{
}
catch(ApplicationException)
{
}

Причем если в диспозе я освобождаю N ресурсов, то на каждый метод освобождения должен быть свой блок try\catch.

Оправдан он для того что бы не было косяков описанных в пункте 7. Т.е когда:

finally
{
    stream.Dispose();
    stream2.Dispose();
}

Ибо, не всегда можно использовать using, а писать кучу try\finaly как-то нехочется Да и к тому же вряд ли если например при закрытии потока вылетает Exception ты его грамотно сможешь обработать, т.к. в этих методах обычно вылетают уж очень специфические исключения.

АМ>>14. Если вы обрабатываете исключение, то верните приложение в согласованное состояние (т.е. восстановить значение переменных, смещение в потоках и.т.п)

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

Согласен, нужно переписать:

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


АМ>>15. Не перехватывайте специальные исключения OutOfMemoryException, StackOverflowException, ThreadAbortException и ExecutionEngineException или перехватывайте их только для регистрации. Если перехватываете какое-либо из этих исключений его нужно сгенерировать повторно.

_FR>А разве все из них перехватываются?
Да.
Фильтр cathc{} перехватывает даже не CLR исключения.

Здравствуйте, Александр Малафеев, Вы писали:

АМ>>>3. При реализации собственного типа исключений обязательно реализовывать ВСЕ конструкторы родительского исключения.
_FR>>Пока не согласен. Можно узнать причины такого требования?
…
АМ>Ну а вообще грамотнее было бы переписать этот пункт, как нибуть так:
АМ>При реализации собственного типа исключений обязательно реализовывать следующие конструкторы:
АМ>1. Конструктор по умолчанию.
АМ>2. Конструктор принимающий два параметра: SerializationInfo и StreamingContext
АМ>3. Конструктор, который принимает одним из параметров Exception и устанавливает свойство InnerException

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

АМ>>>15. Не перехватывайте специальные исключения OutOfMemoryException, StackOverflowException, ThreadAbortException и ExecutionEngineException или перехватывайте их только для регистрации. Если перехватываете какое-либо из этих исключений его нужно сгенерировать повторно.

_FR>>А разве все из них перехватываются?
АМ>Да.
АМ>Фильтр cathc{} перехватывает даже не CLR исключения.

Не CLR — возможно, но не StackOverflow, например:

namespace ConsoleApplication1
{
  using System.Diagnostics;

  class Program
  {
    static void OO() {
      OO();
    }

    static void Main(string[] args) {
      try {
        OO();
      } catch {
        Debug.Fail("OO!"); // Этот код не должен никогда выполниться
      }//try
    }
  }
}

Здравствуйте, _FRED_, Вы писали:

_FR>Здравствуйте, Александр Малафеев, Вы писали:

АМ>>>>3. При реализации собственного типа исключений обязательно реализовывать ВСЕ конструкторы родительского исключения.
_FR>>>Пока не согласен. Можно узнать причины такого требования?
_FR>…
АМ>>Ну а вообще грамотнее было бы переписать этот пункт, как нибуть так:
АМ>>При реализации собственного типа исключений обязательно реализовывать следующие конструкторы:
АМ>>1. Конструктор по умолчанию.
АМ>>2. Конструктор принимающий два параметра: SerializationInfo и StreamingContext
АМ>>3. Конструктор, который принимает одним из параметров Exception и устанавливает свойство InnerException

_FR>Имхо, только второй пункт может претендовать на безусловность, остальные могут не иметь смыска для какого-нить специфического sealed исключения

Несогласен, хотя и ни один пункт не претендует на безусловность — везде есть исключения!
Но все ситуации предусмотреть невозможно. Это то же самое что почти любому классу нужно ставить атрибут [Serializable], имхо это сейчас может не нужно его сеарилизовывать а потом...

Только вот не понятно причем тут sealed?



АМ>>>>15. Не перехватывайте специальные исключения OutOfMemoryException, StackOverflowException, ThreadAbortException и ExecutionEngineException или перехватывайте их только для регистрации. Если перехватываете какое-либо из этих исключений его нужно сгенерировать повторно.

_FR>>>А разве все из них перехватываются?
АМ>>Да.
АМ>>Фильтр cathc{} перехватывает даже не CLR исключения.

_FR>Не CLR — возможно, но не StackOverflow, например:
_FR>

_FR>namespace ConsoleApplication1
_FR>{
_FR>  using System.Diagnostics;

_FR>  class Program
_FR>  {
_FR>    static void OO() {
_FR>      OO();
_FR>    }

_FR>    static void Main(string[] args) {
_FR>      try {
_FR>        OO();
_FR>      } catch {
_FR>        Debug.Fail("OO!"); // Этот код не должен никогда выполниться
_FR>      }//try
_FR>    }
_FR>  }
_FR>}
_FR>

У меня выполняется

_FR>>Не CLR — возможно, но не StackOverflow, например:
_FR>>

_FR>>namespace ConsoleApplication1
_FR>>{
_FR>>  using System.Diagnostics;

_FR>>  class Program
_FR>>  {
_FR>>    static void OO() {
_FR>>      OO();
_FR>>    }

_FR>>    static void Main(string[] args) {
_FR>>      try {
_FR>>        OO();
_FR>>      } catch {
_FR>>        Debug.Fail("OO!"); // Этот код не должен никогда выполниться
_FR>>      }//try
_FR>>    }
_FR>>  }
_FR>>}
_FR>>

Хотя, да во второ фреймворке невыполняется.
Но все равно не нужно их перехватывать

Здравствуйте, Александр Малафеев, Вы писали:

АМ>>>1. Конструктор по умолчанию.
АМ>>>2. Конструктор принимающий два параметра: SerializationInfo и StreamingContext
АМ>>>3. Конструктор, который принимает одним из параметров Exception и устанавливает свойство InnerException
_FR>>Имхо, только второй пункт может претендовать на безусловность, остальные могут не иметь смыска для какого-нить специфического sealed исключения
АМ>Несогласен, хотя и ни один пункт не претендует на безусловность — везде есть исключения!
АМ>Но все ситуации предусмотреть невозможно. Это то же самое что почти любому классу нужно ставить атрибут [Serializable], имхо это сейчас может не нужно его сеарилизовывать а потом...

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

АМ>Только вот не понятно причем тут sealed?

При том, что если я не позволяю наследоваться от моего класса (то есть не позволяю изменить\дополнить его смысл) и подразумеваю только одно использование укземпляров, то зачем городить конструкторы, которые никто и не должен вызывать? Например, зачем нужен конструктор ArgumentNullException(string, Exception)? Только для возможности добавить логику в наследнике.

АМ>У меня выполняется

Фреймфорк не второй?

Здравствуйте, _FRED_, Вы писали:

_FR>Здравствуйте, Александр Малафеев, Вы писали:

АМ>>>>1. Конструктор по умолчанию.
АМ>>>>2. Конструктор принимающий два параметра: SerializationInfo и StreamingContext
АМ>>>>3. Конструктор, который принимает одним из параметров Exception и устанавливает свойство InnerException
_FR>>>Имхо, только второй пункт может претендовать на безусловность, остальные могут не иметь смыска для какого-нить специфического sealed исключения
АМ>>Несогласен, хотя и ни один пункт не претендует на безусловность — везде есть исключения!
АМ>>Но все ситуации предусмотреть невозможно. Это то же самое что почти любому классу нужно ставить атрибут [Serializable], имхо это сейчас может не нужно его сеарилизовывать а потом...

_FR>Без этого нельзя будет передать исключение между процессами, так что специфика исключения не важна и обеспечивать сериализуймость нужно.

АМ>>Только вот не понятно причем тут sealed?

_FR>При том, что если я не позволяю наследоваться от моего класса (то есть не позволяю изменить\дополнить его смысл) и подразумеваю только одно использование укземпляров, то зачем городить конструкторы, которые никто и не должен вызывать? Например, зачем нужен конструктор ArgumentNullException(string, Exception)? Только для возможности добавить логику в наследнике.

Ну что такое sealed я знаю Ну это он сейчас не нужен (хотя на самом деле и нужен, можно придумать кучу ситуаций где используется ArgumentNullException(string, Exception) ), а потом кто знает. И то что этого конструктора не было во 1 фреймворке — это баг о котором мелкомягкие сами и написали. Так что во 2м он уже появился.

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

Насчет 3го пункта:

protected Exception(
   SerializationInfo info,
   StreamingContext context
);

This constructor is called during deserialization to reconstitute the exception object transmitted over a stream. For more information, see XML and SOAP Serialization.

Здравствуйте, Александер Малафеев, Вы писали:

как-то видел такой код:

try
{
   bla-lbla-bla
}
catch( Exception ex )
{
   throw new Exception("ERROR");
}

Здравствуйте, Константин Л., Вы писали:

КЛ>Здравствуйте, Александер Малафеев, Вы писали:

КЛ>как-то видел такой код:

КЛ>

КЛ>try
КЛ>{
КЛ>   bla-lbla-bla
КЛ>}
КЛ>catch( Exception ex )
КЛ>{
КЛ>   throw new Exception("ERROR");
КЛ>}

КЛ>

КЛ>

Я в одном проекте похожее видел:

try
{
...
}
catch(Exception ex)
{
   throw new Exception(ex.Message);
}

Здравствуйте, Александер Малафеев, Вы писали:

АМ>15. Не перехватывайте специальные исключения ThreadAbortException

Это почему? Вроде стандартный способ тормознуть поток...

Здравствуйте, adontz, Вы писали:

A>Здравствуйте, Александер Малафеев, Вы писали:

АМ>>15. Не перехватывайте специальные исключения ThreadAbortException

A>Это почему? Вроде стандартный способ тормознуть поток...

Здесь имелось в виду в главном потоке, согласен пункт надо дописать.

Здравствуйте, Александр Малафеев, Вы писали:

АМ>Здравствуйте, adontz, Вы писали:

A>>Здравствуйте, Александер Малафеев, Вы писали:

АМ>>>15. Не перехватывайте специальные исключения ThreadAbortException

A>>Это почему? Вроде стандартный способ тормознуть поток...

АМ>Здесь имелось в виду в главном потоке, согласен пункт надо дописать.

Можно еще ловить это исключение, чтобы отменить аборт потока через Thread.ResetAbort.

MSDN.

ThreadAbortException is a special exception that can be caught, but it will automatically be raised again at the end of the catch block. When this exception is raised, the runtime executes all the finally blocks before ending the thread. Since the thread can do an unbounded computation in the finally blocks, or call Thread.ResetAbort to cancel the abort, there is no guarantee that the thread will ever end.

В ASP.NET при вызове HttpResponse.Redirect Method (String, Boolean) бросается это же самое исключение:

Redirect calls End which raises a ThreadAbortException exception upon completion.

То есть иногда ловить это ислючение очень даже полезно