如何进行代码审查

摘自Google工程实践文档

本指南提供了基于多年经验进行代码审查的最佳实践。 它们共同构成一个文档,分为多个部分。 不必全部阅读,但对于您自己和团队来说,最好全面阅读本手册。


另请参阅《 CL作者指南》,以获取有关审核其提交的开发人员的详细建议。

代码审查标准


代码审查的主要目的是保证Google代码库的不断改进。 所有工具和过程都致力于实现这一目标。

这里需要许多折衷方案。

首先,开发人员应该能够成功解决他们的问题 。 如果您从不发送代码,则代码库将永远无法改善。 另外,如果审阅者将任何工作都大大复杂化,那么将来,开发人员将无意提出改进建议。

另一方面,审阅者有责任确保CL的质量不会随时间降低代码库的整体质量。 这可能很困难,因为随着时间的推移,代码质量会略有下降,通常会导致性能下降,尤其是如果团队在截止日期之前承受着巨大的压力,并且认为它有权增加技术债务的话。

此外,审阅者还对所审阅的代码负责。 他想确保代码库保持一致,受支持并与“检入代码”部分中提到的所有其他内容匹配。

因此,我们将以下规则作为代码审查的标准:

通常,即使CL不够完善,审阅者也应在CL肯定会改善系统代码整体质量的状态下立即批准CL。

这是所有原则中的主要代码审查。

当然,他有局限性。 例如,如果CL添加了审阅者不想在系统中看到的功能,那么即使代码质量很好,审阅者也可以肯定地拒绝提交。

这里的关键是没有“完美”的代码-只有更好的代码。 审稿人不应该要求作者打磨每个微小的片段。 相反,审阅者应在进一步改进的需求与提议的更改的重要性之间取得平衡。 审稿人不应追求理想,而应努力持续改进 。 通常不会提高系统的可维护性,可读性和可理解性的提交,因为它不是“完美的”,因此不能延迟数天或数周。

审阅者可以随时对改进代码发表任何评论,但不是必须对非常重要的更改进行标记,例如,使用Nit前缀这样,作者就知道这只是他可以忽略的观点。

注意事项 本文档中没有任何理由证明肯定会降低系统代码整体质量的CL。 这仅在紧急情况下才有可能。

指导


代码复习对于教给开发人员一些有关软件设计的语言,结构或一般原理的新知识也很重要。 留下有助于开发人员学习新知识的评论总是很高兴的。 随着时间的流逝,知识共享有助于改善系统代码。 请记住,如果您留下的纯教育性评论对于满足此处描述的标准不是至关重要的,请在其前面加上Nit前缀否则,表明不需要作者允许。

原则


  • 技术事实和数据胜过观点和个人喜好。
  • 在样式方面,绝对权威是样式指南 。 样式指南中未包含的任何纯风格细节(空格等)都是个人喜好问题。 样式应该匹配。 如果没有以前的样式,请接受作者。
  • 软件设计的各个方面几乎绝不是纯粹的样式或个人喜好问题。 它们基于基本原则,并且应由这些原则确定,而不仅仅是个人意见。 有时有几个有效的选项。 如果作者可以证明(使用数据或基于扎实的工程原理)某些方法同样有效,那么审阅者应接受作者的偏爱。 否则,选择取决于标准开发原则。
  • 如果没有其他规则适用,则审阅者可以要求作者遵守当前代码库的统一性,如果这不会使系统的总体状况恶化。

解决冲突


在任何冲突中,第一步都应该始终是开发人员和审阅者希望根据本文档以及《 CL作者指南》和本《 审阅者指南》中其他文档的内容达成共识的愿望。

当特别难以达成共识时,审稿人和作者之间的个人会议或视频会议可以为您提供帮助(如果这样做,请务必在讨论中写下讨论的结果,以供将来的读者阅读)。

如果这不能解决问题,那么最常见的方法是升级。 通常,它包括与团队进行更广泛的讨论,吸引团队领导,与维护人员或开发经理联系。 由于作者和审稿人无法达成协议,因此请勿拖延提交。

要检查什么代码


注意事项 在考虑所有这些项目时,请务必考虑《 标准规范审查》

设计方案


最重要的是在代码审查中考虑整个项目(设计)。 代码不同部分之间的交互是否有意义? 此更改是否适用于您的代码库或库? CL是否与系统的其余部分很好地集成? 现在该添加此功能了吗?

功能性


此CL是否达到开发人员的预期目的? 这对使用此代码的用户有用吗? 所谓“用户”,是指最终用户(如果他们受到更改的影响)和开发人员(他们将来必须“使用”此代码)。

基本上,我们希望即使在提交之前,开发人员也将对其代码进行足够好的测试,以使其正常工作。 但是,作为审阅者,您仍然需要考虑极端情况,查找并发问题,尝试以用户身份思考,甚至​​在阅读代码时也要注意没有明显的错误。

如果需要, 可以检查性能。 如果代码会影响用户(例如更改UI) ,那么这是最重要 。 当您阅读代码时,很难理解某些更改将如何影响用户。 对于此类更改,您可以要求开发人员提供一个演示,如果您觉得很难深入研究代码并自己尝试。

特别重要的一点是,在代码检查期间考虑功能性是在CL中是否发生某种并行编程 ,这在理论上可能导致死锁或竞争条件。 仅通过运行代码很难检测到此类问题。 通常,有人(开发人员和审阅者)都必须仔细考虑他们,并确保不引入任何问题(请注意,这也是在可能出现竞争条件或死锁的情况下不使用并行模型的一个很好的理由-这可以通过代码完成非常难以理解或进行代码审查)。

难点


CL是否比应有的要复杂? 在每个级别进行检查:单独的行,函数,类。 “过于复杂”通常意味着在阅读时无法快速理解 。 这也可能意味着开发人员在尝试调用或修改此代码时更有可能引入错误

一种特殊类型的复杂性是过度设计,即开发人员使代码变得比应有的通用性更高,或者增加了系统当前不需要的功能。 评论者应特别警惕过度设计。 鼓励开发人员解决必须立即解决的问题,而不是将来可能需要解决的问题。 将来的问题出现时应该解决,您可以在物理宇宙中看到它的实际形式和要求。

测验


请求与变更相关的单元,集成或端到端测试。 通常,如果CL无法处理紧急情况 ,则应将测试添加到与生产代码相同的CL中。

确保测试正确,合理且有用。 测试不会自我测试,我们很少为测试编写测试-一个人必须确保测试有效。

测试真的会因代码损坏而失败吗? 如果代码更改,会出现误报吗? 每个测试都做出简单有用的陈述吗? 是否在不同的测试方法之间正确划分了测试?

请记住,测试是必须支持的代码。 不要因为它们不是主二进制文件的一部分而让它们变得复杂。

命名


开发人员是否到处都选择了好名字? 一个好名字足够长,可以充分传达元素的含义或作用,而又不会太长而难以阅读。

留言


开发人员是否用通俗易懂的语言写了清晰的评论? 所有评论都必要吗? 当注释解释了为什么存在某些代码并且不应解释该代码做什么时,它们通常是有用的。 如果代码不够清晰,无法解释自身,则应将其简化。 有一些例外情况(例如,解释代码动作的注释对于正则表达式和复杂算法通常非常有用),但大多数注释旨在用于代码本身无法包含的信息,例如,证实决策。

查看前面代码中的注释也可能很有用。 也许有一个TODO(现在可以删除)或不建议引入此更改的注释等。

请注意,注释与类,模块或函数的文档不同,后者描述了代码的任务,应如何使用代码以及行为方式。

款式


我们拥有适用于所有主要语言甚至大多数次要语言的Google 样式指南 。 确保CL与相关的样式指南不冲突。

如果要改进样式指南中未包含的某些元素,请在注释中添加注释( Nit :) 。 开发人员将知道这是您的个人言论,并不具有约束力。 不要仅根据您的个人喜好阻止发送提交。

作者不应将重大的样式更改与其他更改结合在一起。 这使得很难看到CL中的更改,使合并变得复杂,代码回滚并导致其他问题。 例如,如果作者要重新格式化整个文件,请在一个CL中请求样式更改,然后将功能更改发送给CL。

该文件


如果CL输出在汇编,测试,交互过程或代码发布中有更改,请检查相关文档的更新,包括README文件,g3doc页面和所有生成的参考文档。 如果CL删除了代码或使其过时,请考虑是否还要删除文档。 如果缺少文档,请请求创建。

每行


查看代码中的每一行。 尽管可以简要回顾任何数据文件,生成的代码或大型数据结构,但请仔细阅读一个人编写的每个类,函数或代码块,默认情况下切勿假设一切都井井有条。 显然,某些代码比其他代码值得更彻底的研究-您自己决定,但至少应确保您了解整个代码操作。

如果您觉得代码太难阅读而又拖慢了审核速度,则应通知开发人员并等待开发人员明确说明再继续。 在Google,我们聘请了出色的软件工程师,您就是其中之一。 如果您无法理解代码,则其他开发人员很可能无法理解。 这样,当要求开发人员澄清时,您还可以帮助将来的开发人员理解此代码。

如果代码是可以理解的,但是您没有足够的资格来评估某个片段,请确保在CL中有一位合格的审阅者,尤其是对于诸如安全性,并发性,可访问性,国际化等复杂问题的审阅者。

语境


在广泛的背景下查看CL通常很有用。 通常,代码查看工具仅在更改位置附近显示几行。 有时,您需要查看整个文件以确保更改确实有意义。 例如,您看到仅增加了四行,但是如果查看整个文件,则这四行位于50行方法中,现在实际上必须分成较小的一行。

在整个系统的上下文中考虑CL也很有用。 它会提高系统代码的质量,还是使其变得更加复杂,测试更少等? 不接受会使系统代码降级的提交。 大多数系统由于许多小的变化而变得复杂,因此重要的是要防止即使很小的困难。

好啊


如果您在提交中看到良好的效果,请告知开发人员,尤其是当开发人员以最佳方式解决了您的注释中描述的问题时。 代码审查通常只是将重点放在错误上,但它们也应鼓励并重视良好实践。 从指导的角度来看,有时甚至更重要的是告诉开发人员他确实做了正确的事情,而不是他在哪里犯了错误。

总结


执行代码审查时,请确保:

  • 代码经过精心设计。
  • 功能对代码用户来说是好的。
  • 用户界面的任何更改都是合理的,而且看起来不错。
  • 任何并行编程都是安全的。
  • 代码并不比应有的复杂。
  • 开发人员不会实现前景未知的将来可能需要的功能。
  • 该代码内衬适当的单元测试。
  • 测试设计合理。
  • 开发人员到处都使用清晰的名称。
  • 评论是可以理解和有用的,并且基本上可以回答为什么的问题 但不是什么?
  • 该代码已正确记录(通常在g3doc中)。
  • 该代码符合我们的样式指南。

在审阅过程中,请务必查看代码的每一行 ,查看上下文 ,确保您正在改善代码的质量,并赞扬开发人员所能做的事情。

代码审查中的CL导航


总结


既然您知道要检入什么代码 ,对多个文件进行代码检查的最有效方法是什么?

  1. 这种变化有意义吗? 他的描述很好吗?
  2. 首先看最重要的部分。 设计得好吗?
  3. 按适当的顺序查看其余的CL。

第一步:查看整个提交


查看CL描述及其一般功能。 此更改是否完全有意义? 如果最初不应该编写它,请立即回复并解释为什么不需要它。 当您拒绝此类更改时,也可以向开发人员提供替代方法。

例如,您可能会说:“看起来您做得不错,谢谢!” 但是,实际上我们将删除FooWidget系统,因此我们现在不想进行任何新更改。 您如何重构我们的新BarWidget类呢?”

请注意,审阅者不仅拒绝了当前的CL并提出了替代建议,还礼貌地做到了。 这样的礼貌很重要,因为即使我们彼此不同意,我们也希望表明我们彼此尊重开发人员。

如果得到了很多不需要的CL,则应在编写CL时检查团队的开发过程或已发布的外部贡献者规则,以改善沟通。 最好在人们进行大量不得不扔掉或大量重写的工作之前告诉他们“不”。

第二步:学习CL的基本部分


找到一个或多个文件代表该CL的“主要”部分。 通常,一个文件的逻辑更改最多,这是CL的主要部分。 首先看一下这些主要部分。 这有助于了解CL较小部分的上下文,并且通常可以加快代码审查的执行速度。 如果CL太大,请询问开发人员首先要看的内容,或要求他将CL分成几部分

如果在CL的主要部分中发现严重问题,则即使没有时间立即查看CL的其余部分,也应立即发送这些评论。 实际上,查看其余的代码可能会浪费时间,因为如果问题足够严重,那么许多其他有问题的代码将消失并且不会有问题。

立即发送这些主要评论如此重要的主要原因有两个:

  • 开发人员经常发送CL,然后在等待代码审查结果的同时立即基于它开始新的工作。 如果CL存在严重问题,则他们将不得不重新进行下一个CL。 建议尽早发现错误,然后再在有问题的设计上做很多额外的工作。
  • 主要更改要比次要修改花费更长的时间。几乎所有开发人员都有最后期限。为了保留它们而不降低代码的质量,任何重大的重构都应尽快开始。

第三步:以适当的顺序滚动浏览CL的其余部分。


确保整个CL的设计没有严重问题之后,尝试找出查看文件的逻辑顺序,并确保没有遗漏任何内容。通常,当您查看主文件时,最简单的方法是简单地按照代码检查工具显示其余文件的顺序浏览其余文件。有时先阅读测试,再阅读主要代码有时也很有用,因为这样您将了解更改的含义。

代码审查速度


为什么要快速进行代码审查?


在Google,我们优化了协作的速度,而不是单个开发人员的速度。个人工作的速度很重要,与团队工作的速度相比,它并不是那么重要。

当您慢慢查看代码时,会发生几件事:

  • 整个团队的速度下降。是的,如果一个人对代码审查没有迅速做出反应,他将从事另一项工作。但是,对于团队的其他成员,由于每个CL等待代码审查和重复代码审查,因此新功能和错误修复会延迟数天,数周或数月。
  • -. , , . , «» . (, ), , . - .
  • 代码的质量可能会受到影响。放慢代码审查的速度,增加了开发人员将无法发送尽可能好的CL的风险。缓慢的审核也会阻碍代码清理,重构和对现有CL的进一步改进。

如何快速执行代码审查?


如果您不参与重点工作,则应在代码到达后立即进行审核。

一个工作日是回答问题的最长时间(也就是说,这是第二天早晨的第一件事)。

遵循这些准则意味着典型的CL应在一天之内接受几轮审核(如有必要)。

速度与分心


在某一时刻,个人速度优先于团队速度。如果您正在处理诸如编写代码之类的重点任务,请不要被代码审查分散注意力。研究表明,开发人员在分心之后要花很长时间才能恢复到平稳的开发流程。因此,分心编码实际上使团队付出了比要求另一名开发人员稍等一会儿进行代码审查的代价。

相反,请等待工作中的断点。例如,在完成当前任务之后,午餐后,从会议返回,从办公室小厨房返回等。

快速回复


当我们谈论代码审查速度时,我们对响应时间感兴趣,而不是整个过程结束需要多少时间。理想情况下,整个过程也应该是快速的,但与整个过程相比单个答案的迅速到达甚至更为重要

即使整个代码审查过程很耗时,审查员在整个过程中快速响应的可用性也大大简化了开发人员的工作,这些开发人员可能会对“缓慢”的响应感到恼火。

如果您在收到请求后太忙而无法立即进行全面审核,则仍然可以快速回复有关截止日期的消息,或将审核提供给可以更快答复的其他审核者,或者提供一些初步的一般性注释(注意:这都不意味着您必须中断编码甚至发送此类响应-在工作中的合理断点处发送响应)。

审阅者应花费足够的时间进行审阅,并确保其“ LGTM”的意思是“该代码符合我们的标准,这一点很重要但是,理想情况下,单个答案应该应该很快

时区之间的代码审查


在不同时区之间工作时,尝试在作者还在办公室时回答作者。如果他已经回家,请务必在第二天返回办公室之前尝试发送答案。

保留LGTM


出于速度原因,在某些情况下,即使在CL上未答复的情况下,审阅者也必须给予LGTM /批准。如果满足以下条件,则完成此操作:

  • 审阅者确信开发人员将适当考虑所有剩余的评论。
  • 其他更改是次要的和可选的

如果不清楚,审阅者必须指出他指的是哪个选项。

当开发人员和审阅者位于不同时区时,预留的LGTM特别有用,否则开发人员将全天等待获得“ LGTM批准”。

大CL


如果有人向您发送了一个很大的审阅代码,您不确定何时可以查看它,那么通常的答案是要求开发人员将CL拆分为几个较小的CL即使需要开发人员的其他工作,这通常也是可能的,并且对于审阅者非常有用。

如果无法将CL分解为较小的CL,并且您没有时间快速浏览所有这些内容,则至少要对整体CL设计发表一些评论,然后将它们发送回给开发人员以进行改进。作为审阅者,您的目标之一应该始终是“解除开发人员的锁定”,或者允许他在不牺牲代码质量的情况下迅速采取进一步的措施。

随着时间的推移,代码审查的改进


如果遵循这些建议并严格执行代码审查,您会发现整个过程会随着时间的推移而加速发展。开发人员将找出高质量代码的要求,并从一开始就向您发送出色的CL,所需的查看时间越来越少。审阅者学会快速做出响应,而不会在审阅过程中添加不必要的延迟。但是,不要为了提高速度而牺牲代码审查标准或质量-实际上,从长远来看,您不会实现整体加速。

紧急情况


在某些紧急情况下,CL必须非常快速地通过整个代码审查过程,并且质量原则必须得到软化。但是,请阅读有关哪些情况属于紧急情况而哪些情况不属于紧急情况描述

如何在代码审查中编写注释


总结


  • 要有礼貌。
  • 说明您的推理。
  • 只需指出问题并让开发人员决定,就可以避免明确的命令。
  • 鼓励开发人员简化代码或添加注释,而不是简单的复杂说明。

礼貌


通常,礼貌和尊重是很重要的,并且对于正在查看其代码的开发人员也非常清楚和有帮助。一种方法是确保您始终对代码发表评论,而不对开发人员发表评论您不必总是遵循这种做法,但是当您说不愉快或有争议的内容时,必须使用它。例如:

坏:“ 如果很明显并发没有好处,为什么要在这里使用流?”

好:“这里的并发模型增加了系统的复杂性,我看不到任何实际的性能优势。由于没有性能优势,因此最好将此代码为单线程而不是使用多个线程。”

说明原因


在上面的“好”示例中,您可以看到它可以帮助开发人员理解为什么要发表评论。您不一定总是在注释中包含此信息,但是有时适当地多解释一些逻辑,遵循的最佳实践或您的建议如何提高代码质量。

方向


通常,进行更正是开发人员而不是审阅者的任务。您无需开发详细的解决方案草稿或为开发人员编写代码。

但是,这并不意味着审阅者无法在这里提供帮助。一般而言,应在发现问题和提供直接援助之间取得适当的平衡。指出问题并为开发人员提供决策的机会通常有助于开发人员学习并简化代码审查。这也可能导致更好的解决方案,因为开发人员比审阅者更接近代码。

但是,直接的指令,建议甚至代码有时更有用。代码审查的主要目的是获得最佳的CL。次要目标是提高开发人员的技能,以便他们进行审核的时间越来越少。

接受说明


如果您要求开发人员解释一段难以理解的代码,通常这将导致他将更清楚地重写它有时添加注释也是一个适当的答案,如果这不仅仅是对过于复杂的代码的解释。

仅在审阅工具中编写的说明对将来的读者没有帮助。它们仅在某些情况下是可以接受的,例如,当您查看一个不太熟悉的区域时,开发人员将说明普通代码读者应该已经知道的内容。

如何克服代码审查过程中的阻力


有时,开发人员会在代码审查过程中争论。他要么不同意您的建议,要么抱怨您总体上过于严格。

谁是对的?


如果开发人员不同意您的建议,请先花一些时间考虑他的立场。通常,它更接近您的代码,因此对某些方面可能确实有更好的了解。他的论点有意义吗?就代码质量而言,这有意义吗?如果是这样,则承认他是对的,这个问题将消失。

但是,开发人员并不总是正确的。在这种情况下,审阅者必须进一步解释为什么他认为自己的建议是正确的。良好的解释既可以说明对开发人员的响应,也可以说明为什么需要进行更改。

特别是,当审阅者认为他的提案将改善代码的质量时,如果他认为所产生的质量改进可证明进行其他工作是合理的,则应坚持执行。通常,提高代码质量的过程很少。

有时需要经过几轮解释才能真正被接受。只是确保您始终保持礼貌,并让开发人员知道您听到了,只是不同意。

关于开发商的不满


如果审稿人坚持要改进,那么审稿人有时会感到开发人员很沮丧。有时开发人员真的很沮丧,但通常不会持续很长时间,后来他们将非常感谢您帮助他们提高了代码质量。通常,如果您在评论中很有礼貌,那么开发人员根本就不会感到烦恼,而问题只在审阅者的头上。通常,编写注释样式比审阅者对代码质量的持久性更令人沮丧

待修改


引起争议的典型原因是,开发人员(出于明显的原因)希望完成工作。他们不想对该CL进行另一轮审查。因此,他们说他们将在以后的CL中删除某些内容,现在您必须为此 CL 创建LGTM 。一些开发人员做得很好,然后立即编写另一个CL来解决问题。但是,经验表明,原始CL之后经过的时间越长,进行此编辑的可能性就越小。实际上,通常情况下,如果开发人员不立即进行编辑通常不会。并不是因为开发人员不负责任,而是因为他们有很多工作,而在其他工作的推动下编辑工作却被丢失或遗忘了。因此,通常最好在提交进入代码库并“完成”之前坚持立即修复。允许挂起的编辑是退化代码库的一种典型方法。

如果CL引入了新的复杂性,那么如果不是紧急情况,则必须在发送之前将其修复。如果CL显示其他目前无法解决的问题,则开发人员应在跟踪器中注册一个错误并将其分配给自己,以免他迷路。他可以选择在有关此错误的代码中编写TODO注释。

一般严重性投诉


如果您过去进行了相当肤浅的代码审查,并切换到严格模式,则某些开发人员将开始大声抱怨。提高代码审查速度通常可以解决这些问题。

有时,投诉可能要花费数月的时间才能消失,但最终,开发人员往往会看到严格的代码审查的价值,因为他们看到了代码的功能。有时,当某些事情使他们真正看到严格审核的价值时,最响亮的抗议者甚至会成为您最强大的支持者。

解决冲突


如果您已完成上述所有操作,但仍遇到冲突,请参阅“ 代码审查标准”部分,以获取有助于解决冲突的建议和原则。

Source: https://habr.com/ru/post/zh-CN467039/


All Articles