Google 的工程实践指南 (上)：代码审核指南

原文由知無涯发表于TesterHome社区，点击原文链接可与作者直接交流。

在代码审核的长期实践中，Google总结出了最佳实践，并在此基础上整理出了这些建议。整篇文档各部分的衔接性并不大，在阅读时，你可以选取自己感兴趣的部分，而不必按顺序阅读全文。当然，我们仍旧建议你按顺序通读全篇，你会发现这份文档对你非常有用。
​

一. 代码审核的标准

标准

代码审核的目的是为了保证代码库中的代码质量持续改进，代码审核的工具和流程都是为了实现这个目的而设计。

为了达到目标，我们需要权衡得失。

首先，开发人员必须能在任务上 取得进展 。如果从没向代码库提交代码，那么代码库就不会改善。同时，如果审核者让开发者在提交代码时变得很困难，那么开发者不得不花费大量的精力解决审核评论，没有动力在未来的提交中改进代码质量。

另一方面，审核者有责任确保提交者的代码质量。随着时间的推移，代码库的质量不会降低。这有点棘手，冰冻三尺非一日之寒，代码库质量的降低是随着每次代码提交的微小降低累积而成的，尤其当团队面临很大的时间压力时，为了完成任务，他们不得不采取一些临时方案。

另外，代码审核者对他们审核的代码有所有权和责任，他们有义务确保代码库是一致的、可维护的，所有这些内容可参见代码审核过程中要看些什么？（What to Look For In a Code Review）这篇文章。

因此，我们希望在代码审核中能遵循这条原则：

一般情况下，如果代码提交者的代码能显著提高代码库的质量，那么审核者就应该批准它，尽管它并不完美。

这是代码审核中所有规则的 最高原则。

当然，也有例外。例如，一次提交包含了系统中不应加入的功能，那么审核者就不应批准它，即使它设计得非常完美。

还有一个关键点，那就是世上根本就没有“完美”的代码——只有 更好 的代码。审核者不应该要求代码提交者在每个细节都写得很完美。审核者应该做好修改时间与修改重要性之间的平衡。无需追求完美，而应寻求 持续的改进 。倘若一个 CL 能够改进系统的可维护性、可读性，那么它不应该仅仅因为不够完美而延迟数天（甚至数周）才批准提交。

我们应该营造这种氛围：当审核者发现某些事情有更好的方案时，他可以无拘束地提出来。如果这个更好的方案并不是非改不可，可以在注释前加上：“Nit:”，让提交者明白，这段评论只是锦上添花，你可以选择忽略。

注意：在提交代码时不应显著地 恶化 代码质量，唯一的例外是 紧急情况。

指导

代码审核还有一项重要的功能：能让开发者学到新知识，可能是编程语言方面的，也可能是框架方面的，或一些常规的软件设计原则。作为审核者，如果你认为某些评论有助于开发者学到新知识，那就毫不犹豫地写下来吧。分享知识是提高代码质量的一种方式。记住，如果你的评论是纯学习相关的，与文档中提及的标准关系不大，那就最好在前面加上“Nit”，否则就意味着开发者必须在 CL 中修正这个问题。

原则

以技术因素与数据为准，而非个人喜好。

在代码样式上，遵从代码样式指南的权威。任何与样式指南不一致的观点（如空格）都是个人偏好。所有代码都应与其保持一致。如果某项代码样式在文档中并未提及，那就接受作者的样式。

任何涉及软件设计的问题，都不应由个人喜好来决定。 它应取决于基本设计原则，以这些原则为基础进行权衡，而不是简单的个人看法。当有多种可行方案时，如果作者能证明（以数据或公认的软件工程原理为依据）这些方案基本差不多，那就接受作者的选项；否则，应由标准的软件设计原则为准。

如果没有可用的规则，那么审核者应该让作者与当前代码库保持一致，至少不会恶化代码系统的质量。

冲突解决

在代码审核中碰到冲突时，首先要做的永远是先尝试让双方（开发者和审核者）在两份文档（开发者指南 和 审核者指南）的基础上达成共识。

当很难达成共识时，审核者与开发者最好开一个面对面的会议（或视频会议），而不要继续通过代码审核评论进行解决。（在开会讨论之后，一定要在评论中记录讨论的结果，以便让以后阅读的人知道前因后果。）

如果仍旧无法解决问题，最好的方式是升级。常见的升级方式比较多，如让更多的人（如团队的其他人）参与讨论，把团队领导卷进来，征询代码维护人员的意见，让工程经理来决定。千万不要因为开发者与审核者无法达成一致而让CL停留在阻塞状态。

二. 代码审核过程中要看些什么？

设计

审核一个 CL 最重要的事情就是考虑它的整体设计。CL 中的代码交互是否有意义？这段代码应该放到代码库（codebase）里，还是库（library）里？它能很好地与系统其他部分集成吗？现在加入这个功能是时机正好吗？

功能

这个 CL 所实现的功能与开发者期望开发的功能是一致的吗？开发者的意图是否对代码的“用户”有好处？此处提到的“用户”通常包含最终用户（使用这些开发出来的功能的用户）和开发者（以后可能会“使用”这些代码的开发者）。

绝大多数情况，我们期望开发者在提交 CL 进行审核之前，已经做过充分的测试。但作为审核者，在审核代码时仍要考虑边界情况、并发问题等等。确保消灭那些通过阅读代码就能发现的缺陷。

作为审核者，你 可以 根据需要亲自验证 CL 的功能，尤其是当这个 CL 的行为影响用户交互时，如UI改变。仅通过阅读代码，你很难理解有哪些改变，对用户有哪些影响。对于这种修改，可以让开发者演示这个功能。当然，如果方便把 CL 的代码集成到你的开发环境，你也可以自己亲自尝试。

在代码审核过程中，对功能的考虑还包含一种重要场景：CL 中包含一些“并行计算”，可能会带来死锁或竞争条件。运行代码一般很难发现这类问题，通常需要（开发者和审核者）仔细考虑，以确保不会引入新的问题。（这也是不要引入并发模型的一个好理由，因为它可能引入死锁或竞争条件，同时也增加了代码审核和代码理解的难度。）

复杂性

是不是 CL 可以不必这么复杂？在 CL 的每个层次上检查——哪一行或哪几行是不是太复杂了？功能是否太复杂了？类（class）是否太复杂了？“太复杂”的定义是代码阅读者不易快速理解。 同时意味着以后其他开发者调用或修改它时，很容易引入新的缺陷。

另一种类型的复杂是过度工程化（也称为过度设计）。开发者在设计代码时太过于在意它的通用性，或在系统中加入了目前不需要的功能。审核者应该特别警惕过度工程化。鼓励开发者解决 当前 应该解决的问题，而不是开发者推测将来 可能 需要解决的问题。将来的问题，等碰到的时候，你才能看到它的实际需求和具体情况，到那时再解决也不迟。

测试

同时要求开发者提供 CL 对应的单元测试、集成测试或端到端的测试。测试代码与开发代码应放到同一个 CL 中，除非碰到紧急情况。

确保 CL 中的测试是正确的、明智的、有用的。测试代码并不是用来测试其自身，我们很少为测试代码写测试代码——这就要求我们确保测试代码是正确的。

当代码出问题时，是否测试会运行失败？如果代码改变了，是否会产生误报？是否每个测试都使用了简单有用的断言？不同的测试方式是否做了合适的拆分？

谨记：测试代码也是需要维护的代码。不要因为不会编译打包到最终的产品中，就接受复杂的测试代码。

命名

开发者是否为所有的元素（如类、变量、方法等）选取了一个好名称。一个好名称应该足够长，足以明确地描述它是什么，他能做什么，但是也不要长到难以阅读。

注释

开发者是否使用英文写了清晰的注释？是否所有的注释都是必须的？通常当注释解释为什么这些代码应该存在时，它才是必须的，而不是解释这些代码做什么。如果代码逻辑不清晰，让人看不懂，那么应该重写，让它变得更简单。当然，也有例外（例如，正则表达式和复杂的算法通常需要注释来说明），但大部分注释应该提供代码本身没有提供的信息，如这么做背后的原因是什么。

有时候也应该看一下这个 CL 相关的历史注释。例如，以前写的TODO，现在可以删掉了；某段代码修改了，其注释也应随之修改。

注意，注释与类、模块、功能的 文档 是不同的，这类文档应该描述代码的功能，怎样被调用，以及被调用时它的行为是什么。

代码样式
在Google，我们所有的主要编程语言都要遵循代码样式指南，确保 CL 遵守代码样式指南中的建议。

如果发现某些样式在代码样式指南中并未提及，在注释中加上“Nit”，让开发者知道，这是一个小瑕疵，他可以按照你的建议去做，但这不是必须的。不要因为个人的样式偏好而导致 CL 延迟提交。

作者在提交 CL 时，代码中不应包含较大的样式改变。因为这样很难比较出 CL 中有哪些代码修改，其后的代码合并、回滚会变得更困难，容易产生问题。如果作者想重新格式化文件，应该把代码格式化作为单独的 CL 先提交，之后再提交包含功能的 CL。

文档

如果 CL 修改了编译、测试、交互、发布的方式，那么应检查下相关的文档是否也更新了，如 README 文件、g3doc 页面，或其他所有生成的参考文档。如果 CL 删除或弃用（deprecate）了一些代码，考虑是否也应删除相应的文档。如果没有这些文档，让开发者（ CL 提交者）提供。

每行代码

在审核代码时，仔细检查 每行 代码。某些文件，如数据文件、生成的代码或较大的数据结构，可以一扫而过。但是人写的代码，如类、功能或代码块不能一目十行，我们不应假设它是正确的。有些代码得尤其小心——这需要你自己权衡——至少你应该确认你 理解 这些代码在做什么。

如果代码很难读懂，那就放慢审核速度，告诉开发者你没读懂代码，让他解释与澄清，之后继续审核。在Google，我们雇佣都是伟大的工程师，你是其中一员。如果你读不懂代码，很有可能其他工程师也不懂。实际上，这么做也是在帮助以后的工程师，当他读到这段代码时更容易理解代码。所以，让开发者解释清楚。

如果你理解这些代码，但是感觉自己不够资格审核它，确保找到一个够资格的人来审核，尤其是比较复杂的问题，如安全、并发、可访问性、国际化等等。

上下文

把 CL 放到一个更广的上下文中来看，通常很有用。在审核工具中，我们往往只能看到开发者修改的那部分代码。更多时候从整个文件的角度来读代码才有意义。例如，有时候你只看到添加了几行代码，但从整个文件来看，你发现这4行代码添加到了一个50行的方法中。在增加之后，需要把它拆分成更小的方法。

把 CL 放到系统的上下文中来考虑也很有用。CL 能提升系统的代码健康状况，还是让系统变得更复杂、更难测试？不要接受恶化系统健康状况的代码。大多数系统变得很复杂都是由每个细小的复杂累积而成的，在提交每个 CL 时都应避免让代码变得复杂。

好的方面

如果在 CL 中看到一些比较好的方面，告诉开发者，尤其是当你在审核代码时添加了评论，他在回复你的评论，尝试向你解释的时候。审核者往往只关注代码中的错误，他们也应该对开发者的优秀实践表示鼓励和感谢。有时候，告诉开发者他们在哪些方面做得很好，比告诉他们在哪些方面做得不足更有价值。

三. 代码审核的步骤

第一步：全面了解 CL

阅读 CL 描述，了解CL实现的功能。判断这个修改是否有意义？如果答案是“否”，请立即回复，并解释为什么要取消这个修改。当拒绝的同时，你最好向开发者给出建议，这种情况应该怎么做。

例如，你可能会这么说：“这个 CL 的代码看起来挺不错的，谢谢你！不过，我们正在删除 FooWidget 系统，并用新的系统代替他，目前最好不要修改它（或对它加入新的功能）。建议换种方式，你看重构一下 BarWidget 类，怎么样？”

在上面的例子里，你拒绝了这个 CL ，并提供了一种可选方案。整个过程，你都 很有礼貌 。这种礼貌非常重要，这在告诉对方：尽管不同意你的观点，但我很尊重你。

如果这种情况（开发者提交了你认为不应该这么做的 CL）经常出现，那么你应该考虑一下，是不是应该优化团队的开发流程或外部贡献者（针对某些与外部开发人员共同协作的场景）的发布流程，与开发者先进行充分的沟通确保他已经理解开发的内容，再进行开发。最好在开发者开发一大堆工作之前就说“不”，以避免大量不必要的返工。

第二步：检查 CL 的主体部分

找到包含 CL “主体”部分的文件。通常，如果一个文件包含大量的逻辑修改，那么它就是 CL 的主体部分。先审视这些主体部分有助于为其他部分理出上下文。如果 CL 太大，很难找到主体部分的位置，可以征询开发者的建议，你应该先看哪些部分，并建议他把一个 CL 拆分成多个。

如果发现 CL 中有一些重要的设计缺陷或设计问题，立即给出反馈，即使现在还没来得及审核其他部分。实际上，审核其他部分很有可能是浪费时间。只要这个设计问题足够大，在重新设计时，其他代码很有可能会消失或变得无关紧要了。

为什么要立即对这重要设计问题给出反馈呢？有两个原因：

开发者经常在发出 CL 之后就立即基于这个 CL 开始新的工作。如果你发现正在审核的 CL 有重要设计问题，那么他正在做的新 CL 还得返工。我们应该及时指出，避免开发者在基于错误的设计下做了太多工作。

重要设计错误比小修改花费更多的时间。每个开发者在进行开发工作时都有最后期限；为了在保证代码质量的前提下按时交付，开发者需要尽快重新设计 CL。

第三步：以合适的顺序审视 CL 的其他部分

在确认 CL 没有重要设计问题之后，整理出审视文件的顺序，并确保不会遗漏任何文件。通常，在审视了主要文件之后，最简单的方式就是按照代码审核工具呈现出来的顺序遍历每个文件。有时候，先阅读测试代码更有帮助，因为看了测试代码之后，你就明白这个 CL 的期望行为是什么。

四. 代码审核的速度

为什么应该尽快审核代码？

在Google，我们优化了团队开发产品的的速度，而不是优化单个开发人员写代码的速度。单个开发人员的开发速度固然重要，但远没有整个团队的开发速度 重要 。

当代码审核者很慢的时候，会发生以下几件事：

作为整体，团队的进度降低了。 是的，单个审核者没有对代码审核及时响应，而是在完成其他的工作。如果每个人都这样的话，团队的新功能开发或缺陷修复就会延期，累积下来，延期可能是几天、几周，甚至几个月，团队中每个人都在等待别人审核（或再次审核）自己的代码。

开发者开始抗议代码审核流程。 如果审核者几天反馈一次，每次都要求 CL 重大改变，这样开发者就会变得很沮丧，很困惑。通常，这表达为对审核者太过“严格”的抱怨。如果审核者 同样 要求大量的修改（的确有利于改善代码质量的修改），并且每当开发者更新后，审核者 迅速 响应，抱怨就会消失。大多数关于代码审核流程的抱怨实际上可以通过让流程变得更快来解决。

影响代码质量。 当审核很慢时，会增加开发者的压力，他会认为自己的代码不尽人意。迟缓的审核也会阻碍代码清理、重构，阻碍已有 CL 的进一步改善。

代码审核应该多迅速？

如果你现在没有进行一项需要集中精力的任务，你应该在收到审核邀请时，短时间内就开始代码审核。

在收到审核请求时，一个工作日是审核响应的最长期限 ，即第二天早上做的第一件事情。

遵循这些规则意味着一个典型的 CL 的几轮审核（如果有必要的话）都会在一天内完成。

速度 vs. 中断
当然也有列外。如果你正在做一项需要集中精力的任务时，例如写代码时，不要打断自己。 研究显示，在被打断之后，开发者很长时间才能进入打断前的状态。所以，与其在写代码的时候打断自己，不如让另外一位开发者稍候。否则 成本更高 。

什么时候审核呢？在下一个工作断点之后再审核。这个断点可能是你当前的代码已经完成的时候、午饭后、某个会议结束、从公司的餐厅回来，等等。

快速响应
当我们谈及代码审核的速度时，我们指的是 响应时间 ，而不是审核 CL 整个流程走下来直到提交代码所花费的时间。整个流程也应该快，但 更重要的是个人的快速响应，而不是整个流程快点完成。

即使整个 审核 流程走下来会花费很多时间，但在整个过程中，审核者都在快速响应，这也会很大程度减轻开发者对 “慢” 代码审核的挫败感。

当收到一个 CL 审核的时候，如果你当时太忙没有时间做完整的审核，你仍然可以快速响应，告知开发者你稍后会做审核（但是当时没时间），他可以让他其他能快速响应的人先审核，或者先提供一些初步的反馈。（注意：这并不意味着你可以打断当前的编码工作，还是应该在工作的断点后再审核。）

有一点很重要，当审核者反馈“LGTM”时，意味着他花了足够的时间审核代码，并且认为代码满足代码标准。 然而，每个人还是应该迅速响应。

跨时区的审核
当有时差时，尽量在开发者离开办公室之前给他反馈。如果对方已经下班回家，尽量确保在他在第二天早上来公司之前给出反馈。

LGTM的评论
为了加快代码审核，有一些确定的场景，你应该给出 LGTM/赞同 的反馈，即使开发者仍有一些未处理的反馈（unresolved comments）。这些场景如下（满足任一场景即可）：

审核者相信开发者会对所有未处理的反馈做出合适的响应。

未处理的反馈无关紧要，开发者 没必要 处理。

审核者应该阐明他做出的 LGTM 是哪种场景。

LGTM 特别值得考虑，尤其是当开发者与审核者跨时区时，否则开发者又得等一整天时间才能得到“LGTM，赞成”。

大 CL
如果某位开发者提交一份很大的代码审核，你不大确认自己是否有时间审核它，一种典型的响应是让开发者把一个CL拆分成多个，而不是让审核者一次审核大 CL。这样对审核者比较有用，虽然开发者有些额外的工作要做，这是值得的。

如果一个 CL 不能 拆分成多个，并且你很难在短时间内审核代码，至少在CL的整体设计上向开发者提出反馈，以便让开发者改进。作为一个审核者，你的目标之一是：尽量不要阻塞开发者，让他能迅速采取下一步行动。当然，前提是不会降低代码质量。

代码审核在不断改善中
在遵循本文中的建议进行代码审核之后，尽管代码审核很严格，你会发现，在运行一段时间后，整个流程会越来越快。开发者学会了健康的代码需要什么，在发送 CL 之前会尽量保证代码质量，因此需要审核的时间会越来越短。审核者学会了快速响应，不会在审核中增加不必要的延时。

但是， 不要为了想象中的速度提升，在代码审核标准或质量上妥协 ——实际上，从长期来，这样做并不会节省时间。

紧急情况
当然，也会有紧急情况，要求审核流程尽快完成，此时代码质量也有适当的弹性空间。但是，请先确保它的确属于紧急情况。如果不确认，先查看一下什么是紧急情况，这篇文章详细讲述了哪些情况属于紧急情况，哪些不是。

五. 怎样写代码审核的评论？

保持友善。

解释原因。

给出明确的信息，指出问题所在，让开发者最后做决定。

鼓励开发者简化代码，给代码添加注释，而不是向你解释为什么这么复杂。

礼貌评论
在审核代码时，礼貌和尊重都很重要，与此同时，评论应该描述清晰，有利于开发者改进代码。确保你对代码的评论应该是针对“代码”，而不是针对“开发者”本人。当然，不必总是遵循这条原则，但是当你要说某些可能让人沮丧或引起争议的话时，一定要对事不对人。例如：

不好的说法：“为什么你在这儿使用线程？明显这儿使用并发没有任何好处。”

好的说法：“这儿的并发模型增加了系统的复杂度，我在性能上没有看到好处。因为没有性能提升，这段代码最好还是由多线程改成单线程。”

解释为什么
从上面“好的说法”中，我们看到，它有助于开发者理解 为什么 你要写这条评论。当然，不必每次都解释为什么，但某些情形——阐明你的意图、你正在遵循的最佳实践、你在提升代码健康程度——解释原因是有必要的。

提供指导
一般而言，修复 CL 的责任人是开发者，而不是审核者。 你不必为开发者写出详细的设计方案，也不必为他写代码。

但这不代表审核者可以不提供任何帮助。作为审核者，你应该在指出问题所在与提供直接指导之间做好平衡。指出问题，并让开发者自己做决定，这样有助于开发者自我学习，审核者自己也很省时间。这种方式，开发者也更容易找出更好的解决方案，因为相对审核者，开发者对自己的代码更熟悉。

当然，有些时候也可以给出直接的指示、建议或代码。毕竟，代码审核的首要目标是尽可能让 CL 变得更好；次要目标才是提升开发者的技能，以缩短审核时间。

接受解释
如果某段代码你看不懂，让开发者解释，通常结果是他们会重写代码让它更清晰。有时候，添加注释也可以，只要它不是用来解释一段过于复杂的代码。

仅仅把解释写到代码审核工具里不利于以后的代码阅读者。 只有几种情况可以这样，如当你正在审核一段你并不是很熟悉的代码时，开发者向你解释的文字，其他开发者都知道，那这种解释就不必写到代码里。

六. 代码评论被拒绝时，应如何处理？

有时候，面对审核者的评论，开发者可能会拒绝。他们不同意你的建议，或者他们认为你太过于苛刻了。

谁是对的?
当开发者不同意你的建议时，先考虑他们是否正确。开发者往往更熟悉代码，在某些方面他们对代码有更好的见解。他们的论点是否理由充分？站在代码健康的角度，是否应该如此？如果回答“是”，告诉他们，他们是对的，可以忽略这条评论。

但开发者并非总是对的。此时，代码审核者应该进一步解释为何他相信自己的建议是正确的。一个很好地解释通常包含两部分：对开发人员的回复的解释和进一步说明这么改的必要性。

尤其当审核者相信他们的建议能够有效提升代码质量时，他们应该继续坚持这种修改。即使有证据显示这种代码提升需要一些额外的工作，也值得做。提升代码质量往往是聚沙成塔的过程。

有时候，需要经过好几轮的解释与澄清，你的建议才会被接受。确保自始至终都保持礼貌 ，让开发者知道你在 听 他说话，只是不 同意 他的观点而已。

烦躁的开发者
有时候，审核者相信，如果自己坚持改进，那么开发者可能会心烦。这种事的确偶有发生，但通常持续时间很短，稍后他们会感谢审核者，正是他的坚持让代码质量得以提升。通常，如果在评论中保持礼貌，开发者根本就不会烦躁，审核者不必担忧。烦躁大多是因为写评论的方式没有把焦点放在代码质量上。

稍后再清理
一种常见的拒绝原因是：开发者想尽快完成它（这种心理很常见）。他们不想再进行一轮审核，只想快点把 CL 提交到代码库。所以，他们说他们会在稍后的 CL 中再清理代码，这样你应该对 当前 CL 评论 LGTM了。有些开发者的确是这么做的，他们随后的确创建了一个新的 CL，用于修复当问题。然而，经验告诉我们，从开发者提交了原始 CL 之后，时间过得越久，他们清理代码的可能性越小。除非开发者在当前 CL 之后 立即 清理代码，否则以后也不会清理。这并不是说开发者不靠谱，而是因为他们有太多的开发工作要做，迫于其他工作的压力已经忘了清理之前提交的代码这件事。因此，我们建议，最好坚持让开发者 现在 就开始清理代码，而不是提交到代码库 之后 。我们应该有种理念， 代码退化的常见原因有几个，“稍后再清理” 便是其中之一。

如果 CL 让代码变得复杂，那么它必须在提交之前清理完毕，除非是紧急情况。如果 CL 处处是问题却不立即解决，开发者应该给自己发一个清理代码相关的 bug，把它分配给自己，以避免遗忘。除此之外，在代码中加上 TODO 注释和相关的 bug 编号。

与严格相关的常见抱怨
如果以前你在做代码评审时比较宽松，现在突然变得严格起来，有些开发者可能会抱怨。没关系，通过提升审核代码的速度通常会让抱怨消失。

有时候，让抱怨消失的过程比较漫长，长达数月，但最后，开发者会往往趋于赞同严格审核代码的价值，因为在严格审核的帮助下，他们写出了伟大的代码。一旦大家认识到这种严格带来的价值，甚至最强烈的抗议者可能会变成最强大的拥护者。

冲突解决
作为审核者，如果你已经遵循本文中提到的所有规则，但仍旧与开发者之间产生了难以解决的冲突，可以参考代码审核的标准 ，以其中提到的规则与理论来指导冲突解决。

原文由知無涯发表于TesterHome社区，点击原文链接可与作者直接交流。

今日份的知识已摄入~
想了解更多前沿测试开发技术：欢迎关注「第十届MTSC大会·上海」>>>
1个主会场+12大专场，大咖云集精英齐聚