这段代码是防御性编程，还是不好的做法？ [关闭]

Posted 2023-03-30

【中文标题】这段代码是防御性编程，还是不好的做法？ [关闭]

【英文标题】：Is this code defensive programming, or bad practice? [closed]

【发布时间】：2014-03-07 00:46:06

【问题描述】：

我和我的同事就这段代码进行了辩论：

var y = null;
if (x.parent != null)
    y = x.parent.somefield;

我的观点是，在代码所在的地方，x.parent 不应该为空。当它为空时，我们有一个严重的问题，我想知道它！因此，不应该有空检查，让下游异常发生。

我的同事说这是防御性编程。并且 null 检查确保代码不会破坏应用程序。

我的问题是，这是防御性编程吗？还是不好的做法？

注意：关键不是谁对。我正在尝试从这个例子中学习。

【问题讨论】：

x.parent 为空时抛出异常并处理！

最好在问题被删除之前将其更改为基于较少意见的问题。

@failedprogramming 我想这样做，但我被告知它会在任何地方创建异常处理代码......我仍然认为这比隐藏问题更好。

如果您选择使用防御方法，我建议您将输入防御代码条件的案例记录到日志文件中。 （如果需要，限制这些日志记录）。知道您的代码没有真正按预期运行总是一件好事！因此将其记录到日志文件中（可能会进行节流以避免日志爆炸）。您输入此代码块的次数的度量计数器也很有用。

@MichaelFreidgeim 这个问题在代码审查中将是题外话，因为它缺少上下文。将来，请链接到帮助中心，并在推荐 Code Review 时使用表明帖子可能偏离主题的措辞。采取，“这可能是代码审查的主题。请在发布之前检查if it is on-topic 和how to post a good question。”

【参考方案1】：

有趣的问题。在我看来，是否包括检查是数据验证的好坏、数据来自哪里以及检查失败时会发生什么的问题。

“x.parent 不应该为空”是一个严肃的假设。你需要非常确定它才能安全起见，当然有经典的“它永远不会发生”......直到它发生，这就是为什么我认为回顾可能性很有趣。

我看到两件不同的事情需要考虑。

数据从何而来？

如果它来自同一个类中的另一个方法，或者来自某个相关类，由于您或多或少可以完全控制它，因此放松防御是合乎逻辑的，因为您可以合理地假设它不太可能开始时有错误的数据，或者如果发生这种情况，在调试/测试期间很容易尽早发现错误，甚至对其进行一些单元测试。

1234563由于您无法控制程序正在处理的内容：无论如何，在以任何方式使用它之前尽可能彻底地验证它，因为您会接触到可能导致问题的虚假/缺失/不完整/不正确/恶意信息路径。

当输入来自同一系统中的另一个层/层时，可能是中间情况。决定是否进行全面验证或认为它是理所当然的更加困难，因为它是另一个内部软件，但以后可能会被替换或独立修改。我更倾向于在跨越边界时再次验证。

如何处理验证？

在某些情况下，使用if（如您的示例）简单地跳过某些分配或使用可能没问题。例如，如果用户正在输入一些数据，而这只是显示一个工具提示或其他次要信息，那么跳过它可能是安全的。但是，如果这段代码做了一些重要的事情，进而满足了一些强制性条件或执行了一些其他过程，那么这不是正确的方法，因为它会导致下一次代码运行出现问题。问题是，当你跳过一些代码时，必须这样做是安全的，没有任何副作用或不想要的后果，否则你会隐藏一些错误，而且以后很难调试发展阶段。

优雅地中止当前进程是早期验证的一个不错选择，此时完全可以预料到失败并且您确切地知道如何响应它。一个示例可能是缺少必填字段，该过程被中断，并向用户显示一条消息，询问缺少的信息。简单地忽略错误是不行的，而且还不够严重到抛出破坏正常程序流程的异常。当然，您仍然可以使用异常，具体取决于您的架构，但无论如何都要捕获它并优雅地恢复。

当“不可能”真正发生时，抛出异常始终是一种选择。在这些情况下，您可能无法为继续进行某些变化或仅取消当前流程提供合理的响应，这可能是由于某处的错误或输入错误，但重要的是您想知道它并拥有关于它的所有细节，所以最好的方法是让它尽可能大声地爆炸，以便异常冒泡并到达一个中断所有内容的全局处理程序，保存到日志文件/DB/whatever，发送一个向您报告崩溃并找到恢复执行的方法（如果可行或安全）。至少如果您的应用崩溃了，请以最优雅的方式进行，并留下痕迹以供进一步分析。

与往常一样，这取决于具体情况。但仅仅使用 if 来避免编写异常处理程序肯定是一种不好的做法。它必须始终存在，然后一些代码可能会依赖它——无论是否合适——如果失败不是关键的话。

【讨论】：

数据从何而来案例3。这个答案很有用。

【参考方案2】：

您的同事似乎误解了“防御性编程”和/或例外情况。

防御性编程

防御性编程是关于防止某些类型的错误。

在这种情况下x.parent == null 是一个错误，因为您的方法需要使用x.parent.SomeField。如果parent 为空，那么SomeField 的值显然是无效的。使用无效值执行的任何计算或任务都会产生错误和不可预测的结果。

因此，您需要防范这种可能性。如果您发现x.parent == null，则抛出NullPointerException 是一种非常好的保护方法。该异常将阻止您从SomeField 获取无效值。它会阻止您使用无效值进行任何计算或执行任何任务。并且它将中止所有当前工作，直到错误得到适当解决。

注意异常是不是错误； parent 中的无效值是 实际 错误。异常实际上是一种保护机制。异常是一种防御性编程技术，它们是不可避免的。

由于 C# 已经抛出异常，因此您实际上不必执行任何操作。事实上，您的同事“以防御性编程的名义”所做的努力，实际上撤消语言提供的内置防御性编程。

例外情况

我注意到许多程序员对异常过度偏执。异常本身并不是错误，它只是报告错误。

您的同事说：“null 检查确保代码不会破坏应用程序”。这表明他认为异常会破坏应用程序。它们通常不会“破坏”整个应用程序。

异常可能如果糟糕的异常处理使应用程序处于不一致的状态，则会破坏应用程序。 （但如果错误被隐藏，这种情况更有可能发生。）如果异常“逃脱”线程，它们也可能破坏应用程序。 （转义主线程显然意味着你的程序已经相当不优雅地终止了。但即使转义子线程也已经够糟糕了，操作系统的最佳选择是 GPF 应用程序。）

但是，异常会中断（中止）当前操作。这是他们必须做的事情。因为如果你编写一个名为DoSomething 的方法，它调用DoStep1； DoStep1 中的 错误 意味着 DoSomething 无法正确地完成其工作。继续拨打DoStep2 毫无意义。

但是，如果在某个时候您可以完全解决特定错误，那么请务必：这样做。但请注意强调“完全解决”；这并不意味着只是隐藏错误。此外，仅仅记录错误通常不足以解决它。这意味着要达到这样的程度：如果另一个方法调用您的方法并正确使用它，“已解决的错误”不会对调用者正确完成其工作的能力产生负面影响。 （不管调用者是什么。）

也许完全解决错误的最佳示例是在应用程序的主处理循环中。它的工作是：等待队列中的一条消息，从队列中拉出下一条消息并调用适当的代码来处理该消息。如果在返回主消息循环之前引发了异常并且未解决，则需要解决它。否则异常会逃出主线程，应用程序会被终止。

许多语言在其标准框架中提供了默认的异常处理程序（具有程序员覆盖/拦截它的机制）。默认处理程序通常只会向用户显示一条错误消息，然后吞下异常。

为什么？因为如果您没有实现糟糕的异常处理，您的程序将处于一致状态。当前消息已中止，可以处理下一条消息，就好像没有任何问题一样。你当然可以覆盖这个处理程序：

写入日志文件。 发送调用堆栈信息以进行故障排除。 忽略某些类别的异常。 （例如Abort 可能意味着您甚至不需要告诉用户，可能是因为您之前显示了一条消息。） 等

异常处理

如果您可以在不首先引发异常的情况下解决错误，那么这样做会更干净。但是，有时错误无法在首次出现的地方解决，或者无法提前检测到。在这些情况下，应该引发/抛出异常以报告错误，并通过实现异常处理程序（C# 中的 catch 块）来解决它。

注意：异常处理程序有两个不同的目的：首先，它们为您提供执行清理（或回滚代码）的地方，特别是因为存在错误/异常.其次，它们提供了解决错误和吞下异常的地方。 注意：在前一种情况下，重新引发/抛出异常非常重要，因为它尚未解决。

在关于抛出异常并处理它的评论中，您说：“我想这样做，但我被告知它会在任何地方创建异常处理代码。”

这是另一个误解。根据前面的旁注，您只需要在以下情况下进行异常处理：

您可以解决错误，在这种情况下您就完成了。 或者您需要在哪里实现回滚代码。

这种担忧可能是由于有缺陷的因果分析造成的。您不需要因为抛出异常而回滚代码。引发异常的原因还有很多。回滚代码是必需的，因为如果发生错误，该方法需要执行清理。换句话说，在任何情况下都需要异常处理代码。这表明对过度异常处理的最佳防御是设计，以减少对错误进行清理的需要。

所以不要“不抛出异常”以避免过多的异常处理。我同意过多的异常处理是不好的（参见上面的设计考虑）。但是在应该回滚的时候不回滚要糟糕得多，因为你甚至不知道有一个错误。

【讨论】：

@Gabriel 虽然您试图改进答案是出于善意，但x.parent 肯定是不是的论点。因此，您建议的更改实际上会使错误产生误导。

优秀的答案！我100%同意。换句话说，抛出异常有助于开发人员知道他们何时传递了错误的输入。我经常喜欢考虑哪些代码（客户端、API 等）以及涉及哪些资源，以了解何时应该处理或冒泡异常。

【参考方案3】：

我根本不会称之为防御性编程——我会称之为“啦啦啦我听不见你”的编程:) 因为代码似乎有效地忽略了潜在的错误条件。

显然我们不知道您的代码接下来会发生什么，但由于您没有包含 else 子句，我只能假设您的代码即使在 x.parent 的情况下也会继续运行是实际上是null。

请记住，“不应该为空”和“绝对、肯定地保证永远不会为空”不一定是同一回事；因此，在这种情况下，当您尝试取消引用 y 时，可能会导致进一步的异常。

那么问题是 - 在您尝试解决的问题（“域”）的上下文中，什么更可接受，这取决于您打算稍后对 y 做什么。

如果y 在这段代码之后成为null 没有问题（假设您稍后对y!=null 进行防御性检查），那没关系 - 尽管个人我不喜欢那种风格 - 你最终会防御性地检查每一个取消引用，因为你永远无法确定你是否是一个远离崩溃的空引用......

如果代码后面的y 不能是null，因为这会导致异常或稍后丢失数据，那么当您知道不变量不正确。

【讨论】：

如果 y 为空，代码将继续执行。但结果是客户端将收到包含重要缺失信息的响应。它会破坏客户。我认为这是错误的。

我同意——因此是第二点——在这种情况下继续下去是个坏主意。所以真正的“防御”意图在这里失败了——这里正确的防御应该是快速失败，但有一个例外。

【参考方案4】：

简而言之，我想说这不是防御性编程。我同意那些认为这段代码隐藏系统错误而不是暴露和修复的人的观点。此代码违反了“快速失败”原则。

当且仅当 x.parent 是一个强制的非空属性时才成立（这从上下文中似乎很明显。） 但是，如果 x.parent 是一个可选属性（即可以合理地有一个空值）那么这段代码就可以根据你表达的业务逻辑。

我一直在考虑使用空值（0、“”、空对象）而不是需要大量无关 if 语句的空值。

【参考方案5】：

在考虑了这个问题几年后，我使用了以下“防御性”编程风格 - 我的方法 95% 都返回字符串作为成功/失败的结果。

我返回 string.Empty 表示成功，如果失败则返回信息文本字符串。在返回错误文本的同时，我将其写入日志。

public string Divide(int numA, int numB, out double division)

    division = 0;
    if (numB == 0)
        return Log.Write(Level.Error, "Divide - numB-[0] not valid.", numB);

    division = numA / numB;
    return string.Empty;

然后我用它：

private string Process(int numA, int numB)

    double division = 0;
    string result = string.Empty;
    if (!string.IsNullOrEmpty(result = Divide(numA, numB, out divide))
        return result;

    return SendResult(division);

当你有日志监控系统时，它让系统显示继续，但通知管理。

【讨论】：

“不返回错误代码”（或错误字符串）blogs.msdn.microsoft.com/kcwalina/2005/03/16/…

您应该阅读有关 Either 类的信息。这肯定会改进您的设计。