漫谈Code Review的错误实践
从刚开始工作时到现在,已经写了7年的代码,大部分代码都被人review过,自己也review了很多人的代码。在上一家公司的时候,我负责的一轮面试是专门进行Code Review的练习和经验谈。
通过在工作/面试中做Code Review的过程,有一些自己任务错误的实践分享出来,也欢迎大家来一起讨论。
什么时候都一定要做Code Review
Code Review是否一定有必要呢?我的答案是不一定。
我觉的需要分时间,分项目。
在公司创业之始,1,2两个人吭哧吭哧的把整个产品从0到1的搭建出来,Code Review既没有条件(没有别人可以review),也没有必要,将产品实现,让项目活下来才是最重要的。
在线上出了Bug,分分钟损失成百上千的时候,显然以救急为主,等Code Review黄花菜都凉了。当然这里分情况,如果是非常显而易见的bug,比如你非常确定是一个条件写反了,那么不用废话赶紧上。如果这个修改不那么轻巧,那么多一双眼睛看一遍往往会大大降低再次引入bug的几率。
Code Review主要是用来让别人检查Bug的
将Code Review视为别人替自己检查Bug的手段,可能代表了相当一部分程序员的想法。
在Code Review中查错确实是一个重要的目的,但并不应该成为唯一的目的。
除了检查Bug,Code Review还是:
- 维持代码质量、统一代码风格的手段。每个人的技术能力,背景各不相同,放任自由的发挥可以写出很多质量不同,风格迥异的代码。通过Code Review,可以迫使大家将代码质量维持在大家都可以接受的程度,并且是的项目的代码风格尽量统一。
- 一个互相学习的过程。 对于初学者,我们可以指出其代码中的风格、设计等方面的不足,可以直接“教育”别人该怎么写代码。反过来,在Review别人的代码时,我们也可以学习作者是怎么写代码的,如何调用的API,用到了哪些设计模式。不仅新手可以向老鸟学习,老鸟们也有很多机会向新手们学习。我就是通过Review毕业的同事的代码学些的Java的JOOQ框架。
- 一个了解同事工作内容的机会。程序员平时可能会比较关注在自己的开发任务上,而自己的同事正在做什么,怎么做,这些信息通过Code Review可以最直观的了解。比如你的同事可能需要和你改同一个文件,这样你可以知道他改的思路,可以避免在代码合并之前产生冲突。
初级工程师的代码需要检查Bug,高级工程师的代码不需要检查Bug
很多时候,对于初级工程师的代码,大家都会很踊跃的去review,并且会有以找到代码中的Bug而显示自己的“资深”的荣耀。但是对于高级工程师写的代码,要么人家根本不给你建Code Review,要么即使把你加到Code Review里,也碍于自己的地位不敢去提意见。或者大家都对高级工程师给予了充分的信任,对于他们建立的Code Review请求都不怎么仔细看,直接LGTM(Looks Good To Me)。
然而这是不对的, 初级工程师写小bug,高级工程师写要命的bug。
初级工程师做的任务也比较初级,而高级工程师做的任务也会比较重要。如果你去关注一下公司的重大生产环境事故,事故的引起者是高级工程师的可能性远远大于初级工程师。这和“出车祸的都是老司机”,“淹死的都是会游泳的”的道理一样,“高级”工程师由于专注于高屋建瓴的设计,反而有可能忽视了代码中的细节。同时“高级”工程师对于自身的技术信心较足,往往会忽视一些基本的自我测试等环节。
所以不论初级,高级工程师写的代码,Code Review都是必要的,只是侧重点可能会不同。在检查Bug这件事上,我们在战略上相信同事,战术上要怀疑同事。
Code Review提的问题越多越好
我在工作中遇到过一些这样的“Code Review喷子”,同事提交了一段100行不到的代码,被洋洋洒洒的提了十几二十处问题。同事对在有些问题上回复了一下自己的意见,双方就一些萝卜白菜的问题盖了十几层高楼。结果两三天过去了,论战还在进行。
我曾经在被这样“猛烈”的Code Review轰炸中,为了让所有的Reviewer都高兴,前后更改数十次,每次修改都有不同的同事提出各种各样的意见。并且最后关头为了一个设计上的分歧,将代码进行了大的重构,最终终于被接受(Accept)。然而代码合并之后却出现了低级的Bug,回头查了一下,正是最后一次的大重构引入了这个bug。为什么会出现这样的结果呢?一个Code Review进行的时间越长,Author的耐心越来越差,Reviewer也越来越只关注他们“希望”你做出的改变,反而属于捡了芝麻丢了西瓜。
如果一个Code Review中的提交太多,对于review过程是不利的。每个Reviewer的记忆是有限的, 可能只能记得第一二次代码的修改。如果一次Code Review中出现多个提交,审阅最新的提交则会丧失之前的上下文,则会出现我上面遇到的问题。
我们做Code Review的目的并不是追求完美的Code,世界上没有完美的Code,任何代码片段,只要足够长,都可以有可改进的地方。每个人心中的完美代码各不相同,有的人喜欢这样写,有的人喜欢那样写。在不违反前面说的代码质量、统一风格的基础上,应该遵守“谁写代码谁做主”的原则。