代码审查标准

代码审查的主要目的是确保 Google 代码库的整体代码 无序列表 健康状况随着时间的推移而得到改善。 代码审查的所有工具和流程都是为此目的而设计的。

为了实现这一点，必须做一系列的取舍来达到平衡。 首先，开发人员必须能够在完成工作任务过程中有进展。 如果您从不提交对代码库的改进，那么代码库就永远不会有优化。 此外，如果审查者让任何更改都变得非常困难，那么开发人员就没有动力在未来进行改进。

另一方面，审查者有责任确保每个 CL 的质量，以确保代码库的整体代码健康状况不会随着时间的推移而下降。 这可能很棘手，因为通常情况下，代码库会随着时间的推移代码健康状况因不断小幅下降而逐步退化，尤其是当团队受到严重的时间限制并且他们觉得必须走捷径才能实现目标时。

此外，审查者对其正在审查的代码拥有所有权和责任。 他们希望确保代码库保持一致、可维护，以及“代码审查哪些方面”中提到的所有方面。

因此，我们得到以下规则作为我们在代码审查中期望的标准：

通常情况，一旦确定 CL 一定有助于改善系统的整体代码健康状况，即使 CL 并不完美，审查者也应该赞成批准使用 CL。

这是所有代码审查指南条例中的首要原则。

当然，这也有局限性。 例如，如果 CL 添加了审查者不希望在他们的系统中使用的功能，那么审查者当然可以拒绝批准，即使代码设计得很好。

这里的一个关键点是没有“完美”代码这样的东西——只有更好的代码。 审稿人不应该要求作者在批准前对CL的每一个细小部分进行打磨。相反，与他们建议更改的重要性相比，审查人应该平衡取得进展的必要性。审查人应该追求的不是完美，而是持续改进。总体上提高系统的可维护性、可读性和可理解性的 CL 不应该因为它不“完美”而延迟数天或数周。

审查人应该自由发表评论，来表达哪些地方可以做得更好，但如果建议不是很重要，评论可以以“Nit:”之类的前缀开头，让作者知道这只是一个他们可以选择性忽略的润色点。

注意：本文档中没有任何内容可以证明引入 CL 肯定会恶化系统的整体代码健康状况。 您只有在紧急情况下才允许这样做。

辅助

代码审查的一个重要作用就是向开发人员传授有关语言、框架或一般软件设计原则的新知识。 留下有助于开发人员学习新东西的评论总是好的。 随着时间的推移，共享知识是改善系统代码健康状况的一部分。请记住，如果您的评论纯粹是教育性的，但对于满足本文档中描述的标准并不重要，请在其前面加上“Nit：”或以其他方式表明不是强制性作者在本 CL 中解决它。

原则

• 技术事实和数据凌驾于观点和个人偏好之上。
• 在风格问题上，风格指南是绝对的权威。 任何不在风格指南中的纯粹的风格点（留白等）都是个人偏好的问题。风格应该与现有的东西一致。如果没有以前的风格，就接受作者的风格。
• 软件设计在各个方面基本上都不会是纯粹的风格问题或单纯的个人偏好。它们基于基本原则，应该根据这些原则进行权衡，而不仅仅是个人意见。有时会有几个有效的选项。 如果作者可以证明（通过数据或基于可靠的工程原理）几种方法同样有效，那么审查人应该接受作者的偏好。 否则，应选择符合软件设计标准的的方案。
• 如果没有其他适用的规则，那么审查者可能会要求作者与当前代码库中的风格保持一致，前提是这样不会恶化系统整体代码的健康度。

解决冲突

代码审查过程中有任何冲突，开发人员和审查人员都应始终根据本文档以及 CL 作者指南和审查指南中的其他文件的内容尝试达成共识。

当双方很难达成共识时，审查者和作者之间最好进行面对面的会议或者是视频会议，而不要仅仅试图通过代码审查评论来解决冲突。 （不过，如果您这样做，请确保将讨论结果记录到该CL的评论上，以供将来的读者使用。）

如果这样依然不能解决问题，最常见的解决方法就是将问题升级。 通常，升级路径可以是进行更广泛的团队讨论，让技术负责人参与，要求代码维护者做出决定，或者要求工程经理提供帮助。 不要让 CL 无所事事，因为作者和审查人无法达成一致。

代码审查时检查什么

注意：在考虑这些要点时，一定要确保考虑到代码审查标准。

设计

审查中要涵盖的最重要的事情是 CL 的整体设计。 CL 中各段代码的交互有意义吗？ 此更改属于您的代码库还是依赖库？ 它是否与您系统的其余部分很好地集成？ 现在是添加此功能的好时机吗？

功能

这个 CL 是否按照开发人员的意图进行？ 开发人员的意图是否适合使用此代码的用户？ “用户”通常既是最终用户（当他们受到更改的影响时）又是开发人员（他们将来必须“使用”此代码）。

大多数情况下，我们希望开发人员能够充分测试 CL，以便在他们进行代码审查时能够正常工作。 然而，作为审查者，您仍然应该考虑边缘情况，寻找并发问题，尝试像用户一样思考，并确您通过阅读代码找不到错误。

如果需要，您可以验证 CL——审阅者检查 CL 行为最重要的契机是它具有面向用户的影响，例如 UI 更改。 当您只是阅读代码时，很难理解某些更改将如何影响用户。 对于这样的更改，如果不方便在 CL 中打补丁并自己尝试，您可以让开发人员给您演示功能。

另一个在代码审查期间考虑功能问题特别重要的时候是，如果CL中存在某种并行编程，理论上可能导致死锁或竞争条件。仅通过运行代码很难检测到这类问题，通常需要有人（开发人员和审查人员）仔细考虑它们以确保不会引入问题。 （请注意，这也是在可能出现竞争条件或死锁的情况下不使用并发模型的一个很好的理由——它会使代码审查或理解代码变得非常复杂。）

复杂性

CL 是否比它应呈现的更复杂？ 在 CL 的每个级别检查这一点——单独的行是否太复杂？ 功能是否太复杂？ 类是否太复杂？ “太复杂”通常意味着“阅读代码无法快速理解”。 这也可能意味着“开发人员在后续调用或修改此代码时容易写出bug。”

有一种特殊类型的复杂态叫过度工程，开发人员使代码比需要的更通用，或者添加了系统当前不需要的功能。 审查人应该特别警惕过度工程。 鼓励开发人员解决他们已知的待解决问题，而不是开发人员推测将来可能要解决的问题。 未来的问题应该遇到了再解决，您可以在物理宇宙中看到它的实际形态和要求。

测试

要求针对更改的部分进行单元、集成或端到端测试。 一般来说，除非 CL 需要做紧急合并，否则应将测试添加到与生产代码相同的 CL 中。p

确保 CL 中的测试是正确的、合理的和有用的。 测试不会测试它自己，我们很少为我们的测试编写测试——人类必须确保测试是有效的。

当代码被破坏时，测试真的会失败吗？ 如果它们下面的代码发生变化，它们会开始产生误报吗？ 每个测试是否做出简单有用的断言？ 不同测试方法之间的测试是否适当分开？

请记住，测试也是必须维护的代码。 不要仅仅因为它们不是主二进制文件的一部分就不关注测试代码的复杂性。

命名

开发者是否在代码里都取了个好名字？一个好的名字要足够长，以充分传达此处是什么或者要做什么，但也不要长到难以阅读。

注释

开发者是否用可理解的英语写了清晰的注释？所有的注释是否真的有必要？通常情况下，注释有必要解释一些代码存在的原因，而不要解释那些代码在做什么。如果代码不够清晰，自己都无法解释清楚，那么就应该把代码变得更简单。有一些例外（例如，正则表达式和复杂的算法经常从解释它们在做什么的注释中受益匪浅），但大多数情况下，注释是为了解释代码本身无法包含的信息，比如一个决定背后的理由。

请注意，注释与类、模块或函数的文档不同，后者应该表达一段代码的目的，它应该如何使用，以及使用时的行为方式。

风格

在谷歌，我们所有的主流语言都有风格指南，甚至大部分的小语种也有。请确保CL遵循相应的风格指南。

如果您想改进一些样式指南中没有的样式点，在您的评论前加上 "Nit:"，让开发者知道这是一个您认为可以改进性建议但不是强制性的修改。不要仅仅因为个人风格的偏好而阻止CL的提交。

CL的作者不应该把主要的风格变化代码与其他代码变化放在一起。这使人很难看清CL中的修改内容，使合并和回滚更加复杂，并易引起其他问题。例如，如果作者想重新格式化整个文件，让他们把只重新格式化的相关代码作为一个CL发给您，然后再把另一个CL连同他们的功能变化一起发给您。

一致性

如果现有的代码与风格指南不一致怎么办？根据我们的代码审查原则，风格指南是绝对的权威：如果某些东西是风格指南所要求的，CL就应该遵循该指南。

在某些情况下，风格指南提出建议而不是宣布要求。在这些情况下，新的代码是否应该与建议或上下文的代码一致，这是一个需要做出判断的问题。大多情况都应倾向于遵循风格指南，除非局部的不一致会让人太困惑。

如果没有其他规则，作者应该保持与现有代码的风格一致性。

无论哪种方式，鼓励作者提交一个bug，并增加一个清理现有代码的TODO。

文档

如果CL改变了用户构建、测试、交互或发布代码的方式，请检查它是否也更新了相关文档，包括READMEs、g3doc页面和任何生成的参考文档。如果CL删除了或废弃了代码，请考虑是否也应该删除文档。如果文档缺失，就要求提供。

每一行

在一般情况下，查看分配给您审查的每一行代码。 有时您可以检阅数据文件、生成的代码或大型数据结构等内容，但不要检阅人工编写的类、函数或代码块并假设其中的内容没问题。 显然，有些代码比其他代码更值得仔细审查——这是您必须做出的判断——但您至少应该确保您理解所有代码的作用。

如果您很难阅读代码就会减慢审查速度，那么您应该让开发人员知道这个情况，并让开发人员在您审查代码之前做代码澄清。 在谷歌，我们聘请了优秀的软件工程师，而您就是其中之一。 如果您看不懂代码，很可能其他开发人员也看不懂。 因此，当您要求开发人员对其进行澄清时，您也在帮助未来的开发人员理解这段代码。

如果您理解代码但您觉得没有资格做某些部分的审查，请确保 CL 上有一个合格的审查者，特别是涉及到隐私、安全、并发、可访问性、国际化等复杂问题时.

特例

如果回顾每一行对您来说没有意义怎么办？ 例如，您是 CL 上的多个审阅者之一，可能会被问及：

• 仅查看属于较大更改一部分的某些文件。
• 只审查 CL 的某些方面，例如高级设计、隐私或安全影响等

相反，如果您希望在确认其他审阅者已经审阅了 CL 的其他部分后授予 LGTM，请在评论中明确注明这一点以设定期望。 目标是在 CL 达到所需状态后快速响应。

上下文

在宽泛背景下查看 CL 通常很有帮助。 通常，代码审查工具只会向您显示更改代码和它前后的几行代码。 有时您必须查看整个文件才能确定这部分更改确实有意义。 例如，您可能只看到添加了四行代码，但当您查看整个文件时，您会发现这四行代码位于有一个 50 行代码的方法中，现在确实需要将其分解为更小的方法。

在整个项目的上下文中考虑 CL 也很有用。 这个 CL 是改进了系统的代码健康度还是使整个系统更复杂、更少测试等等？ 不要接受会降低系统代码健康度的 CL。 大多数项目都是通过累加许多小变化导致项目变得复杂，因此很重要的就是要防止新变化中的小复杂性。

正向肯定

如果您在 CL 中看到一些不错的东西，请告诉开发人员，尤其是当他们以很好的方式处理您的评论时。 代码审查通常只关注错误，但它们也应该在有良好实践时给予鼓励和赞赏。 有时，就指导者而言，告诉开发人员他们做对了什么比告诉他们做错了什么更有价值。

总结

在进行代码审查时，您应该确保：

• 代码设计得很好。
• 该功能对代码的用户有好处。
• 任何 UI 更改都是合理的并且看起来不错。
• 任何并行编程都是安全完成的。
• 代码并不比它需要的更复杂。
• 开发人员没有实现他们现在不需要，但将来有可能需要的东西。
• 代码有对应的单元测试。
• 测试是精心设计的。
• 开发人员对所有内容都使用了清晰的名称。 注释清晰有用，主要解释为什么而不是什么。
• 代码已适当记录（通常在 g3doc 中）。
• 代码符合我们的风格指南。

确保您已经检查了您被要求检阅的每一行代码，查看上下文，确保您正在改善代码健康度，并称赞开发人员所做的好事。