谷歌工程师是如何评审代码的？|无明

作者丨谷歌官方博客

译者丨无名

策划丨赵钰莹

谷歌成立于1998年，以搜索起家，发展至今已有21年。 在过去的 21 年里，谷歌不断创新，开发了七款产品，拥有超过 10 亿的活跃用户。 谷歌的工程师文化一直被认为是优秀和特殊的。 最近，谷歌开源了其内部一直在使用的代码审查规范。 InfoQ 将其翻译整理并分享给开发者，看看谷歌工程师是如何审查代码的。

代码审查标准

代码审查的主要目的是确保代码库的整体质量随着时间的推移而提高，所有代码审查工具和流程都是为了实现这一目标而设计的。

为了实现这个目标，必须进行一系列的权衡。 首先，开发者的开发任务必须有进展。 如果他们不提交改进后的代码，代码库的质量就不会提高。 此外，如果审查者过于严格，开发人员就没有动力进行持续改进。

审阅者的作用是确保每个 CL（更改列表）的质量，以便代码库的整体质量不会随着时间的推移而下降。 这是一项艰巨的任务，因为代码库的整体质量通常会随着每次提交的代码质量的小幅下降而下降，尤其是当开发团队时间紧迫并且觉得他们不得不走捷径来交付时。

审阅者对他们审阅的代码负责，确保代码库保持一致和可维护。

以下是可用于代码审查的准则：

一般来说，如果 CL 达到了提高系统整体代码质量的程度，那么你可以让它们通过，即使它们可能并不完美。

这是所有代码审查指南的最高宗旨。

当然，也有例外。 例如，如果 CL 包含系统不需要的功能，即使代码写得很好，审阅者也可以拒绝通过。

这个世界上没有“完美”的代码，只有更好的代码。 审阅者不应该要求开发人员完善 CL 的每一小部分，而应该在满足要求和变更的重要性之间做出权衡。 审稿人不应力求完美，而应不断改进。 如果一个 CL 提高了整个系统的可维护性、可读性和可理解性，那么它不应该因为它不“完美”而延迟几天甚至几周。

审阅者应该就开发人员可以做得更好的地方提出建议。 但是如果这些建议不是很重要，你可以在它们前面加上类似“Nit:”的东西，让开发者知道这只是一个改进建议，或者他们可以选择忽略它。

指导

代码审查的一个作用是向开发人员传授知识，例如有关语言、框架或通用软件设计原则的知识。 共享知识是提高系统代码质量不可或缺的一部分。 但请注意，如果您的建议纯粹是教育性的，而不是满足本文所述标准的关键，请在其前面加上“Nit：”或以其他方式告诉开发人员他们不一定非要在 CL 中解决这些问题。

原则

解决冲突

当代码审查期间出现冲突时，开发人员和审查人员首先尝试根据本文、CL 作者指南和审查人员指南达成一致。

如果难以达成一致，审查者和开发者可以面对面或视频会议，而不是仅仅通过代码审查评论板来解决冲突。

如果这不能解决问题，则考虑将问题升级以进行更广泛的团队讨论。 让团队负责人参与进来，让代码维护者做出决定，或者向工程经理寻求帮助。 不要因为开发人员和审阅者无法达成一致而让 CL 悬而未决。

code review 应该注意什么？

设计

代码审查最重要的部分是 CL 的整体设计。 CL 中不同代码段之间的交互有意义吗？ 此更改应该属于存储库还是包？ 它是否与系统的其余部分很好地集成？ 现在是引入此更改的好时机吗？

功能

这个 CL 是否满足开发人员的目标？ 开发人员的意图是否对代码的用户有利？ 代码“用户”可以指代最终用户（受代码更改影响的人）和开发人员（将来“使用”代码的人）。

在大多数情况下，我们希望开发人员首先测试 CL 以确保它们正常工作。 但是作为审阅者，您仍然必须考虑边缘情况，例如发现并发问题、尝试像用户一样思考以及发现仅通过阅读代码看不到的错误。

如果愿意，您也可以验证 CL。 如果 CL 影响用户，例如进行 UI 更改，那么现在是验证 CL 的好时机。 如果只看代码，很难理解某些更改会对用户产生怎样的影响。 对于这样的改动，如果不方便自己运行，可以要求开发者提供功能demo。

另一个重要的考虑因素是 CL 中是否存在可能导致死锁或竞争条件的并发问题。 这些类型的问题很难通过简单地运行代码来发现，并且通常需要某人（开发人员和审查人员）仔细考虑并确保它们不会被引入系统。

复杂

CL 是否比实际需要的更复杂？ 层层检查 CL，细化到每一行代码，是不是太复杂了？ 功能是否过于复杂？ 课程复杂吗？ “太复杂”通常意味着“阅读代码的人很难快速理解它”，也意味着“开发人员在调用或修改代码时可能会引入错误”。

过度工程是一种特殊的复杂性，开发人员编写的代码比实际需要的更通用，或者添加系统当前不需要的功能。 审稿人对过度工程持谨慎态度，鼓励开发人员只解决现在需要解决的问题，而不是将来可能需要解决的问题。 未来的问题应该在出现之后解决，因为这样我们才能看到他们的实际状态和需求。

测试

要求开发人员进行单元测试、集成测试或端到端测试。 一般而言，测试应包含在 CL 中，除非 CL 仅用于紧急情况。

确保 CL 中的测试是正确的、合理的和有用的。 因为测试本身不能测试自己，而我们很少为了测试而写测试，所以我们必须保证测试是有效的。

如果代码出现问题为什么我的完美国际代码无法修改，测试会失败吗？ 如果代码更改，它们会误报吗？ 每个测试都有断言吗？ 测试是否按不同的测试方法分类？

请记住，测试代码也需要维护。

姓名

开发人员是否使用良好的命名习惯？ 好的命名应该能够充分表达一个项（变量、类名等）是什么或者它的用途是什么，又不会太难读。

笔记

开发人员是否使用自然语言编写清晰的注释？ 他们写的所有评论都是必需的吗？ 一般来说，注释应该用来解释代码做了什么，而不是它们做了什么。 如果代码不够清晰，无法自我解释，则应进行简化。 当然也有一些例外（例如正则表达式和复杂的算法，代码的读者可以解释他们在做什么，这对代码的阅读者有很大的好处），但是大多数注释应该指出代码中不可能包含的信息，例如这些代码背后的基本原理。

CL 附带的其他注释也很重要，比如告诉可以删除的待办事项，或者建议不要更改代码等。

请注意，注释不同于类、模块或函数文档。 文档的目的是解释代码的目的、用法和行为。

代码风格

Google 为主要编程语言和大多数次要编程语言提供代码风格指南，因此请确保您的 CL 遵循适当的指南。

如果你想做指南中没有提到的风格改进，你可以在评论前加上“Nit:”，让开发人员知道这是你认为可以改进的地方，但不是强制性的。 但请不要仅仅根据个人喜好阻止 CL 提交。

开发人员不应将样式更改与其他更改混为一谈，这样很难看出 CL 中发生了什么更改，从而使合并和回滚更加复杂。 如果开发人员想要重新格式化整个文件为什么我的完美国际代码无法修改，让他们将重新格式化的文件作为单独的 CL，并将功能更改作为另一个 CL。

文档

如果 CL 导致用户构建、测试、交互或发布代码的方式发生变化，请确保相关文档也已更新，包括 README、g3doc 页面和其他生成的参考文档。 如果 CL 删除或弃用代码，请考虑是否也应删除相关文档。 如果缺少文档，请向开发人员索取。

看每一行代码

查看每一行代码。 有些东西需要查看，例如数据文件、生成的代码或大型数据结构，但不要只看一眼类、函数或代码块就假设它们都可以正常工作。 很明显，有些代码需要仔细检查，具体是哪一段代码完全由你决定，但你至少应该明白这段代码是干什么的。

如果代码很复杂，或者您无法快速阅读它们，从而减慢了审查速度，请告知开发人员并要求他们在进行进一步审查之前进行一些澄清。 如果您看不懂代码，其他开发人员很可能也看不懂。 所以，让开发者把代码说清楚，其实是在帮助以后的开发者更好的理解代码。

如果您了解代码但觉得没有资格进行代码审查，请确保合格的 CL 审查人员在审查代码时将安全性、并发性、可访问性、国际化等因素考虑在内。

上下文

代码审查工具通常只显示更改的代码，但有时您需要查看整个文件以确保代码更改有意义。 比如你可能只看到新增了四行代码，但是如果你看整个文件，你会发现这四行代码在一个50多行的方法中，需要拆分成更小的方法此时 。

您需要从整个系统的角度来考虑 CL。 这个 CL 是提高了系统的代码质量，还是让整个系统变得更加复杂和不可预测？ 不要接受会降低系统代码质量的 CL。 大多数系统都是由许多小变化的积累而变得复杂的，所以要尽量避免小变化带来的复杂性。

美好的一面

如果您在 CL 中看到一些不错的东西，请让开发人员知道，尤其是如果他们以好的方式解决了问题。 代码审查通常只关注问题所在，但也应鼓励和赞赏良好的代码实践。 有时，对于开发人员来说，知道他们在做正确的事情比让他们知道他们在做错事情更有价值。

总结

在进行代码审查时，您需要确保：

检查每一行代码，查看上下文，确保您正在提高代码质量，并表扬做得好的开发人员。

检查CL

知道code review要关注什么之后，如何有效的进行跨文件code review呢？

第 1 步：整体查看代码更改

先看一下 CL 描述，看看这个 CL 是干什么的。 进行此更改是否有意义？ 如果不需要进行此更改，请立即回复并解释为什么不应该进行此更改。 当您拒绝这样的更改时，您可以向开发人员建议他们应该做什么。

例如，你可以说：“看起来你在这方面做得很好，谢谢！但是，我们正计划删除这个系统，所以现在不想改变任何东西。也许你可以重构另一个类“？

请注意，审稿人在拒绝 CL 和建议替代方案时必须保持礼貌。 礼貌很重要，因为作为开发人员，我们应该互相尊重，即使我们可能意见不一。

如果有许多您不想要的 CL，请考虑重新调整开发过程，让开发团队或外部贡献者在开发新的 CL 之前进行更多沟通。 提前告诉人们什么不该做，比把它扔掉或在他们做完后完全重写要好得多。

第 2 步：检查 CL 的主要部分

找到 CL 的主文件。 通常一个CL会有一个文件包含主要的逻辑变化，即CL的主要部分。 首先查看这些主要部分有助于了解整个上下文并加快代码审查。 如果 CL 太大以至于您不确定哪些部分是主要的，请询问开发人员，或者让他们将 CL 拆分为多个 CL。

如果 CL 的主要部分存在严重的设计问题，请立即返回给开发人员，即使您没有时间检查 CL 的其余部分。 此时检查 CL 的其余部分可能是浪费时间，因为如果主要部分存在严重的设计问题，那么其他部分就变得无关紧要了。

为什么要立即回复开发商？ 原因有二：

第 3 步：按正确顺序检查 CL 的其余部分

在确认整个 CL 没有严重的设计问题后，尝试按照某种逻辑顺序检查其他文件，确保不会遗漏任何需要检查的文件。 通常，在签出主要文件后，按照代码审查工具显示的顺序浏览每个文件就足够了。 也可以在检查主要代码之前先看一下测试代码，这样可以对代码改动有个大概的了解。

代码审查的速度

为什么代码审查需要快速？

在谷歌，我们优化的是开发团队作为一个整体交付的速度，而不是单个开发人员编写代码的速度。 个人的开发速度也很重要，但不如整个团队的开发速度重要。

如果代码审查缓慢，可能会发生以下情况：

代码审查应该多快？

如果您没有专注于手头的任务，您应该首先检查代码。

对代码审查的回应最好不超过一个工作日。

如果遵循这些原则，典型的 CL 可以在一天内完成多轮审核（如果需要）。

速度和中断

有一种情况，如果你正专注于手头的任务，比如编写代码，那么就不要打扰自己去做代码审查。 研究表明，开发人员可能需要很长时间才能从中断中恢复过来。 因此，在编写代码时打扰自己对整个团队来说比让其他开发人员等待代码审查的成本更高。

因此，在这种情况下，您可以等到可以停止手头的工作后再开始代码审查。 可能是在完成手头的编码任务后、午餐后、会议后、休息后等。

快速反应

当我们说代码审查速度时，我们指的是响应时间，而不是 CL 完成整个审查过程并提交到代码存储库所花费的时间。 理想情况下，整个审稿过程也应该很快，但对单个审稿请求的响应速度比整个过程的速度更重要。

有时可能需要很长时间才能完成整个审核过程，但是审核者在整个过程中的快速响应可以极大地缓解开发人员对“慢”审核的沮丧情绪。

如果您太忙，请先向开发人员发送回复，让他们知道您何时准备好开始审查，或者建议代码由另一位能够更快回复的审查者审查，或者提供一些初步反馈。

最重要的是审查人员花足够的时间进行审查，以确保代码符合标准。 但无论如何，还是反应快点好。

跨时区代码审查

在跨时区进行代码审查时，尽量在开发人员还在办公室时做出回应。 如果他们已经在家，最好确保他们在第二天回到办公室时能够看到代码审查已经完成。

带注释的 LGTM

为了加快代码审查，审查者应该给 LGTM（Look Good to Me，no issues）或通过以下两种情况，即使他们在 CL 中留下未解决的问题：

当开发者和审查者处于不同时区时，最好使用带注解的 LGTM，否则开发者可能要整天等待“LGTM，已批准”。

大CL

如果有人向您发送大型代码审查，而您不确定自己是否有足够的时间来完成审查，通常的做法是要求开发人员将 CL 分成几个较小的 CL。 这样做通常是合理的，并且对审阅者有益，即使这需要开发人员做一些额外的工作。

如果一个 CL 不能拆分成更小的 CL，而且你没有足够的时间快速回顾，至少写一些关于 CL 的整体设计的笔记并将它发送给开发人员。 审阅者的目标之一是在不影响代码质量的情况下快速响应开发人员，或者使他们能够快速采取进一步的行动。

持续改进代码审查

如果您遵循这些准则并严格遵守代码审查流程，您会发现整个代码审查流程会随着时间的推移变得越来越快。 开发人员知道需要做什么来确保代码质量，并从一开始就向您发送出色的 CL，以便审查花费的时间越来越少。 审稿人还学会了如何快速回应。 但是不要为了审查速度而牺牲代码审查标准或质量——从长远来看，这样做不会让任何事情变得更快。

紧急情况

在一些紧急情况下，CL 必须非常快速地完成整个审核流程，在质量方面稍有松懈。 查看这些“紧急情况”，了解哪些符合紧急情况标准，哪些不符合。

审稿意见怎么写

概括

有礼貌的

一般来说，礼貌和尊重很重要。 一是确保您的评论是针对代码的，而不是针对开发人员的。 你不必一直这样做，但当你想对开发人员说一些可能会引起情绪化或争议的话时，这是绝对必要的。 例如：

错误的陈述：“你为什么要在这里使用线程，这样做显然不会获得任何好处”。

好的说法：“在这里使用并发模型会增加系统的复杂性，但我没有看到任何真正的性能优势，所以这段代码最好是单线程而不是多线程”。

解释为什么

从上面的正面示例中可以看出，这有助于开发人员理解您提供建议的原因。 您不必总是在评论中提供此信息，但如果您能就您的意图、您遵循的最佳实践或您的建议将如何提高代码质量提供更多解释，那就太好了。

给予指导

通常，修复 CL 是开发人员的责任，而不是审阅者的责任。 您不需要为开发人员提供详细的解决方案或为他们编写代码。

不过，这并不意味着审阅者不应该帮助开发人员。 您最好能够在指出问题和提供指导之间做出权衡。 指出问题并让开发人员做出决定有助于开发人员学习并使代码审查更容易。 这也会带来更好的解决方案，因为开发人员比审阅者更了解代码。

不过，有时直接说明、建议或代码更有用。 代码审查的主要目的是获得最好的 CL。 第二个目的是提高开发人员的技能，以便将来需要更少的审查。

接受意见

如果你要求开发人员解释一段你不理解的代码，他们通常会重写代码以使其更清晰。 有时候给代码加上注释是合适的，只要不只是用来解释太复杂的代码即可。

不要只在代码审查工具中写注释，因为它对将来阅读代码的人帮助不大。 只有少数情况是可以接受的，比如你对review不是很熟悉，但是开发者的解释很多人都熟悉。

代码审查推回

有时开发人员会拒绝代码审查。 他们可能不同意你的看法，或者他们可能抱怨你太严格了。

谁是对的

如果开发人员不同意您的意见，请花点时间考虑一下他们是否正确。 通常，他们比您更熟悉代码，因此可能对代码的某些方面有更好的理解。 他们的论点有道理吗？ 从代码质量的角度来看，他们的反对是否有意义？ 如果是，让他们知道他们是对的，问题就解决了。

然而，开发商并不总是对的。 在这种情况下，审稿人进一步解释了为什么他们的建议是正确的。

如果审阅者相信他们的建议可以提高代码质量，并且认为审阅带来的代码质量改进值得开发人员付出额外的努力，那么他们应该坚持下去。 提高代码质量通常是一系列小步骤。

有时你需要花很多时间来回解释，但要保持礼貌，让开发人员知道你知道他们在说什么。

兴奋的开发者

有时，审阅者认为如果他们坚持进行更改，他们会让开发人员不高兴。 开发人员有时确实会感到沮丧，但这种感觉通常是短暂的，之后他们会感谢您帮助他们改进了代码。 如果您在审查过程中保持礼貌，开发人员根本不会感到不安，而且这种担忧可能是没有根据的。 通常，让开发人员不高兴的是您编写注释的方式，而不是您对代码质量的坚持。

稍后修复

推迟的一个常见原因是开发人员希望尽快完成任务。 他们不想经历一轮又一轮的代码审查，他们说他们会在后续的 CL 中修复遗留问题，你现在就让 CL 过去吧。 一些开发人员做得很好，一旦提交就开发了后续的 CL。 但经验表明，开发人员在原始 CL 上工作的时间越长，他们进行后续修复的可能性就越小。 除非开发者在提交 CL 后立即修复它，否则通过后通常不会完成。 这不是因为开发人员不负责任，而是因为他们有很多工作要做，而且经常忘记修复。 因此，最好让开发人员立即修复 CL。

如果 CL 引入了新的复杂性，则必须在提交之前对其进行清理，紧急情况除外。 如果 CL 暴露了一些暂时无法解决的问题，开发人员需要记录错误并将其分配给自己，以免遗漏。 他们还可以向代码添加 TODO 注释，指出已记录的错误。

抱怨审核太严

如果之前比较宽松的code review，突然严格起来，可能会引起一些开发者的抱怨。 不过没关系，加快代码审查通常会使这些抱怨消失。

有时这些投诉可能需要几个月的时间才能解决，但开发人员通常最终会看到代码审查的价值，因为他们看到严格的代码审查有助于产生好的代码。 有时，抗议声音最大的人甚至可以成为你最坚定的支持者。

解决冲突

如果您已采用上述方法，但在审查过程中仍然遇到未解决的冲突，请再次参阅代码审查标准以获取有助于解决冲突的指南。

谷歌风格指南：

CL 作者指南：

审稿人指南：

代码风格指南：

应急指南：

＃什么