来自Airbnb、Netflix等公司的代码评审最佳实践

本文最初发表于 blogboard.io（《Code Review Best Practices - Lessons from the Trenches》），由 InfoQ 翻译并分享。

我们将涵盖这些话题：

• 为什么要进行代码评审？

除了明显的质量保证外，代码评审还有其它好处。

• 代码评审作为质量保证

我们将介绍在代码评审中寻找什么的一般建议，为什么有一个评审清单是有益的，并且你将得到一个相当长的清单，你可以用它作为自己的评审清单。

• 代码评审作为一种团队提升工具

如果你已经做过一些代码评审，你就会知道它们不仅仅对阻止 bug 有用。我们将总结代码评审作为学习和团队纽带工具的益处。

• 准备一个拉取请求用来评审

针对拉取请求作者的经验教训。有一些经验法则一致指出，准备一个拉取请求有助于使评审更顺利。

• 评审代码——人性化！

针对评审者的经验教训，关于你的评论的措辞和语气如何会对整个评审工作的有效性产生巨大的影响。

这些话题都是相对独立的，因此如果你对某个特定的话题感兴趣，可以直接跳过。

显然，代码评审的首要目的是评审引入的变化的质量。我的意思是，字典中评审的定义准确地说明了这一点

评审 (review，名词) - 对如有必要实行改变意图的某件事的正式评估。

当然，代码就是代码，有很多东西可以自动检查和测试，所以在实际的代码评审中实际上需要检查的内容有细微差别。我们会在下一节介绍这一点。

另一方面，代码评审是变更（现在通常是一个拉取请求）作者与一个或几个评审者之间的一种交流形式。因此，它的作用不仅仅是防止引入 bug 或者保持代码库在风格和架构上的一致性。

如果做得好，代码评审有助于加速团队之间的学习，为所有团队成员创造心理安全，有助于建立和交流最佳实践，教授适当的沟通，并提高团队活力。如果做得差，它们会使所有上述情况恶化。

代码评审有很多方法可以帮助维护代码库和产品的质量标准。最终，它落实在捕获很难进行自动测试级别的错误，例如架构不一致。另外，自动化测试的代码也应该被评审，所以有一个元级别的评审可以有助于 QA。

在提供高效的代码评审（Giving High Leverage Code Reviews）中，Casey Rollins 提倡使用一个清单，列出所有需要注意的常规事项。

当我评审一个拉取请求时，我通常会做多个“来回”，每次专注于一个属性。我从头开始，先考虑单个属性来审查拉取请求，然后再继续考虑下一个属性。当我检查完清单之后，我会提交评审。

这个清单从一般检查到特定检查，因为首先关注高级别的属性很重要。如果你建议整个类或方法进行重构，那么再建议更改其中的某个变量名是没有意义的。

你可以有自己的清单，也可以将其作为团队或项目的共享清单。关于检查清单的有用性有大量的材料。在 Getting Things Done 中，David Allen 提出了一个简单的想法——我们的大脑在处理信息方面很在行，但在存储和回忆信息方面很糟糕。这就是为什么检查清单是一种很好的方法，在外部存储和分解一个计划的或重复的任务。

根据几篇文章 (1, 2, 3) 编译而成，下面是在评审代码变更时要注意的事项列表：

• 目标一致——更改是否完全满足任务需求；即，代码是否实现了某些或全部指定的功能？

• 整个代码库的一致性

• 架构注意事项——新代码是否适应现有架构。新的特性架构能否改进，它是否足够通用或者可扩展？

• 简单性 / 过度工程化

• 性能问题——是否存在代码中断的特定情况（例如峰值加载次数）？查询是否拉取了比所需更多的数据？向数据库中增加新索引是否有助于新查询？

• 意外的错误，例如拼写错误或者数学公式错误——这些可能是显而易见的，也可能是很难发现的，尤其是在数学繁重的代码中

• 遵守法律法规——根据业务，这可能是最重要的事情

• 安全问题——是否引入了任何可被利用的代码？是否有密钥被不安全地分享或存储？

• 可读性和风格——一段看似完美的代码可能无法立即被另一双眼睛理解和阅读。没有作者的解释，是否能够理解这些变更？

• 最佳实践——编程语言通常有各自的最佳实践——它们是否在拉取请求中得到了满足？另外，随着时间的推移，任何项目、团队和公司都将发展他们自己的一套最佳实践——代码评审是一种加强和传播有关知识的方法

• 本地化——所有依赖语言的资源是否都已正确地本地化？

• 依赖关系——是否引入了外部库或者 API？是否有其它用不同依赖或者没有依赖的更简单 / 更快速 / 更好的方法来实现这一点？

• 交互和副作用——新代码和代码库的其余部分是如何交互的；新功能是否破坏了任何现有的功能；是否所有相关的单元测试都被更新或添加

• 日志记录——如果没有良好的日志，几乎不可能正确地调试服务器代码。是否所有东西都正确地记录或追踪

• 异常处理——后端异常是如何处理的；它们是如何与用户沟通的；反馈是否在可能情况下激活？

• 可测试性 / 测试覆盖率——新代码是否被自动测试覆盖？是否所有可疑的测试都被自动或手动地检查过？代码的编写方式是否适合单元测试？

• 外部文档——如果有必要，更新外部文档反映变更？

这是一个很长的列表。除此之外，一条老生常谈的建议是不要用代码评审取代静态代码分析工具。如果你的评审大部分都是关于代码格式、变量命名和字符顺序的，那么最好将自动化代码分析工具包含到你的开发工作流中。

在来自 PayPal 工程的高效代码评审：更好的产品、团队和工程师（Effective Code Reviews: Bettering Products, Teams, and Engineers）中，Gabriel McAdams 指出了与团队活力相关的代码评审的几个重要好处：

• 团队凝聚力——通过让每个人的代码都接受同行评审，代码评审过程促进了个人的责任感、健康的冲突以及每个人都在一起努力使产品变得更好的想法。正如在代码评审最佳实践（Code Review Best Practices）中所说: 代码评审是没有等级的：作为团队中等级最高的人员并不意味着你的代码不需要评审。McAdams 总结得很好：信任（Trust） + 健康的冲突（healthy conflict） + 个人责任感（individual accountability） + 一起努力使团队变得更好（working together to better the team） = 团队凝聚力（team cohesion）.

• 免费的职业发展培训——仅仅通过评审别人的代码，你就能更熟练地阅读和理解新代码。我听说过，杰出工程师最重要的特质之一就是深入研究和剖析一段完全陌生的代码的能力。随着时间的推移，你将学会如何发现常见的实践、小技巧、语法糖、架构抽象，以及如何欣赏用来解决同一问题的不同思维模式。

在来自 Palantir Blog 的代码评审最佳实践（Code Review Best Practices）中，Robert Flink 列出了通过代码评审实现知识分享和社交的几种方式：

• 作者受到同行评审过程的激励，去做所有必需的事前检查，收紧松散的部分，并在发送评审之前整理好代码

• 代码评审会将对产品功能所做的更改显式沟通给团队成员

• 作者可能使用了一种评审者不熟悉的技术、抽象或算法。反过来——评审者可能知道一种解决某个问题的更好的方法

• 积极的沟通可以加强团队内部的社交纽带（对于远程团队尤其如此）

代码评审应该被看作是一种团队工作。一旦你以这种方式看待它们，就会发现双方——作者和评审者——都有各自不同的责任。

在 Medium 工程博客的这篇短文中，Xiao Ma 描述了一种不同的视角如何改变代码评审的方式、反馈的方式，以及每一方的人如何通过对代码评审采取积极的心态而受益。

当我们谈论拉取请求作者的责任时，有一些在所有代码评审指南中都重复出现的关键事项。

1. 尽可能使拉取请求原子化在 Shopify，他们建议保持拉取请求很小——这有助于评审者深入研究，并将它作为他们工作日中的一件原子性工作完成。在实践中，这意味着将你的拉取请求限制在单个关注点上。这里的单个关注点意味着单个 bug 修复、一个功能或者一个 API 变更等。不要将不会改变行为的重构与 bug 修复或新功能混合。这既有利于简化代码评审，也有助于保持代码库的可维护性（例如，原子性的拉取请求更容易回滚）。你可以在 Kickstarter Engineering、Gusto Engineering 和 Palantir 的博客中找到几乎相同的建议。

2. 提供一个有用的拉取请求描述"给你的评审者一张地图"。确实，你应该挑选最熟悉你所修改的代码部分的同事。但即使是几句话来描述拉取请求的为什么 why/ 是什么 what/ 在哪里 where，也可以极大地帮助你的评审者导航到你的拉取请求。

3. 在评审之前进行测试确保在提交评审之前，你已经评审和测试了拉取请求。你需要确保所有相关的文件已经包含在内，这个 PR 能够通过构建和自动化测试，所有来自自动化评审工具的建议都已经得到了解决。

最常被提起但是最不明显的一条建议，是代码评审中沟通语气的重要性。

在 Kickstarter Engineering 的一篇文章代码评审中的谨慎沟通指南（A Guide to Mindful Communication in Code Reviews）中，Amy Ciavolino 列出了许多改进代码评审双方沟通的技巧。用 Amy 的话来说：“全面快速的代码评审需要技术技能。但除此之外，代码评审也是一种沟通、教学和学习的形式。无论是作为代码的作者还是评审者，在沟通中考虑周到可以使代码评审对每个人都更有价值。"

这篇文章包含了一些技巧，教你如何在做评审时注意作者和过程的目的：

• 不要急于下结论，要多问问题——假设作者知道他们在做什么，即使乍一看似乎完全是错的

• 不要吹毛求疵——事实上，你注意到的一些微小的事情，比如格式不一致，应该是一个信号你应该在项目中使用一种 linter。在提供高效的代码评审（Giving High Leverage Code Reviews）中，Casey Rollins 将吹毛求疵与 bikeshedding 现象 (或帕金森定律，Parkinson's Law of triviality) 联系起来。长话短说——虽然小问题很容易发现，但并不意味着你必须坚持要纠正它们。要谨慎务实。

• 批准要有倾向性；弄清楚是否有些事可以稍后再修复——作为一名评审者，你不一定要做一个有权阻塞任何拉取请求的守门员。可能一个架构问题或者很久之后才会有影响的问题会在下一个开发周期内解决，最好尽可能快地将修复推到生产环境。

• 包含示例代码或文档——特别是你查找过的文档。重要的一点是，通过承认你需要查找一些东西，可以帮助那些患有“江湖骗子综合症”的初级成员。

bug 就是 bug，拼写错误就是拼写错误，这无可回避。但即使这是一个明显的错误，通常也有很多种传递信息的方式。代码评审中充斥这样的评论“这重复了；修复这个...；感觉很慢。让它再快一点；阅读风格指南”，无论作者是谁，可能都感觉过于苛刻。

在提供高效的代码评审（Giving High Leverage Code Reviews）中很好地指出了这一点：

代码评审的核心是，你在向你的同事提供反馈，虽然可能很难。但是接受反馈更难。你团队中的每个人都在努力做到最好，所以在传递信息时要小心。例如，如果你指出一个错误或者问一个问题，让它成为一种团队行为，而不是某个人的错。这可能是这样的：“我们可以删除这个文件中的一些重复代码吗？”而不是“你漏了一个边缘情况”。

Alejandro Lujan Toro 提供了几个严厉评论的实际示例，你可以轻易将这些转变为更具建设性的语气：

诀窍就是将代码评审作为一个团队工作来做。在建议修改时，尝试更多地使用我们而不是你。Amy Ciavolino 建议，如果你心情不好而无法给出周到的反馈，你就不应该开始评审：

当你开始评审时，也要考虑一下你的总体感受。给出善意和周到的反馈需要时间和精力。如果你很饿、很累、很匆忙或者有很多会议等等，不要评审代码或者发出一个评审。你首先需要修复这些事情。如果你不关心自己，你也就不能关心他人。

一旦你意识到代码评审不仅仅是发现 bugs，这就很自然了。也许你从拉取请求中学到了一些东西，或者作者投入了大量的精力并且对细节表现出令人印象深刻的关注。让他们知道这些。

对新手来说，在代码评审中给予表扬尤其重要。在如何让好的代码评审变得更好（How to Make Good Code Reviews Better）中， Gergely Orosz 建议，代码评审对于新手来说应该是一种积极的体验：

更好的代码评审会特别注意让新加入者的前几次评审成为一次很好的体验。评审者应该同理心看待这样的事实，新加入者可能还不知道所有的编程风格，并且可能对代码的某些部分还不熟悉。这些评审投入额外的精力来解释替代方案和指向指南。它们在语气上也很积极，对作者建议的代码库的最初几处修改表示赞赏。

• 
  

  

  
  

  

  
  

  点个在看少个 bug 

  关注公众号：拾黑（shiheibook）了解更多

  [广告]赞助链接：

  四季很好，只要有你，文娱排行榜：https://www.yaopaiming.com/
  让资讯触达的更精准有趣：https://www.0xu.cn/