26 Feb 2022

谷歌工程实践 | 学习笔记

以下摘自我公众号『太白技术』的内容：

你好，我是太白。

最近公司同事作了个Code Review的分享，于是乎我系统学习了下《谷歌工程实践》，里面主要讲的是Code Review。写下本文作为笔记。

前言

《谷歌工程实践》是 Google 团队长期以来的内部项目最佳实践。其目的是帮助开发者更好地进行代码审查工作，通过 Code Review 来提升并优化当前项目的代码质量，便于开发人员维护和维护旧项目。不管你是代码开发者还是代码的审查者都值得读一读。本文主要是基于《谷歌工程实践》，摘取了关键部分，并做了笔记和扩展。 建议看下英文原版《Google Engineering Practices Documentation 》。

术语

• CL（Change List）：变更列表
• LGTM（Looks Good to Me）：我觉得不错。表示认可这次PR，同意merge合并代码到远程仓库。

扩展

• MR（Merge Request）：合并请求，GitLab中术语。
• PR（Pull Request）：合并请求，GitHub中术语。
• CR（Code Review）：代码审查。
• ACK（Acknowledgement）：我确认了或者我接受了,我承认了。
• WIP（Work in progress）： do not merge yet. 开发中。
• ASAP（As Soon As Possible）： 尽快。
• TL;DR（Too Long; Didn’t Read）：「太长懒得看」README 文档常见。
• PTAL（Please take a look ）：帮我看下，一般都是请别人 review 自己的 PR。
• CC（Carbon copy ）：一般代表抄送别人的意思。
• RFC（Request for comments）：我觉得这个想法很好, 我们来一起讨论下。
• NACK/NAK（Negative acknowledgement）：「不同意」 i.e. disagree with change and/or concept。
• RFC（Request For Comments）：「请求进行讨论」 i.e. I think this is a good idea, lets discuss。
• AFAIK/AFAICT：As Far As I Know / Can Tell 「据我所知；就我所知」。
• TBR：To Be Reviewed「提示维护者进行 review」。

扩展参考Git 团队协作常用术语、LGTM : code review 行话

代码审查者指南

Code Review 标准

所有 Code Review 指南中的高级原则：

一般来说，审核人员应该倾向于批准CL，只要CL确实可以提高系统的整体代码健康状态，即使CL并不完美。

Code Review 要点

• 1. 设计：主要是看CL整体设计是否合理，比如代码的分层是否合理等。
• 1. 功能：主要看是否符合开发者的意图，并考虑到一些边缘情况，如并发问题等。
• 1. 复杂度：阅读代码的人无法快速理解。还要注意过度工程（over-engineering）。
• 1. 测试：确保 CL 中的测试正确，合理且有用。
• 1. 命名：一个好名字应该足够长，可以完全传达项目的内容或作用，但又不会太长，以至于难以阅读。
• 1. 注释：是否所有注释都是必要的？通常，注释解释为什么某些代码存在时很有用，且不应该用来解释某些代码正在做什么。
• 1. 风格：参考《Google Style Guides》 ，里面包含Google的多种语言的风格指南。
• 1. 文档：如果 CL 变更了，请检查相关文档是否有更新。
• 1. 每一行：查看分配给您审查的每行代码。如果不了解某些部分的审查，请确保 CL 上有一个合格的审查人，特别是对于安全性、并发性、可访问性、国际化等复杂问题。
• 1. 上下文：通常，代码审查工具只会显示变更的部分的周围的几行。有时您必须查看整个文件以确保变更确实有意义。
• 1. 好的事情：比起告诉他们做错了什么，有时更有价值的是告诉开发人员他们做对了什么。

查看CL的步骤

第一步：全面了解变更。

• 查看 CL 描述和 CL 大致上用来做什么事情。

第二步：检查 CL 的主要部分。

• 通常，包含大量的逻辑变更的文件就是 CL 的主要部分
• 如果 CL 太大而无法确定哪些部分是主要部分，请向开发人员询问您应该首先查看的内容或者要求他们将 CL 拆分为多个 CL。
• 如果在该部分发现存在一些主要的设计问题时，即使没有时间立即查看 CL 的其余部分，也应立即留下评论告知此问题。

第三步：以适当的顺序查看CL的其余部分。

• 通常在查看主要文件之后，最简单的方法是按照代码审查工具向您提供的顺序浏览每个文件。
• 有时在阅读主代码之前先阅读测试也很有帮助，因为这样您就可以了解该变更应当做些什么

Code Review 速度

为什么尽快进行 Code Review？

不然就会发生：

• 整个团队的速度降低了。
• 开发者开始抗议代码审查流程。
• 代码健康状况可能会受到影响。

Code Review 应该有多快？

如果您没有处于重点任务的中，那么您应该在收到代码审查后尽快开始。

速度 vs. 中断

• 如果您正处于重点任务中，例如编写代码，请不要打断自己进行代码审查。
• 在回复审查请求之前，请等待工作中断点。可能是当你的当前编码任务完成，午餐后，从会议返回等。

快速响应

快速的个人响应比整个过程快速发生更为重要。

带评论的LGTM

为了加快代码审查，在某些情况下，即使他们也在 CL 上留下未解决的评论，审查者也应该给予 LGTM/Approva。

大型CL

如果有人向您发送了代码审查太大，您不确定何时有时间查看，那么您应该要求开发者将 CL 拆分为几个较小的 CL 而不是一次审查的一个巨大的 CL。

代码审查随时间推移而改进

不要为了提高想象中的代码审查速度，而在代码审查标准或质量方面妥协，实际上这样做对于长期来说不会有任何帮助。

紧急情况

CL必须非常快速地通过整个审查流程，并且质量准则将放宽。

紧急 CL 是这样的小更新：允许主要发布继续而不是回滚，修复显著影响用户生产的错误，处理紧迫的法律问题，关闭主要安全漏洞等。

如何撰写 Code Review 评论

总结

• 保持友善。
• 解释你的推理。
• 在给出明确的指示与只指出问题并让开发人员自己决定间做好平衡。
• 鼓励开发人员简化代码或添加代码注释，而不仅仅是向你解释复杂性。

礼貌

一般而言，对于那些正在被您审查代码的人，除了保持有礼貌且尊重以外，重要的是还要确保您（的评论）是非常清楚且有帮助的。

糟糕的示例

为什么这里你使用了线程，显然并发并没有带来什么好处？

好的示例

这里的并发模型增加了系统的复杂性，但没有任何实际的性能优势，因为没有性能优势，最好是将这些代码作为单线程处理而不是使用多线程。

解释为什么

关于上面的“好”示例，您会注意到的一件事是，它可以帮助开发人员理解您发表评论的原因。 并不总是需要您在审查评论中包含此信息，但有时候提供更多解释，对于表明您的意图，您在遵循的最佳实践，或为您建议如何提高代码健康状况是十分恰当的。

给予指导

一般来说，修复 CL 是开发人员的责任，而不是审查者。您无需为开发人员详细设计解决方案或编写代码。但这并不意味着审查者应该没有帮助。一般来说，您应该在指出问题和提供直接指导之间取得适当的平衡。

接受解释

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

处理 Code Review 中的拖延

有时开发人员会拖延（Pushback）代码审查。他们要么不同意您的建议，要么抱怨您太严格。

谁是对的？

当开发人员不同意您的建议时，请先花点时间考虑一下是否正确。通常，他们比你更接近代码，所以他们可能真的对它的某些方面有更好的洞察力。

有时需要几轮解释一个建议才能才能让对方真正理解你的用意。只要确保始终保持礼貌，让开发人员知道你有听到他们在说什么，只是你不同意该论点而已。

沮丧的开发者

审查者有时认为，如果审查者人坚持改进，开发人员会感到不安。有时候开发人员会感到很沮丧，但这样的感觉通常只会持续很短的时间，后来他们会非常感谢您在提高代码质量方面给他们的帮助。

通常情况下，如果您在评论中表现得很有礼貌，开发人员实际上根本不会感到沮丧，这些担忧都仅存在于审核者心中而已。开发者感到沮丧通常更多地与评论的写作方式有关，而不是审查者对代码质量的坚持。

稍后清理

经验表明，在开发人员编写原始 CL 后，经过越长的时间这种清理发生的可能性就越小。实际上，通常除非开发人员在当前 CL 之后立即进行清理，否则它就永远不会发生。

因此，在代码进入代码库并“完成”之前，通常最好坚持让开发人员现在清理他们的 CL。让人们“稍后清理东西”是代码库质量退化的常见原因。

如果 CL 引入了新的复杂性，除非是紧急情况，否则必须在提交之前将其清除。如果 CL 暴露了相关的问题并且现在无法解决，那么开发人员应该将 bug 记录下来并分配给自己，避免后续被遗忘。又或者他们可以选择在程序中留下 TODO 的注释并连结到刚记录下的 bug。

关于严格性的抱怨

如果您以前有相当宽松的代码审查，并转而进行严格的审查，一些开发人员会抱怨得非常大声。通常提高代码审查的速度会让这些抱怨逐渐消失。

有时，这些投诉可能需要数月才会消失，但最终开发人员往往会看到严格的代码审查的价值，因为他们会看到代码审查帮助生成的优秀代码。而且一旦发生某些事情时，最响亮的抗议者甚至可能会成为你最坚定的支持者，因为他们会看到审核变严格后所带来的价值。

解决冲突

如果上述所有操作仍无法解决您与开发人员之间的冲突，请参阅 “Code Review 标准”以获取有助于解决冲突的指导和原则。

代码开发者指南

写好 CL 描述

CL 描述是进行了哪些更改以及为何更改的公开记录。CL 将作为版本控制系统中的永久记录，可能会在长时期内被除审查者之外的数百人阅读。

首行

• 正在做什么的简短摘要。
• 完整的句子，使用祈使句。
• 后面跟一个空行。

Body 是信息丰富的

其余描述应该是提供信息的。可能包括对正在解决的问题的简要描述，以及为什么这是最好的方法。如果方法有任何缺点，应该提到它们。如果相关，请包括背景信息，例如错误编号，基准测试结果以及设计文档的链接。

即使是小型 CL 也需要注意细节。在 CL 描述中提供上下文以供参照。

糟糕的 CL 描述

“Fix bug ”是一个不充分的 CL 描述。什么 bug？你做了什么修复？

其他类似的不良描述包括：

• “Fix build.”
• “Add patch.”
• “Moving code from A to B.”
• “Phase 1.”
• “Add convenience functions.”
• “kill weird URLs.”

其中一些是真正的 CL 描述。他们的作者可能认为自己提供了有用的信息，却没有达到 CL 描述的目的。

好的 CL 描述

以下是一些很好的描述示例。

功能变化例子：

rpc：删除 RPC 服务器消息空闲列表的大小限制。

像 FizzBuzz 这样的服务器有非常大的消息，并且可以从重用中受益。使 freelist 更大，并添加一个 goroutine，随着时间的推移缓慢释放 freelist 条目，以便空闲服务器最终释放所有 freelist 条目。

前几个词描述了 CL 的实际作用。描述的其余部分讨论了正在解决的问题，为什么这是一个好的解决方案，以及有关具体实现的更多信息。

重构例子：

使用 TimeKeeper 构造一个 Task 以使用其 TimeStr 和 Now 方法。

给Task添加一个Now方法，这样borglet()的getter方法就可以被移除了（它只被OOMCandidate用来调用borglet的Now方法）。这替换了 Borglet 上委托给 TimeKeeper 的方法。

允许 Tasks 提供 Now 是消除对 Borglet 依赖的一步。最终，依赖于从 Task 获取 Now 的协作者应 该改为直接使用 TimeKeeper，但这是对小步骤重构的一种适应。

继续重构 Borglet Hierarchy 的长期目标。

第一行描述了 CL 的作用以及与过去相比的变化。描述的其余部分讨论了具体的实现、CL 的上下文、解决方案并不理想以及可能的未来方向。它还解释了为什么要进行这种更改。

需要一些上下文的小型 CL例子：

为 status.py 创建一个 Python3 构建规则。

这允许已经在 Python3 中使用它的消费者依赖于原始状态构建规则旁边的规则，而不是他们自己树中的某个位置。它鼓励新消费者尽可能使用 Python3，而不是 Python2，并显着简化当前正在开发的一些自动构建文件重构工具。

第一句话描述了实际正在做的事情。描述的其余部分解释了为什么要进行更改，并为审阅者提供了很多背景信息。

小型 CL

为什么提交小型 CL?

小且简单的 CL 是指：

• 审查更快。审查者更容易抽多次五分钟时间来审查小型 CL，而不是留出 30 分钟来审查一个大型 CL。
• 审查得更彻底。如果是大的变更，审查者和提交者往往会因为大量细节的讨论翻来覆去而感到沮丧——有时甚至到了重要点被遗漏或丢失的程度。
• 不太可能引入错误。 由于您进行的变更较少，您和您的审查者可以更轻松有效地推断 CL 的影响，并查看是否已引入错误。
• 如果被拒绝，减少浪费的工作。 如果您写了一个巨大的 CL，您的评论者说整个 CL 的方向都错误了，你就浪费了很多精力和时间。
• 更容易合并。 处理大型 CL 需要很长时间，在合并时会出现很多冲突，并且必须经常合并。
• 更容易设计好。 打磨一个小变更的设计和代码健康状况比完善一个大变更的所有细节要容易得多。
• 减少对审查的阻碍。 发送整体变更的自包含部分可让您在等待当前 CL 审核时继续编码。
• 更简单的回滚。 大型 CL 更有可能触及在初始 CL 提交和回滚 CL 之间更新的文件，从而使回滚变得复杂（中间的 CL 也可能需要回滚）。

什么是小型 CL？

一般来说，CL 的正确大小是自包含的变更。这意味着：

• CL 进行了一项最小的变更，只解决了一件事。
• 审查者需要了解的关于 CL 的所有内容（除了未来的开发）都在 CL 的描述、现有的代码库或已经审查过的 CL 中。
• 对其用户和开发者来说，在签入 CL 后系统能继续良好的工作。
• CL 不会过小以致于其含义难以理解。

关于多大算“太大”没有严格的规则。对于 CL 来说，100 行通常是合理的大小，1000 行通常太大，但这取决于您的审查者的判断。变更中包含的文件数也会影响其“大小”。一个文件中的 200 行变更可能没问题，但是分布在 50 个文件中通常会太大。

什么时候大 CL 是可以的？

在某些情况下，大变更也是可以接受的：

• 您通常可以将整个文件的删除视为一行变更，因为审核人员不需要很长时间审核。
• 有时一个大的 CL 是由您完全信任的自动重构工具生成的，而审查者的工作只是检查并确定想要这样的变更。但这些 CL 可以更大，尽管上面的一些警告（例如合并和测试）仍然适用。

分离出重构

通常最好在功能变更或错误修复的单独 CL 中进行重构。例如，移动和重命名类应该与修复该类中的错误的 CL 不同。审查者更容易理解每个 CL 在单独时引入的更改。

但是，修复本地变量名称等小清理可以包含在功能变更或错误修复 CL 中。如果重构大到包含在您当前的 CL 中，会使审查更加困难的话，需要开发者和审查者一起判断是否将其拆开。

将相关的测试代码保存在同一个 CL 中

避免将测试代码拆分为单独的 CL。验证代码修改的测试应该进入相同的 CL，即使它增加了代码行数。

但是，独立的测试修改可以首先进入单独的 CL，类似于重构指南。包括：

• 使用新测试验证预先存在的已提交代码。
• 重构测试代码（例如引入辅助函数）。
• 引入更大的测试框架代码（例如集成测试）。

不要破坏构建

如果您有几个相互依赖的 CL，您需要找到一种方法来确保在每次提交 CL 后整个系统能够继续运作。

如果不能让它足够小

有时你会遇到看起来您的 CL 必须如此庞大，但这通常很少是正确的。习惯于编写小型 CL 的提交者几乎总能找到将功能分解为一系列小变更的方法。

在编写大型 CL 之前，请考虑在重构 CL 之前是否可以为更清晰的实现铺平道路。与你的同伴聊聊，看看是否有人想过如何在小型 CL 中实现这些功能。

如果以上的努力都失败了（这应该是非常罕见的），那么请在事先征得审查者的同意后提交大型 CL，以便他们收到有关即将发生的事情的警告。

如何处理审查者的评论

当您发送 CL 进行审查时，您的审查者可能会对您的 CL 发表一些评论。以下是处理审查者评论的一些有用信息。

不是针对您

审查的目标是保持代码库和产品的质量。当审查者对您的代码提出批评时，请将其视为在帮助您、代码库，而不是对您或您的能力的个人攻击。

永远不要愤怒地回应代码审查评论。这严重违反了专业礼仪且将永远存在于代码审查工具中。

修复代码

如果审查者说他们不了解您的代码中的某些内容，那么您的第一反应应该是澄清代码本身。 如果无法澄清代码，请添加代码注释，以解释代码存在的原因。 只有在想增加的注释看起来毫无意义时，您才能在代码审查工具中进行回复与解释。

如果审查者不理解您的某些代码，那么代码的未来读者可能也不会理解。在代码审查工具中回复对未来的代码读者没有帮助，但澄清代码或添加代码注释确可以实实在在得帮助他们。

自我反思

编写 CL 可能需要做很多工作。在终于发送一个 CL 用于审查后，我们通常会感到满足的，认为它已经完成，并且非常确定不需要进一步的工作。

这通常是令人满意的。因此，当审查者回复对可以改进的事情的评论时，很容易本能地认为评论是错误的，审查者正在不必要地阻止您，或者他们应该让您提交 CL。但是，无论您目前多么确定，请花一点时间退一步，考虑审查者是否提供有助于对代码库和公司有价值的反馈。

解决冲突

解决冲突的第一步应该是尝试与审查者达成共识。 如果您无法达成共识，请参阅“代码审查标准”，该标准提供了在这种情况下遵循的原则。

总结

代码审查者视角

• 1. 以制定审核标准为前提，尽可能快速、及时的反馈代码开发者的合并请求。
• 1. 对于评论要保持友善、提供引导，以最终代码质量、公司价值为导向。
• 1. 做好沟通，有疑问及时沟通、接受解释。

代码开发者视角

• 1. 写好CL的描述。
• 1. 提供小的CL、相关测试代码不分离、重构代码分离、不破坏构建。
• 1. 以代码质量、公司价值为结果导向，及时修复、做好沟通。

Til next time,
Xujiajun at 00:00