关于代码评审(CodeReview)那些不得不说的事儿
在一个成熟的团队中,CodeReview是整个研发流程中不可或缺的一步,而那些即将走向成熟的团队可能对CodeReview有很多的误解和问题,也不清楚CodeReview该如何去做,本文笔者将结合自己的经验和知识,谈谈我对CodeReview流程的一些理解和建议 。
什么是CodeReview
CodeReview 国内也称 代码评审或者代码审查,也简称CR,是指在软件开发过程中,工程师对其他人所写代码做审阅(后文统称CodeReview),以达到控制代码质量的目的。通常的流程都是由代码写作者发起,请团队内其他人审阅代码,其他人对代码提出改进建议,再由代码写作者修改重新提交,直至代码通过大家的审阅为止。
为什么要做CodeReview?
其实很多人都不是很重视CodeReview,因为CodeReview的效果短期很难看到,也很难量化衡量。就如同运动一样,偶尔过量的运动不仅对身体无益,可能还会起反作用,不过长期的坚持肯定会让你更健康,但能让你健康多少很难量化,不过前一段时间github上爆火的项目《程序猿延寿指南》里给出了可参考的数据,每周3次45分钟挥拍运动可以减少全因死亡率47%,按其公式折算大概增加9年的预期寿命。运动除了增加预期寿命外,也能显著减少很多疾病的发病率。坚持CodeReview如同坚持运动一样,趋势肯定是让整个代码库更为健康长寿。
CodeReview从长期来看,有几个明显的好处,接下来就一一讲一下。
提升代码质量
假如将一个系统比作一个生命体,一行行代码比作一个个细胞,不好的设计宛如癌细胞,会逐渐扩散,终将杀死系统。而CodeReview的过程就像是T细胞吞噬掉癌细胞,保证系统的健康成长。让系统有更长久的生命力。
没有人Review的代码,其代码水准就是写代码人的水准,而被一个团队Review过的代码,它的水准将接近甚至超过整个团队的最高水准。
因为单个人可能在某些方便做的比较好,集大家之所长就能在各个方面都做的比较好。另外,随着CodeReview流程日常化,每个参与人的编码能力也会逐步提升,无限趋近于团队最高水准,因为在CodeReview的过程中,你可以看到别人做的好的地方,可以学习到经验,也可以看到别人做的不好的地方,吸取到教训。随着时间的流逝,逐渐积累为参与者的能力。
提前发现问题
在没有CodeReview流程的时候,我们都是依赖于测试,甚至是依赖于功能上线后用户暴露问题,这种发现方式已经偏晚了,尤其是让用户暴露问题的时候可能问题已经非常大了。 问题暴露的越晚,风险也就越大。 而CodeReview一般是放在代码测试之前,如果能在这个阶段就发现问题就能提前将各种风险扼杀在摇篮中。 但是,CodeReview真的能提前发现问题吗?如果能的话,可以提前发现什么样的问题?
我个人觉得CodeReview可以提前发现流程或者实现上的问题,尤其是在做业务需求的时候,不同工作经验的人对同一个需求的理解肯定会有差异,代码实现上也会有很大的差异,有时候一点点的理解偏差,导致出现那种失之毫厘谬以千里的后果,写代码的人也会因为处在自己的思维定式中发现不了问题,而别人可能因为经验丰富或者需求相关背景了解更多,就能很轻易发现问题。像这种情况一般出现在团队新人身上,他们对已有系统和业务了解不多,实现需求时很容易出现问题,这时候如果有人帮忙指出就能避免更严重的后果,另外也有助于新人了解业务和融入团队。
在CodeReview过程中另外一种问题也比较容易发现,就是那些别人之前踩过的坑。比如Java中Simple DateFormat其实有线程安全的问题,不了解的人很容易就踩坑了。如果别人踩过这个坑,就可以在CodeReview过程中提前帮你指出来。 很多开源的类库,甚至Jdk中很多包或多或少都有使用上的坑…… 这种例子就数不胜数了。
当然CodeReview也不是万能的,也有很多问题发现不了的,比如一些边缘Case或者是代码计算结果的准确性,比如你代码实现了一个很复杂公式的计算,你总不能指望有人能通过看代码来发现问题吧。 这些还是老老实实去做测试吧!
经验和知识的传递
程序猿可以写出能运行的代码,但真正的工程师才能写出低bug、易扩展、易维护的高质量代码,更高级的工程师还能帮其他人成为一名合格的工程师。CodeReview的流程也是高级工程师来帮助其他人最直接的方式。
在CodeReview中,你可以看到其他人写出来的优秀代码、优秀的设计,甚至和改动相关的业务背景知识……,Review越多,学到的也越多。另外,即便是哪些不太好的代码,经过别人的Review,必然会留下很多评论或者是改动建议,甚至是别人之前踩的坑,你都能看到,从这些内容上你也可以学到很多新的知识。CodeReview不仅仅是一个提升代码质量方式,它也可以肩负起知识积累和消息集散的功能。
举个我之前遇到过的例子,我们之前有个功能需要操作机器上的文件,当然用Java的File也能实现,但是对操作多层级的文件夹时就很不方便了,需要自己写很多的代码,搞不好还容易出bug。后来我发现apache-common包中提供了IOUtils类,可以很方便操作文件夹。我把这个写到CodeReview的评论中,只要看到过的人都会知道原来有这个东西可以用。
所以,Review别人代码时请毫不吝啬地留下你的建议吧,另外,CodeReview时也不要忘记关注下别人的建议,说不定可以学到新的东西。
如何做好CodeReview
说完了CodeReview的好处,相信你肯定已经跃跃欲试了,如何能做好CodeReview?这里我根据我的知识和经验分享下我的看法。
CodeReview的步骤
了解改动的背景
CodeReview不是一上来就看代码,这样有可能你会看的云里雾里,纯粹是浪费时间。 CodeReview虽然是Review代码,但是首先你的知道你要看的代码实现了什么样的功能,是在什么样的背景下去做的,清楚前因后果之后,你才能知道这个代码大概应该怎么去写,你才能更好的去Review别人的代码、去发现别人的问题。
纵观全局
知道背景之后,在你脑海中就会有一个大概的编码思路,也有个流程主线。这个时候可能有两种情况,你和写代码人的思路相同,那你就顺着你们共同的思路去帮忙Review整个流程是否正确。另一种情况就是你们思路不同,你就得看代码去了解写作者的思路,然后确认是谁的思路有问题,或者是谁的思路更好,然后同写作者一起将这个流程优化到更优。
逐层细化
确定完整个流程之后,就可以逐步深入到代码细节中了,细节可以Review的地方就很多了,可以看下下一节的内容,这里就先不展开了。
CodeReview的关注点
在CodeReview的过程中,如果有一些立足点的话可以帮助大家更好的完成CodeReview的过程,我大概总结出以下几点:
- 功能性: 代码所实现的功能是否和预期一样,是否实现了所有必须的功能?
- 复杂性: 功能实现是否过于复杂? 过于复杂的代码更容易出问题,而且可维护性也会更低。
- 代码风格: 代码是否符合团队编码规范?
- 文档&注释: 如果代码功能有改动,关注下相关文档和注释有没有同步改动。 错误的注释和文档可能会让未来的开发者产生理解成本。
- 代码亮点: 如果你看到变更中做得好的地方,也别吝啬你的赞美。
上面描述更偏概括性,我们来举一些更详细的例子,帮助大家理解上面几点。
- 代码设计良好,可读性和可维护性高。
- 是否有线程安全的bug。
- 代码没有增加不必要的复杂性。
- 开发者没有写一些目前没有在用的代码,这种无用的代码未来大概率也不会用,所以不要假设。
- 代码有适当的单元测试。
- 测试逻辑设计良好。
- 开发者使用了清晰明了的变量和函数命名。
- 注释清晰明了且实用,并且解释清楚了 为什么这么做,而不仅仅是 做了啥。
- 代码有相应完善的文档。
- 代码风格符合规范。
- 代码有没有更优的实现?(性能、资源占用……)
- ……
更多更细节的内容,可以参考 谷歌工程实践——代码评审
注意事项
CodeReview的礼节
首先CodeReview不是你个人炫技的舞台,看到别人代码的问题需要礼貌指出,切忌diss别人。其次,CodeReview不要为了提问题而提问题,有些代码就是没问题,你也没必要纠结必须要提个问题,直接通过即可。 这里在额外说一点,大部分Review别人的代码,只关注到别人做的不好的地方,而忽视了别人做的好的地方,遇到好的代码、好的设计也别忘记点个赞。
CodeReview应当及时
别人提的CodeReview应当及时处理,在很多公司中CodeReview是开发流程中必要的一环,没有Review通过的肯定是不能上线的,如果CodeReview长时间不处理可能会延误后续的流程。 另外一点,CodeReview是相互的,今天你及时帮别人Review了,明天别人也会及时帮你Review, 与人方便即与己方便。
如何写出对CodeReview友好的代码
程序写出来是给人看的,附带能在机器上运行。 ——Harold Abelson
时刻谨记上面这句话。
提交前先做好自审
提前自己处理掉一些低级错误,可以先借助一些工具完成一些简单的检查。例如我们团队会借助checkstyle、spotbug、sonar、pmd等工具完成代码风格和一些潜在bug的检查。然后自己做好功能的自测,尽可能先消灭一部分bug,为CodeReview减少一些负担。
写清楚变更描述
这里对应上文中CodeReview步骤中的了解改动背景,作为代码的写作者,你不能一上来就让别人从代码入手吧,可能看你代码的很多其他人都缺少代码改动相关的上下文信息,完全理解代码成本很高,所以需要在变更描述中将这些信息描述清楚,包括但不仅限于改动背景、改动点、流程、具体设计…… 一句话概括就是 好的变更描述应该说清楚这次是做了什么变更,以及为什么要这么做。
更详实的变更描述可以让Review代码人减少理解成本,更快完成CodeReview的流程,你代码改动也就能更快进入后续流程了。
单个变更尽可能短
一个非常大的变更几乎没有Review,因为大的改动首先就很难Review,其实也更耗费时间,还有出问题的概率也更大,谁Review通过了代码但之后上线出问题,可能是要和你一起背锅的。所以建议大家将大的代码变更尽可能拆小,因为小的变更有以下这些好处:
- 代码评审更快 与比起花30分钟评审一个大的变更相比,对Review代码的人来说花5分钟审查一系列小的变更更加容易.
- Review 更加彻底。 进行较大的更改后,审阅者和作者往往会因大量详细评论的来回移动而感到沮丧,有时这些评论会漏掉或遗漏重要的观点。
- 减少导致bug的可能性。 由于您所做的更改较少,因此您和您的审阅者更容易有效地推断出CL的影响,并查看是否导致bug。
- 减少不必要的工作 当你写了一个巨大的变更,然后评审者觉得你总体方向错了,这会导致你浪费大量的工作。
- 更方便合并代码 因为大型的变更会导致大量的冲突,因此合代码的时候会耗费很多时间,而且可能因合并代码导致问题,我们就出现过好几次代码合并的时候冲掉别人代码的情况。
- 有助于你作出更好的设计 优雅的设计并且完善一个小的改动比大的改动更加容易
- 降低评审者的难度 提审部分改动,不会影响你继续编码。
- 如果真出问题,回滚更容易 这个就不用多解释了吧。
对于那些很难拆分的变更,也不是需要完全禁止,有时候就是有改动非常大,而且无法再去拆分了,这时候还是建议通过其他方式来保证代码质量,比如全流程测试。 然后也建议做好出问题时的快速恢复方案。
注意事项
注意自己的情绪
代码的写作者在CodeReview的流程中是弱势的一方,很多人代码在收到评论被要求要修改代码的时候,都有一种不太愿意接受的心态,其实我自己也经常会有这种心态,尤其是在时间比较紧张的情况下。 其实这是很正常的心态,但是还是需要去控制自己的情绪的,从评论者的角度出发,他肯定也不是在刁难你,也是希望代码质量能更高。如果真遇到这种情况,首先可以和评论者讨论清楚他的要求是否合理,如果确定了合理性当然还是要改的。但如果在时间比较紧张的情况下,协商是否可以不修改或者是之后再修改,确实有些锦上添花的修改没必要阻塞之后的流程。
CodeReview流程应该尽早提交
日常情况下,还是建议早点提交CodeReview,并留出一定的时间来做修改,尽可能不要让这个流程变的匆忙。 大部分公司的实践都是在进入测试流程的时候同时进入CodeReview流程。
关于CodeReview的几个误区
CodeReview是纯浪费时间?
如果你的团队刚开始推行CodeReview流程,这个问题肯定是会被很多人问到的。 所以是不是呢? 这里我拿锻炼身体类比下,大家都知道坚持运动对健康有益,但如果只是一时兴起,偶尔过量的运动不仅对身体无益,可能还会起反作用。实际上,坚持运动对健康的影响其实很难量化,但我们现在都知道,运动是好的。同样,CodeReview如同运动一样,它可能会耗费一定的精力和时间,但长期坚持下来必然会让整个代码库、整个系统甚至这个团队更加健康。
另外,如果一个团队CodeReview机制很成熟,写代码的人随着被Review的次数增加,其代码质量必然也会逐步提交,那么代码中的问题肯定也会逐步减少,随之而来的就是CodeReview的过程越来越轻松。
困难的路会越走越简单,而简单的路会越走越困难。
工期很紧,没时间做CodeReview!
这也是很多团队不做CodeReview的借口。 你不做CodeReview省下的时间,会在后面测试,甚至是在后面长期维护中花费更多的时间。 我们有句耳熟能详的老话叫做“磨刀不误砍柴工”,其实CodeReview和测试在软件开发过程中就是“磨刀”的工作。
做好事前控制而不是事后弥补。 因为时候弥补的代价会非常高,
只有高级工程师才有资格Review别人代码?
虽然事实上几乎都是高级工程师在Review别人的代码,我理解造成这种现象的原因其实也比较简单,高级工程师确实经验丰富一些,也更容易Review出别人代码中的问题,所以会留下更多的Review记录,造成只有高级工程师才参与CodeReview的假象。 但这绝不意味着初级工程师没有资格Review别人的代码,所谓三人行必有我师,即便是初级工程师也有可能发现Review出别人代码中的问题。
即便你暂时确实经验不够丰富,很难找出别人的问题,但你也可以从别人Review代码的过程中学到很多东西,就像上文所说CodeReview也是团妒经验和知识的一种传递方式。 参与CodeReview也是成为高级工程师必不可少的一步。
都有测试流程了,为什么还要做CodeReview?
这个问题算是已经在上文中回答过了,CodeReview需要看很多个方面的内容,而测试只能保证其结果的准确性。
有了CodeReview就不需要测试了?
问这个问题的人相比于问上个问题的人又走了另一个极端,又把CodeReview和测试放在了对立面上。虽然CodeReview如果做的比较好的话,确实能提前发现一些比较明显的问题,但是做CodeReview的是人,人是无法大规模且精准的去校验程序的执行过程,而这恰恰是自动化测试所擅长的,所以说CodeReview和测试并不是对立关系,而是一种互补关系。
只要我在团队推行了CodeReview流程,代码质量就会迅速提高?
首先可以肯定的是代码质量是会提高的,但也许没有那么快。我之前从一个没有CodeReview流程的团队加入到一个有严格CodeReview流程的团队时,有较长一段适应期。我当时最大的感受就是CodeReview太耗时间了,尤其是我刚开始还没适应新团队开发规范的时候,写代码和被Review后改代码所花费的时间基本上五五开,当然之后会好很多。另外,每个人也需要花20%左右的时间和精力去Review别人的代码,相当于花接近三分之一的开发时间来处理CodeReview相关工作,这个比例高到可能你们老板都会怀疑,但我们当时就做到了,结果就是我们的代码质量非常高。
以我的经历来看,如果CodeReview想要有效果,时间投入肯定是少不了的,冰冻三尺非一日之寒,滴水石穿非一日之功。不要高估它的短期价值,也不要低估它的长期价值。
参考资料
今天的分享都到这里了,大家也都知道现在外面大环境不好,这个时候更不能停下松懈的脚步。刘易斯·卡罗尔在《爱丽丝梦游仙境》中借红桃皇后之口说出了下面这句话,现在看来也是出奇的合适呢!
在我们这个地方,你必须不停地奔跑,才能留在原地。如果你要抵达另一个地方,必须以双倍于现在的速度奔跑。
——【英】刘易斯·卡罗尔《爱丽丝梦游仙境》