互联网Code Review最佳实践分享

点击上方蓝色字关注我们~

Code Review是互联网公司的技术部门的基本要求，或者对技术比较重视的公司，在这方面也会比较重视吧。如果哪家互联网公司没有这个要求，也许称不上一个真正的互联网公司吧。 

这里分享下我们项目组针对于Code Review这个环节的相关问题进行总结及其思考。

其实大部分人都知道Code Review的作用。 这里简述下 几点主要作用 ：

①尽量避免bug的出现。只能尽量降低bug的”产出率”，提前发现bug，降低生产事故发生率。

②方便维护，降低维护成本。编写规范的代码和优秀的代码可以给未来接手的同学更好的理解其意图进行维护或者在其基础上继续开发，不会让新同学想“打死前任”的冲动。

③正确处理业务需求逻辑。开发同学实现的业务需求是否与需求对应，是否会存在理解偏差，是否存在处理不当等等。

④尽量最优实现。在编写代码的过程中，每个人的逻辑思维都可能存在不同，在评审过程中，开发同学写的代码还存在现有更好、更优的实现方式，或者把实现的最优逻辑分享给其它同学讨论学习。

分享下我们这边的一些Code Review经验。

3.1、 背景

最近整理了下每次评审存在的问题，并且做了统计分析。大概经历了1年多的时间，接到30来个中、高等级的业务需求，参与了项目组的每次代码评审，也作为主要的评审人员，并且把每次的评审都做了记录，登记在wiki上。至今代码评审有问题的记录数达到200来个。因新同学加入我们团队，每次接手的需求开发完成后进行Code Review，我们都会比较针对性的对其代码进行比较仔细的审核，所以对于比较初级的问题也是比较多，所以出现这类问题占比比较多也是比较合理的。我们团队的“老油条”经历过多次“洗礼”，代码质量也是得到很高的提升，相应的评审问题也是比较少的。所以下面的统计数据也是比较客观，如果低级问题比较多也是能理解的。

使用的工具：编码规范+业务规范+技术规范+数据库规范。

3.2、 问题分类

这些问题分类是我自定义分类的，可能存在不严谨的地方。

主要分为两大类： 代码问题 和 业务问题 。

问题类别 汇总 ：从统计上来说，业务问题占了差不多1/4，代码问题占了3/4。所以我们能看出业务问题也是在Code Review过程中占有很大的一部分，不容忽视；代码问题占用了绝大多数，是我们技术人员比较着重关注的。

3.3、 业务问题

业务处理、业务规范、规范习惯。

◇ 业务处理 ：占了 7成 ，说明业务处理占了业务问题的绝大部分。主要是与业务需求相关的处理上，不能达到业务上的需求，出现不合理或者与需求上存在差异，还有与具体的使用方式存在问题，需求实现上存在的疑惑点等等。例子：有无登录用户cookies区分处理；接口是否需要增加登录访问限制；汇总任务时间应该灵活支持自定义；瓜分红包处理后无需再查库等等。

◇ 业务规范 ：占了 28% ，主要是在业务实现上，使用的方式或者特定业务上使用不合理，没有与业务规范上保持一致。比如：发奖应该调用统一发奖接口；接收短信的手机号是哪种号码类型；判断活动时间其它取字段等等。

◇ 规范习惯： 在业务规范上没有养成一种习惯。比如：文档没有更新，SQL没有实时更新等。

3.4、 代码问题

编码习惯、命名规范、日志打印、代码优化、数据库、规范习惯、事务处理、异常处理、公共复用、bug、并发问题、代码注释。

从统计数据得知，我们这边主要出现的代码问题 集中在 ：编码习惯、命名规范、日志打印、代码优化这几块，占用了 62% 。但是其它的问题也是同等重要，基本上每一项在代码评审中都是非常重要的！

◇ 编码习惯： 占23% ，这一块在全部问题中占比最大，主要是在编写的代码中，使用的方式不是较好或者存在不合理的地方，而且有一些也可能跟我们业务上紧密相关。比如：①if包含return，不需要再else；②Map判断key是否存在无需get后判断对象是否为空，直接用Map api方法contains即可；③RPC调用结果应增加状态码判断；④避免在循环中写过于复杂度逻辑，可以抽取一下，等等。

◇ 命名规范： 占16% ，这一类问题排在第二位。也是大部分新同学出现的比较多，没有按照规范来，在命名上容易出现“随心所欲”。比如：①接口名不要以Ixxx命名；②方法命名长度太长；③命名拼写错误；④数据库表索引名太长；⑤命名与实际业务含义不符，等等。

◇ 日志打印： 占14% ，这类问题在评审过程中，也出现不少日志打印比较粗心或者极少打印关键日志。我们都知道日志的重要性，帮助我们分析逻辑，排查线上问题无比重要。比如：①日志级别使用错误；②关键日志无打印；③方法入参与业务数据没有打印，存在“失联”；④异常堆栈未打印，等等。

◇ 代码优化： 占14% ，编写的代码存在可优化的空间或者使用方式不是很合理，性能上存在问题等。比如：①方法长度过长；②if嵌套太多，影响阅读；③校验方法可以抽取成为单独方；④不要在循环中查库，可优化为查一次库；⑤逻辑不清晰需重构，等等。

◇ 数据库： 占7% ，主要是在与SQL打交道上存在问题。比如：①使用union all语句应优化；②SQL中变量尽量传递进来，不要写死；③排序使用自增主键ID来代替create_time，等等。

◇ 规范习惯： 占7% ，主要是在编写代码中一些规范上存在不合理的地方，这个也比较紧跟业务习惯上。比如：①Magic数字应定义为常量或根据业务需要使用配置参数；②不要在方法中修改入参；③无用代码别注释直接删除，等等。

◇ 事务处理： 占4% ，主要是在事务处理上存在不合理或者使用有误，容易存在数据不一致造成bug。比如：①机会发放和奖品发放应该在同一事务；②事务范围不合理，应该只包含insert、update放在事务里，其它rpc，mq调用应该在事务外；③分布式锁和事务处理应该拆分，等等。

◇ 异常处理： 占3% ，在处理异常时存在不合理。比如：①RPC调用业务，出现业务异常，应该往后抛；②RPC请求出现异常需捕获③兑换码出现更新失败应抛出异常，等等。

◇ 公共复用： 占3% ，公共复用主要是工具已经封装好了无需重复写同样的代码，或者多次使用存在可以复用的代码应该抽取出来。比如：①多次return同一结果，应封装成公共的处理结果方法，直接返回即可，简化代码，容易维护；②RPC调用尽量封装成service，供其它业务可能需要复用，等等。

◇ bug： 占4%，比较明显的bug其实也不多，在业务处理上容易存在逻辑漏洞。在前面所说的编码习惯、事务处理等，也有的算是bug，没有归属在这个分类里。

◇ 并发问题： 占3% ，并发问题在我们每次评审的过程中，并发问题都是特别关注的。比如：①奖品的发放逻辑是否存在并发问题；②用户抽奖是否存在并发问题，等等。

◇ 代码注释： 占2% ，代码注释上这块问题是最少的，但是也是需要我们留意的，特别是对以后维护有很重要的意义。比如：①在比较复杂的业务逻辑，需要增加注释；②弹框提醒返回默认特定值时，应增加注释，方便理解。

3.5、 业务开发规范

我们在Code Review的过程中，也根据实际业务进行总结了跟业务紧密关联的问题，也就是业务开发规范。在业务处理的时候，根据业务特点或者业务处理方式，我们需要按照实际业务去调用我们规定的方式，因为曾多次出现，所以做下记录，防止后人或新人重复掉坑里，甚至重复出现同一类问题事故。同时，我们针对总结的问题，也按照建议和强制的规范级别来要求或者指导我们的同学进行更好的开发。

在经历了这么一段时间的Code Review，在代码层面上容易出现编码不规范、命名不规范、日志打印问题、代码优化等问题，在数据库层面上也出现了不规范使用问题等。同时也遇到了很多容易出现问题并且处理不当的问题，比如，在事务处理，异常处理、并发问题处理上都存在比较严重的问题。所以通过对这类问题我们基本知道了每次Code Review的关注点在哪，容易出现问题的地方在哪，或者在设计实现上是否合理等，都能很好的协助我们对待每次评审，都是尽量高质量的完成我们的目标。

我们也发现了这么多问题，也不是仅仅指出了事，负责人需要将相关问题处理解决，有时甚至需要有经验丰富的同学帮忙审核，而且每次评审完之后我们经理都会在周会上询问相关人员是否把最近评审的相关问题处理的如何，以此达到有始有终。

本次分析了全部的问题，并且进行归类，发现很多同学容易出现编码习惯方面的问题，有时为了追求“高效”，而快速编码，手敲代码一把梭，容易忘了基本规范。

编写可维护的、高性能的、有效的代码，能够帮助我们整个项目达成比较好的共识，这非常有益于我们以后维护前人留下的“坑”，而不会坑到新人。有些问题似乎看起来非常低级，也许会说怎么能犯这种错误，其实我们应该都应该有包容心，毕竟每个人都是一步一步成长，有错就指出，有错我们就改。

大家相互促进，相互提升，在评审的过程中，能够更好的提高自己的代码质量、技术能力，甚至设计能力。

以下是我工作多年的7点建议，希望对各位有所帮助：

1）制定规范：编码规范+技术规范+业务规范+数据库规范，形成标准文档化。

2）根据需求开发，定期安排相关开发人员、评审人员参与Code Review，有需要的话可以邀请测试人员参与。

3）开始评审前，要求相关代码通过阿里编码规范/FIndBugs/SonarLint等插件进行前期扫描，避免出现比较多的基本问题。

4）新同学首次参入进来，需要着重关注新人，给予更好的建议，帮助提升代码质量。

5）针对评审质量很高的代码给予肯定，并且表扬相关人员。

6）定期总结Code Review存在的问题，记录相关问题于wiki(文档)上，按月度或季度定期输出总结。

7）制定与实际业务相关的开发规范，防止重复掉坑，重复出现同一类问题事故等。

这是搬运工一些工作心得，如果觉得哪里说的不好，或者有其它见解，欢迎评论，大家一起探讨。