你好,我是太白。
最近公司同事作了个Code Review的分享,于是乎我系统学习了下《谷歌工程实践》,里面主要讲的是Code Review。写下本文作为笔记。
《谷歌工程实践》是 Google 团队长期以来的内部项目最佳实践。其目的是帮助开发者更好地进行代码审查工作,通过 Code Review 来提升并优化当前项目的代码质量,便于开发人员维护和维护旧项目。
不管你是代码开发者还是代码的审查者都值得读一读。本文主要是基于《谷歌工程实践》[1],摘取了关键部分,并做了笔记和扩展。建议看下英文原版《Google Engineering Practices Documentation 》[2]。
扩展
扩展参考Git 团队协作常用术语[3]、LGTM : code review 行话[4]
所有 Code Review 指南中的高级原则:
一般来说,审核人员应该倾向于批准CL,只要CL确实可以提高系统的整体代码健康状态,即使CL并不完美。
不然就会发生:
如果您没有处于重点任务的中,那么您应该在收到代码审查后尽快开始。
快速的个人响应比整个过程快速发生更为重要。
为了加快代码审查,在某些情况下,即使他们也在 CL 上留下未解决的评论,审查者也应该给予 LGTM/Approva。
如果有人向您发送了代码审查太大,您不确定何时有时间查看,那么您应该要求开发者将 CL 拆分为几个较小的 CL 而不是一次审查的一个巨大的 CL。
不要为了提高想象中的代码审查速度,而在代码审查标准或质量方面妥协,实际上这样做对于长期来说不会有任何帮助。
CL必须非常快速地通过整个审查流程,并且质量准则将放宽。
紧急 CL 是这样的小更新:允许主要发布继续而不是回滚,修复显著影响用户生产的错误,处理紧迫的法律问题,关闭主要安全漏洞等。
一般而言,对于那些正在被您审查代码的人,除了保持有礼貌且尊重以外,重要的是还要确保您(的评论)是非常清楚且有帮助的。
糟糕的示例
为什么这里你使用了线程,显然并发并没有带来什么好处?
好的示例
这里的并发模型增加了系统的复杂性,但没有任何实际的性能优势,因为没有性能优势,最好是将这些代码作为单线程处理而不是使用多线程。
关于上面的“好”示例,您会注意到的一件事是,它可以帮助开发人员理解您发表评论的原因。并不总是需要您在审查评论中包含此信息,但有时候提供更多解释,对于表明您的意图,您在遵循的最佳实践,或为您建议如何提高代码健康状况是十分恰当的。
一般来说,修复 CL 是开发人员的责任,而不是审查者。您无需为开发人员详细设计解决方案或编写代码。但这并不意味着审查者应该没有帮助。一般来说,您应该在指出问题和提供直接指导之间取得适当的平衡。
如果您要求开发人员解释一段您不理解的代码,那通常会导致他们更清楚地重写代码。偶尔,在代码中添加注释也是一种恰当的响应,只要它不仅仅是解释过于复杂的代码。
有时开发人员会拖延(Pushback)代码审查。他们要么不同意您的建议,要么抱怨您太严格。
当开发人员不同意您的建议时,请先花点时间考虑一下是否正确。通常,他们比你更接近代码,所以他们可能真的对它的某些方面有更好的洞察力。
有时需要几轮解释一个建议才能才能让对方真正理解你的用意。只要确保始终保持礼貌,让开发人员知道你有听到他们在说什么,只是你不同意该论点而已。
审查者有时认为,如果审查者人坚持改进,开发人员会感到不安。有时候开发人员会感到很沮丧,但这样的感觉通常只会持续很短的时间,后来他们会非常感谢您在提高代码质量方面给他们的帮助。
通常情况下,如果您在评论中表现得很有礼貌,开发人员实际上根本不会感到沮丧,这些担忧都仅存在于审核者心中而已。开发者感到沮丧通常更多地与评论的写作方式有关,而不是审查者对代码质量的坚持。
经验表明,在开发人员编写原始 CL 后,经过越长的时间这种清理发生的可能性就越小。实际上,通常除非开发人员在当前 CL 之后立即进行清理,否则它就永远不会发生。
因此,在代码进入代码库并“完成”之前,通常最好坚持让开发人员现在清理他们的 CL。让人们“稍后清理东西”是代码库质量退化的常见原因。
如果 CL 引入了新的复杂性,除非是紧急情况,否则必须在提交之前将其清除。如果 CL 暴露了相关的问题并且现在无法解决,那么开发人员应该将 bug 记录下来并分配给自己,避免后续被遗忘。又或者他们可以选择在程序中留下 TODO 的注释并连结到刚记录下的 bug。
如果您以前有相当宽松的代码审查,并转而进行严格的审查,一些开发人员会抱怨得非常大声。通常提高代码审查的速度会让这些抱怨逐渐消失。
有时,这些投诉可能需要数月才会消失,但最终开发人员往往会看到严格的代码审查的价值,因为他们会看到代码审查帮助生成的优秀代码。而且一旦发生某些事情时,最响亮的抗议者甚至可能会成为你最坚定的支持者,因为他们会看到审核变严格后所带来的价值。
如果上述所有操作仍无法解决您与开发人员之间的冲突,请参阅 “Code Review 标准”以获取有助于解决冲突的指导和原则。
CL 描述是进行了哪些更改以及为何更改的公开记录。CL 将作为版本控制系统中的永久记录,可能会在长时期内被除审查者之外的数百人阅读。
其余描述应该是提供信息的。可能包括对正在解决的问题的简要描述,以及为什么这是最好的方法。如果方法有任何缺点,应该提到它们。如果相关,请包括背景信息,例如错误编号,基准测试结果以及设计文档的链接。
即使是小型 CL 也需要注意细节。在 CL 描述中提供上下文以供参照。
“Fix bug ”是一个不充分的 CL 描述。什么 bug?你做了什么修复?
其他类似的不良描述包括:
其中一些是真正的 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 的上下文、解决方案并不理想以及可能的未来方向。它还解释了为什么要进行这种更改。
为 status.py 创建一个 Python3 构建规则。 这允许已经在 Python3 中使用它的消费者依赖于原始状态构建规则旁边的规则,而不是他们自己树中的某个位置。它鼓励新消费者尽可能使用 Python3,而不是 Python2,并显着简化当前正在开发的一些自动构建文件重构工具。
第一句话描述了实际正在做的事情。描述的其余部分解释了为什么要进行更改,并为审阅者提供了很多背景信息。
小且简单的 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。但是,无论您目前多么确定,请花一点时间退一步,考虑审查者是否提供有助于对代码库和公司有价值的反馈。
解决冲突的第一步应该是尝试与审查者达成共识。如果您无法达成共识,请参阅“代码审查标准”,该标准提供了在这种情况下遵循的原则。
[1]《谷歌工程实践》: https://jimmysong.io/eng-practices/
[2]《Google Engineering Practices Documentation 》: https://github.com/google/eng-practices
[3]Git 团队协作常用术语: https://blog.csdn.net/qq_15988951/article/details/108331701
[4]LGTM : code review 行话: "https://blog.csdn.net/weixin_41287260/article/details/108676433"
[5]《Google Style Guides》: https://google.github.io/styleguide/