GitHub 工程师揭底：代码审查常犯这 5 个错，难怪你改到崩溃！网友：差点全中了

以下文章来源于算法爱好者，作者小蒜

【导读】：最近 GitHub 高级工程师 Sean Goedecke 新分享的一篇文章《Mistakes I see engineers making in their code reviews》在 HN 引发热议。

他在文中结合当下 AI 生成代码增多、审查难度加大的背景，指出工程师审代码常犯的 5 个错误，如只盯 diff、留太多意见等，还给出“优先通过常规 PR”“AI 生成代码需严审” 等实用建议，帮大家避开误区，提升审查效率。

工程师在代码审查中常犯的 5 个错误

过去两年里，代码审查的重要性大幅提升。现在用大语言模型（LLMs）生成代码很容易，但审查代码的难度丝毫没减。许多软件工程师花在审查自己 AI 工具输出上的时间，已经和审查同事代码的时间持平，甚至更久。

我发现很多工程师的代码审查方式并不恰当。当然，代码审查的方法有很多种，所以我的观点很大程度上只是基于个人的工程偏好。

别只审查代码差异（diff）

我见过的最大错误，就是审查时只盯着代码差异（diff）。实际上，大部分影响力最大的审查意见，都和当前的 diff 没太大关系，而是来自你对系统其他部分的理解。

比如，最实用的审查意见之一可能是“你不用在这儿加这个方法，另一个地方已经有了”。单看 diff 本身，根本不可能得出这样的结论。你必须提前熟悉代码库中那些提交者可能不知道的部分。

同样，“这段代码或许应该放在另一个文件里”这类意见，对维护代码库的长期质量也很有帮助。在大型代码库中工作，最重要的原则就是一致性。显然，只看 diff 是无法判断一致性的。

只审查 diff 本身，比思考它在整个代码库中的适配性要容易得多。你可以快速扫一遍 diff，然后留下逐行意见，比如“重命名这个变量”或“这个函数的逻辑流程应该调整”。这些意见或许有价值，但只做这种审查，会错过很多更重要的东西。

别留太多审查意见

在代码审查这件事上，我最有争议的一个观点是：一次好的审查，意见不应超过五六条。大多数工程师都会留太多意见。如果收到一份有上百条意见的审查结果，你基本只能关注那些无关紧要的细节，真正重要的意见反而会被淹没在一堆信息里。

要是 diff 里有 20 处地方你希望修改（比如二十个用了camelCase的变量，而团队规范是snake_case）该怎么办？与其留 20 条意见，不如只留 1 条，说明你希望调整的风格规范，然后让提交者自己去逐行修改。

不过这个原则有个例外：当团队有新成员加入时，多留一些风格相关的意见可能有帮助，能帮他们快速熟悉团队在代码库中使用的特定规范。

但即便如此，也要注意：你留下的那些“真正重要”的意见，很可能会被这些风格意见淹没。所以，与其在 diff 里每处“提前返回”（early return）都留一条意见，不如直接留一条总括性的“咱们代码库不允许提前返回”，效果可能更好。

别用“我会怎么写”的标准来审查

工程师留太多意见的原因之一，是用了这样的审查逻辑：

看 diff 里的一段代码；

问自己“如果是我写这段代码，会怎么写？”；

只要实际代码和自己的写法有差异，就留一条意见。

用这种方式审查，很容易让一个拉取请求（pull request）堆上百条意见：比如“我会把这两个操作的顺序换一下”、“我会把这个函数的结构改得不一样”，诸如此类。

我不是说这些细节意见全是错的。有时候操作顺序确实很重要，函数结构也可能真的有问题。但我对软件工程有个很坚定的看法：任何软件问题，都有多种可行的解决方案，选择哪一种往往取决于个人偏好。

作为审查者，当你遇到“换种写法也能行”的情况时，只要两种写法都合理，就应该不加意见直接通过。

否则会让同事陷入尴尬：要么为了避免冲突接受你所有意见，花很多没必要的时间，还让你无形中变成代码库所有变更的“守门人”；要么就每条小事都和你争论，反而更耗时。代码审查不是让你把个人偏好强加给同事的场合。

不想让变更合并，就明确标记“阻塞性审查”

到目前为止，我聊的都只是审查意见。但代码审查的“核心”不是意见内容，而是审查状态：是“通过”（approval）、“仅提意见”（just comments），还是“阻塞性审查”（blocking review）。审查状态会影响所有意见的解读——“通过”里的意见，读起来像“做得很好，想改就改点小细节”；“阻塞性审查”里的意见，读起来则是“我不希望你合并的原因就在这”。

如果想阻止合并，就明确标记“阻塞性审查”。很多工程师即便发现大问题，也觉得标记“阻塞”不礼貌，于是只留下描述问题的意见。千万别这么做。这会导致一种混乱的文化：没人知道自己的变更到底能不能合并。

“通过”应该意味着“就算你忽略我的意见，我也同意合并”；“仅提意见”应该意味着“只要有别人通过，就算你忽略我的意见，也能合并”。如果变更合并后你会不满，就必须标记“阻塞性审查”。这样提交者能清楚知道自己能不能合并，不用挨个找提意见的人要“口头许可”。

大多数审查都应该是“通过”状态

首先要说明一点：这一点是否适用，很大程度上取决于代码库的类型。比如像 SQLite 这样的项目，拉取请求（PR）大多被标记“阻塞”是合理的。但对于标准的 SaaS（软件即服务）代码库——团队要频繁开发新功能——审查结果应该以“通过”为主。。

如果大量 PR 被阻塞，通常说明“守门”过度了。我见过很多次这样的情况：某个团队成了其他很多团队的“瓶颈”——比如他们负责边缘网络配置（新的公开路由必须在这里定义），或者负责数据库结构（新功能需要修改这里）。这个团队通常比普通的功能团队更关注稳定性，团队里的工程师可能有不同的头衔（比如 SRE），甚至隶属于不同的部门。因此，他们的目标和自己名义上支持的功能团队是不一致的。

举个例子：功能团队想更新公开的入口路由（ingress routes），以便发布某个重要项目。但边缘网络团队根本不关心这个项目——它不影响他们或他们上级的考核。真正影响他们考核的，是这个变更可能引发的生产事故。这就导致他们会尽可能长时间地阻塞任何有风险的变更。而功能团队为了交付新功能，愿意承担一定风险，这种阻塞会让他们非常沮丧。

当然，大量 PR 被阻塞也可能有其他原因。比如公司刚招了一批能力不足的工程师，必须阻止他们合并代码；又比如公司最近出了个影响很大的事故，未来几周内需要阻塞所有有风险的变更，直到用户淡忘。但在正常情况下，高阻塞率往往意味着结构性问题。

对很多工程师（包括我自己）来说，标记“阻塞性审查”会带来一种满足感——就像“守门”本身会带来满足感一样。你会觉得自己在独力守护代码库质量，或是在避免生产事故。这也能满足工程师一个常见的小毛病：在能力不如自己的同事面前炫耀技术。比如心里想“看来你不知道这段代码会导致 N+1 查询问题！但我知道。幸好我花时间看了你的代码，不然你就惨了”。

“尽量优先通过变更”这个原则非常重要，谷歌自己的代码审查指南都把它放在开头，称之为“所有审查准则中的首要原则”⁴。

最后的思考

我很清楚，很多优秀的工程师可能会反对这篇文章里的大部分观点，甚至全部观点。

这很正常！关于代码审查，我也有很多认为“显而易见是对的”的观点，但并没有写在这里。

根据我的经验，代码审查时不妨遵循这些原则：

别只审查 diff，多想想这个 PR 里“没写的代码”是否合理；

留少量经过深思熟虑的意见，而不是边看边随手留，最后堆上百条；

用“这段代码能正常工作吗”的标准审查，而不是“这和我会写的方式完全一样吗”；

不想让变更合并，就明确标记“阻塞性审查”；

除非有非常严重的问题，否则就通过审查。

这些原则基本也适用于审查大语言模型（LLM）生成的代码。LLM 生成的代码尤其容易“漏写该写的部分”，而且如果一次性给它上百条意见，它也会混乱，更何况它还有自己的“风格”。唯一不适用的是“优先通过”这一点——对于 AI 生成的 PR，你完全可以尽情“守门”。

最后想说明的是，代码审查的方式有很多种。一套审查流程可能想满足的目标包括：确保团队多人熟悉代码库的每个部分、让团队讨论每个变更的软件设计、发现单个人可能忽略的细微 bug、在团队内横向传递知识、增强对每个变更的“主人翁意识”、在代码库中统一代码风格和格式规范、满足 SOC2“不能让单个人修改系统”的约束。

这些目标是按我个人的重视程度排序的，但如果工程师对这些目标的排序不同，他们的代码审查方式也会截然不同。

网友留言

siva7：

说得好。根据我与 40 多个团队合作的经验，最重要的一点是：“代码审查不是你把个人喜好强加给同事的时候。”

在软件开发中，解决一个问题总有很多可接受的方法，所以如果有人把自己的个人偏好强加给同事，这会给团队带来紧张气氛，还会浪费时间 —— 不要让这种情况成为阻碍审查的因素！

当开发人员痴迷于代码，却缺乏远见和经验，看不到在业务的大格局中，并非所有事情都会成为技术债务的相关因素时，这一点也显得尤为重要。

criemen：

文章写得很棒，我基本都同意。不过想针对一点补充些细节：

别用“我会怎么写”的标准来审查

对我来说，审查时思考“如果是我会怎么写”其实能发现很多问题——我会先想自己的实现思路，再对比 PR 里的写法，看看两种方案是否一致。如果不一致，往往能从中找到值得探讨或学习的点。当然，这些思考都只在我脑子里进行！

只有当我确定 PR 里的方案客观上存在问题（比如没能妥善处理边界 case，甚至完全没考虑），我才会提意见。否则，就让大家按自己的方式来就好！

所以我同意“别用这种思路堆大量意见”，但我认为它是个很有用的思考方法，能让大脑主动参与审查，而不是随便扫一眼 diff 就结束了。

kiliancs：

审查不该成为主观意见的堆砌地 —— 那些只会拖慢进度、阻碍推进，或是为了刷存在感、过度吹毛求疵的评论，都不该出现在审查里。但审查者也不能只盯着 “哇，有进展了” 这一点，还得考虑这个变更会给代码库带来什么影响，以及是否符合功能方向和技术方向。要是贡献者经验不足，就算次次轻松通过审查，原本优质的代码库也会慢慢变得难用。

代码审查也是学习的机会，能提供真实场景来辅助决策 —— 要是没有这些场景，很多决策可能只是脱离实际的主观判断。当然，不是所有评论和讨论都要用来阻止通过审查，这里需要找到平衡。要是能换来更优的结果，还能助力团队成长，那功能晚几天上线，也没什么大不了的。

（英文：https://news.ycombinator.com/item?id=45701404，中文由 AI 大模型优化）

- EOF -