我在2000个Pull Request后建立的代码审查检查清单

我对18个月里留下的2147个PR评论进行了分类。34%是关于错误处理的。22%是关于命名的。只有8%是关于性能的。这不是我在开始记录我的审查时所期望的。我以为我会捕捉到架构问题和复杂的bug。相反，我一次又一次地指出同样的基本问题：缺少空检查、模糊的变量名以及没有提供有用信息的错误消息。在看到相同的问题出现在生产事故中后，我意识到这些不是微不足道的挑剔——它们是优雅失败的系统和在凌晨3点把你叫醒的系统之间的区别。

为何大多数代码审查检查清单没有抓住重点

每个人都告诉你检查代码风格、测试覆盖率和文档。这很好，但这不是生产崩溃的地方。我看到的在工程博客上分享的检查清单专注于容易测量的方面，而不是实际重要的方面。它们会告诉你验证函数是否在50行以内，但不会告诉你检查错误处理是否真的有助于某人在凌晨2点调试问题，当日志是你唯一的朋友时。

最好的代码审查不仅能防止bug——它们能防止引发事件的那类bug。缺少一个空检查不仅是潜在崩溃；当崩溃发生在事务中时，它可能导致数据损坏事件。

我开始记录我的PR评论是因为我感到沮丧。我会审查代码，批准它，然后看到同一个开发者在下一个PR中犯同样的错误。反馈没有被吸收。因此，我建立了一张电子表格。我留下的每条评论都被分类：错误处理、命名、测试、安全、性能、架构或其他。经过六个月，我收集的数据足以看出模式。经过十八个月，这些模式已经转化为真正有效的检查清单。

令人惊讶的不仅仅是排名最高的项目——还有那些没有登上榜单的项目。性能优化评论在我的审查中只占8%。安全问题占6%。这些是我们在架构讨论中所 obsess 的事情，但在日常PR中，它们很少见。什么是常见的？开发人员假设顺利路径、命名不当，以及为自己而不是为午夜调试的人编写错误消息。

不断破坏生产的错误处理模式

这是我对错误处理问题的编号列表，按实际引发事件的频率排序：

1. 后台作业中的静默失败。 代码捕获一个异常，记录它，然后继续。看起来合情合理，直到你意识到该作业本该发送付款确认邮件。现在你的客户认为他们没有被收费，但实际上他们被收费了。我在40%的后台作业PR中看到了这种模式。解决方案很简单：如果操作是关键的，不要捕获异常——让它上升并触发警报。如果它不是关键的，请记录为何静默失败是可以的。
2. 隐藏上下文的通用错误消息。 “发生错误”告诉我什么也没有。“处理付款失败”更好，但仍然无用。“未能收取以4242结尾的卡：资金不足（错误代码：card_declined）”确实有帮助。我在大约30%的PR中标记了这一点。我使用的测试是：如果你在凌晨3点的生产日志中看到此错误，能否在不增加更多日志和重新部署的情况下诊断出它？
3. 捕获异常而不是特定异常。 这是有争议的，因为一些风格指南推荐这样做，但我见过它隐藏bug太多次。当你捕获异常时，你也捕获了 NullPointerException、IllegalStateException 和所有其他指示程序员错误而非运行时条件的异常。捕获你预期要处理的。让程序员错误崩溃——这就是你发现它们的方式。
4. 未在边界验证外部数据。 API响应、用户输入、文件内容——如果它来自你的过程外部，请立即验证。我看到开发者在业务逻辑层进行验证，这意味着无效数据已经传播通过几个函数。到那时，你正在调试为何空值会进入你的数据库。在边界处进行验证，快速失败，返回清晰的错误。
5. 没有指数退避的重试逻辑。 服务停止，因此你立即重试。它仍然停止，因此你再次立即重试。恭喜，你已经通过不断重试使服务降级变成了完全故障。我在15%的添加重试逻辑的PR中看到了这种情况。始终使用指数退避和抖动。始终设定最大重试次数。始终考虑重试是否对这个操作有意义。
6. 未处理批量操作中的部分失败。 你正在处理1000条记录。第500条记录失败。第501-1000条记录会发生什么？在我审查的大多数代码中，它们从未被处理。批次失败，重试，在第500条记录时再次失败，然后你就被卡住了。明确处理部分失败：跟踪成功的、失败的及其原因。让你的批量操作能够恢复。
7. 假设数据库事务总是会成功。 你启动一个事务，做一些工作，然后提交。但如果提交失败呢？如果你在事务中丢失数据库连接呢？在25%的涉及数据库代码的PR中，我看到代码没有处理这个问题。结果是：你的应用程序状态和数据库状态不一致，你花费数小时调试为何数据与日志显示的不同。

一个变量名导致5万美元事故的那一天

那是一个星期二的早晨，我收到了Slack消息：“我们刚刚向错误的客户退款5万美元。”我打开事故频道开始阅读。一位开发者在前一晚上对我们的退款处理系统进行了修复。修复只是一行代码。PR得到了两位高级工程师的批准。测试通过。所有看起来都很好。

错误出在一个变量名上。原始代码有一个叫做`refundAmount`的变量，表示以分为单位的退款金额。开发者添加了一个名为`refundAmount`的新变量，表示以美元为单位的金额。他们忘记重命名原始变量。代码编译得很好——它们都是整数。测试通过，因为测试数据恰好使用了金额，其中分和美元的接近程度没有让断言捕捉到。

在生产中，我们在那天早上处理了200笔退款。其中一半金额错误。一个10.00美元的退款变成了1000美元的退款。一个5.00美元的退款变成了500美元的退款。当有人注意到时，我们已经多支付了5万美元。我们不得不手动审核每一笔退款，联系客户，并在某些情况下提出退款。清理工作花了三天。

根本原因不是开发者的错误——每个人都会犯错。问题是我们有两个同名变量表示不同单位且在同一作用域中。代码审查没有发现这个问题，因为审查人员专注于逻辑，而不是命名。在那次事故之后，我在我的检查清单中添加了一条规则：如果一个变量表示带单位的数量，则单位必须在名称中。不是`amount`，而是`amountCents`或`amountDollars`。不是`duration`，而是`durationSeconds`或`durationMilliseconds`。

这看起来似乎是多此一举。