野塘漫水

代码评审实践 ​

代码评审（Code Review）在很多团队里会变成这样：

  • 两千行的 PR，评审者看了五分钟，回复“LGTM”；
  • 评论区里争论了二十条“这里要不要加空行”，一个越权漏洞却没人发现；
  • PR 提交三天了没人看，作者只好去催；
  • 评论写着“这样写不好”，作者不知道哪里不好，心里也不舒服。

评审真正的价值是：在合并之前，用第二双眼睛发现结构和风险上的问题，同时让知识在团队里流动。这篇讲怎么让评审发挥这个价值。

读完这篇你会

  • 知道评审应该重点看什么，以及按风险排序的检查清单
  • 写出具体、可执行、不伤人的评审意见
  • 作为作者，准备一个容易被认真评审的 PR

1. 原理：人做机器做不了的事 ​

评审者的时间和注意力是稀缺资源。能交给机器的，就不要让人来做：

交给机器交给人
格式、缩进、引号、导入顺序（Prettier）设计是否合理、职责是否清晰
未使用的变量、常见错误写法（ESLint）边界情况、失败路径是否处理
类型是否匹配（TypeScript）安全、并发、数据一致性风险
测试是否通过（CI）命名是否准确表达意图
依赖漏洞（依赖扫描）测试是否覆盖了关键行为

如果评审意见里大部分是格式问题，说明自动化工具还没配好，而不是评审做得好。

2. 按风险排序的检查清单 ​

评审时，按下面的顺序看，先看最可能造成严重后果的部分：

  1. 安全：外部输入是否校验？权限是否在服务端判断？有没有信任客户端传来的身份？有没有把敏感信息写进日志或返回给前端？
  2. 数据正确性：多步写入是否在事务里？并发时会不会超卖、重复创建？重试会不会重复执行？
  3. 失败路径：请求失败、超时、返回空数据时会发生什么？加载状态会不会卡住？错误有没有被吞掉？
  4. 兼容性：接口字段有没有删除或改名？数据库变更能否回滚？旧版本客户端还能用吗？
  5. 性能：有没有 N+1 查询、在循环里发请求、一次加载全部数据？
  6. 可读性和测试：命名是否准确？复杂逻辑有没有说明？关键行为有没有测试？

3. 坑一：PR 太大，评审只能走过场 ​

现象：一个 PR 改了四十个文件、两千多行，评审者只能粗略扫一眼就批准。上线后出了问题，回头一看，问题代码就在 PR 里，只是没人认真看。

原因：研究和经验都表明，单次评审超过 400 行左右，发现问题的效率就会急剧下降。

修复：

  • 一个 PR 只做一件事，控制在几百行以内；
  • 大功能拆成几个可以依次合并的小 PR：先合并数据结构和接口，再合并业务逻辑，最后合并界面；
  • 机械性的改动（重命名、格式化、移动文件）单独提一个 PR，并在描述里说明“只有机械改动，无逻辑变化”。

4. 坑二：评审变成了格式和风格之争 ​

现象：评论区大量讨论“单引号还是双引号”“这里要不要换行”，每个人的偏好不同，争论没有结果。

修复：格式问题交给格式化工具和 lint 规则，团队一次性约定好，写进配置，此后评审不再讨论格式。真正有分歧的风格问题，在团队会议上讨论后写进规范，而不是在每个 PR 里重新辩论。

5. 坑三：反馈模糊，或者让人不舒服 ​

现象：

text
❌ “这样写不好。”
❌ “为什么不用 X？”
❌ “你怎么又写了个并发 bug。”

作者不知道问题在哪，也不知道该怎么改，还觉得被针对了。

修复：好的评审意见说清楚问题、原因、影响和建议，对事不对人：

text
✅ “这里直接用 req.body.items.length，没有校验 items 是否为数组。
    如果客户端传入 null，这一行会抛出异常并返回 500。
    建议在入口处用 schema 校验，非法输入返回 400。”

用前缀标明意见的性质，作者就知道哪些必须处理：

前缀含义
必须有 bug 或风险，不修改不能合并
建议有更好的做法，作者可以决定是否采纳
疑问我没看懂，请解释一下（可能只需要补充注释）
小问题无关紧要的细节，改不改都行

看到写得好的地方，也说出来。评审不只是找错，强化好的做法同样重要。

6. 坑四：只看 diff，漏掉了 diff 之外的影响 ​

现象：一个函数的参数顺序被调整了，diff 里这个函数和调用它的那一处都改对了，评审通过。上线后另一个模块调用这个函数时出错了，那个调用点不在 diff 里，没人看到。

修复：

  • 改动涉及公共函数、接口、数据结构时，全局搜索所有使用它的地方，确认都被考虑到了；
  • 复杂的改动，把分支拉到本地实际运行一下，而不是只在网页上读 diff；
  • 作者在 PR 描述里说明“影响范围”，帮助评审者知道该去哪里看。

7. 坑五：评审拖延，成了交付瓶颈 ​

现象：PR 提交后两三天没人看，作者要么去催，要么开始做下一件事，回头再改时已经忘了上下文。

修复：

  • 团队约定响应时间，比如一个工作日内给出第一轮反馈；
  • PR 小了，评审自然就快了（见坑一）；
  • 每天固定一两个时间段处理评审，而不是攒到周五一起看。

8. 作者怎么准备一个好的 PR ​

评审的质量，一半取决于作者的准备：

text
标题：修复网络慢时重复提交导致订单重复创建

背景：用户在网络较慢时连续点击“提交”，会产生两笔相同的订单。

方案：
- 前端：提交期间禁用按钮，复用进行中的请求
- 后端：订单接口接收客户端生成的幂等键，并对其加唯一约束

影响范围：订单创建接口、下单页

验证方式：
- 新增并发测试：同一幂等键并发提交 20 次，只创建 1 笔订单
- 在 Slow 4G 下手动连点测试

请重点看：唯一约束冲突时的返回值处理（OrderService.create）

提交前，作者先自己评审一遍 diff，往往能发现忘删的调试代码、注释掉的旧逻辑和遗漏的边界情况。

9. 意见不一致时 ​

  1. 先确认理解：复述对方的方案和理由，很多分歧其实是误解；
  2. 讲权衡，不讲偏好：说清两种方案各自的代价，而不是“我觉得这样更好”；
  3. 用事实说话：有争议时写个小实验、跑一下数据；
  4. 及时升级：来回讨论三轮还没结论，找负责人或更有经验的同事当面决定；
  5. 接受合理的不完美：不涉及正确性和安全的分歧，尊重作者的选择，记录下决策原因。

检查清单 ​

评审者

  • [ ] 按安全、数据正确性、失败路径、兼容性、性能的顺序检查
  • [ ] 意见说明了问题、原因、影响和建议，并标明“必须”还是“建议”
  • [ ] 检查了 diff 之外受影响的调用点
  • [ ] 在约定时间内给出反馈

作者

  • [ ] PR 只做一件事，规模适中
  • [ ] 描述里写清背景、方案、影响范围、验证方式和重点
  • [ ] 提交前自己评审过一遍
  • [ ] 格式和 lint 问题已由工具处理

延伸阅读 ​

为复用而记录，为理解而整理。

MIT Licensed · Copyright © 2019–present qun ye