Google 是如何做 Code Review 的?【10bet】

来源:http://www.chinese-glasses.com 作者:Web前端 人气:57 发布时间:2020-04-22
摘要:时间: 2019-09-25阅读: 134标签: Reviewcr的标准 我和几个小伙伴一起翻译了Google前一段时间放出来的Google’s EngineeringPracticesdocumentation,翻译后的GitHub仓库:,欢迎加star。目前只是翻译完了,

时间: 2019-09-25阅读: 134标签: Reviewcr的标准

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

cr(Code review)主要目的在于确保Google 的代码库代码质量越来越好。而所有相关的工具与流程皆是因应这个目的而生。为达到此目的,势必需要做出一连串的权衡与取舍

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

首先,开发人员必须能够在自己负责的任务上有所进展。如果你从来没有向代码库提交改进过的代码,那么代码库就永远不会改进。另外,如果一个reviewer使cr都很难进行的话,那么开发人员就不愿意在将来进行改进。

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

另一方面,reviewer有责任确保每个change list(下称CL)的质量,保证其代码库的整体代码质量不会越来越差。这可能很棘手,因为通常会随着时间的推移,代码需要降级才能让代码运行起来,特别是当团队受到严重的时间限制时,大家觉得必须走捷径才能实现他们的目标。

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

此外,reviewer对他们正在review的代码拥有所有权和责任。他们希望确保代码保持一致、可维护,以及下文的“在cr中可以得到什么”中提到的内容。

术语

因此,我们有以下规则,作为我们在cr中所期望的标准:

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

一般来说,当CL存在的时候,reviewer应该赞成它,此时它肯定会提高正在工作的系统的整体代码质量,即使这个CL并不完美。这是所有cr中的首要原则。当然,这是有局限性的。例如,如果一个CL添加了一个reviewer不希望在其系统中使用的功能,那么reviewer当然可以拒绝,即使代码设计得很好。

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

这里的一个关键点是,没有“完美”的代码,只有更好的代码。reviewer不应该要求作者在approve之前对一篇文章的每一小段进行润色。相反,reviewer应该权衡发展的需要和他们所建议的change的重要性。reviewer不应追求完美,而应追求持续改进。作为一个整体,提高系统的可维护性、可读性和可理解性的CL不应该因为它不是“完美的”而被延迟几天或几周。

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

reviewer应该随时可以留下评论,表达一些更好的东西,但如果不是很重要,可以在评论前加上“nit:”这样的前缀,让作者知道这只是一个他们可以选择忽略的修饰点。

评审者指南

指导

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

cr有一个重要的功能,教开发人员一些关于语言、框架或一般软件设计原则的新知识。留下有助于开发人员学习新知识的评论是可以的。随着时间的推移,共享知识是提高系统代码健康度的一部分。你只需要记住,如果你的评论纯粹是教育性的,但对达到本文档中描述的标准并不重要,请在其前面加上“nit:”,或者以其他方式表明作者不必在本CL中解决它。

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

原则

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

现实和数据推翻了个人喜好。在代码风格方面,风格指南(style guide)是绝对的权威。任何不在style guide中的一些点(如空格等)都是个人偏好的问题。代码风格应该与现有的一致。如果项目没有以前的统一样式,那就接受作者的样式。

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

软件设计的各个方面从来都不仅仅是一个纯粹的代码风格问题或者是个人喜好问题。它们是以基本原则为基础的,应当以这些原则为依据,而不仅仅是以个人意见为依据,有时几乎都没有选择的。如果作者能够证明(通过数据或基于原理的一些事实)他的方法是同样有效的,那么reviewer应该接受作者的偏好。否则,代码风格选择取决于软件设计的标准原则。

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

如果没有其他规则适用,那么reviewer可以要求作者与当前代码库中的内容保持一致,只要这些代码不会恶化系统的整体代码健康状况。

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

解决冲突

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

在cr的任何冲突中,第一步应该始终是开发人员和reviewer根据本文和《CL Author’s Guide》,尝试达成共识。

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

当达成共识变得特别困难时,reviewer和作者需要进行面对面会议,而不是仅仅试图通过代码审查注释来解决冲突。(不过,如果这样做,请确保将讨论结果记录在CL的评论中,以供将来的读者阅读。)

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

如果这不能解决问题,最常见的解决方法就是升级。通常情况下,升级的途径是进行更广泛的团队讨论,让team leader参与进来,请求代码维护人员做出决定,或者请求技术经理提供帮助。不要因为作者和审稿人不能达成一致意见而让一个其他人袖手旁观。

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

在cr中要看些什么

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

注意:在考虑每一点时,一定要考虑cr的标准。

指导性

设计

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

在cr中重要的是看CL的总体设计。CL中不同代码段的交互是否有意义?此更改属于你的业务代码库还是属于引进来的其他代码库?它是否与系统的其他部分很好地集成?现在是添加此功能的合适时机吗?

原则

功能

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

这个CL做了开发者想要的吗?开发者对这些代码的设计初衷用户有好处吗?“用户”通常既是最终用户(当他们受到更改的影响时)又是开发人员(他们将来必须“使用”此代码)。

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

大多数情况下,我们希望开发人员在进行cr时能够对CL进行充分的测试,使其能够正常工作。但是,作为reviewer,仍然应该考虑边缘状况,寻找问题,尝试像用户一样思考,并确保仅通过阅读代码就不会看到错误。

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

如果你愿意的话,你可以自己去验证CL。如果改动会直接带来的用户可见的影响,比如说ui改动,验证CL的变化是非常关键的。

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

在阅读代码时,很难理解某些更改会对用户产生怎样的影响。对于这样的更改,如果不方便自己测试,则可以让开发人员演示该功能(demo)。

解决代码冲突

另外,在cr期间考虑功能性特别重要的点是,cl中是否有平行式编程,理论上可能导致死锁或竞争条件。这些类型的问题很难通过运行代码来发现,通常需要有人(开发人员和reviewer)仔细考虑,以确保不会引入问题。(注意,这也是不使用平行式编程的一个很好的理由,在这种情况下,可能出现竞争条件或死锁,这会使代码检查或理解代码变得非常复杂。)

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

复杂性

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

一个CL是否复杂到超过预期的必须?针对任何层级的CL必须确认这几点:单行代码是否过于复杂?函数是否过于复杂?class是否过于复杂?“复杂”通常意味着该代码很难阅读,很有可能也意味着其他开发者在修改这段代码的时候引入其他bug

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

其中一种复杂性就是过度设计(Over-engineering)造成的,开发者会让那段代码过度通用化,超过了原本所需,或者还新增了系统目前不需要的功能。reviewer应特别注意一下过度设计。鼓励开发者解决他们知道现在需要解决的问题,而不是推测将来可能需要解决的问题。当那些将来出现的问题出现的时候才开始解决它们,因为那时候你可以更加清晰看见问题的原样子

Code Review应该关注什么?

Donald Knuth说过:过早的优化是万恶之源(Premature optimization is the root of all evil)

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

测试

设计

请将单元测试、整合测试、端到端测试视为要求CL所做的适当变更。一般CL内除了生产环境的业务代码外,测试也应该要被加入其中。除非该CL是为了处理某个紧急事情而存在。

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

另外,也要确保测试是正确、合理、有用的。测试并非来测试它们本身,一般也极少为了测试而测试(如测试一下测试代码有没有问题又走了测试流程),因此我们要保证测试是有效的。

功能性

当代码真的有问题,测试是否会失败?如果被测试的程序发生改动时,测试是否会产生误报?每一个测试是否做出了简单而有用的断言?不同的测试方法之间的测试是否适当分开?

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

请记住,测试代码也是必须维护的代码,不要因为它们不在主要关注的范围内。

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

命名

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

开发者是否为了每一个东西都挑了一个适当的名字?一个好的命名意味着,通过名字就足以完整表达该东西的作用是什么或者要做什么。但是同时名字也不要长得难以阅读

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

推荐参考文章「Clean code 101 — Meaningful names and functions」:-skills/clean-code-101-meaningful-names-and-functions-bf450456d90c

复杂性

注释

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

开发者是否用可理解的英文留下清晰的注释? 这些注释是否真的必要?

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

通常注释是解析这段代码为什么存在的时候是相当有用的,而不应该去解释某段代码正在做什么。如果代码本身不能解释清楚的话,意味着它更加需要简化了。当然也有例外,比如解释正规的表达式或者复杂的算法正在做什么的时候,注释解释这段代码正在做什么就相当有用。但对于大部分注释来说是用来说明那些不包含在程序本身但资讯,比如说为什么要这样子做的理由

测试

查看该CL之前的注释也很有帮助,或许有一个todo项目现在可以移除、一个评论建议为什么不要进行这种更改等等。

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

要注意的是,注释与class、module、function的文件不同。后三者要能够表达一段代码的目的、如何使用它、使用时行为

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

风格

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

Google 对于主要语言都有提供风格指南(style guide: ),甚至包括大多数冷门语言,因此要确保CL遵循适当的指南上的说明。

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

如果你想改进CL中某些不包含在风格指南中的要点时,请在评论前加上Nit: ,让开发人员知道这是你认为可以改善代码的小问题且并非强制性的。但记住,不要仅根据个人风格偏好阻止提交CL。

命名

开发者不应该在 CL内同时包含主要风格的改动与其他代码的修改,这样会导致难以看出CL到底做出什么改动。同时也会让合并和回滚更为复杂,并产生其他问题。例如,如果作者想要重新格式化代码的话,让他们将新格式在一个新CL里面重构。

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

文档

注释

如果CL更改了构建、测试、交互、发布的时候,请检查是否也同时更新相关文档, 包括README,g3doc 页面和其他生成(generated) 的参考文件。如果CL 删除或弃用 了一些代码,请考虑是否应该删除对应文档,如果缺少文档时请询问。

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

每一行代码

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

仔细review分配给你的每一行代码。有些东西,比如资料文件(data files)、生成的代码(generated code)、大型数据结构(large data structures),你可以稍微扫过。千万不要在扫过开发者写的 class、函数、代码区块 时,去假设它内部是没问题的。很显然的某些代码需要比其他代码更仔细的review。这是必须由你做出的判断,但至少你应该确定你理解所有代码在做些什么。

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

如果阅读代码过于复杂并且减慢review速度时,那么你再继续review前,要让开发者知道这件事,并等待他们为这段代码做出解释、说清楚。在Google 我们聘请许多优秀的软件工程师,而你也是其中的一员。如果连你也无法理解的话,很可能其他人也不会。因此,你要求开发者去说清楚这段代码时,同时也在帮助未来的开发人员理解这些代码。

风格

如果你能够理解,但觉得没有资格进行某部分的审核,请确保 reviewer中有一个适合(合格) 的人来review该部分。尤其是针对安全性、并发性、可访问性、国际化等复杂问题时。

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

上下文

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

在充足的上下文下查看CL通常很有帮助。一般来说,cr工具只会显示修改部分周围的几行代码而已。但有时你必须查看整个文件以确保改动是否合理。比方说,你可能只看到添加4行新代码,但实际上查看整个文件时会发现这4行是被加在有50行的代码里,此时需要将它拆解为更小的函数。

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

以整个系统的角度出发来思考CL也是很有用的。该CL是否改善整体系统的代码质量,亦或是会让整个系统更加复杂?是否缺少测试?千万不要接受会降低整体系统的代码质量的CL。因为大多数系统是由于许多小改动的积累而变得复杂,因此阻止新的改动引入复杂性(尽管很小)也非常重要。

文档

优点

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

当你在CL里看到优点时记得告诉开发者,尤其是当他们用很棒的方式来解决你的评论时。cr通常只关注存在的错误,但它们也应该同时应该为良好实践提供鼓励与欣赏。这点在指导开发者时尤其重要:与其告诉他们做错什么,还不如告诉他们做对了什么。

代码细节

个人认为并非不要指出错误,而是多以鼓励来代替指出错误,让其他开发者更有动力想将事情做好。其实透过简单的一句话让对方知道哪里做得很好,未来他们会将继续保持下去,并为其他开发者带来的正面影响

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

标明每个commit 修改什么,可以帮助reviewer快速进入状况

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

此时就不要吝啬你的称赞了!

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

总结

上下文

在cr时,请务必确保:

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

代码经过完善的设计

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

功能性对于使用者们是好的

亮点

对于任何UI改动要合理且好看

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

任何并行编程的实现是安全

总结

代码不应该复杂超过原本所必须的

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

开发者不该实现一个现在不用而未来可能需要的功能

代码设计良好。

代码有适当的单元测试

代码功能对用户有用。

测试经过完善的设计

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

开发者对于每样东西有使用清晰、明了的命名

保证线程安全。

注释要清楚且有用,并只用来解释why而非what

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

代码有适当的写下文件(一般在g3doc)

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

代码风格符合style guide

代码有适当的单元测试。

确保你查看被要求review的每一行代码、确认上下文、确保你正在改善代码质量,并赞扬开发人员所做的好事与优点吧!

测试逻辑设计良好。

如何浏览CL

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

现在你已经知道review时要看些什么,但怎样才是review分散在多个文件中的改动最有效的方法?

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

改动是否合理?他是否有好的描述(description)

代码又相应完善的文档。

优先看CL 最重要的改动。它整体是否有完善的设计?

代码风格符合规范。

用合理的顺序看CL 剩余的改动

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

步骤1: 用宏观的角度来看待改动,查看CL描述以及它做什么

Code Review步骤

该改动是否有意义、合理?如果在第一时间认为不应该发生这种变化,请立即说明为什么不该这样做的原因。当拒绝类似这样的更改时,向开发人员提供建议告诉他们应该怎么做什么也是一个好主意。

概要

例如,你可以说:“看起来你已经完成一些不错的事情,谢谢!但我们实际上正朝着删除你正在修改的FooWidget系统的方向前进,所以现阶段我们不想对它进行任何新的修改。不如重构我们的新BarWidget class如何?”

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

需要注意的是,reviewer在委婉拒绝该CL的同时也要提供替代方案,而且仍然保持礼貌。这种礼貌是很重要,因为我们希望表明即使不同意也会相互保持尊重。

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

如果你有几个CL里包含你不想这样做的改动时,你应该重新考虑开发团队的开发过程,或发布开发流程给外部贡献者知道,以便在任何CL被撰写前有更多的沟通。最好是在他们开始投入前说“不”,避免那些已经投入心血的工作现在必须被抛弃或彻底重写。

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

提供替代方案让对方知道该怎么做,而非让其自行独自摸索。

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

指出问题,告知替代方案或指点方向

第一步: 综观这个改动

步骤2: 检查CL主要的部分

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

找到CL最核心的部分的那些文件。通常CL内会有文件包含大量的逻辑改动,而它正是CL的主要部分。因此我们要首先查看这些主要部分。这有助于为CL的其他较小部分提供适当上下文,而且这样通常可以提高review速度。如果CL太大导致于无法确定哪里是主要部分时,请向开发者询问首先应当查看的内容,或者要求他们将CL拆分为多个CL。

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

如果在主要部分发现存在一些主要的设计问题时,即使没有时间立即查看CL的其余部分,也应立即留下评论告知此问题。因为事实上,因为该设计问题足够严重的话,继续review其他部分的代码可能只是浪费时间,因为其他正在审查的其他代码可能都将无关紧或消失。

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

立刻发送关于主要设计的评论非常重要有两个主要原因:

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

通常开发者在送出CL后,在等待review过程中便已经开始着手基于该CL的新工作。此时如果正在review的CL存在重大设计问题的话,开发者将不得不重新写所有基于它的后续所有CL。因此你要在他们有问题的设计上投入之前阻止他们。

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

主要设计变更通常需要较长时间才能完成,但每个开发人员几乎都有自己deadline。为了赶上deadline并保证代码质量,开发者需要尽快开始或重做CL 任何的主要设计变更。

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

步骤3: 用合理的顺序看CL 其余的改动

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

一旦确认整个CL没有重大的设计问题时,试着找出一个有逻辑顺序来review剩余档案,并确保不会错过其中任何一个。通常在浏览主要部分后,最简单的方式是按照cr工具提供的顺序来浏览每个文件。有时在阅读主要代码前先阅读测试也是非常有帮助的,如此一来你就可以了解应该做、看些什么。

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

review的速度为什么review速度要快

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

在Google我们优化开发团队共同生产产品的速度,而不是优化个人开发的速度。个人的开发速度很重要,但它不如整个团队的开发速度重要。当cr很慢时,会发生以下几件事:

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

团队整体的速度下降。review慢的话,对于团队其他人来说重要的新功能与缺陷修复将会被延迟数天、数周甚至数月,只因为它们正在或者等待review。

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

开发人员开始抗议cr。若reviewer每隔几天才回应一次,但每次都要求对CL进行重大更改的话,那么开发人员可能会感到非常沮丧与觉得困难,而这通常会演变成抱怨。如果reviewer请求相同的实质性更改(且确实可以改善代码质量状况),但在每次开发人员提交新的改动时都能快速反应的话,这些抱怨往往会消失。大多数关于cr的抱怨,往往可以通过加快流程来解决。

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

代码质量会受到影响。当review很慢时,会造成开发者提交不完全尽如人意的CL 的压力越来越大。评论的越慢也会阻止他人进行代码清理、重构、甚至是对现有CL 的进一步改进。

Code Review的速度

cr应该要多快?

为什么Code Review应该快?

如果你并没有处于需要专注工作的时候,那么你应该在CL被提交后尽快进行review。review回复最长的极限是一个工作日。若遵循以上指南,意味着一般的CL应该在一天内得到多轮review(如果必要的话)。

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

速度vs中断

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

但有时候个人的速度优先度会胜过团队速度。如果你处于需要专注工作的时候(比方说写代码),不要打断自己去做cr。

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

研究证实,若开发者在被打断后会需要很长时间才能恢复到原本顺畅的开发流程。因此,开发的时候,打断自己实际上会比让另一位开发者等待review来得更加昂贵。

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

取而代之的是,我们可以在投入到处理他人给的review评论之前,找个适当的时机点来进行cr。这有可能是当你的当前开发任务完成后、午餐、刚从会议脱身或从微型厨房回来等等。

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

快速回应

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

当我们谈论cr的速度时,我们关注的是回应时间,而非CL需要多长时间才能完成并提交。在理想情况下,整个过程应该是快速的。

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

总的来说个人回应评论的速度,比起让整个cr过程快速结束来得更为重要。即使有时需要很长时间才能完成整个流程,但若在整个过程中能快速获得来自reviewer的回应,这将会大大减轻开发人员对于缓慢的cr过程的挫败感。

速度 vs 中断

如果真的忙到难以抽身而无法对CL进行全面review时,你依然可以快速的回应让开发者知道你什么时候会开始审核、建议其他能够更快回复reviewer,又或者提供一些初步的广泛评论。(注意:这并不意味着你应该中断开发去回复——请找到适当的中断时间点去做)

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

很重要的是,reviewer员要花足够的时间来进行review,确保他们给出的LGTM,意味着“此代码符合我们的标准”。

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

尽管如此,理想的个人的回应速度还是越快越好。

快速响应

跨时区review

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

在面对时区不同的问题时,尝试在他们还在办公室时回复作者。如果他们已经回家了,那么请确保在他们第二天回到办公室前完成。

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

LGTM 评论

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

为加快速度,在某些情况下reviewer可以给予LGTM或Approval,即便CL上仍有未解决的评论。类似情况会发生在:

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

reviewer确信开发人员会适当地处理所有剩余的评论

跨时区Code Review

剩余的评论是微不足道的或它们不需由开发者来处理

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

reviewer必须清楚指明他们是指上面哪种情况

LGTM和注释

LGTM 评论对双方处于不同的时区时尤其值得考虑,否则开发人员会等待一整天才得到“LGTM,Approval”。

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

大型改动

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

如果有人要求reivew时,但由于改动过于庞大导致你难以确定何时才有时间review它时,你通常该做的是要求开发人员将CL拆解成多个较小的CL,而不是一次review巨大的CL。这种事是可能发生的,而且对于reviewer非常有帮助,即便它需要开发人员付出额外人力来处理。

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

如果CL无法分解为较小的CL,且你没有足够时间快速查看整个CL内容时,那么至少对它的整体设计写些评论,并发送回开发人员以便进行改进。身为reviewer,你的目标之一是在不牺牲代码质量的状况下,避免阻碍开发人员进度,或让他们能够快速采取其他更进一步的动作。

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

cr的能力会随着时间进步

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

如果你遵循这些准则,并且对于cr非常严格的话,后面你会发现整个cr流程会越来越快。因为开发者学到什么是质量好的代码,并于开头就提交一个很棒的CL,而这正恰好只需要越来越少的时间。而reviewer则学会快速回应,而非在过程中加入不必要的延期。

大的CL

但不要为提高想象中的速度,而对cr标准和代码质量做出妥协,毕竟从长远来看它实际上并不会让任何事情发生得更快。

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

紧急状况

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

在某些紧急情况下,CL会希望放宽标准以求迅速地通过整个cr过程。但请看什么是紧急情况(-practices/review/emergencies.html#what)来知道哪些情况实际上属于紧急情况,而哪些不符合。

持续提升Code Review

如何写review评论如何面对被推迟处理的评论

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

有时开发人员会推迟处理cr产生的评论。要么他们不同意你的建议,要么他们会抱怨你太严格了。

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

谁是对的

紧急情况

当开发人员不同意你的建议时,请先花点时间考虑一下他们是否是正确的。因为通常他们比你更了解代码,所以他们可能真的比起你来说对代码的某些层面具有更好的洞察力。他们的论点有意义吗?从代码质量的角度来看它是否合理?如果是的话,让他们知道他们是对的,然后让问题沉下去。

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

但开发人员也并不总是对的。在这种情况下,reviewer应该更进一步解释为什么认为自己的建议是正确的。一个好的解释通常展示了“对开发人员的回覆的理解”以及有关“为什么被要求做出改动”等信息。尤其是当reviewer认为给出的建议会改善代码质量时,便应该继续宣扬自己的论点。只要他们相信所需的额外的工作量最终会改善整体代码质量。提高整体代码质量这件事,往往是经由每个微小的改动来发生。有时往往需要几番解释一个建议才能让对方真正理解你的用意。切记,始终保持礼貌,让开发人员知道你有听到他们在说什么,只是你不同意该论点而已。

怎样写代码评论

让开发者沮丧

概要

reviewer有时会认为若自己坚持改进的话,会让开发人员觉得沮丧不安。的确开发人员有时会感到很沮丧,但这通常是十分短暂的,甚至后来他们非常感谢你帮助他们提高代码质量。一般来说,只要你在评论中表现得很有礼貌,开发人员实际上根本不会感到沮丧,这些担忧都仅存在于reviewer心中而已。沮丧很多时候是对于cr评论的写作方式有关,而并非来自reviewer对于代码质量的坚持。

礼貌

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

关键词:

最火资讯