Это плохая практика для перехвата неспецифического исключения, такого как System.Exception? Почему?

Мантра такая:

• Вы должны ловить исключения, только если вы можете правильно с ними справиться

Таким образом:

• Вы не должны ловить генерала исключения.

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

Ваш кодировщик использует throw (не throw ex), который является хорошо.

Вот как вы можете поймать несколько конкретных исключений:

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    // Log Error and eat it
}
catch (HttpException ex)
{
    // Log Error and eat it
}
catch (WebException ex)
{
    // Log Error and eat it
}

Это в значительной степени эквивалентно тому, что делает ваш код. Ваш разработчик, вероятно, сделал это таким образом, чтобы избежать дублирования блоков «log error and eat it».

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

Проблема с перехватом и повторным генерированием одного и того же исключения заключается в том, что, хотя .NET делает все возможное, чтобы сохранить трассировку стека неповрежденной, она всегда заканчивается модификацией, что может затруднить отслеживание того, откуда действительно произошло исключение (например, исключение номер строки, скорее всего, будет являться строкой оператора re-throw, а не строкой, в которой изначально возникло исключение). Здесь есть масса дополнительной информации о разнице между захватом / повторным выбросом, фильтрацией и отсутствием захвата.

Когда есть повторяющаяся логика, вам действительно нужен фильтр исключений, чтобы вы могли ловить только те типы исключений, которые вам интересны. VB.NET имеет эту функциональность, но, к сожалению, C# нет. Гипотетический синтаксис может выглядеть так:

try
{
    // Call to a WebService
}
catch (Exception ex) if (ex is SoapException || ex is HttpException || /* etc. */)
{
    // Log Error and eat it
}

Поскольку вы не можете этого сделать, я предпочитаю использовать лямбда-выражение для общего кода (вы можете использовать delegate в C# 2.0), например.

Action<Exception> logAndEat = ex => 
    {  
        // Log Error and eat it
    };

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    logAndEat(ex);
}
catch (HttpException ex)
{
    logAndEat(ex);
}
catch (WebException ex)
{
    logAndEat(ex);
}

I am currently doing a code review and the following code made me jump. I see multiple issues with this code. Do you agree with me?

Не совсем, см. Ниже.

• Catch a generic exception (Exception ex)

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

• The use of "if (ex is something)" instead of having another catch block

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

Однако с точки зрения эстетики и удобочитаемости я бы предпочел несколько блоков catch вместо «if (ex is SoapException || ..)». После рефакторинга кода общей обработки в метод, несколько блоков catch лишь немного сложнее набирать текст и их легче читать для большинства разработчиков. Кроме того, финальный бросок легко не заметить.

• We eat SoapException, HttpException and WebException. But if the Web Service failed, there not much to do.

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

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

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

try
{
  // Do some work
}
catch (Exception ex)
{
  if (ex is SoapException)
  {
    // SoapException specific recovery actions
  }
  else if (ex is HttpException)
  {
    // HttpException specific recovery actions
  }
  else if (ex is WebException)
  {
    // WebException specific recoery actions
  }
  else
  {
    throw;
  }
}

Тогда, я думаю, у вас будет немного больше причин для беспокойства, поскольку нет смысла выполнять работу для конкретного исключения, проверяя его в общем блоке исключений.

Иногда это единственный способ обработать сценарии «перехвата всех исключений», фактически не перехватывая каждое исключение.

Я думаю, что иногда, скажем, низкоуровневый код фреймворка / времени выполнения должен гарантировать, что никакое исключение никогда не исчезнет. К сожалению, код фреймворка также не может знать, какие исключения вызываются кодом, выполняемым потоком.

В этом случае возможный блок catch может выглядеть так:

try
{
   // User code called here
}
catch (Exception ex)
{
   if (ExceptionIsFatal(ex))
     throw;

   Log(ex);
}

Однако здесь есть три важных момента:

1. Это не для каждой ситуации. В обзоре кода мы очень разборчивы в отношении мест, где это действительно необходимо и, следовательно, разрешено.
2. Метод ExceptionIsFatal () гарантирует, что мы не едим исключения, которые никогда не следует проглатывать (ExecutionEngineException, OutOfMemoryException, ThreadAbortException и т. д.)
3. То, что проглочено, тщательно регистрируется (журнал событий, log4net, YMMV).

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

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