💡 Key Takeaways
- The First 30 Seconds: What the PR Description Tells You
- Size Matters: The 400-Line Rule
- The Business Logic Layer: Where Most Bugs Live
- Error Handling: The Difference Between Good and Great Code
三年前，我批准了一个拉取请求，导致我的公司在一个周末失去了 47,000 美元的收入。代码看起来没问题，测试也通过了，逻辑似乎也合理。但是我在处理我们订阅计费系统的时区转换时错过了一些微妙之处，某些地区的客户被两次收费。
💡 关键要点
- 前 30 秒：PR 描述告诉你的信息
- 大小很重要：400 行规则
- 业务逻辑层：大多数错误出现的地方
- 错误处理：优秀代码与普通代码的区别
那个事件彻底改变了我审查代码的方式。我是 Sarah Chen，过去十年里，我在三家不同的 SaaS 公司担任高级工程经理，每周审查平均 15-20 个拉取请求。这大约是我职业生涯中的 8,000 个 PR。我见过发布了错误的出色代码，也见过多年来在生产中运行完美的杂乱代码。我了解到，代码审查并不是寻找语法错误——你的 linter 会处理这一点。它是关于捕捉只有经验才能揭示的隐性问题。
这篇文章是我希望在开始时拥有的清单。它并不全面——没有任何清单可以做到——但它代表了我学会识别的模式，我训练自己提出的问题，以及那些拯救我的团队避免无数生产事故的思维模型。
前 30 秒：PR 描述告诉你的信息
在查看一行代码之前，我会花 30 秒阅读 PR 描述。这可能看起来有些反常，但我发现有 60% 问题 PR 会通过糟糕的沟通立即暴露出来。好的 PR 描述回答三个问题：什么改变了，为什么改变了，以及可能出现什么问题。
当我看到描述中写着“修复了 bug”或“更新了组件”时，我立即知道这次审查会花更长的时间。作者没有考虑到他们更改的影响，这意味着我需要为他们完成这项工作。相比之下，当我看到描述为“重构用户身份验证，使用 JWT 令牌替代会话 cookie。这在高峰时段减少了 40% 的数据库负载，但需要客户端处理令牌刷新。风险：现有的移动应用版本（< 2.3）需要重新身份验证，”我知道作者做了他们的功课。
我寻找描述中的具体指标。不模糊的改进，而是实际的数字。“提高性能”没有意义。“在 1000 个并发请求下，将用户配置文件端点的 API 响应时间从 340 毫秒减少到 180 毫秒”告诉我作者衡量了他们的工作并理解了影响。根据我的经验，在 PR 描述中包含指标的开发人员整体上编写更有思考的代码。
描述还应链接到相关上下文——票据、设计文档、讨论方法的 Slack 线程。如果我在孤立中审查 PR，而不理解更广泛的上下文，我将会错过很多东西。我曾经批准过一个“简单重构”，却不知情地破坏了关键集成，因为我不知道某个依赖关系未在任何地方记录，除了三个月前的电子邮件线程。
PR 描述中的红旗包括：根本没有描述、描述长度超过代码更改本身（通常表示过度设计）、描述中包含“正在进行中”或“草稿”，但 PR 被标记为准备好审查，以及描述中包含“我不确定这是否是正确的方法”（那么你为什么要我审查它？）等短语。
大小很重要：400 行规则
我有一个严格的规则：我不会深入审查超过 400 行实际代码更改的 PR（不包括生成的代码、package-lock 文件和测试夹具）。这不是随意的。思科和 SmartBear 的研究表明，超过 400 行后，代码审查的有效性会急剧下降，而我自己的经验也证实了这一点。在审查大约 200 行代码后，我的大脑开始忽略细节。到 400 行时，我基本上只是在浏览。
“代码审查并不是寻找语法错误——你的 linter 会处理这一点。它是关于捕捉只有经验才能揭示的隐性问题。”
当有人提交一个 1,200 行的 PR 时，我会要求他们拆分开来。我不在乎它是否“相关”。大型 PR 是隐藏错误的地方。我见过关键的安全漏洞在巨大的重构 PR 中溜走，因为审查者感到疲倦，停止关注大约 600 行时。漏洞位于第 847 行。
当然也有例外。有时你需要移动文件，或者更新生成的代码，或者进行大规模更改以满足新的 lint 标准。在这些情况下，我会要求作者将机械更改与逻辑更改分开。将文件移动的部分提交为一个 PR，而实际逻辑更改的部分提交为另一个 PR。这使得两个 PR 更容易审查，也更容易在出现问题时回滚。
我还关注新增代码与删除代码的比例。根据我的经验，最佳 PR 的净行数是负的——在缩小代码库的同时实现了一些功能。当我看到一个 PR 添加了 500 行而删除了 50 行时，我感到怀疑。我们是在增加复杂性吗？我们是在重复现有功能吗？作者是否知道代码库中已经有哪些内容？
大小问题同样适用于单个文件。如果一个 PR 涉及 30 个不同的文件，即使总行数合理，这也是一个红旗。这表明更改要么范围不当，要么代码库存在耦合问题。无论哪种方式，这都值得额外的审查。
业务逻辑层：大多数错误出现的地方
经过十年的代码审查，我可以告诉你错误藏在哪里：业务逻辑层，特别是在边缘情况和状态转换中。不是在 UI 组件中，也不是在数据库查询中，而是在实现实际业务规则的代码中。
| PR 描述质量 | 它所说的 | 审查时间 | 风险等级 |
|---|---|---|---|
| 差 | “修复了 bug”或“更新了组件” | 2-3 倍长 | 高 |
| 一般 | 基本的什么和为什么，没有风险分析 | 正常 | 中等 |
| 好 | 清楚的什么、为什么和潜在风险的识别 | 高效 | 低 |
| 优秀 | 全面的上下文，讨论了权衡，注意了边缘情况 | 快速 | 非常低 |
当我审查业务逻辑时，我在寻找假设。每一个假设都是一个潜在的错误。我曾审查过一个折扣计算系统的代码，该系统假设所有折扣都是百分比。代码完美运行，直到市场部门想要提供“减免 10 美元”的促销。系统崩溃了，因为有人用折扣金额进行了除法，而在期待0到1之间的数字时用10进行除法会导致错误的结果。
我问自己：边界上会发生什么？如果输入为零会怎样？如果是负数呢？如果是空值呢？如果是空字符串与 null 字符串呢？如果数组为空呢？如果用户没有权限会怎样？他们有所有权限会怎样？如果数据库没有返回结果会怎样？如果返回一百万个结果呢？
我在业务逻辑中寻找魔法数字。当我看到if (user.loginAttempts > 5)时，我会问：为什么是5？这是否有文档记录？可以配置吗？如果我们想为某些用户类型将其更改为3，会发生什么？业务逻辑中的魔法数字是潜在的技术债务。
状态机是另一个常见的错误源。当我看到管理状态转换的代码——订单状态、用户生命周期、工作流阶段——我会在脑海中画出状态图。所有转换是否都考虑到了？你能进入一个无效的状态吗？我曾经发现一个错误，用户可以同时处于“活动”和“暂停”状态，因为设置一个状态的代码没有检查另一个状态。
我还观察到分散在多个层中的业务逻辑。如果我在 UI 中看到验证，在 API 中看到了更多的验证，而在数据库层中又看到更多的验证，我知道我们会有不一致性。业务规则应该生活在一个地方，而那个地方应该是显而易见的。