本文做者:IMWeb IMWeb团队 原文出处:IMWeb社区 未经赞成,禁止转载html
原文地址: google.github.io/eng-practic…前端
本文的名词解释:git
若是嫌文章太长,能够直接到后面看总结github
CR(Code review)主要目的在于确保Google 的代码库代码质量愈来愈好。而全部相关的工具与流程皆是因应这个目的而生。为达到此目的,势必须要作出一连串的权衡与取舍web
首先,开发人员必须可以在本身负责的任务上有所进展。若是你历来没有向代码库提交改进过的代码,那么代码库就永远不会改进。另外,若是一个reviewer使CR都很难进行的话,那么开发人员就不肯意在未来进行改进。算法
另外一方面,reviewer有责任确保每一个change list(下称CL)的质量,保证其代码库的总体代码质量不会愈来愈差。这可能很棘手,由于一般会随着时间的推移,代码须要降级才能让代码运行起来,特别是当团队受到严重的时间限制时,你们以为必须走捷径才能实现他们的目标。编程
此外,reviewer对他们正在review的代码拥有全部权和责任。他们但愿确保代码保持一致、可维护,以及下文的“在CR中能够获得什么”中提到的内容。安全
所以,咱们有如下规则,做为咱们在CR中所指望的标准:数据结构
通常来讲,当CL存在的时候,reviewer应该同意它,此时它确定会提升正在工做的系统的总体代码质量,即便这个CL并不完美。这是全部CR中的首要原则。固然,这是有局限性的。例如,若是一个CL添加了一个reviewer不但愿在其系统中使用的功能,那么reviewer固然能够拒绝,即便代码设计得很好。并发
这里的一个关键点是,没有“完美”的代码,只有更好的代码。reviewer不该该要求做者在approve以前对一篇文章的每一小段进行润色。相反,reviewer应该权衡发展的须要和他们所建议的change的重要性。reviewer不该追求完美,而应追求持续改进。做为一个总体,提升系统的可维护性、可读性和可理解性的CL不该该由于它不是“完美的”而被延迟几天或几周。
reviewer应该随时能够留下评论,表达一些更好的东西,但若是不是很重要,能够在评论前加上“nit:”这样的前缀,让做者知道这只是一个他们能够选择忽略的修饰点。
CR有一个重要的功能,教开发人员一些关于语言、框架或通常软件设计原则的新知识。留下有助于开发人员学习新知识的评论是能够的。随着时间的推移,共享知识是提升系统代码健康度的一部分。你只须要记住,若是你的评论纯粹是教育性的,但对达到本文档中描述的标准并不重要,请在其前面加上“nit:”,或者以其余方式代表做者没必要在本CL中解决它。
现实和数据推翻了我的喜爱。在代码风格方面,风格指南(style guide)是绝对的权威。任何不在style guide中的一些点(如空格等)都是我的偏好的问题。代码风格应该与现有的一致。若是项目没有之前的统一风格,那就接受做者的风格。
软件设计的各个方面历来都不只仅是一个纯粹的代码风格问题或者是我的喜爱问题。它们是以基本原则为基础的,应当以这些原则为依据,而不只仅是以我的意见为依据,有时几乎都没有选择的。若是做者可以证实(经过数据或基于原理的一些事实)他的方法是一样有效的,那么reviewer应该接受做者的偏好。不然,代码风格选择取决于软件设计的标准原则。
若是没有其余规则适用,那么reviewer能够要求做者与当前代码库中的内容保持一致,只要这些代码不会恶化系统的总体代码健康情况。
在CR的任何冲突中,第一步应该始终是开发人员和reviewer根据本文和《CL Author’s Guide》,尝试达成共识。
当达成共识变得特别困难时,reviewer和做者须要进行面对面会议,而不是仅仅试图经过CR的注释来解决冲突。(不过,若是这样作,请确保将讨论结果记录在CL的评论中,以供未来的读者阅读。)
若是这不能解决问题,最多见的解决方法就是升级。一般状况下,升级的途径是进行更普遍的团队讨论,让team leader参与进来,请求代码维护人员作出决定,或者请求技术经理提供帮助。不要由于做者和审稿人不能达成一致意见而让一个其余人袖手旁观。
注意:在考虑每一点时,必定要考虑CR的标准。
在CR中重要的是看CL的整体设计。CL中不一样代码段的交互是否有意义?此更改属于你的业务代码库仍是属于引进来的其余代码库?它是否与系统的其余部分很好地集成?如今是添加此功能的合适时机吗?
这个CL作了开发者想要的吗?开发者对这些代码的设计初衷用户有好处吗?“用户”一般既是最终用户(当他们受到更改的影响时)又是开发人员(他们未来必须“使用”此代码)。
大多数状况下,咱们但愿开发人员在进行CR时可以对CL进行充分的测试,使其可以正常工做。可是,做为reviewer,仍然应该考虑边缘情况,寻找问题,尝试像用户同样思考,并确保仅经过阅读代码就不会看到错误。
若是你愿意的话,你能够本身去验证CL。若是改动会直接带来的用户可见的影响,好比说ui改动,验证CL的变化是很是关键的。 在阅读代码时,很难理解某些更改会对用户产生怎样的影响。对于这样的更改,若是不方便本身测试,则可让开发人员演示该功能(demo)。
另外,在CR期间考虑功能性特别重要的点是,cl中是否并发式编程,理论上可能致使死锁或竞争条件。这些类型的问题很难经过运行代码来发现,一般须要有人(开发人员和reviewer)仔细考虑,以确保不会引入问题。(注意,这也是不使用平行式编程的一个很好的理由,在这种状况下,可能出现竞争条件或死锁,这会使代码检查或理解代码变得很是复杂。)
一个CL是否复杂到超过预期的必须?针对任何层级的CL必须确认这几点:单行代码是否过于复杂?函数是否过于复杂?class是否过于复杂?“复杂”一般意味着该代码很难阅读,颇有可能也意味着其余开发者在修改这段代码的时候引入其余bug
其中一种复杂性就是过分设计(Over-engineering)形成的,开发者会让那段代码过分通用化,超过了本来所需,或者还新增了系统目前不须要的功能。reviewer应特别注意一下过分设计。鼓励开发者解决他们知道如今须要解决的问题,而不是推测未来可能须要解决的问题。当那些未来出现的问题出现的时候才开始解决它们,由于那时候你能够更加清晰看见问题的原样子
Donald Knuth说过:过早的优化是万恶之源(Premature optimization is the root of all evil)
请将单元测试、整合测试、端到端测试视为要求CL所作的适当变动。通常CL内除了生产环境的业务代码外,测试也应该要被加入其中。除非该CL是为了处理某个紧急事情而存在。
另外,也要确保测试是正确、合理、有用的。测试并不是来测试它们自己,通常也极少为了测试而测试(如测试一下测试代码有没有问题又走了测试流程),所以咱们要保证测试是有效的。
当代码真的有问题,测试是否会失败?若是被测试的程序发生改动时,测试是否会产生误报?每个测试是否作出了简单而有用的断言?不一样的测试方法之间的测试是否适当分开?
请记住,测试代码也是必须维护的代码,不要由于它们不在主要关注的范围内。
开发者是否为了每个东西都挑了一个适当的名字?一个好的命名意味着,经过名字就足以完整表达该东西的做用是什么或者要作什么。可是同时名字也不要长得难以阅读
推荐参考文章「Clean code 101 — Meaningful names and functions」
开发者是否用可理解的英文留下清晰的注释? 这些注释是否真的必要?
一般注释是解析这段代码为何存在的时候是至关有用的,而不该该去解释某段代码正在作什么。若是代码自己不能解释清楚的话,意味着它更加须要简化了。固然也有例外,好比解释正规的表达式或者复杂的算法正在作什么的时候,注释解释这段代码正在作什么就至关有用。但对于大部分注释来讲是用来讲明那些不包含在程序自己但资讯,好比说为何要这样子作的理由
查看该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该部分。尤为是针对安全性、并发性、可访问性、国际化等复杂问题时。
在充足的上下文下查看CL一般颇有帮助。通常来讲,CR工具只会显示修改部分周围的几行代码而已。但有时你必须查看整个文件以确保改动是否合理。比方说,你可能只看到添加4行新代码,但实际上查看整个文件时会发现这4行是被加在有50行的代码里,此时须要将它拆解为更小的函数。
以整个系统的角度出发来思考CL也是颇有用的。该CL是否改善总体系统的代码质量,亦或是会让整个系统更加复杂?是否缺乏测试?千万不要接受会下降总体系统的代码质量的CL。由于大多数系统是因为许多小改动的积累而变得复杂,所以阻止新的改动引入复杂性(尽管很小)也很是重要。
当你在CL里看到优势时记得告诉开发者,尤为是当他们用很棒的方式来解决你的评论时。CR一般只关注存在的错误,但它们也应该同时应该为良好实践提供鼓励与欣赏。这点在指导开发者时尤为重要:与其告诉他们作错什么,还不如告诉他们作对了什么。
我的认为并不是不要指出错误,而是多以鼓励来代替指出错误,让其余开发者更有动力想将事情作好。其实透过简单的一句话让对方知道哪里作得很好,将来他们会将继续保持下去,并为其余开发者带来的正面影响
在CR时,请务必确保:
如今你已经知道review时要看些什么,但怎样才是review分散在多个文件中的改动最有效的方法?
步骤1: 用宏观的角度来看待改动,查看CL描述以及它作什么
该改动是否有意义、合理?若是在第一时间认为不该该发生这种变化,请当即说明为何不应这样作的缘由。当拒绝相似这样的更改时,向开发人员提供建议告诉他们应该怎么作什么也是一个好主意。
例如,你能够说:“看起来你已经完成一些不错的事情,谢谢!但咱们实际上正朝着删除你正在修改的FooWidget系统的方向前进,因此现阶段咱们不想对它进行任何新的修改。 不如重构咱们的新BarWidget class如何?”
须要注意的是,reviewer在委婉拒绝该CL的同时也要提供替代方案,并且仍然保持礼貌。这种礼貌是很重要,由于咱们但愿代表即便不一样意也会相互保持尊重。
若是你有几个CL里包含你不想这样作的改动时,你应该从新考虑开发团队的开发过程,或发布开发流程给外部贡献者知道,以便在任何CL被撰写前有更多的沟通。最好是在他们开始投入前说“不”,避免那些已经投入心血的工做如今必须被抛弃或完全重写。
提供替代方案让对方知道该怎么作,而非让其自行独自摸索。
步骤2: 检查CL主要的部分
找到CL最核心的部分的那些文件。一般CL内会有文件包含大量的逻辑改动,而它正是CL的主要部分。所以咱们要首先查看这些主要部分。这有助于为CL的其余较小部分提供适当上下文,并且这样一般能够提升review速度。若是CL太大致使于没法肯定哪里是主要部分时,请向开发者询问首先应当查看的内容,或者要求他们将CL拆分为多个CL。
若是在主要部分发现存在一些主要的设计问题时,即便没有时间当即查看CL的其他部分,也应当即留下评论告知此问题。由于事实上,由于该设计问题足够严重的话,继续review其余部分的代码可能只是浪费时间,由于其余正在审查的其余代码可能都将无关紧或消失。
马上发送关于主要设计的评论很是重要有两个主要缘由:
步骤3: 用合理的顺序看CL 其他的改动
一旦确认整个CL没有重大的设计问题时,试着找出一个有逻辑顺序来review剩余档案,并确保不会错过其中任何一个。一般在浏览主要部分后,最简单的方式是按照CR工具提供的顺序来浏览每一个文件。有时在阅读主要代码前先阅读测试也是很是有帮助的,如此一来你就能够了解应该作、看些什么。
在Google咱们优化开发团队共同生产产品的速度,而不是优化我的开发的速度。我的的开发速度很重要,但它不如整个团队的开发速度重要。当CR很慢时,会发生如下几件事:
若是你并无处于须要专一工做的时候,那么你应该在CL被提交后尽快进行review。review回复最长的极限是一个工做日。若遵循以上指南,意味着通常的CL应该在一天内获得多轮review(若是必要的话)。
但有时候我的的速度优先度会赛过团队速度。若是你处于须要专一工做的时候(比方说写代码),不要打断本身去作CR。
研究证明,若开发者在被打断后会须要很长时间才能恢复到本来顺畅的开发流程。所以,开发的时候,打断本身实际上会比让另外一位开发者等待review来得更加昂贵。
取而代之的是,咱们能够在投入处处理他人给的review评论以前,找个适当的时机点来进行CR。这有多是当你的当前开发任务完成后、午饭、刚从会议脱身或从微型厨房回来等等。
当咱们谈论CR的速度时,咱们关注的是回应时间,而非CL须要多长时间才能完成并提交。在理想状况下,整个过程应该是快速的。
总的来讲我的回应评论的速度,比起让整个CR过程快速结束来得更为重要。即便有时须要很长时间才能完成整个流程,但若在整个过程当中能快速得到来自reviewer的回应,这将会大大减轻开发人员对于缓慢的CR过程的挫败感。
若是真的忙到难以抽身而没法对CL进行全面review时,你依然能够快速的回应让开发者知道你何时会开始审核、建议其余可以更快回复reviewer,又或者提供一些初步的普遍评论。(注意:这并不意味着你应该中断开发去回复——请找到适当的中断时间点去作)
很重要的是,reviewer员要花足够的时间来进行review,确保他们给出的LGTM,意味着“此代码符合咱们的标准”。
尽管如此,理想的我的的回应速度仍是越快越好。
在面对时区不一样的问题时,尽可能在他们还在办公室时回复做者。若是他们已经回家了,那么请确保在他们次日回到办公室前完成。
为加快速度,在某些状况下reviewer能够给予LGTM或Approval,即使CL上仍有未解决的评论。相似状况会发生在:
LGTM 评论对双方处于不一样的时区时尤为值得考虑,不然开发人员会等待一成天才获得“LGTM,Approval”。
若是有人要求reivew时,但因为改动过于庞大致使你难以肯定什么时候才有时间review它时,你一般该作的是要求开发人员将CL拆解成多个较小的CL,而不是一次review巨大的CL。这种事是可能发生的,并且对于reviewer很是有帮助,即使它须要开发人员付出额外人力来处理。
若是CL没法分解为较小的CL,且你没有足够时间快速查看整个CL内容时,那么至少对它的总体设计写些评论,并发送回开发人员以便进行改进。身为reviewer,你的目标之一是在不牺牲代码质量的情况下,避免阻档开发人员进度,或让他们可以快速采起其余更进一步的动做。
若是你遵循这些准则,而且对于CR很是严格的话,后面你会发现整个CR流程会愈来愈快。由于开发者学到什么是质量好的代码,并于开头就提交一个很棒的CL,而这正刚好只须要愈来愈少的时间。而reviewer则学会快速回应,而非在过程当中加入没必要要的延期。 但不要为提升想象中的速度,而对CR标准和代码质量作出妥协,毕竟从长远来看它实际上并不会让任何事情发生得更快。
在某些紧急状况下,CL会但愿放宽标准以求迅速地经过整个CR过程。但请看什么是紧急状况来知道哪些状况实际上属于紧急状况,而哪些不符合。
有时开发人员会推迟处理CR产生的评论。要么他们不一样意你的建议,要么他们会抱怨你太严格了。
当开发人员不一样意你的建议时,请先花点时间考虑一下他们是不是正确的。由于一般他们比你更了解代码,因此他们可能真的比起你来讲对代码的某些层面具备更好的洞察力。他们的论点有意义吗?从代码质量的角度来看它是否合理?若是是的话,让他们知道他们是对的,而后让问题沉下去。
但开发人员也并不老是对的。在这种状况下,reviewer应该更进一步解释为何认为本身的建议是正确的。一个好的解释一般展现了“对开发人员的回覆的理解”以及有关“为何被要求对出作出改动”等信息。尤为是当reviewer认为给出的建议会改善代码质量时,便应该继续宣扬本身的论点。只要他们相信所需的额外的工做量最终会改善总体代码质量。提升总体代码质量这件事,每每是经由每一个微小的改动来发生。有时每每须要几番解释一个建议才能让对方真正理解你的用意。切记,始终保持礼貌,让开发人员知道你有听到他们在说什么,只是你不一样意该论点而已。
reviewer有时会认为若本身坚持改进的话,会让开发人员以为沮丧不安。的确开发人员有时会感到很沮丧,但这一般是十分短暂的,甚至后来他们很是感谢你帮助他们提升代码质量。通常来讲,只要你在评论中表现得颇有礼貌,开发人员实际上根本不会感到沮丧,这些担心都仅存在于reviewer心中而已。沮丧不少时候是对于CR评论的写做方式有关,而并不是来自reviewer对于代码质量的坚持。
一个常见的推迟缘由是开发人员但愿完成任务(这能够理解)。他们不想经过另外一轮CR来批准这个CL。此时他们一般会说在之后的CL进行整理,因此你如今应该要给LGTM。固然部分开发人员很是擅长这一点,而且当即送出一个修复问题的后续CL (follow-up CL),但根据过往经验,这种后续“清理行为”很是少见。除非开发者在CL得到批准以后马上进行清理动做,不然这事根本不可能发生。这不是由于开发人员不负责任,而是由于他们可能有不少其余工做要完成,因而清理工做便会在成堆的工做中被遗忘。所以,一般最好坚持开发人员在代码在合并后清理它们。由于让人们「稍后清理」是导致代码库质量情况降低最多见的情况。
若是CL引入了新的复杂性的话,除非是紧急状况,不然必须在提交以前将其处理掉。若是CL致使暴露周围的问题且如今没法解决它们的话,开发人员应该将缺陷记录下来并分配给本身,避免后续被遗忘。又或者他们能够选择在代码中留下TODO的注释并连接到刚记录下的缺陷。
若是你先前以至关宽松的标准并转趋严格的进行CR的话,一些开发人员会开始大声地抱怨。通常来讲,提升review的速度会让这些抱怨逐渐消失。这些抱怨可能须要数个月才会消失,但最终开发人员会看到严格的review所带来的价值,由于严格的review帮助他们产生的优秀代码。并且一旦发生某些事情时,最大声的抗议者甚至可能会成为你最坚决的支持者,由于他们会看到变得review变严格后所带来的价值。
若是你遵循前面全部操做,但仍然遇到没法解决的双方之间的冲突时,请参考前面的CR的标准以获取有助于解决冲突的准则和原则。
CR的标准
在CR中要看些什么
如何浏览CL
review的速度
如何写review评论
review太严格被抱怨在怎么办
提升review的速度会让这些抱怨逐渐消失。这些抱怨可能须要数个月才消失,但最终开发人员会看到严格的review所带来的价值,由于严格的review帮助他们产生的优秀代码。
IMWeb 团队隶属腾讯公司,是国内最专业的前端团队之一。
咱们专一前端领域多年,负责过 QQ 资料、QQ 注册、QQ 群等亿级业务。目前聚焦于在线教育领域,精心打磨 腾讯课堂、企鹅辅导 及 ABCMouse 三大产品。
社区官网:
加入咱们:
careers.tencent.com/jobdesc.htm…
扫码关注 IMWeb前端社区公众号,获取最新前端好文
微博、掘金、Github、知乎可搜索 IMWeb或 IMWeb团队关注咱们。