Google 是如何做 Code Review 的?10bet

来源:http://www.chinese-glasses.com 作者:Web前端 人气:165 发布时间:2020-03-31
摘要:时间: 2019-10-19阅读: 179标签: 开源代码评审标准 我和几个小伙伴一起翻译了Google前一段时间放出来的Google’s EngineeringPracticesdocumentation,翻译后的GitHub仓库:,欢迎加star。目前只是翻译

时间: 2019-10-19阅读: 179标签: 开源代码评审标准

我和几个小伙伴一起翻译了Google前一段时间放出来的Google’s Engineering Practices documentation,翻译后的GitHub仓库:,欢迎加star。目前只是翻译完了,因为译者水平有限,还需要审校。另外后续Google肯定还会有新内容放出来,欢迎想参与贡献的小伙伴加入,也欢迎在GitHub上加star。

代码评审的主要目的是确保 Google 代码库的整体代码运行状况随着时间的推移而不断改善。代码评审的所有工具和过程都是为此设计的。

这篇文章是Google’s Engineering Practices documentation的第一章Code Review实践指南:。

为了实现这一点,必须做出一系列的权衡。

谷歌Code Review指南, 包含两个子章节:

首先,开发人员必须能够在其任务上取得进展。如果开发人员从未向代码库提交过改进,那么代码库将永远不会得到改善。另外,如果评审人员不进行任何更改,那么之后开发人员也没有动力进行改进。

评审者指南:开发者指南:

另一方面,评审人员有责任确保每个 CL(变更列表)的质量,使得其代码库的整体代码运行状况不会随着时间的流逝而减少。这可能很棘手,因为随着时间的推移,代码库通常会由于代码运行状况的小幅下降而退化,尤其是在开发团队时间紧迫,认为必须采取捷径才能实现其目标的情况下。

术语

此外,评审人员对他们正在评审的代码要负起责任。确保代码库保持一致性和可维护性,以及“代码评审中的查找内容”中提到的其他事项 。

部分文档中会用到一些谷歌内部的术语,特在此说明:

因此,我们将以下规则作为代码评审中期望的标准:

CL: “changelist"的缩写,代表已经进入版本控制软件或者正在进行代码评审的变更。其他组织经常称为"change"或者"patch”。

通常,即使 CL 并不完美,不过如果达到可以提升系统整体代码质量的程度,评审人员就可以批准它。

LGTM: "Looks Good to Me."的缩写,看起来不错,评审者批准CL时会这么说。

这是所有代码评审指南中的最高原则。

评审者指南

当然,是有例外的。例如,如果 CL 添加了评审人员不希望在其系统中使用的功能,那么即使代码设计得当,评审人员也可以肯定的拒绝批准。

Code Review标准:Code Review的主要目的是始终保证随着时间的推移,谷歌代码越来越健康,所有Code Review的工具和流程也是针对于此设计的。

这里的关键点是,没有“完美”的代码,只有更好的代码。评审人员不应要求开发人员在批准前抛光 CL 的每一小块细节。评审人员要追求的不应该是完美,而应该是持续改进。总体而言,如果一个 CL 可以提高系统的可维护性,可读性和可理解性,那就不应该因为它不是“完美的”而被延迟几天甚至几周。

为了完成这点,我们不得不权衡利弊。

评审人员应该随时提供建议,这样开发人员可能会做得更好,但是如果不是很重要的建议,可以在其前面加上“ Nit:”之类的字眼,以使开发人员知道这只是一个改进建议,他们可以选择忽略。

首先,开发者应当在他们代码中做一些 改进 ,如果你永远都不做出改进,代码库整体质量也不会提升。但是如果审查者为难所有变更,开发者未来也会失去改进的动力。

指导

另一方面,保证代码库随时间推移越来越健康是审查者的责任,而不是让代码库质量变得越来越差。这很棘手,因为代码质量一般都会随着时间推移越来越差,尤其是在团队有明确时间限制、而且他们觉得不得不采取一些投机取巧的方式才能完成任务的情况下。

代码评审具有重要的功能,可以传授开发人员关于语言,框架或通用软件设计原理的新知识。随着时间的推移共享知识会成为改善系统代码运行质量的一部分。但要注意,如果你的建议纯粹是带有教育性质的,并且对于满足本文所描述的标准来说并不是那么重要,那么请在前面加上“Nit:”,或者以其他方式告诉开发人员,他们并不一定要在 CL 中解决这些问题。

但是,代码评审者得对他们Review的代码负责,所以他们想始终确保代码库一致、可维护

原则客观的技术和数据比个人意见和偏好更重要。在代码风格方面,样式指南是绝对权威。不在样式指南中的任何纯样式点(空格等)都是个人喜好问题。样式应与那里的样式保持一致。如果没有以前的样式,请接受作者的样式。软件设计方面从来都不是纯粹的样式问题,也不是个人喜好。它们基于基本原则,应权衡这些原则,而不仅仅是个人意见。有时有一些有效的选择。如果开发 人员可以证明(通过数据或基于可靠的工程原理)几种方法同样有效,那么评审人员应接受开发人员的选择。否则就应该基于软件设计标准原则做出决定。如果没有其他的适用规则,那么评审人员可以要求开发人员与当前代码库中的内容保持一致,只要不会使系统的总体代码运行状况恶化就可以。解决冲突

依据这些,我们将以下准则作为我们期望的Code Review标准:通常而言,只要代码对系统有明显的提升且正常工作,即便不完美,评审者也应该倾向于通过这次变更。

在代码评审中发生冲突时,第一步应始终是使开发人员和评审人员根据本档以及《 CL 作者指南》 和《审阅者指南》中其他文档的内容达成共识。

这是所有Code Review指南中的高级原则。

当达成共识变得特别困难时,让开发人员和评审人员进行面对面的会议或 VC 可能会有所帮助,而不仅仅是尝试通过代码评审注释来解决冲突。(但是,如果这样做,请确保将讨论的结果记录在 CL 上的注释中,以备将来阅读。)

当然这也有些局限。例如,如果变更里加入了有些评审者在系统里不想要的功能,即便代码设计的很好,评审者也可以拒绝掉。

如果还不能解决问题,那么就要考虑把问题升级。升级的途径通常是进行更广泛的团队讨论,其中包括让团队负责人参与进来,请求代码维护人员作出决定,或请求工程经理提供帮助。不要因为开发人员和评审人员无法达成一致意见就让 CL 一直挂在那里。

关键点是没有任何完美的代码,只有更好的代码。代码评审者绝对不应该要求开发者在批准前对变更中的每一个小块进行打磨,相反,评审者应该权衡向前推进和他们采纳他们建议二者的重要性。评审者应该追求 持续提高 ,而不是追求完美。那些可以提升整个系统可维护性、可读性和可以理解性的变更不应该因为代码不够完美而被推迟几天甚至几周。

代码评审中的查找内容设计

评审者要 始终 不拘谨于在代码评论里提示可以更好的想法。但如果不是很重要信息,可以在评论前面加上标识告诉他们可以忽略。

代码评审中最重要的部分是 CL 的总体设计。CL 中各种代码的交互是否有意义?此更改是属于代码库还是属于某个包?它与系统的其余部分是否集成良好?现在是添加此功能的好时机吗?

注意:这篇文档中没有任何地方辩解在变更中的检查会让整个系统代码变得 更糟糕 。你唯一能做的在紧急状况中说明。

功能性

指导性

此 CL 是否达到开发人员的预期目的?开发人员打算为该代码的用户带来什么好处?“用户”通常既是最终用户(当他们受到更改影响时)又是开发人员(将来他们将不得不“使用”此代码)。

Code Review有个重要的作用,那就是可以教会开发者关于语言、框架或者通用软件设计原理。在Code Review中留下评论来帮助开发者学习新东西是很值得提倡的,毕竟共享知识也是长期提升系统代码健康度的一部分。但请注意,如果你的评论纯粹是教育性的,并且不是这篇文档中提到的关键标准,请在前面加上“Nit:”标识,或者明确指出不需要在这次变更中解决。

通常,我们希望开发人员能够对 CL 进行良好的测试,以确保它们在进行代码审查时能够正常工作。但是,作为评审人员,仍然应该考虑边缘情况,寻找并发性问题,尝试像用户一样思考,并找出仅仅通过阅读代码不能看到的错误。

原则

您可以根据需要验证 CL,对于评审人员来说,检查 CL 的行为最好的时机是当它对用户产生了影响,例如 UI 更改。当只是阅读代码的时候,很难理解一些更改将如何影响用户。对于这样的更改,如果过于麻烦而无法在 CL 中打补丁并自己尝试,则可以让开发人员提供功能演示。

技术和数据高于意见和个人偏好。

在代码评审期间考虑功能性的另一个时机点是,CL 中是否正在进行某种并行编程,从理论上讲可能导致死锁或竞争条件。仅通过运行代码很难检测到这类问题,通常需要有人(开发人员和评审人员)仔细考虑它们,以确保不会引入问题。

关于风格问题, 风格指南是绝对的权威。任何不在样式指南中指出的样式(比如空格等)都是个人偏好的问题。风格应该与现有的一致。如果没有以前的风格,就按作者的风格来。

复杂性

软件设计从来不是纯粹的代码风格或是个人偏好问题,它们是基于一些应当被权衡的规则而不仅仅是个人倾向。有时候也会有多种有效的选项,如果开发者能证明(通过数据或者原理)这些方法都同样有效,那么评审者应该接受作者的偏好,否则应该遵从软件设计标准。

CL 是否比实际需要的要复杂?在 CL 的每个级别都进行检查 —— 个别行是否太复杂?功能太复杂?类太复杂?“过于复杂”通常意味着“代码阅读器无法快速理解。”也可能意味着“开发人员在尝试调用或修改此代码时可能会引入错误。”

如果没有其他的规则使用,只要保证不会影响系统的健康度,评审者可以要求开发者保持和现有的代码库一致。

过度设计一种特殊的复杂性,即开发人员使代码变得比实际需要的通用,或者增加了系统目前不需要的功能。评审人员应特别警惕过度设计。鼓励开发人员解决他们现在需要解决的已知问题,而不是解决开发人员推测的将来可能需要解决的问题。未来的问题应该在它们出现之后再去解决,因为到了那个时候我们可以看到它们的实际状况和需求。

解决代码冲突

测验

如果Code Review中有任何冲突,开发人员和评审人员都应该首先根据开发者指南和评审者指南中其他文档的内容,尝试达成一致意见。

根据更改要求进行单元测试,集成测试或端到端测试。通常,除非 CL 处理紧急情况,否则应在与生产代码相同的 CL 中添加测试 。

当很难达成一致时,开发者和评审者不应该在Code Review评论里解决冲突,而是应该召开面对面会议或者找个权威的人来协商。

确保 CL 中的测试正确,合理且有用。测试无法自我测试,我们很少为测试编写测试,所以必须确保测试有效。

如果这样都解决不了问题,那解决问题的方式就应该升级了。通常的方式是拉着团队一起讨论、让团队主管来权衡、参考代码维护者的意见,或者让管理层来决定。不要因为开发者和评审者不能达成一致而把变更一直放在那里。

代码破损时,测试会失败吗?如果代码在其下面更改,它们将开始产生误报吗?每个测试都会做出简单而有用的断言吗?测试是否在不同的测试方法之间适当地分开?

Code Review应该关注什么?

请记住,测试也是必须维护的代码。

注意:当我们考虑以下点时,应当始终遵循Code Review标准。

命名

设计

开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么,但又不会因为太长而难以阅读。

Code Review中最重要的一个点就是把握住变更中的整体设计。变更中各个部分的代码交互是否正常?整个改动是否属于你负责的代码库?是否和你系统中其他部分交互正常?现在是否是添加整个功能的恰当时间?

注释

功能性

开发人员是否用可理解的自然语言写下清晰的注释?所有注释都是必要的吗?通常,好的注释应该解释为什么存在某些代码,而不应该解释某些代码在做什么。如果代码不够清晰,无法解释自身,则应使简化代码。也有一些例外情况(例如,正则表达式和复杂算法通常会在注释中解释它们的作用,会让阅读代码的人受益匪浅),但大多数注释是针对代码本身可能无法包含的信息,比如这些代码背后的缘由。

开发者在这个变更中想做什么?开发人员打算为该代码的用户带来什么好处?

查看此 CL 之前的注释也会有所帮助。也许有一个待办事项现在可以删除,有意见建议不要进行此更改,等等。

重要的是,我们希望开发者能充分测试代码,以确保代码在Code Review期间正常运行。但无论如何,你作为审查者也要考虑一些特殊情况,像是有些并发问题。站在用户的角度,确保你正在看的代码没有bug。

请注意,注释与类,模块或函数的文档不同,注释应表示一段代码的用途,应如何使用以及使用时的行为。

你可以验证变更,尤其是在有面向用户的影响时,评审者应该仔细检查整个变更。有时候单纯看代码很难理解这个变更如何影响到用户,这种情况下如果你不方便自己在CL中打补丁并亲自尝试,你可以让开发者为你提供一个功能性的demo。

代码风格

另一个在Code Review时需要特别关注的点是,CL中是否有并发编程,并发理论上可能会导致死锁或资源争抢,这种问题在代码运行时很难被检测出来,所以需要有人仔细考虑整个逻辑,以确保不会引入线程安全的问题。

谷歌为主要编程语言和大多数次要编程语言提供了代码风格指南,确保 CL 遵循适当的风格指南。

复杂性

如果您想改善指南中没有的样式点,请在注释前面加上“ Nit:”,以使开发人员知道这是您认为可以改善代码但不是强制性的选择。但请不要只是基于个人偏好来阻止 CL 的提交。

变更是否比预期的更复杂?检测变更的每个级别,是否个别行太复杂?功能是否太复杂?类是否太复杂?”复杂“意味着代码阅读者很难快速理解代码,也可意味着开发者尝试调用或修改此代码时可能会引入bug。

开发人员不应将主要风格更改与其他更改结合在一起。这使得很难看到 CL 中的更改,使合并和回滚更加复杂,并导致其他问题。例如,如果开发人员想要重新格式化整个文件,请他们仅将重新格式化的格式作为一个 CL 发送给评审人员,然后再发送另一个具有功能更改的 CL。

一个典型的复杂性问题就是 过度设计,当开发者让代码变得更通用,或者增加了许多当前不需要的功能时,评审者应该额外注意是否过度设计。鼓励开发者解决现在遇到的问题,而不是揣测未来可能遇到的问题。未来的问题应当在遇到时解决,到那个时候你才能看到问题本质和实际需求。

文档

测试

如果 CL 改变了用户构建,测试,与代码交互或释放代码的方式,请检查其是否还更新了相关文档,包括 README,g3doc 页面和其他生成的参考文档。如果 CL 删除或弃用了代码,请考虑是否还应删除该文档。如果文档缺失,要向开发人员索要。

开发人员应当进行单元测试、集成测试或端到端测试。一般来说,变更中应该包含测试,除非这个变更只是为了处理紧急情况。

每一行代码

确保变更中的测试是正确、合理和有用的。因为测试本身无法测试自己,而且我们很少会为测试编写测试,所以必须确保测试是有效的。

查看每一行代码。有时可以扫描诸如数据文件,生成的代码或大型数据结构之类的东西,但不要扫描人工编写的类,函数或代码块,并假设其中的内容是可以正常运行的。显然,某些代码比其他代码更需要仔细检查 —— 至于是哪些代码完全取决于你,但你至少应该要理解这些代码都在做些什么。

如果代码出了问题,测试会失败吗?如果代码发生改动,它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?

如果代码很复杂或者你难以快速看懂它们,这会使评审速度变慢,那么你应该让开发人员知道这一点,并等待他们解释,然后再尝试评审。Google 聘请了出色的软件工程师,如果他们看不懂代码,其他开发人员也很可能看不懂。因此,要求开发人员进行解释,也可以帮助将来的开发人员理解此代码。

记住,不要以为测试不是二进制中的一部分就不关注其复杂度。

如果您理解代码,但又没有资格进行代码评审,请确保在 CL 上有一位合格的评审人员,特别是在复杂性问题(例如安全性,并发性,可访问性,国际化等)上。

命名

上下文

开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项是什么或者用来做什么,但又不至于让人难以阅读。

通常,代码查看工具只会显示需要更改部分的几行代码。但是有时必须查看整个文件,以确保更改确实有意义。例如,你可能会看到仅添加了四行代码,但是当你查看整个文件时,您会看到这四行位于 50 行方法中,这个时候需要将其分解为较小的方法。

注释

需要基于整个系统来考量 CL 。此 CL 是在改善系统的代码运行状况,还是使整个系统更加复杂,更不可测等?不要接受会降低系统代码运行状况的 CL。大多数系统会通过许多小的更改而变得复杂,因此,防止新更改中出现很小的复杂性是很重要的。

开发者有没有写出清晰易懂的注释?所有的注释都是必要的吗?通常注释应该解释清楚为什么这么做,而不是做了什么。如果代码不清晰,不能清楚地解释自己,那么代码可以写的更简单。当然也有些例外,但大多数注释应该指代码中没有包含的信息,比如代码背后的决策。

好的方面

变更中附带的其他注释也很重要,比如列出一个可以移除的待办事项,或者一个不要做出代码变更的建议,等等。

如果在 CL 中看到不错的东西,请告诉开发人员,尤其是当他们以出色的方式回答了你的评论时。代码评审通常只关注错误,但是也应该鼓励和赞赏良好的实践。有时候告诉开发人员正确的做法比告诉他们错误的做法更有价值。

注意,注释不同于类、模块或函数文档,文档是为了说明代码片段如何使用和使用时代码的行为。

总结

风格

在进行代码评审时,你应确保:

在谷歌内部,主流语言甚至部分非主流语言都有编码风格指南 ,确保变更遵循了这些风格规范。

代码经过精心设计。该功能对代码的用户来说有用。UI 的变更合理。任何并行编程都是安全完成的。代码复杂性不要超过应有的程度。不需要实现可能会在未来出现的需求。代码具有适当的单元测试。精心设计的测试用例。开发人员对所有内容都清晰的命名。清晰而有用的代码注释,要解释“为什么”,而不是“什么”。恰如其分的代码文档化。该代码符合我们的风格指南。

如果你想对风格指南中没有提及的风格做出改进,可以在注释前面加上“Nit:”,让开发人员知道这是一个你认为可以改进的地方,但不是强制性的。但请不要只是基于个人偏好来阻止变更的提交。

确保检查的每一行代码,查看上下文,确保改善代码运行状况,并称赞开发人员所做的出色工作。

开发人员不应该将风格变更与其他变更放在一起,这样很难看出变更里有哪些变化,让合并和回滚变得更加复杂,也会导致更多的其他问题。如果开发者想要重新格式化整个文件,让他们将重新格式化后的文件作为单独的变更,并将功能变更作为另一个变更。

检查 CL摘要

文档

在知道了代码评审要关注哪些东西之后,如何有效地进行跨文件代码评审呢?

如果变更改变了用户构建、测试、交互或者发布代码相关的逻辑,检测是否也更新了相关文档,包括READMEs, g3doc页面,以及任何相关文档。如果开发者删除或者弃用某些代码,考虑是否也应该删除相关文档。如果文档有确实,让开发者补充。

更改代码有意义吗?它有一个很好的描述吗?首先看一下变化中最重要的部分,整体设计得如何?以适当的顺序查看其余的 CL。第一步:全面了解代码变更

代码细节

查看 CL 说明以及 CL 的一般功能。这种变更有意义吗?如果这个变更是不必要的,请立即做出答复,并说明为什么不应该进行更改。当您拒绝这样的更改时,最好还是向开发人员建议他们应该做些什么。

查看你整个Code Review中的每一行代码,比如你可以扫到的数据文件,生成的代码或大型数据结构,但不要只扫一遍手写的类,函数或者代码块,然后假设它们都能正常运行。显然,有些代码需要仔细检查,这完全取决于你,但你至少应该理解所有的代码都在做什么。

例如,您可能会说:“看起来您为此做了一些出色的工作,谢谢!但是,实际上我们正打算移除这个系统,因此我们现在不希望对其进行任何新的修改。或许,您可以重构我们的新 BarWidget 类吗?”

如果你很难看懂代码,导致Code Review的速度慢了下来,你要让开发者知道,并且在Review前澄清原因。在谷歌,我们有最优秀的工程师,你也是其中之一。如果你都看不懂,很可能其他人也看不懂。所以要求开发者理清代码逻辑也是在帮助未来的开发者。

请注意,评审人员在拒绝当前的 CL 时要一并提出其他建议,而且还要表现地礼貌。这种礼貌很重要,因为作为开发人员,即使我们不同意,也要表明我们彼此尊重。

如果你理解代码,但又觉得没有资格做代码评审,确保有资格的变更评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。

如果出现多个 CL 是你不想进行的更改,则应考虑重新设计团队的开发流程或外部贡献者的已发布流程,以便在编写 CL 之前进行更多的沟通。最好在人们完成大量现在必须扔掉或彻底重写的工作之前告诉他们“不”。

上下文

第二步:检查 CL 的主要部分

看改动上下文代码对Code Review很有帮助,因为通常Code Review工具只会显示改动部分上下几行代码,但有时你必须查看整个文件确保这次变更可以正常运行。例如,你可能看到加了4行代码,但是看到整个文件时才会发现这加的4行代码是在一个50多行的方法里,这个方法现在应该被拆分为多个小的方法了。

查找属于此 CL 的“主要”部分的文件。通常,逻辑更改数量最多的文件是 CL 的主要部分。首先看这些主要部分。这有助于为 CL 的所有较小部分提供上下文,并通常加快执行代码评审的速度。如果 CL 太大,您无法确定哪些部分是主要的,请询问开发人员您应该首先看什么,或要求他们将 CL 分成多个 CL。

Code Review时考虑到整个系统的上下文也很重要,这次改动提升了系统健康度?或者增加了系统复杂性?少了测试用例?…… 不要通过哪些会损害系统健康的代码。很多系统变复杂都是因为一点一点的小改动日积月累造成的,所以千万不要放进来任何增加复杂性的改动。

如果您发现 CL 的这一部分存在一些主要的设计问题,要立即回复开发人员,即使您现在没有时间评审 CL 的其余部分。实际上,复查 CL 的其余部分可能会浪费时间,因为如果设计问题足够严重,那么许多其他代码都会变得无关紧要。

亮点

为什么要立即回复开发人员有两个主要原因:

如果你看到变更中做的好的地方,告诉开发者他们做的很棒,尤其是当他们用某种很精巧的方式解决你评论中提到的问题时更应不吝赞美。Code Review过多关注于错误,但你也应该为开发者展现出好的一面点赞。在指导他人的时候,有时候告诉开发者正确的做法比告诉他们错误的做法更有价值。

开发人员通常会发出一个 CL,然后在等待审核时立即基于该 CL 进行新工作。如果您正在评审的 CL 中存在重大设计问题,那么他们也将不得不重新设计其以后的 CL。所以,最好赶在开发人员在有问题的设计上花费不必要的时间之前告诉他们。重大的设计变更比小的变更需要更长的时间。为了在截止日期之前完成工作,并在代码库中保留高质量的代码,开发人员需要尽快开始 CL 的所有重写工作。第三步:按适当顺序检查其余的 CL

总结

确认 CL 整体上没有大的设计问题后,请尝试找出逻辑顺序来浏览文件,同时还要确保不要错过对任何文件的评审。通常,在浏览了主要文件之后,按照代码评审工具向您展示它们的顺序浏览每个文件是最简单的。有时在阅读主要代码之前先阅读测试也是有帮助的,因为这样您就可以知道更改应该做什么。

在做Code Review时,你需要注意以下:

代码审查速度为什么代码审查应该很快?

代码设计良好。

在谷歌,我们对开发团队的整体交付速度(而不是针对个体开发人员写代码的速度)进行了优化。个体开发速度也很重要,但其重要性比不上整个团队的开发速度。

代码功能对用户有用。

当代码评审速度很慢时,会发生以下几件事:

任何UI改动应当是深思熟虑过而且看起来不错的。

团队的整体开发速度降低了。如果个体开发人员无法快速地对评审做出响应,可能是因为他们有其他事情要做。但是,如果每个 CL 都要等待一次又一次的评审,那么团队其余成员的新功能和错误修复就会被延迟了几天,几周或几个月。开发人员开始抗议代码评审过程。如果评审人员仅每隔几天回复一次,但每次都要求对 CL 进行重大更改,那么这对于开发人员来说可能是非常令人沮丧且困难的。通常,这表示为对评审人员的“严格”程度的抱怨。如果评审人员能够快速提供反馈(确实可以改善代码运行状况的更改),抱怨就会消失,即使他们要求做出的修改是一样的。实际上,大多数对代码评审过程的抱怨都可以通过加快评审过程来解决。代码的质量可能会受到影响。当评审速度缓慢时,开发人员的压力也会随之增加,因为他们不能提交不甚完美的 CL。缓慢的评审流程还会阻碍代码清理、重构和对现有 CL 做出进一步改进。代码审查应该有多快?

保证线程安全。

如果你不是在集中精力完成手头的任务,那就应该在第一时间评审代码。

代码没有增加不必要的复杂性。

一个工作日是响应代码检查请求所需的最长时间(即第二天早上的第一件事)。

开发者没有写有些将来需要但现在不知道是否需要的东西。

遵循这些准则意味着典型的 CL 应在一天之内进行多轮评审(如果需要)。

代码有适当的单元测试。

速度与中断

测试逻辑设计良好。

有一种情况,个人速度的考虑胜过团队速度。如果您正在处理诸如编写代码之类的重点任务,请不要打断自己去进行代码检查。研究表明,开发人员在中断之后要花很长时间才能恢复到平稳的开发流程。因此,在编写代码时打断自己,实际上对团队来说,要比让其他开发人员稍等一会儿进行代码评审更为昂贵。

开发者使用了清晰明了的命名。

所以,对于这种情况,可以等到你手头工作可以停了再开始代码评审。可以是在完成手头的编码任务之后,午饭后,会议结束后,休息结束后等。

注释清晰明了实用,通常解释清楚了为什么这么做,而不是做了啥。

快速响应

代码又相应完善的文档。

我们所说代码评审速度指的是响应时间,而不是 CL 通过整个评审流程并提交所花的时间。整个过程在理想情况下也应该是很快的,但单次评审请求的响应速度比整个过程的响应速度更重要。

代码风格符合规范。

即使有时需要很长时间才能完成整个评审过程,在整个评审过程中获得评审人员的快速响应也可以极大地缓解开发人员对“缓慢”代码评审的不满。

确保你review了要求你看的每一行代码,确保你正在提升代码质量,并且为开发者做的提升点赞。

如果您忙于在 CL 上进行全面评审时,仍然可以先发送一个快速响应,以使开发人员知道什么时候可以开始评审,或者建议让其他可以更快做出响应的评审人员来评审代码,或者提供一些初步反馈。

Code Review步骤

重要的是,评审人员应花费足够的时间进行评审,以确保此代码符合标准。但不管怎样,最好响应速度还是要快一些。

概要

跨时区代码评审

现在你知道了要从CL中得到什么,那能在众多文件中完成评审的方法是什么?

在处理跨时区代码评审时,请在他们仍在办公室时尝试与开发人员联系。如果他们已经回家了,那么最好可以确保他们在第二天回到办公室时可以看到代码评审已经完成。

这条改动是否生效?这次变更是不是描述清晰?

LGTM 带注释

首先阅读CL最重要的一部分,整体上是否设计良好?

为了加速代码评审速度,在某些情况下,即使审核人员也将未解决的评论留在了 CL 上,评审人员仍应该给予 LGTM/批准。在以下情况之一时完成此操作:

按照合适的顺序阅读剩下的变更。

评审人员确信开发人员将会处理好评审人员给出的建议和意见。其余的改动是次要的,不一定要求开发人员完成。

第一步: 综观这个改动

除非另有明确说明,否则评审人员应使用这些选项中的一个。

阅读CL描述并且明白CL大体内容。这些修改是否有意义?首先如果这个修改不应该有,请立刻说明为什么这些修改不应该有。当你因为这个拒绝了这次改动提交时,告诉开发人员该怎么去做是非常好的。

当开发人员和评审人员处于不同时区时,带有注释的 LGTM 特别值得考虑,否则开发人员将等待一整天才获得“ LGTM,批准”。

例如,您可能会说:“看起来您为此做了一些出色的工作,谢谢!但是,我们实际上正着手删除FooWidget系统,您正在此处进行修改,因此我们不想进行任何新的修改现在。所以,您可以重构我们的新BarWidget类吗?"

大型的 CL

请注意,审阅者不仅拒绝了当前的CL并提供了替代建议,但他们礼貌地做到了。这种礼貌是重要,因为我们想表明我们甚至在开发人员之间也相互尊重,尤其是当我们意见不同时。

如果有人向您发送了一个很大的代码评审,您不确定何时可以有时间对其进行评审,通常的响应应该是要求开发人员将 CL 拆分为多个相互构建的较小的 CL。 而不必一次审查所有巨大的 CL。这样通常是合理的,并且对评审人员很有帮助,即使需要开发人员做些额外的工作。

如果您得到的多个CL并且您不想进行的更改,您应该考虑重新设计团队的开发流程或发布的外部贡献者的流程,以便在CL完成之前进行更多的交流。最好在做大量工作之前告诉人们“不必做”,因为现在要将其丢弃或彻底重写。

如果不能将一个 CL 分解为较小的 CL,并且您没有时间快速评审整个 CL,那么至少要对 CL 的总体设计写一些注释,然后将其发回给开发人员进行改进。作为评审人员,您的目标之一应该是是在不影响代码质量的情况下快速对开发人员做出响应,或者让他们能够快速采取进一步行动。

第二步: 检查CL的主要部分

持续代码评审的改进

查找属于此CL的“主要”部分的文件。通常来说一个CL最重要的部分是它逻辑修改数最多的那个文件。这样有助于了解相关的CL,通常来说会加快code review。如果CL太大,您无法确定哪些部分是主要部分,请开发人员告诉你应该首先看什么,或要求他们将CL拆分为多个CL。

如果您遵循了这些准则,并且严格执行代码评审,则应该发现整个代码评审过程会随着时间的流逝而越来越快。开发人员知道为了保证代码质量需要做些什么,并从一开始就向你发送非常棒的 CL,这样评审所需的时间就会越来越少。评审人员学会如何快速做出响应,并且不会在评审过程中增加不必要的延迟。但是,不要在代码评审标准或质量上做出让步以提高速度 —— 从长远来看,这不会使任何事情更快地发生。

如果您发现CL的这一部分存在一些主要的设计问题,则即使您现在没有时间审查CL的其余部分,也应立即写下这些评注。实际上,检查其余的CL可能会浪费时间,因为如果设计问题足够严重,那么许多其他正在检查的代码将消失并且无论如何都不会发生。

紧急情况

立即写下这些主要设计评注非常重要有两个主要原因:

在某些紧急情况下,CL必须非常快速地通过 整个审核过程,并且质量准则将得到放宽。但是,请参阅什么是紧急情况?描述哪些情况实际上可以视为紧急情况,哪些不可以。

开发人员通常会发送给审核者一个CL,然后在等待审核时立即基于该CL进行新工作。如果您正在审查的CL中存在重大设计问题,那么他们也将不得不重新设计其以后的CL。你能在他们做太多无用功之前制止他们。

评审注释摘要礼貌。说明您的理由。在给出明确的指示与指出问题并让开发人员决定之间做出权衡。鼓励开发人员简化代码或添加代码注释,而不仅仅是让他们解释复杂性。礼貌

重要的设计变更比小的变更需要更长的时间。开发人员几乎都有截止日期;为了在截止日期之前完成工作, 并在代码库中保留高质量的代码,开发人员需要尽快开始CL的所有主要的重做。

通常,重要的是要礼貌和尊重,同时也要对要查看其代码的开发人员非常了解。一种方法是确保您的注释是针对代码,而是对开发人员。肃然不必总是遵循这种做法,但是在说出可能会令人沮丧或引起争议的内容时,一定要使用它。例如:

第三步:依序阅读CL的其余部分

不好的说法:“为什么要在这个地方使用线程,这样做显然不会获得任何好处”

确认CL整体上没有大的设计问题后,请尝试找出逻辑顺序来浏览文件,同时还要确保不要错过对任何文件的审查。通常,在浏览了主要文件之后,按照代码审查工具向您展示它们的顺序浏览每个文件是最简单的。有时在阅读主要代码之前先阅读测试代码也是有帮助的,因为这样您就可以知道更改应该做什么。

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

Code Review的速度

解释理由

为什么Code Review应该快?

从上面的正确示例中可以看出,这样有助于开发人员理解为什么要给出这些建议。您不一定总是需要在评论注释中包含此信息,但是有时适当的做法可以成为对您的意图的理解,所遵循的最佳实践或您的建议如何改善代码质量进行更多说明。

在谷歌内部,我们在持续优化团队开发新产品的速度,而不是优化单个开发人员编写代码的速度。个人开发的速度固然重要,但它没有团队整体的速度那么重要。

提供指导

如果Code Review速度慢了,可能会发生以下这些事:

通常,修复 CL 是开发人员的责任,而不是评审人员的责任。您无需执行解决方案的详细设计或为开发人员编写代码。

整个团队的速度会降低,是的,你不快速响应别人的Code Review也可以完成其他的工作。但是,整个团队的新功能、bug修复可能会因为没有人做cr被延迟几天、几周甚至几个月。

但是,这并不意味着评审人员不应该给予帮助。通常,您应该在指出问题和提供直接指导之间取得适当的平衡。指出问题并让开发人员做出决定通常可以帮助开发人员学习,并使代码评审更容易。这也可以带来更好的解决方案,因为开发人员比评审人员更接近代码。

开发者应维护Code Review的流程,如果审查者很少回复Code Review,但是每次都对CL提出大量的改动,这可能会打击到开发者。通常,开发者可能会抱怨审查者太严格。如果审查者能在开发者更新后快速响应,并提出有实质性提升的建议,抱怨就会消失。Code Review中绝大多数抱怨会随着CR速度的提升而消失。

但是,有时直接给出指令指令,建议甚至代码会更有帮助。代码评审的主要目的是获得最佳的 CL。第二个目的是提高开发人员的技能,以便他们之后的评审会越来越少。

可能影响到代码质量。如果CR慢了,可能会给开发者一种需要提交不太好代码的压力。CR慢了,也会影响到代码清理、重构、和现有CL的进一步提升。

接受注释

应该以什么样的速度做Code Review?

如果您要求开发人员解释一段您不理解的代码,他们通常会去重写代码。有时,在代码中添加注释也是一种适当的响应,只要它不只是解释过于复杂的代码即可。

如果你不是在做需要高度专注的任务,Code Review应该越快越好。应当在一个工作日之内回应Code Review请求。遵循这些指导意味着典型的CL应该在一天内进行多轮审查。

仅在代码检查工具中编写的说明对将来的代码阅读者没有帮助。只有少数情况可以接受这种做法,例如,你对评审的东西不太熟悉,而开发人员的解释却是很多人所熟知的。

速度 vs 中断

代码评审回推

只有一种情况下,个人的速度要胜过团队速度。如果您正在处理诸如编写代码之类的重要工作,请不要打断自己的工作去做Code Review,研究人在专注中被打断后需要很长的时间才能重新恢复到专注状态。所以,为了不让其他开发者等会而且中断自己的编码工作,明显得不偿失。

有时,开发人员会回推代码评审。他们可能会不同意您的建议,或者会抱怨您总体上过于严格。

所以,建议你在工作断点的时候回应Code Review,比如写完代码、午饭后、会议结束后、从茶水间回来……

谁是对的?

快速响应

当开发人员不同意您的建议时,请先花点时间考虑一下它们是否正确。通常,它们比您更接近代码,因此他们实际上可能对代码的某些方面有更好的了解。他们的论点有意义吗?从代码质量的角度来看,这有意义吗?如果是这样,请让他们知道他们是对的,然后问题就解决了。

当我们谈论Code Review的速度时,一方面是指响应时间,另一方面是指整个Review从提交到通过的时间。整个过程应该足够快,但是对于每个人来说,迅速做出反应比迅速完成整个过程更为重要。

但是,开发人员并不总是正确的。在这种情况下,评审人员应进一步解释为什么他们认为自己的建议正确。良好的解释不仅说明了对开发人员的理解,而且还说明了为何要求他们进行更改其他信息。

即使有时需要很长时间才能完成整个Code Review流程,评审者在整个过程中的快速响应也可以大大缓解开发人员因为Code Review“慢”而产生的挫败感。

如果评审人员认为他们的建议可以改善代码质量,并认为评审所带来的代码质量改进值得开发人员做出额外的工作,那么他们就应该坚持。改善代码质量往往是由一系列的小步骤组成的。

如果你太忙不能全身心投入到Code Review中,你也应该让开发者知道你什么时候会去Review,或者建议其他评审者快速响应,再或者提供一些初步的建议。

有时,在真正提出建议之前,需要花很多时间去解释。请确保始终保持礼貌,并让开发人员知道您了解他们在说什么,您只是不同意。

评审者有必要花时间去做Code Review来保证代码符合标准,不管怎么样,每个回应应当保证足够快速。

烦恼的开发人员

跨时区Code Review

评审人员有时认为如果他们坚持要开发人员做出改动,会使开发人员感到沮丧。有时,开发人员确实会感到不高兴,但这通常是短暂的,之后他们会非常感谢您帮助他们提高了代码质量。如果您表现的很有礼貌,那么开发人员根本不会感到不开心,这种担心可能是多余的。烦恼通常与评审人员的注释编写方式有关 ,而不是与评审人员对代码质量的坚持。

当遇到跨时区的Code Review时,尽量在作者回家前回复。如果他们已经回家了,尽量在他们每天来公司前完成Code Review。

稍后解决

LGTM和注释

回退的一个常见原因是开发人员想要完成任务(可以理解)。他们不想仅仅为了获得此 CL 而进行另一轮评审。因此,他们说他们将在以后的 CL 中清理某些内容,因此评审人员现在应该 LGTM 此 CL。一些开发人员对此非常好,并会立即编写后续的 CL 来解决此问题。但是,经验表明,开发人员编写原始 CL 的时间越长,他们进行后续修复的可能性就越小。实际上,除非开发人员在提交当前的 CL 之后立刻修复,否则在通过之后通常不会再去做这件事情。这不是因为开发人员不负责任,而是因为他们有很多工作要做,所以修复工作通常会被遗忘。因此,最好是坚持要求开发人员在代码进入代码库并“完成”之前立即修复其 CL 。让开发人员“稍后解决”是代码库退化的一种常见方法。

为了让CR更快,有些情况下也应该让CR提前通过,即便有些评论没有被解决,比如:

如果 CL 引入了新的复杂性,除非是紧急情况,否则必须在提交之前将其清除。如果 CL 暴露了一些问题,并且现在无法解决,那么开发人员应把错误记录下来,分配给自己,以免丢失。他们还可以选择在引用已提交错误的代码中编写 TODO 注释。

审查者信任开发者能适当解决所有评审者的建议。

抱怨评审过于严格

其余的改动很小,开发者不必做。

如果您以前对代码的评审不是那么严格,但是却突然变得严格起来,那么一些开发人员将会大声抱怨。不过没关系,提高代码评审的速度通常会使这些抱怨消失。

除非另有明确说明,评审者应指明他们打算使用这些选项中的哪一个。

有时,这些抱怨可能需要经过数月的时间才会消失,但是最后开发人员会看到严格的代码评审的价值,因为他们会看到严格的代码评审帮助自己产出优秀代码。有时候,如果发生这种事情,声音最强烈的抗议者甚至会成为您最坚强的支持者。

当开发者和评审者在不同时区时,应当着重考虑下通过CR,否则开发者还得等一天。

解决冲突

大的CL

如果您遵循上述所有内容,但是在代码评审过程中仍然遇到无法解决的冲突,请再次参阅以上内容以获取有助于解决冲突的指导准则。

如果有人给你提交了一个非常大的Code Review,你也不确定你有时间看,你最好建议开发者把CL拆分成几个小的部分,分多次Code Review,而不是一次性全部提交上来。这即便开发者多做一些额外的工作,也是可以把它拆分开的,而且拆分也有利于评审者。

原文链接:How to do a code review

如果CL不能拆分成多个小CL,你也没有时间快速完整的Review整个代码,只是对整体设计提一些建议,然后发回给开发者改进。作为评审者,你的目标之一是在不牺牲代码质量的前提下,不阻碍开发者的进程或者尽可能让他们向前推进。

持续提升Code Review

如果你遵从这些建议并在Code Review中严格执行这些准则,你就会发现整个Code Review的流程会越来越快。开发者将了解健康代码的要求,并从一开始就交出完美的代码,然后Code Review的时间会越来越少。评审者将学会如何快速做出响应,并且不是在这个Code Review过程中增加不必要的延迟。

但是,不要为了提升速度牺牲Code Review的标准和代码质量。从长远来看,这并不会提升速度。

紧急情况

当然也有一些紧急的CL需要快速走完这个Code Review流程,这时候在质量上的把控可以稍微放松一些。可以参考紧急事件一文来了解哪些是紧急事件哪些不是。

怎样写代码评论

概要

礼貌

本文由10bet发布于Web前端,转载请注明出处:Google 是如何做 Code Review 的?10bet

关键词:

最火资讯