13

我目前正在进行代码审查，下面的代码让我跳了起来。我看到此代码存在多个问题。你是否同意我的观点？如果是这样，我该如何向我的同事解释这是错误的（顽固的类型......）？

  • 捕获一个通用异常（Exception ex）
  • 使用“if (ex is something)”而不是另一个 catch 块
  • 我们吃 SoapException、HttpException 和 WebException。但是如果 Web 服务失败了，就没有什么可做的了。

代码：

try
{
    // Call to a WebService
}
catch (Exception ex)
{
    if (ex is SoapException || ex is HttpException || ex is WebException)
    {
        // Log Error and eat it.
    }
    else
    {
        throw;
    }
}
4

8 回答 8

17

咒语是：

  • 只有在可以正确处理异常的情况下才应该捕获异常

因此：

  • 您不应该捕获一般异常。

在你的情况下，是的，你应该抓住这些异常并做一些有用的事情（可能不仅仅是吃掉它们——你可以throw在记录它们之后）。

您的编码器正在使用throw(not 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
}

这几乎等同于您的代码所做的。您的开发人员可能这样做是为了避免重复“记录错误并吃掉它”块。

于 2009-01-08T22:47:21.083 回答
7

我目前正在进行代码审查，下面的代码让我跳了起来。我看到此代码存在多个问题。你是否同意我的观点？

不完全是，见下文。

  • 捕获一个通用异常（Exception ex）

一般来说，捕获一个通用异常实际上是可以的，只要你在得出无法处理它的结论时重新抛出它（使用 throw;）。代码就是这样做的，所以这里没有直接的问题。

  • 使用“if (ex is something)”而不是另一个 catch 块

catch 块的最终效果是实际上只处理了 SoapException、HttpException 等，而所有其他异常都沿调用堆栈向上传播。我想功能方面这是代码应该做的，所以这里也没有问题。

但是，从美学和可读性 POV 来看，我更喜欢多个 catch 块而不是“if (ex is SoapException || ..)”。一旦您将通用处理代码重构为一个方法，多个 catch 块的类型只会稍微多一些，并且对于大多数开发人员来说更容易阅读。此外，最后的投掷很容易被忽视。

  • 我们吃 SoapException、HttpException 和 WebException。但是如果 Web 服务失败了，就没有什么可做的了。

这里可能潜伏着代码的最大问题，但如果不了解更多关于应用程序的性质，很难给出建议。如果 Web 服务调用正在执行您稍后依赖的操作，那么仅记录并吃掉异常可能是错误的。通常，您让异常传播给调用者（通常在将其包装到例如 XyzWebServiceDownException 之后），甚至在重试 Web 服务调用几次之后（以便在存在虚假网络问题时更加健壮）。

于 2009-01-09T13:13:55.367 回答
6

捕获和重新抛出相同异常的问题在于，尽管 .NET 尽最大努力保持堆栈跟踪完好无损，但它总是最终会被修改，这会使跟踪异常实际来自何处变得更加困难（例如，异常行号可能看起来是重新抛出语句的行，而不是最初引发异常的行）。这里有很多关于捕获/重新抛出、过滤和不捕获之间的区别的更多信息。

当存在这样的重复逻辑时，您真正需要的是一个异常过滤器，以便您只捕获您感兴趣的异常类型。VB.NET 具有此功能，但不幸的是 C# 没有。假设的语法可能如下所示：

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

虽然你不能这样做，但我倾向于对公共代码使用 lambda 表达式（你可以delegate在 C# 2.0 中使用 a ），例如

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);
}
于 2009-01-08T23:40:26.797 回答
2

有时这是处理“捕获每个异常”场景的唯一方法，而不是实际捕获每个异常。

我认为有时，例如，低级框架/运行时代码需要确保没有异常逃逸。不幸的是，框架代码也无法知道线程执行的代码引发了哪些异常。

在这种情况下，可能的 catch 块可能如下所示：

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

   Log(ex);
}

然而，这里有三个重要的点：

  1. 这并不适用于所有情况。在代码审查中，我们对实际上有必要并因此允许的地方非常挑剔。
  2. ExceptionIsFatal() 方法确保我们不吃不应该被吞下的异常（ExecutionEngineException、OutOfMemoryException、ThreadAbortException 等）
  3. 被吞下的东西被仔细记录（event-log、log4net、YMMV）

通常，我完全赞成通过终止 CLR 让未捕获的异常简单地“崩溃”应用程序的做法。然而，尤其是在服务器应用程序中，这有时是不明智的。如果一个线程遇到一个被认为是非致命的问题，则没有理由将整个进程撕下来，杀死所有其他正在运行的请求（例如，WCF 以这种方式处理某些情况）。

于 2009-01-10T17:34:06.543 回答
2

我想在这里补充一下，因为我见过的几乎所有 java / C# 代码中的异常处理都是不正确的。即，对于忽略的异常，您最终会遇到非常难以调试的错误，或者同样糟糕的是，您会得到一个晦涩的异常，它什么也没告诉您，因为盲目地遵循“捕获（异常）是不好的”，事情只会更糟。

首先，了解异常是一种促进跨代码层返回错误信息的方式。现在，错误 1：一个层不仅仅是一个堆栈帧，一个层是具有明确职责的代码。如果您只是编写接口和实现代码只是因为，那么您有更好的东西需要修复。

如果层设计得很好并且有特定的职责，那么错误信息在冒泡时具有不同的含义。<-这是做什么的关键，没有普遍的规则。

因此，这意味着当发生异常时，您有 2 个选项，但您需要了解您在层中的位置：

A）如果你在一个层的中间，你只是一个内部的，通常是私有的，辅助函数并且出现了问题：不要担心，让调用者接收异常。它完全没问题，因为您没有业务上下文，并且 1）您没有忽略错误，并且 2）调用者是您的层的一部分，应该知道这可能会发生，但您现在可能没有上下文来处理它。

或者 ...

B）你是层的顶部边界，内部的立面。然后，如果你得到一个异常，默认应该是 CATCH ALL 并阻止任何特定的异常跨越到上层，这对调用者来说没有意义，或者更糟糕的是，你可能会改变并且调用者将依赖于一个实现细节，两者都会打破。

应用程序的强度是层之间的解耦级别。在这里，您将按照一般规则停止一切并使用一般异常重新抛出错误，将信息转换为对上层更有意义的错误。

规则：层的所有入口点都应使用 CATCH ALL 保护，并翻译或处理所有错误。现在这种“已处理”仅发生 1% 的时间，大多数情况下您只需要（或可以）以正确的抽象返回错误。

不，我确信这很难理解。真实示例->

我有一个运行一些模拟的包。这些模拟在文本脚本中。有一个包可以编译这些脚本，还有一个通用的 utils 包，它只读取文本文件，当然还有基本的 java RTL。UML 依赖是->

模拟器->编译器->utilsTextLoader->Java文件

1) 如果某个私有内部的 utils 加载器出现问题，并且我得到 FileNotFound、Permissions 或其他任何内容，那就让它通过吧。您无能为力。

2) 在边界处，在最初调用的 utilsTextLoader 函数中，您将遵循上述规则和 CATCH_ALL。编译器不关心发生了什么，它只需要现在是否加载了文件。因此，在捕获中，重新抛出一个新异常并将 FileNotFound 或其他任何内容转换为“无法读取文件 XXXX”。

3) 编译器现在将知道源没有加载。这就是它需要知道的一切。因此，如果我稍后将 utilsTestLoader 更改为从网络加载，编译器将不会更改。如果您放开 FileNotFound 并在以后进行更改，您将一无所获地影响编译器。

4) 循环重复：为文件调用底层的实际函数在得到异常时将不做任何事情。所以它让它上升。

5）当异常到达模拟器和编译器之间的层时，编译器再次 CATCHES_ALL，隐藏任何细节并抛出更具体的错误：“无法编译脚本 XXX”

6）最后再重复一次循环，调用编译器的模拟器函数就放手了。

7）最终的边界是用户。用户是一个 LAYER 并且都适用。主要有一个 catches_ALL 的尝试，最后只是创建一个漂亮的对话框或页面，并向用户“抛出”一个翻译错误。

所以用户看到了。

模拟器：致命错误无法启动模拟器

-编译器：无法编译脚本 FOO1

--TextLoader: 无法读取文件 foo1.scp

---trl：文件未找到

相比于：

a) 编译器：NullPointer Exception <-common case and a lost night debugging a file name typo

b）加载器：找不到文件<-我是否提到加载器加载了数百个脚本？

或者

c) 什么都没有发生，因为一切都被忽略了！！！

当然，这假设在每次重新抛出时您都没有忘记设置原因异常。

好吧，我的 2cts。这个简单的规则多次救了我的命……

- 啤酒

于 2013-01-10T01:53:28.143 回答
1

您通常仍应在全局处理程序中捕获通用异常（这是记录意外异常的理想场所），但除此之外，如前所述，您应该只在其他地方捕获特定的异常类型，如果您打算对它们做某事。catch 块应该显式地查找这些异常类型，而不是像您发布的代码那样。

于 2009-01-08T23:20:55.477 回答
0

在这种情况下，我认为这不是一件坏事，但我也在我的代码中做了类似的事情，异常可以安全地忽略被捕获，其余的被重新抛出。正如迈克尔的回应所指出的，将每个捕获作为单独的块可能会导致一些可读性问题，这些问题可以通过走这条路线来防止。

关于向你的同事指出这一点，我认为你很难说服他们这是错误的做事方式——如果他们很固执，更是如此——因为以其他方式做事可能会导致可读性问题. 由于此版本仍然抛出无法处理的通用异常，因此符合实践的精神。但是，如果代码符合以下内容：

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;
  }
}

那么我认为你会有更多的理由担心，因为通过在一般异常块中检查特定异常来为特定异常工作是没有意义的。

于 2009-01-09T13:31:13.273 回答
0

王子只是为了捕捉你可以处理的异常。比如，如果你知道如何处理findnotfound，你就捕捉到filenotfoundexception，否则，不要捕捉它，让它被扔到上层。

于 2013-01-16T07:00:06.840 回答