原文：https://medium.com/palantir/code-review-best-practices-19e02780015f  
作者：Robert F. (https://github.com/uschi2000)

图片来自 https://xkcd.com/1513/

本文谈论了以下话题：

代码审查之为什么、查什么、何时查
准备好被审查的代码
代码审查的执行
代码审查实例

动机

之所以要执行代码审查（code reviews），就是为了籍此改善代码质量，并向团队和公司文化中注入正能量。比如：

提交者往往会清理未完成的细枝末节、合并 TODOs，或是进行一般性的改进；完成这些后，提交者则期待有其他审查者对提交的变动进行检查。让许多开发者引以为傲的，正是从同行那里获得的编码专业能力赞赏。
分享知识会在几方面上帮到开发团队：
- 一次代码审查可以将增、删、改等功能性改动清楚明了地传达给团队成员，以便其开展后续的工作
- 审查者可以学习到提交者所使用的某种技术或算法。更概括的说，代码审查有助于组织内部的质量提升
- 审查者可能掌握着能够改善或精简所提交代码的编程技术知识或代码库；举例来说，某人也许正好也在开发类似的特性或修复类似的问题
- 积极的交互和沟通会加强团队成员之间的社交连结
代码库中的一致性让代码易读易懂，有助于预防 bug，并能促进开发者之间的合作
代码片段的易读性对于将其亲手写出的作者来说是难以判断的，而对于没有完整上下文概念的审查者则容易的多。易读的代码更容易复用、bug 较少，也更不易过时
意外错误 (如错别字) 及结构错误 (像是无效代码、逻辑或算法错误、性能或架构上的关注点) 经常更容易被旁观者清的挑剔审查者找出来。有研究显示，即便是简短、非正式的代码审查也能显著影响代码质量和 bug 的出现的频次
合规合法的环境通常需要审查。代码审查是避免常见安全陷阱的很好途径

审查什么

对于这个问题没有四海皆准的答案，每个开发团队都应该达成自己的共识。有些团队乐于审查向 master 分支的每次合并，而其他一些团队有自己的细化标准以判断是否需要审查 -- 要有效使用工程师（包括作者和审查者）的时间，也要维护代码质量，得在两者之间兼顾平衡。在某些需要监管的环境中，即便是微小的调整也需要代码审查。

代码审核不分尊卑长幼：作为团队中最资深的人也并不意味着其代码就不需要审查。即便在很少的情况下代码真的完美无瑕，审查也向团队成员和伙伴们提供了至少能从多元化的角度认识库中的代码的机会。

何时审查

代码审查应该晚于自动检查（自动测试、样式检查、持续集成）成功完成后，但要早于代码合并到仓库的主线分支之前。通常并不对最后一次发布前积累的总量改变执行正式的代码审查。

对于那些应该作为一个整体被合并到主线分支的复杂改变，只做一次代码审查似乎太大了，这时可以考虑一种堆叠式的审查模式：创建一个基础分支如 feature/big-feature，以及一些二级分支（如 feature/big-feature-api、feature/big-feature-testing 等等）；这些二级分支各自封装一个功能性子集，并独自进行代码审查；一旦所有二级分支被合并到基础分支 feature/big-feature，再创建一次为了把后者合并到主分支的代码审查。

为审查准备好代码

提交易于审查的代码，以免浪费审查者的时间和积极性，对于作者来说是责无旁贷的：

作用域和体积。 所有改变都应该有一个覆盖面窄、定义良好、自包含的作用域。举例来说，一次调整可能是实现一个新特性，或是修复一个 bug。小调整好过大改变。如果一次代码审查要处理大量的改变，比如超过 5 个文件、超过一两天的开发量，或是要花超过 20 分钟去审查 -- 就要考虑将其分割成数次自包含的审查了。比如开发者可能提交一次根据接口或文档为新特性定义 API 的更改，而第二次更改则是依据那些接口增加实现。
只提交完成的、自我审查过的（借助 diff）、自测过的代码审查。为了不浪费审查者的时间，应在将审查指派给他们之前，测试已提交的改变（也就是运行测试套件）并保证其通过所有构建，也要保证所有测试和代码质量在本地和持续集成服务器上被检查过。
重构时不能改变行为；相反的，会改变行为的调整应该避免同时去重构或格式化代码。这样做的好处是：
- 重构经常会影响多行代码和多个文件，而这些波及之处在审查中容易被忽略。无意的行为改变可以神不知鬼不觉的渗透到代码库中。
- 用 cherry-pick、rebase 等施展的版本控制小魔法，遇到大的重构也会玩不转。想要撤销一次因为重构而将行为改变引入到版本库中的提交是极为麻烦的。
- 昂贵的人工审查时间应该花在程序逻辑方面，而不是对样式、语法或格式的辩论上 -- 那些应该用自动化工具解决掉。

Commit messages

下面是一个不错的 commit message 示例，来自被广为引用的 http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

简短扼要的概述，是必选的一个部分。有些情况下，首行信息被视为标题，剩下的部分则作为正文对待。

可选的更多细节性的描述文字。

用祈使句（用于表达命令、请求、劝告、警告、禁止等的句子）
描述 commit message，
如："Fix bug" ，而不是 "Fixed bug" 或 "Fixes bug"。
用 git merge 或 git revert 等命令生成的
commit message 也符合这个约定。

可用空行分割段落。

- 用列表符号表示段落也不错

应该同时表述 commit 的 what 和 how：

> 孬
再次编译

> 好
增加 jcsv 依赖项以修复 IntelliJ 编译

找到审查者

建议提交者去找一两个熟悉代码库的审查者，其中的一个通常应该是项目领导或高级别工程师。超过两个审查者就容易因为意见相左而效率底下甚至产生相反效果了，三个和尚没水喝。

执行代码审查

一次代码审查就是一个在不同的团队成员之间同步观点的过程，自然也有延宕进程的隐患。因此代码审核应该麻利些（以小时计而非天），团队成员和领导也需要优先对待审查并承诺完成的时间。如果你觉得不能按时完成一次审查，请让提交者马上知晓，以便另请高明。

一次审查应该足够彻底，也就是审查者能以一个合理的详细程度向其他开发者解释代码的改变。这确保了代码库中的细节可以被不止一个人所熟知。

作为审查者，你有责任强制实施编码规范并保证编码质量不断提升。审查代码与其说是一门科学，不如说是一门艺术。学习它的唯一途径就是去实行它；一个有经验的审查者应该考虑让其他经验不足的审查者参与进来，并让他们优先评审。

假如代码作者已经遵循了上述原则（特别是自我审查和确保代码能运行了），这里还有一份审查者应该注意的清单：

意图

代码是否达成了作者的意图？ 每次改变都应该有个明确的原因（新特性、重构、修复 bug 等等）。提交的代码是否真的完成了这些目的？
问问题。 函数和类的存在应该有意义；当审查者对其意义不明确时，可能就意味着这段代码需要重写、注释或测试了。

实现

想想如果换成你会怎样解决问题。如果有差别，是为什么？你的代码能处理更多（边际）情况吗？在功能相同的前提下你的代码会更短/更容易/更清晰/更快/更安全吗？当前代码中有没有一些未处理的潜在问题被你发现了？
你看到了潜在的可用抽象吗？特别是重复的代码往往意味着一个更抽象或更通用的功能性片段可以被抽取出来，并在不同环境中复用。
以对手的角色思考，以友善的态度待人。通过找出程序性配置或数据输入问题等破坏代码的问题，尝试着“抓住”作者的懈怠或遗漏。
考虑一下库或既有的产品代码。当某人重新实现了已有的功能时，多半只是因为他不知道已经存在的解决方案。有时代码或功能的重复是出于避免依赖等目的，在这种情况下，就应该用注释清晰解释其意图。
代码的更改是否遵循了标准的模式？代码库往往都有自己的模式，如命名约定、程序逻辑分解习惯、数据类型定义等等。
所做更改是否增加了编译时或运行时的依赖（特别是在子项目中）？我们希望保持产品的松耦合，依赖越少越好。对依赖和构建系统的改变应该事无巨细的检查。

易读性和样式

考虑你的阅读体验。 你能在合理的时间内领会相关概念吗？流程是否健全？变量和方法的命名是否易懂？你在多个文件或函数中能全神贯注吗？你有没有被前后不一致的命名弄晕过？
代码是否遵从了编码规范？ 在样式、API 约定等方面，代码是否和项目中保持了一致？当然，上面提到这些，最好还是能用自动化工具解决掉，以免各费口舌。
代码中是否还有 TODOs ？ TODOs 若只是堆积在代码中就只会随着时间流逝变得过时，尤其是没有注释的 TODO 部分更不应该被接受。起码来说，作者应该将问题提交到 GitHub Issues 或 JIRA 上以待解决，并将相应单号写在 TODO 的注释中。

可维护性

读一读测试。 如果该有测试的地方却没写，就让作者去写。真正不可测试的特性是少数，事实上真正糟糕的常见状况是一些代码根本没被测试。在检查测试时也要注意：它们是否覆盖了值得关注和容易出问题的情况？它们是否可读？审查的部分是否整体覆盖率较低？另外测试代码本身的样式标准经常和核心业务代码迥异，但也十分重要。
本次测试是否引入了新的风险？ 比如破坏测试代码、破坏临时堆栈，或破坏集成测试等。和审查相伴的 pre-commit/merge 等经常不被检查，但出现这类问题会让每个人都好受不了。需要特殊关注都事情包括：对测试工具和模式的删改、对配置的改变，以及人为的改变项目结构等。
本次改变是否破坏了向后兼容？ 如果是的话，是当下就合并更改还是延迟到下次发布时再 merge 呢？这种破坏包括了数据库或架构的更改、公共 API 的更改、用户工作流的改变，等等。
这块代码需要集成测试吗？ 有时，仅靠单元测试无法充分验证代码，特别是代码和外部系统或配置存在交互时。
代码注释，以及 commit message。 过于繁复的注释让代码变得混乱，而过于简单的 commit message 又会让后来者一头雾水。尽管不总是可行，但在高质量注释和 commit message 上的付出总是值得的。
外部文档是否更新了？ 如果你的项目维护了 README、CHANGELOG，或其他文档，新的改变是否反映到了这些文档中呢？过期的文档比没有文档更坑人，等到那时再去修正它比现在就更新它也要花大得多的代价。

不要忘记赞美简介/可读/有效/优雅的代码。反之，在一次审查中婉拒或反对也并非无礼。若改动是多余或不对的，解释后拒绝掉就是了。如果你因为一个或多个大的瑕疵觉得这次改动不可接受，那就不要赞成，同样得解释清楚。有时一次代码审查的正确结局就是 “让我们用完全不同的方法来解决这个吧” 甚至 “干脆别这么干了”。

尊重提交审核的伙伴。虽然“对手思维”很有效，但那毕竟不是你的代码，你考虑的并不一定那么周全。如果通过代码你无法苟同，那么面对面交流一下，或是寻求第三方意见或许更有效。

安全性

核实 API 端与代码库中其他部分保持一致，执行了适当的认证和鉴权。检查其他常见薄弱环节，如弱配置、恶意用户输入、缺少 log 事件等等。如果有疑问，寻求安全专家的帮助。

注释：简明、友好、可行

审查者的注释 应该简明，并且用人话写。评论代码，而不是用作者的口气。当有些问题不甚清楚时，询问后弄清楚好过假设那就是愚蠢的。避免人物之间的比较，带上评价就更不好：“你改代码之前我的代码明明能运行的”、“你的函数有 bug” 等等。避免绝对的判断：“这样根本运行不了”、“结果总是错的”。

尝试着分清建议（如 “建议：抽取成方法以增加可读性”）、需要改变（如 “增加 @Override”），和澄清（如 “该行为的确恰当吗？如果是的话，请增加一个注释来解释这个逻辑”）。也可以考虑提供链接等，以深入解释问题。

当你完成一个代码审核之后，指明你希望作者在何种程度上响应你的评论，以及是否想要在本次审查出的问题都被解决后重新审查一次（举例来说，"放轻松些，完成那几个小建议的地方后合并一下就行了" 对比于 "请考虑我的建议，并让我知道何时能再看一眼"）。

回应审查

代码审查的目的之一就是督促作者不断改进；因此，不要因为审查者的建议闷闷不乐，即便你不以为然也要严肃对待。回应每一条注释，即便只是用 “知道了” 或 “已完成”。解释你做出某些决策的原因，为何一些函数存在等等。如果你不能和审查者达成共识，线下交流或寻求外部的意见。

修复应该被提交到相同的分支，但要独立的提交。在审查过程中就不断挤进新的提交会让审查者无所适从。

不同的团队有不同的 merge 策略：有些团队只允许项目拥有者合并，其他的则允许贡献者在代码审查得到肯定后合并代码。

面对面的代码审查

对于多数代码审查，基于 diff 的异步工具，诸如 Reviewable、Gerrit 或 GitHub 是很好的选择。而在专业和经验迥异的各方进行复杂的变动或审查时，面对面进行可能更有效；无论是在同一块屏幕或投影仪面前，或者利用远程分享屏幕工具，都是可以的。

例子

在下面的例子中，代码块中的建议性审查注释以 //R: ... 的形式标出：

不一致的命名

class MyClass {
  private int countTotalPageVisits; //R: 变量命名要统一
  private int uniqueUsersCount;
}

不一致的方法签名

interface MyInterface {
  /** 当 s 无法被提取时，返回 {@link Optional#empty}  */
  public Optional<String> extractString(String s);
  /** 当 {@code s} 不能被重写时，返回 null */
  //R: 应该统一返回值：都用 Optional<> 
  public String rewriteString(String s);
}

使用库

//R: 移除这个，并用 Guava 的 MapJoiner 库代替
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

个人口味

int dayCount; //R: 那谁: 我更喜欢用 numFoo 而非 fooCount; 是你说了算，但我们应该在这个项目中保持一致性

Bugs

//R: 这里执行了 numIterations+1 迭代, 是故意的吗？
//   如果是的话，考虑改变 numIterations 的含义吧？
for (int i = 0; i <= numIterations; ++i) {
  ...
}

架构上的关注点

otherService.call(); //R: 我觉着我们应该避免对 OtherService 的依赖。我们当面聊一下如何？

--End--

搜索 fewelife 关注公众号

转载请注明出处