Google工程实践文档 - CodeReview - 代码审查/ChangeList指南

1. Google 工程实践指南

参考: https://google.github.io/eng-practices/

术语:

• CL: ChangeList，版本的更改，补丁或拉取请求
• LGTM: Look Good To Me，我觉得不错，代码审查 CR 的时候所说

包含内容:

1. 代码审查员指南 2.CL 作者指南：为正在接收审查的开发人员提供详细的指南

1.1. 代码审查开发人员 CR 指南

Google 使用代码审查来维护我们的代码和产品的质量，文档是对 Google 代码审查流程和政策的规范描述。

审查人员关注：

• 设计：代码是否经过精心设计并适合系统
• 功能: 是否符合作者预期，是否对用户有利
• 复杂性：代码可以简单吗？将来遇到此代码，其他开发人员能否轻松理解和使用此代码？
• 测试：代码是否具有正确且设计良好的自动化测试
• 命名：开发人员是否为变量、类、方法等选择了清晰的名称？
• 评论：评论备注是否清晰有用？
• 风格：代码是否遵循了代码风格指南，参考: https://google.github.io/styleguide/
• 文档：开发者是否更新了相关文档？

挑选最佳评论者

• 希望找到能够在合理时间内回复你的评论的最佳评论者，可以邀请多人查看 CL 的不同部分

面对面审查/结对编程

• 结对编程，也可以认为代码是经过审查的
• 面对面 CR，CR 人员提出问题，CL 更改人员记录&发言

2. 如何做代码 CR

CR 内容，包含下面集合：

1. CR 的审查标准
2. CR 中关注哪些内容？
3. CR 中导航到 CL
4. 提升 CR 速度
5. 如何写 CR 评论
6. 处理 CR 中的 PushBack

2.1. CR 的审查标准

2.1.1. 审查标准说明

代码审查的主要目的是确保代码库的整体代码健康状况随着时间的推移而改善，代码审查的所有工具和过程都是为此目的而设计的，为了实现这一点，必须平衡一系列权衡

1. 开发人员需要认同观点：如果从不提交对代码库的改进，那么代码库就永远不会改进
2. 审阅者应该让开发人员能够清楚知道代码问题在哪以及如何改动，如果审阅者让任何更改都很难进行，那么开发人员在未来进行改进的动力就会减弱
3. 审阅者有责任确保每个 CL 都具有这样的质量，代码库质量不会随着时间的推移而下降，即便当团队受到严重的时间限制并且他们觉得必须走捷径才能实现目标时
4. 审阅者对他们正在审阅的代码拥有所有权和责任，他们希望确保代码库保持一致、可维护以及 “在代码审查中寻找什么”中提到的所有其他内容，列入如果 CL 在其系统中添加了审阅者不想要的功能，那么即使代码设计良好，审阅者也可以拒绝批准
5. 审阅者应该清楚没有“完美”的代码——只有更好的代码，即应该平衡取得进展的需要与他们建议的更改的重要性，不应该追求完美，而应该追求的是持续改进，一个整体上提高了系统的可维护性、可读性和可理解性的 CL 不应该因为它不是“完美的”而延迟数天或数周。
6. 审阅者应该随时发表评论，表达某些东西可能会更好，但如果它不是很重要，请在其前面加上**“Nit:”之类的前缀**，让作者知道这只是一个他们可以选择忽略的润色点。

综合，以下规则作为我们在代码审查中所期望的标准（所有代码审查指南中的高级原则）：

一般来说，审查者应该倾向于批准一个 CL，一旦它处于一个可以肯定改善正在处理的系统的整体代码健康状况的状态，即使 CL 并不完美。

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

2.1.2. 审查标准指导

代码审查的重要功能是向开发人员传授有关语言、框架或一般软件设计原则的新知识。留下有助于开发人员学习新知识的评论总是好的。随着时间的推移，共享知识是改善系统代码健康状况的一部分。

请记住，如果代码评论纯粹是教育性的，但对于满足本文档中描述的标准并不重要，请在其前面加上“Nit：”或以其他方式表明作者在此 CL 中解决它不是强制性的

2.1.3. 审查标准原则

1. 技术事实和数据推翻个人意见和偏好
2. 风格指南是绝对权威，风格应该与现有的一致，如果没有以前的风格，接受作者的
3. 基于基本设计原则，应根据这些原则进行权衡而不仅仅是个人意见，如果作者可以证明（通过数据或基于可靠的工程原理）设计有效，那么审查人应该接受作者的偏好，否则，选择由软件设计的标准原则决定。

2.1.4. 解决冲突

1. 审查人员和开发人员遇到冲突，优先遵循审查标准和指南，达成共识
2. 当达成共识变成很困难，双方应该面对面或视频会议沟通，而非持续 CR 评论沟通解决冲突
3. 如果不能解决冲突，最常见就是升级，通过更广泛的团队讨论，比如让主管参与决策，遵循一点：不要因为开发者和审查人员意见不合，未达成共识而让 CL 搁置

2.2. 在代码中审查的内容

2.2.1. 评审设计 - Design

审查中要涵盖的最重要的事情是 CL 的整体设计。

CL 中各段代码的交互是否有意义？此更改属于你的代码库还是库？它是否与你系统的其他部分很好地集成在一起？现在是添加此功能的好时机吗？

2.2.2. 评审功能 - Function

对审阅者来说，检查 CL 的行为最重要的时间是它具有面向用户的影响。

这个 CL 是否符合开发人员的意图？开发人员的意图对这段代码的用户有好处吗？“用户”通常既是最终用户（当他们受到变更影响时）又是开发人员（将来必须“使用”此代码）

2.2.3. 评审复杂性 - Complexity

CL 是否比应有的复杂？在 CL 的每一层都检查这一点——个别行是否过于复杂？功能是否过于复杂？类是不是太复杂了？“太复杂”通常意味着“代码阅读器无法快速理解”。这也可能意味着“开发人员在尝试调用或修改此代码时可能会引入错误。”

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

2.2.4. 评审 Tests - UT、集成、端点测试

根据变更要求进行单元、集成或端到端测试。通常，除非 CL 正在处理紧急情况，否则应在与生产代码相同的 CL 中添加测试，请记住，测试也是必须维护的代码

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

2.2.5. 评审命名 - Naming

开发人员是否为所有内容选择了好名字？一个好的名称足够长，可以充分传达该项目是什么或做什么，而不会太长以至于难以阅读。

2.2.6. 评审注释 - Comments //

开发者有没有用通俗易懂的英文写清楚的评论？所有的评论真的有必要吗？通常，当他们的意见是有用的解释为什么一些代码存在，而不是解释一些代码在做什么。

**如果代码不够清晰，无法自我解释，那么代码应该更简单。**有一些例外（例如，正则表达式和复杂算法通常从解释它们正在做什么的注释中受益匪浅）但大多数注释是代码本身不可能包含的信息，例如决策背后的推理。查看此 CL 之前的评论也很有帮助。也许现在有一个 TODO 可以删除，一条评论建议不要进行此更改，等等。

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

2.2.7. 风格样式 - Style

在 Google，我们有所有主要语言的风格指南，甚至大多数次要语言，确保 CL 遵循适当的样式指南。

如果你想改进样式指南中没有的一些样式点，请在你的评论前加上“Nit:”，让开发人员知道这是你认为可以改进代码但不是强制性的挑剔。不要仅基于个人风格偏好阻止 CL 提交。

CL 的作者不应将主要的样式更改与其他更改结合在一起。这使得很难看到 CL 中发生了什么变化，使合并和回滚更加复杂，并导致其他问题。

例如，如果作者想要重新格式化整个文件，让他们将重新格式化作为一个 CL 发送给你，然后再发送另一个 CL 及其功能更改。

2.2.8. 代码一致性 - Consistency

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

在某些情况下，样式指南会提出建议而不是声明要求。在这些情况下，需要判断新代码是否应与建议或周围代码一致。偏向于遵循风格指南，除非当地的不一致太令人困惑。

如果没有其他规则适用，作者应保持与现有代码的一致性。无论哪种方式，都鼓励作者提交错误并添加 TODO 以清理现有代码。

2.2.9. 文档 - Doucmentation /**/

如果 CL 改变了用户构建、测试、交互或发布代码的方式，请检查它是否也更新了相关文档，包括README``、g3doc页面和任何生成的参考文档。

如果 CL 删除或弃用代码，请考虑是否也应删除文档。如果缺少文件请索取。

2.2.10. 逐行检测（确保理解代码在做什么） Every Line

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

如果阅读代码太难理解了，这会减慢审查速度，那么审核人应该让开发人员知道这一点，并等待他们澄清，然后再尝试审查它。，换位思考：如果审核人看不懂代码，其他开发人员很可能也不会，因此，当要求开发人员对其进行澄清时，也在帮助未来的开发人员理解此代码。

如果了解代码但觉得没有资格进行某些部分的审查，请确保 CL 上有一个合格的审查者，特别是对于复杂问题，例如安全性、并发性、可访问性、国际化等。

2.2.11. 列外情况

如果 CR 每一行对你来说没有意义怎么办？例如，审核人为 CL 的多个审阅者之一，可能会被问到：

• 仅查看属于较大更改的某些文件。
• 仅审查 CL 的某些方面，例如高级设计、安全含义等。

如果你希望在确认其他审阅者已审阅 CL 的其他部分后授予LGTM(Look Good To Me)，请在评论中明确指出这一点以设定期望，目标是在 CL 达到所需状态后快速响应。

2.2.12. 上下文语境 - Context

在广泛的背景下查看 CL 通常会有所帮助。通常，代码审查工具只会向你显示正在更改的部分周围的几行代码，比如在一个 50 行函数中，仅新增了 4 行，现在确实需要将其分解为更小的方法。

在整个系统的上下文中考虑 CL 也很有用。这个 CL 是改善了系统的代码健康，还是使整个系统更复杂、测试更少等？不要接受会降低系统代码运行状况的 CL。大多数系统会因为许多小的变化加起来而变得复杂，因此在新的变化中防止即使是很小的复杂性也很重要。

2.2.13. 赞赏好的内容 - Good Things

如果你在 CL 中看到了不错的内容，请告诉开发人员，尤其是当他们以出色的方式处理你的评论时。

代码审查通常只关注错误，但它们也应该为良好实践提供鼓励和赞赏。有时，在指导方面，告诉开发人员他们做对了什么比告诉他们做错了什么更有价值。

2.2.14. 小结

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

1. 代码设计考虑: 代码设计得很好
2. 功能价值考虑: 该功能对代码的用户有好处
3. 基于用户考虑: 任何 UI 更改都是明智的并且看起来不错
4. 需要安全设计: 任何并行编程都是安全地完成的
5. 不过渡设计: 代码并不比它需要的复杂
6. 扩展性设计: 开发人员没有实现他们将来可能需要但不知道他们现在需要的东西
7. 适当 UT 测试: 代码有适当的单元测试
8. UT 测试目标: 测试是精心设计的
9. 清晰命名: 开发人员对所有内容都使用了清晰的名称
10. 有效注释: 评论清晰有用，主要解释为什么而不是什么(解释 Why 优先)
11. 适当文档: 代码有适当的文档记录（通常在 g3doc 中）
12. 遵循代码风格: 代码符合我们的风格指南

确保检查你被要求检查的每一行代码，查看上下文，确保你正在改善代码健康状况，并称赞开发人员所做的好事

3. 在 CR 中高效查看 CL：先看 CL 目的，再看 CL 设计，最后按序看 CL

1. 改变有意义吗？它有很好的 描述吗？
2. 先看看变化中最重要的部分。整体设计是否合理？
3. 以适当的顺序查看 CL 的其余部分

3.1. 改变有意义吗？它有很好的描述吗？

查看 CL 描述以及 CL 的一般用途。这种改变甚至有意义吗？如果一开始就不应该发生这种变化，请立即回复并解释为什么不应该发生这种变化。当你拒绝更改时，向开发人员建议他们应该做的事情也是一个好主意。请注意，审阅者不仅拒绝了当前的 CL 并提供了替代建议，而且还彬彬有礼。这种礼貌很重要，因为我们想要表明我们作为开发人员相互尊重，即使我们不同意。

另外如果你获得的 CL 表示你不想进行的更改超过几个，则应考虑重新设计团队的开发流程或为外部贡献者发布的流程，以便在编写 CL 之前进行更多的沟通，最好在人们完成大量现在必须丢弃或彻底重写的工作之前告诉他们“不”。

例如，你可以说“看起来你在这方面做了一些很好的工作，谢谢！但是，我们实际上正朝着删除你在此处修改的 FooWidget 系统的方向前进，因此我们现在不想对其进行任何新的修改。相反，你重构我们的新 BarWidget 类怎么样？”

3.2. 检查 CL 的主要部分 - 可以快速识别重大问题

查找作为此 CL 的“主要”部分的文件。通常，有一个文件具有最多的逻辑更改，它是 CL 的主要部分。先看看这些主要部分。这有助于为 CL 的所有较小部分提供上下文，并且通常会加速进行代码审查。如果 CL 太大而你无法确定哪些部分是主要部分，请询问开发人员你应该首先查看哪些内容，或者要求他们 将 CL 拆分为多个 CL。

如果你发现 CL 的这一部分存在一些重大设计问题，你应该立即发送这些评论，即使你现在没有时间查看 CL 的其余部分。事实上，审查 CL 的其余部分可能是浪费时间，因为如果设计问题足够重要，那么许多其他正在审查的代码将会消失，而且无论如何都无关紧要。

立即发送这些主要设计意见非常重要有两个主要原因：

1. 开发人员通常会提交一个 CL，然后在等待 CL 审核开启新的内容，如果正在审查的 CL 中存在重大设计问题，他们也将不得不重新设计他们以后的 CL，在他们在有问题的设计之上做太多额外工作之前阻止他们。
2. 主要的设计更改比小的更改需要更长的时间。开发人员几乎都有截止日期；为了在截止日期前完成并在代码库中保留高质量的代码，开发人员需要尽快开始对 CL 进行任何重大的返工。

3.3. 以适当的顺序浏览 CL 的其余部分

一旦确认整个 CL 没有重大设计问题，请尝试找出一个逻辑顺序来查看文件，同时确保你不会错过查看任何文件。通常在你浏览了主要文件之后，最简单的方法是按照代码审查工具向你呈现它们的顺序浏览每个文件。有时，在阅读主代码之前先阅读测试也很有帮助，因为这样你就知道应该做什么更改了。

4. 代码审查的速度

4.1. 为什么代码审查要快？

在 Google，优化开发人员团队共同生产产品的速度，而不是优化单个开发人员编写代码的速度。个人发展的速度很重要，只是不如整个团队的速度重要。

如果代码审查慢:

1. 整个团队的速度降低了： 没有快速回复评论的人会完成其他工作，但是，团队其他成员的新功能和错误修复会延迟数天、数周或数月，因为每个 CL 都在等待审查和重新审查。
2. 开发人员会抗议代码 CR：如果审阅者每隔几天才回复一次，但每次都要求对 CL 进行重大更改，这对开发人员来说可能会令人沮丧和困难，大多数关于代码审查过程的投诉实际上都是通过加快流程来解决的
3. 代码运行状况可能会受到影响：当审查缓慢时，开发人员会提交坏味道的代码，阻碍了代码清理、重构和对现有 CL 的进一步改进。

4.2. 代码审查应该多快 ?

1. 如果不在重点任务中，应该抽空优先 CR
2. 最长一个工作日限制，是响应代码审查请求（即第二天早上的第一件事）花费的最长时间

遵循这些准则意味着 CL 应该在一天内获得多轮审查（如果需要）

4.3. 速度与中断的 Tips

不要打断心流状态：如果正忙于一项重点任务，个人速度的考虑胜过团队速度，例如编写代码，请不要打扰自己进行代码审查。研究表明，开发人员在被中断后可能需要很长时间才能恢复平稳的开发流程。因此，在编码时打断自己对团队来说实际上 比让另一个开发人员等待代码审查更昂贵。

相反，在当编码任务完成时、午餐后、从会议回来、从休息室回来等中断时间点，这个时间可能比较适合回复 CR 审查请求之前。

4.4. 尽量按时审核

尽量在作者还在办公室时与他们联系。如果他们已经回家了，那么请确保在他们第二天回到办公室之前完成你的审查

4.5. 尽可能快的给予 LGTM

为了加快代码审查，在某些情况下，审查者应该给予LGTM/Approval，即使他们还在 CL 上留下未解决的评论。这是在以下任一情况下完成的：

1. 审阅者确信开发人员将适当地处理所有审阅者的剩余评论
2. 其余的更改很小，不必由开发人员完成
3. 如果不清楚，评审者应指定他们打算使用哪些选项。

当开发人员和审阅者位于不同的时区时，带评论的LGTM尤其值得考虑，否则开发人员将等待一整天才能获得“LGTM，批准”。

4.6. 大型 CL

如果有人让你 CR 一个庞大的 CL，你不确定何时能够有时间审查它，典型反应应该是要求开发人员将 CL 拆分为几个相互构建的较小 CL，而不是必须一次审查的一个巨大的 CL。这通常是可能的，并且对审阅者非常有帮助，即使它需要开发人员进行额外的工作。

如果一个 CL 不能分解成更小的 CL，而且你没有时间快速回顾整个事情，那么至少对 CL 的整体设计写一些评论，然后发回给开发人员进行改进。作为审阅者，你的目标之一应该是始终解除对开发人员的阻碍，或者使他们能够快速采取某种进一步的行动，而不会牺牲代码健康。

4.7. 紧急情况

在紧急情况下，CL 必须非常快速地通过整个审核过程，并且质量指南会被放宽。但是，请参阅什么是紧急情况？了解哪些情况实际上属于紧急情况，哪些情况不属于紧急情况。例如一个线上的紧急 BUG 是高优先级处理。

5. 如何做 CR 代码评审评论

1. 礼貌善待他人
2. 解释你的理由
3. 给出明确的指示与仅指出问题的平衡，并让开发人员决
4. 鼓励开发人员简化代码或添加代码注释，而不仅仅是向你解释复杂性

5.1. 礼貌善待他人

通常礼貌和尊重是很重要的，同时对你正在审查的代码的开发人员也非常清楚和有帮助。做到这一点的一种方法是确保你总是对代码发表评论，而不是对开发人员发表评论。你不必总是遵循这种做法，但在说一些可能令人不安或有争议的事情时，你绝对应该使用它。例如：

• bad：“当并发显然没有任何好处时，你为什么在这里使用线程？”
• good：“这里的并发模型增加了系统的复杂性，而我看不到任何实际的性能优势。因为没有性能优势，所以这段代码最好是单线程的，而不是使用多线程。”

5.2. 解释为什么

有一件事你会注意到有关从上面的“好”的例子是，它可以帮助开发人员了解为什么你是这样考虑的。你并不总是需要在你的评论中包含这些信息，但有时对你的意图、你遵循的最佳实践或你的建议如何改善代码健康进行更多解释是合适的。

5.3. 给予指导

一般来说，修复 CL 是开发人员的责任，而不是审阅者的责任，你不需要为开发人员进行解决方案的详细设计或编写代码。

一般来说，审核者应该在指出问题和提供直接指导之间取得适当的平衡，指出问题并让开发人员做出决定通常有助于开发人员学习，并使代码审查变得更容易。它还可以产生更好的解决方案，因为开发人员比审阅者更了解提交的代码。

但是，有时直接的说明、建议甚至代码会更有帮助，代码审查的主要目标是尽可能获得最好的 CL，第二个目标是提高开发人员的技能，以便他们随着时间的推移需要越来越少的审查。

请记住，人们从强化他们做得好的事情中学习，而不仅仅是他们可以做得更好的事情。如果你在 CL 中看到你喜欢的内容，也请发表赞赏评论！示例：开发人员清理了混乱的算法，添加了示例性测试覆盖率，或者你作为审阅者从 CL 中学到了一些东西。就像所有评论一样，包括你喜欢某样东西的原因，进一步鼓励开发人员继续良好的做法。

5.4. 接受解释

如果你要求开发人员解释一段你不理解的代码，这通常会导致他们更清楚地重写代码。偶尔在代码中添加注释也是一种适当的回应，只要它不只是解释过于复杂的代码。

6. 仅在代码审查工具中编写的解释说明对未来的代码阅读者没有帮助，它们仅在少数情况下是可以接受的，例如当你审查一个你不太熟悉的区域时，开发人员会解释一些普通读者已经知道的代码。

7. 处理代码中的 pushback(回退)

有时开发人员会推迟代码审查。他们要么不同意你的建议，要么抱怨你总体上过于严格。

7.1. 谁是对的？

1. 首先，当开发人员不同意你的建议时，请先花点时间考虑一下它们是否正确。通常，他们比你更接近代码，因此他们可能对代码的某些方面有更好的了解。他们的论点有意义吗？从代码健康的角度来看，这有意义吗？如果是这样，让他们知道他们是对的，让问题消失。
2. 另外，开发人员并不总是正确的，在这种情况下，审查人应该进一步解释为什么他们认为他们的建议是正确的。一个好的解释既表明了对开发人员回复的理解，也表明了关于为什么要求更改的附加信息。
3. 特别是，当审阅者认为他们的建议将改善代码健康时，如果他们认为由此产生的代码质量改进证明所要求的额外工作是合理的，他们应该继续倡导改变， 改善代码健康往往是在小步骤中发生的。

总之，有时需要对一个建议进行几轮解释才能真正理解，只要确保始终保持礼貌，让开发人员知道你不同意的原因，让他们知道你在说什么。

7.2.

8. CR 常见缩写大全

编写的代码审查中使用的缩写列表，以减少必须做的打字量

8.1. 斯坦福 - 软件设计学科 - CS190 缩写

参考: https://web.stanford.edu/~ouster/cgi-bin/cs190-spring15/reviewCodes.php

8.2. TMLI 范例

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

// 嵌套层级太深
for (...) {
    if (A) {
        if (B) {
           if (C) {
               do stuff...
           }
        }
    }
}

// 使用下面的结构
for (...) {
    if (!A) {
        continue;
    }
    if (!B) {
        continue;
    }
    if (!C) {
        continue;
    }
    do stuff...
}