代码评审到底要看什么？Google 工程实践《What to Look For in a Code Review》完整指南-平芜编程栈

文档
教程

【免费下载链接】eng-practices

Google's Engineering Practices documentation

项目地址：https://gitcode.com/gh_mirrors/en/eng-practices

本篇指南源自 Google 工程实践文档（eng-practices 仓库）中评审者指南的核心章节《What to look for in a code review》，它系统回答了"评审一个变更（CL）时应当逐项核查哪些维度"这一问题。文章将结合仓库内评审标准、评审速度、评审导航、评审评论写法等姊妹文档，为你梳理出一套可落地的评审检查清单：从设计、功能、复杂度到测试、命名、注释、风格、一致性、文档，再到"逐行评审""上下文视角""肯定优点"等实操要点。读完本文，你将掌握一套完整的代码评审方法论，知道在每个维度上应该问什么问题、用怎样的标准做判断，以及如何在坚持质量的同时保持评审速度和团队协作的融洽。

评审的总体前提：先对齐"评审标准"

原文档在开篇就强调了一条硬性前提：在按下列任何一点展开评审之前，永远要把《代码评审的标准》考虑在内。也就是说，"看什么"之前先要明确"用什么标准看"。

评审标准给出了这条链条的根基：

代码评审的首要目的是确保整个代码库的"代码健康度"随时间不断提升，所有评审工具与流程都为此服务；
评审者需要平衡"开发者能持续推进"与"代码健康度不下降"这两端，最终落到一条最高准则——只要一个 CL 整体上确实改善了系统的代码健康度，即使它并不完美，评审者也应倾向于批准它；
不存在"完美"代码，只存在"更好"的代码，评审者追求的是持续改进（continuous improvement），而不是为追求完美而把 CL 卡上数天或数周；
任何内容都允许以评论方式提出，但如果它不那么重要，就用 "Nit: " 前缀标明这只是可选打磨点；
唯一的例外是紧急情况：没有任何东西授权你合入一个确定会恶化整体代码健康度的 CL。

带着这个总纲，下面进入本指南的主体——评审中需要逐项关注的内容。

设计（Design）：评审中最重要的一项

原文档明确指出：评审中最需要覆盖的就是 CL 的整体设计。当代码评审工具只展示几行改动时，很容易陷入局部，但设计问题恰恰需要在"全局"层面提问：

CL 中各个代码片段之间的交互是否有意义？
这个改动应该属于你的代码库，还是应该放进某个库（library）？
它是否与系统其余部分集成良好？
现在是不是添加这项功能的好时机？

这四点分别对应：内部逻辑自洽性、归属边界、系统集成度、时机判断。设计评审的结论往往决定整个 CL 的去留——正如评审导航所强调的：如果发现重大设计问题，应当立即发出设计评论，即使你此刻没有时间评审其余部分——因为设计问题足够严重时，CL 中的大量其他代码都会随之消失或重写，继续评审剩余部分可能是浪费时间。

功能（Functionality）：面向"用户"的正确性

功能维度要回答两个问题：这个 CL 是否做了开发者想做的事？开发者想做的事对代码的"用户"是否有益？

这里的"用户"通常有两类：

最终用户（end-users）——当改动直接影响他们时；
开发者（developers）——未来必须"使用"这段代码的人。

评审者仍要主动想边界情况

大部分情况下，Google 期望开发者在提交评审之前已把 CL 测试到能正确工作。但作为评审者，你依然要：

主动思考边界情况（edge cases）；
寻找并发问题（concurrency problems）；
尝试像用户一样思考；
通过读代码找出肉眼可见的 bug。

两种特别值得亲自验证/深度思考的情形

情形一：用户可见的界面变更（UI change）。只靠读代码很难理解某些改动会如何影响用户。如果改动不方便本地打补丁试用，可以请开发者给你演示功能。此时评审者亲自验证 CL 行为最有价值。

情形二：并行编程（parallel programming）。如果 CL 中涉及可能引发死锁（deadlock）或竞态条件（race condition）的并发模型，这类问题极难仅靠运行代码发现，通常需要开发者和评审者双方都仔细"过一遍脑子"才能确认没有引入问题。原文档还顺带给出了一个值得引以为戒的推论：这正是不该采用可能产生竞态/死锁的并发模型的好理由——它会显著增加代码评审和代码理解的复杂度。

复杂度（Complexity）：在每一层追问"是否过复杂"

复杂度要在 CL 的每一层检查：

单行代码是否过于复杂？
函数是否过于复杂？
类是否过于复杂？

"过于复杂"通常有两种含义：代码读者无法快速理解，或者开发者在调用或修改这段代码时很可能引入 bug。

特别警惕：过度工程（over-engineering）

一种特殊的复杂度是过度工程——开发者把代码做得比需要的更通用，或添加了系统当前并不需要的功能。评审者应对此格外警惕，并鼓励开发者：

解决现在确定需要解决的问题，而不是解决开发者推测将来可能需要解决的问题。

未来的问题应该等它真正到来、你能看到它的实际形态和需求时再去解决。这与此仓库中小型 CL 的核心理念（"一个自包含的、只解决一件事的最小改动"）完全呼应——把需求控制在当下，是控制复杂度、保证可评审性的共同前提。

测试（Tests）：要测试，更要"测试的测试"

评审者应当按变更的实际情况要求单元测试、集成测试或端到端测试。一般情况下，测试应该与生产代码在同一个 CL 中提交，除非该 CL 属于紧急情况。

测试本身也需要被评审

原文档有一个非常犀利的提醒：测试不会测试自己，我们也几乎不会为测试写测试——必须由人来确保测试是有效的。因此评审测试时要问：

当代码真的坏了时，这些测试真的会失败吗？
如果测试下面的代码发生变化，它们会不会开始产生误报（false positives）？
每个测试是否做了简单而有用的断言？
不同测试方法之间的测试职责是否划分得当？

最后，原文档特别强调：测试也是需要维护的代码——不要因为测试不在主二进制文件里就接受测试中的复杂度。这一点可以和小型 CL 中"CL 应包含相关测试代码、所有 Google 变更都应有测试"的要求互相印证。

命名（Naming）：名称是沟通的第一介质

检查开发者是否给所有东西起了好名字。一个好名字的评判标准很朴素：

足够长，能完整传达它是什么或做什么；又不能太长，长到难以阅读。

命名是代码可读性的基本面，评审时逐项扫过类名、函数名、变量名、参数名，往往能快速暴露设计层面的模糊。

注释（Comments）：解释"为什么"，而不是复述"是什么"

检查开发者是否用清晰的英文写出了易懂的注释，且所有注释是否真的必要。原文档给出的判据非常实用：

注释在解释某段代码为什么存在时是有用的；
注释不应该解释代码在做什么——如果代码本身不清楚到需要解释，那就应该把代码简化；
例外情况确实存在：正则表达式和复杂算法往往非常受益于"它在做什么"的说明性注释；
但绝大多数注释应该承载代码本身不可能包含的信息，例如某个决策背后的推理。

别忘了看"改动之前的注释"

评审时还值得顺带查看 CL之前就存在的注释：也许某个 TODO 现在可以删除了，也许有一条注释当初就在告诫"不要做这个改动"而现在恰好应验。

此外，原文档明确区分了注释与文档：类、模块或函数的文档应该表达一段代码的目的、它的用法以及被使用时的行为——这是两种不同的职责，评审时都要覆盖。

风格（Style）：风格指南是绝对权威，"Nit:" 用于非强制意见

Google 为其主要语言（甚至大多数次要语言）都制定了风格指南。评审时要确保 CL 遵循相应的风格指南。基于评审标准的原则：在风格问题上，风格指南是绝对权威；任何不在风格指南中的纯风格点（如空白）都属于个人偏好。

原文档还给出两条操作性很强的实践：

想改进某个不在风格指南里的点时，用 "Nit: " 前缀——这向开发者表明：这是你认为能改进代码的吹毛求疵点，但并非强制。不要仅仅基于个人风格偏好而阻止 CL 提交。这也与评审标准中"Mentoring（指导）"一节的用法一致：纯教育性、不关键的评论都用 "Nit: " 标明非强制。
CL 作者不应把大规模风格改动与其他改动混在一起——否则很难看清 CL 到底改了什么，合并（merge）与回滚（rollback）都会更复杂。例如想重排整个文件格式，就应该单独发一个纯格式化 CL，再发另一个包含功能改动的 CL。（"风格变更独立成 CL"这一点，与小型 CL 中"重构与功能变更分离、便于理解每次改动"的原则一脉相承。）

一致性（Consistency）：风格指南优先，局部一致次之

如果现有代码与风格指南不一致怎么办？原文档给出了清晰的裁决顺序：

按代码评审原则，风格指南是绝对权威：凡风格指南要求的内容，CL 必须遵守；
当风格指南只是推荐而非要求时，属于判断题：新代码该与推荐一致，还是与周边代码一致？——偏向遵循风格指南，除非局部不一致会造成太大困惑；
如果没有任何其他规则适用，作者应与现有代码保持一致。

无论选择哪种方式，都要鼓励作者为清理既有代码提交一个 bug 并加上 TODO——这正是"防止代码库在无数小改动中逐步退化"的兜底机制，与评审标准"代码库往往通过一次次小的健康度下降而退化"的判断完全一致。

文档（Documentation）：改动行为，就要同步文档

如果 CL 改变了用户构建、测试、使用或发布代码的方式，就要检查它是否同步更新了相关文档——包括 README、g3doc 页面以及任何生成的参考文档。如果 CL 删除或弃用了代码，也要考虑相应文档是否该一并删除。如果文档缺失，就直接提出要求。

逐行评审（Every Line）：必须看懂每一行，但允许有例外

一般情况下，要查看分配给你评审的每一行代码。数据文件、生成代码或大型数据结构可以偶尔扫读，但绝不能扫过一段人工编写的类、函数或代码块，就想当然地认为里面的内容没问题。显然，有些代码值得比另一些更仔细的审视——这是你的判断——但你至少要确保自己理解所有代码在做什么。

读不懂代码怎么办：要求澄清，而不是硬着头皮审

如果代码太难读、拖慢了评审进度，你应该告诉开发者并等他们澄清后再继续。原文档给出的理由很有人文关怀：Google 雇佣的都是优秀的软件工程师，你也是其中之一——如果你看不懂这段代码，其他开发者很可能也看不懂。所以当你要求开发者澄清时，你实际上也在帮助未来的开发者理解这段代码。

不擅长某些领域怎么办：确保 CL 上有合格的评审者

如果你读懂了代码，但自认没资格完成评审的某一部分，就要确保 CL 上有具备相应资格的评审者——特别是涉及隐私（privacy）、安全（security）、并发（concurrency）、可访问性（accessibility）、国际化（internationalization）等复杂问题时。

例外情况：只审部分时要在评论中注明

当你作为多个评审者之一、被要求只审一部分内容时（例如只审某个更大变更中的若干文件，或只审某个方面如高层设计、隐私/安全影响），原文档的建议是：

在评论中注明你审查了哪些部分；
优先采用带评论的 LGTM（LGTM with comments）的形式；
如果你想在确认其他评审者已审完其余部分后再给 LGTM，要在评论中明确说明这一预期，并在 CL 达到目标状态后快速响应。

上下文（Context）：看整体文件，更看整个系统

代码评审工具通常只展示改动周围的几行代码，但评审者常常需要查看整个文件才能确认改动是否合理。原文档举的例子非常生动：你可能只看到新增了四行，但看完整文件才发现，这四行处在一个 50 行的方法里——而那个方法现在真的需要拆分成更小的函数了。

把 CL 放到"整个系统"的语境中

还要从系统整体角度思考：这个 CL 是在改善系统的代码健康度，还是让整个系统变得更复杂、测试更少？原文档给出了一条不容妥协的红线：

不要接受会降低系统代码健康度的 CL。

因为大多数系统正是通过无数小改动的累积而变得复杂的，所以即使新改动中很小的复杂度，也要防微杜渐。这条原则与评审标准的"代码库通过小步退化"警示互为表里，是评审者需要长期坚守的立场。

肯定优点（Good Things）：评审不只是挑错

如果在 CL 中看到不错的东西，告诉开发者——尤其是当他们以很棒的方式回应了你某条评论时。代码评审常常只聚焦于错误，但同样应该给予鼓励与赞赏。原文档甚至指出：在指导（mentoring）的意义上，告诉开发者"你做对了什么"，有时比告诉他们"你做错了什么"更有价值。

这一点在评审评论的写法中也有呼应：人们从"对做得好的事情的强化"中学习，而不只从"还能改进什么"中学习——看到开发者清理了混乱的算法、写出了堪称典范的测试覆盖，或让你从 CL 中学到了东西，都值得评论，并且要附上"为什么"你欣赏它。

总结清单：一次完整评审应该确认的 14 项

原文档最后给出了一份评审自检清单，评审者在完成一次代码评审时应确保：

代码设计良好（well-designed）；
功能对代码的用户有益（functionality is good for the users of the code）；
任何 UI 变更都合理且观感良好；
任何并行编程都以安全方式完成；
代码没有超出必要程度的复杂度；
开发者没有在实现他们可能需要、但现在并不知道自己需要的未来功能；
代码有恰当的单元测试；
测试本身设计良好；
所有东西的命名都清晰；
注释清晰有用，且大多解释为什么而非是什么；
代码有恰当的文档（一般在 g3doc）；
代码遵循风格指南；
逐行审查了被分配评审的每一行代码，并关注上下文；
确保自己在改善代码健康度，并对开发者做对的好事给予赞赏。

实践建议：把这份清单嵌入你的评审流程

这份"看什么"清单是评审者指南完整文档体系中的一环。与它配套使用的还有：评审的整体标准（判断批准与否的底层原则）、导航一个 CL（先看描述→再看主要部分→按顺序看完其余）、评审速度（一个工作日内的响应上限、带评论的 LGTM）、评论的写法（礼貌、解释理由、标注严重度）以及应对反驳。对侧的 CL 作者指南则从作者视角提供了良好 CL 描述、小型 CL 与处理评审者评论的配套建议。

落地建议：将本文的 14 项清单整理成一张可勾选的评审模板，每次评审前先对照评审标准校准基调，再按"设计→功能→复杂度→测试→命名→注释→风格→一致性→文档→逐行→上下文"的顺序推进；最后不要忘了——指出问题的同时，真诚地肯定做得好的地方。坚持这套流程，代码库的健康度会在无数个小步改进中持续上升，评审本身也会因为开发者的快速成长而变得越来越快。

继续阅读：下一步可以学习如何在评审中导航一个 CL（先看整体、抓主要部分、按序扫完），以及代码评审的速度（何时响应、如何给出带评论的 LGTM）。