让代码审查快 10 倍的思维模型

围绕高效代码审查的思维模式展开深入探讨，经由聚焦风险来实现审查效率的提升，且对高效减轻项目风险有所助益。 原文：The Mental Model That Made Code Reviews 10x Faster。

不要追求完美，而是关注风险

以前进行代码审查，总能让我感到筋疲力尽。一个PR，需要花费30分钟，还会留下15条评论，然而却依旧觉得遗漏了某些重要问题。提交者愈发抵触审查，我也累到不行，完全没有效率可言。

后来，有一位身为技术主管的人，教给了我另外一种审查的思路，凭借一个问题，改变了所有的一切。

对于询问的内容，我不再是问有一段代码存在什么问题，而是去问在一个PR里面，风险程度占据了最高位置的地方究竟是什么。

突然间，代码审查的时长从原本的 30 分钟，缩短到了仅仅 10 分钟。评论数量变少了，然而每一条评论都精准地切中了关键要点。奇怪的是，提交代码的人开始对我的审查表达感谢之情，不再与我进行争论了。

完美审查的问题

以前，我老是想着去找出全部问题，那其中包括每一个风格方面的问题，每一项存在的潜在改进之处，以及每一个细微的优化点。审查清单一般呈现为这般模样：

你来猜猜最终的结果究竟是怎样的呢，提交者要不然就是对大部分的反馈选择忽视掉，要不然就是耗费几个小时去处理那些无关紧要的琐事，然而却把被我放在第8条评论里的实实在在的错误给遗漏掉了。

问题并非是我挑错了，而是在于我在审查期间，将所有的问题，都视作同等重要的存在。

事实上它们根本不一样。

基于风险的框架

这个思维模型呈现的是，每一个代码变更都存在着风险等级。你的工作在于，识别并且解决风险，而不是去追求完美之事。

致命风险 —— 很可能导致生产环境问题：

高风险 —— 可能导致生产环境问题：

中风险 —— 可能导致问题或技术债务：

低风险 —— 代码风格和微小改进：

我以前的审查全都是低风险评论，纯粹是吹毛求疵。

转化：我先是从那种会带来致命后果的风险以及高风险着手去看，到之后倘若寻得了这些问题，在将其解决之前，别的所有一切都不具备重要性。

3 分钟扫描法

现在，我会先做一个快速的扫描，这个扫描时长为3分钟，它专门用于寻找特定模式，而这些特定模式是暗示高风险的。

没有迁移的数据库变更：

// 红旗：模式更改，但没有迁移
class User {
  email: string;
  emailVerified: boolean;  // 新字段，迁移在哪里？
}

没有错误处理的外部 API 调用：

// 红旗：没有重试，没有超时，没有错误处理
async function syncUser(id) {
  const data = await externalAPI.getUser(id)
  return db.users.update(id, data)
}

认证/授权变更：

// 红旗：认证逻辑修改
if (user.role === 'admin' || user.isOwner) {  // 这里以前是'与'吗？
  allowAccess();
}

可能处理大量数据的数组操作：

// R红旗：没有分页，可能有数百万条记录
const users = await db.users.findAll();
return users.map(formatUser);

要是察觉到任何属于上述范畴之内的问题，那么审查便仅仅聚焦于这一个，而其他所有的问题都暂且搁置一旁，先不去管。

两评论规则

我只允许自己留两种评论：

1. 堵住评论，在合并之前，那必须得解决，即要是有两个用户同时去进行编辑，如此一来就会致使数据出现丢失的情况，而针对这种状况，我们是需要乐观锁的。

2. 只是用来供作参考而已，是需要知情了解的，然而却不能进行阻塞式合并，具体内容为：「FYI: 我们在 utils.js 里存在一个与之相类似的函数，其针对于空值的处理手段为有所不同的，这种情况是能够考虑将其进行对齐处理一下的。」。

不存在那种表述为「考虑一下」的评论，不存在那种表述为「也许」的评论，不存在那种表述为「你怎么看？」的评论。

要么重要到需要阻塞合并，要么就只是提供信息。

这使得我不得不具备清晰的立场，倘若我连为此而阻碍PR都不情愿，这事情果真有那般关键吗？

那个被漏掉的 bug 的故事

这个办法以往拯救过我们一回，那时我正审核一个关于支付处理的PR，代码改动了200多行，作者开展了诸多重构，诸如重命名变量、提取函数，存在好多能评论的地方。

放在以前，我会留下 20 条关于代码风格和组织的评论。

但现在，我做了 3 分钟扫描，发现了这个：

async function processRefund(orderId, amount) {
  const order = await getOrder(orderId)
  await stripe.refund(order.chargeId, amount)
  await db.orders.update(orderId, { status: 'refunded', refundedAmount: amount })
  return { success: true }
}

看上去没什么问题，是不是这样呢？然而风险处于这个地方：要是Stripe调用达成了成功的状态，可是数据库更新却出现了失败的情况该如何处理呢？

钱我们已然退了，然而数据库却并不知晓，客户拿到了款项，可我们却不存在记录，这便是致命的风险。

在对我的审查当中，存在着这样一条评论，那就是：「一旦 Stripe 退款成功之后，数据库更新却失败了，那么我们就会出现亏钱的情况。这种状况下，需要采用事务处理或者添加对账机制才行。」。

提交者把这个问题修复了，采用分布式事务进行包装，添加了幂等性，并且加了对账任务。

一条评论，就可能避免了数千美元的退款损失。

那个PR的其他所有方面，包括变量命名，函数提取，以及代码组织，根本不重要。

我现在不再评论的内容

我不再评论这些：

要是团队方面采用了Prettier或者ESLint，那么就应当去相信它们，不要充当人类linter，因为linter能够捕捉到的代码风格方面的问题。

对于从主观角度进行改进，那种表述为「在此段之中能够以更具函数式的方式来呈现」这样的话语，是完全不具备任何助益的。或者呢，存在着具体的问题，又或者根本就不存在问题。

缺乏数据予以支撑的性能优化若是存在，除非你有能力证明确实这不假存在着瓶颈，否则的话这样的优化全部都是过早时间段才出现的。

过度的抽象争论。 如果代码能用且清晰，抽象层级就没问题。

对于个人的喜好倾向，相比较于对象而言，有着一种更偏向于Map的情况，这是挺好的一种状况，然而要是其他人使用对象的话，那也完全不存在任何问题的。

放下这些，我的审查时间减半，而且审查结果有用多了。

「LGTM」的惊人力量

当 PR 没有重大风险时，我开始只用「LGTM」批准。

最开始的时候，我说这样是不正确的。可是难道我不需要去寻觅一些评论，以此来展现出我的价值所在吗？

但呈现出的结果是，PR合并的速度出现了变快的情况。作者能够以更快的速度获取到反馈。并且，当我切实地留下评论之际，他们开始坚信这些评论的确具有重要性。

上个月我给出的最棒的审查就是一句话：

「LGTM —— 实现干净，测试覆盖率好，没有明显风险。」

此PR在一小时之后便被合并了，不存在来回反复的拉扯情况，不存在毫无意义的争论情形，直接实现上线状态。

与之相较，一周之前，我于一个仿若 PR 的事物上留下了十二条小小的评论，然而，我们单单就变量名称展开争论，竟然耗费了三天时间才得以合并。

让一切聚焦的问题

在撰写任何评论以前，我都会去询问这样一个问题：「要是保持原本面貌直接上线，那么最糟糕的情况将会是怎样发生的呢？」。

如果答案是：

这就过滤掉了我以前会评论的 80% 内容。

什么时候需要彻底审查

有些时候还是需要慢下来，一丝不苟：

与安全紧密关联的代码，认证环节、支付流程、个人身份信息处理行为，这些均需进行深度性质的审查，我会去追踪每一条路径走向，检查每一处验证细节，确认每一个权限检查情况。

核心基础设施出现的变更，其中包括数据库schema的变更，还有API契约的变更，再加上部署配置的变更，这些变更影响于方方面面，此等情况值得进行仔细审查。

有的代码是新加入团队的成员编写的，并非是由于他们技术能力欠佳，是缘故他们尚未知悉我们曾经遇到过的那些棘手问题，此刻正是开展教学活动的时段。

变为难以理解的变更，要是你弄不明白其中的逻辑关系，这便是一种信号，或者去请求给予清楚的说明，或者让他们使其变得简单化。

难道是进行普通功能的开发吗？还是要对认证中间件予以重构呢？亦或是修复管理后台所存在的bug呀？要快速扫描风险，之后批准，然后继续下一个步骤。

引以为傲的一次审查

有人员提交了一个PR，其目的是运用WebSocket去添加实时更新，代码长度为500多行之多，在以往的时候呢，我会耗费一个小时的时间逐行去进行检查。

这一次，我用 3 分钟扫描聚焦于：

断开连接后要怎样去处理，服务器重启后会遭遇些什么，消息是否能保持顺序，我们会对所有内容进行持久化处理吗，故障模式是怎样的？

找到了两个致命问题：

要是服务器重新启动，客户端不会再次进行连接。它们就会始终停留在那儿展示过期数据。我们需要带有指数退避功能的自动重新连接。

首先，消息没被持久化到任何一处地方，其次，要是客户端消息抵达时处于不在线状态，那么他们便会永远都看不到，最后，我们是需要消息队列或者事件日志形式的东西。

这些均为阻塞方面的问题，其他的所有方面，也就是代码的结构，命名这一块儿，还有模式，都不存在问题。

审查花了 15 分钟，避免了两起生产事故。

模式识别游戏

做了几百次审查后，你会开始立刻识别风险模式：

碰了数据库之后，又碰了外部 API，这个样的操作之后，要进行检查事务边界以及故障处理。

「新增环境变量」 → 检查是否有文档，是否有合理的默认值。

「修改错误处理」 → 检查错误是否仍然被日志记录/监控。

对于「并发原语（锁、原子操作、通道）」，要进行检查，检查内容是死锁和竞态条件。

「日期/时间处理」 → 检查时区处理。

「删除代码」 → 检查是否真的没人用，会不会破坏功能。

「配置变更」 → 检查向后兼容性。

这种模式匹配使得审查进行得极为迅速，我并非要逐行去阅读，仅仅只需扫描那我晓得存在风险的模式。

好审查的标准

我最近做过的最好的代码审查都有这些共同点：

说实话，最好的审查往往就是一句：「看起来不错，上线吧。」

实践中的思维模型

我现在实际的审查流程是这样的：

1. 依据阅读描述，时长为1分钟，此操作究竟想达成怎样的目的，又为何要开展此项操作，该区域所具备的风险等级究竟是怎样的呢？

2. 扫描关键模式，时长为两分钟，涉及数据库变更，涉及API调用，涉及认证，涉及并发，涉及外部依赖？

3. 如需深入处理，时长为五至十分钟。若察觉风险，要透彻理解它。追踪代码路径，明晰各类故障模式。

4. 做出评论，或者进行批准，限定时间为1分钟，要么明确清晰地阐述理由来阻塞合并，要么批准使其能够继续进入下一个。

总计：根据复杂度和风险，每次审查 5-15 分钟。

相较于以前的我，每一次，时长在三十至四十五分钟之间，会留下十五条评论，其中大部都是并不关键的。

思维转变

思维转变就是：不要再追求代码完美，而是努力防止事故。

你的工作不是抓到所有问题，而是抓到重要问题。

代码审查不是为了代码质量，而是为了缓解风险。

当我明白这一点后，审查变得轻松多了，也有效多了。

现在，我开启 PR，仅会询问一个事项：“在此处，风险程度达到最大的是哪一项？”。

其他一切都是噪音。

本文由mdnice多平台发布

相关标签： # 代码审查 # 风险管理 # 效率提升 # 思维模型 # 风险识别