研发效能工程实践-代码评审

什么是代码评审

Code Review的定义：是一项单人或者多人通过阅读别人的源代码来检查代码质量的软件质量保证活动
定义有点绕口，其实就是写完代码之后让经验相对丰富一点的同事帮你检查一下你的代码，当然这个检查应该是多方面的，包括但不限于你的设计、实现、规范性和一致性等
注意这个检查代码的人应该是团队中除作者的其他的成员，这些检测你代码的人称为reviewers，用过git的人应该都很熟悉

为什么要做代码评审

代码中高频问题

你可能在平时的工作中经常看到自己公司项目里有以下的这些问题

• 经常在不同的包下看到一个相同功能的工具类
• 有时会看到同一个东西命名不同，可能大家英文不是很好，查词典命名的，结果你用的单词和别人的不一样
• 你可能看到大段大段的if-else，好奇他们是怎么合入主干分支的
• 你可能也会看到有人放着标准库或成熟的第三方库不用，自己去实现了一个功能，其实用标准库或第三方库只需要几行代码，并且质量有更高保证
• 你也可能看到一个经验不足的萌新写的代码从设计就有问题，代码分层混乱，但是这些代码却呆在主干分支中
• 太多太多不一一列举
  造成这些问题的原因很大一部分都是团队没有CR制度，在我呆过的几家公司中，有大有小。先不论CR质量，有代码CR机制的就只有一家，其中不乏一些挺出名的公司，都是写完代码，push上去之后，可能自己就merge，或者找个人，对方也懒得看，直接merge。长此以往，主干分支就成为了“屎山”。

CR机制的好处

发现代码中的漏洞以及BUG

我们来看一组数据，其中灰色的直方是CR，可以看到Vulnerabilities、Privacy和Business Logic方面发现问题的比例还是很大的，特别是在Privacy和Business Logic这两个方面，这个是很好理解的，因为这两个方面自动化测试确实不那么好做，特别是业务逻辑的问题，代码扫描基本发现不了这类问题。因此，CR对于发现代码中的漏洞和BUG还是有很重要的作用

提升代码可读性

代码不是写给自己看的，更多时候是写给别人看的。团队协作不是炫技，如果你为了炫技常常通过很难理解的代码实现逻辑，从团队的角度出发，哪怕是你技术再强，你也不是一个优秀的团队伙伴。通过CR可以让别人理解你的代码，如果Reviewer看不懂你的代码，那么你的实现应该是糟糕的

保证代码一致性

代码的一致性很重要，代码中所有不一致的地方都可能对后续维护的开发人员造成理解误差，在开发过程中一致性甚至大于规范性

分享新技术

一个人的强不是强，一个团队的强才是真的强。在CR过程中，开发人员可以相互之间分享新的技术。

检查需求实现是否一致

有时开发人员可能对需求的理解有偏差，通过让别人帮你CR可以确认你的实现是否符合产品需求

提升代码性能

年轻工程师由于缺乏经验，可能不知道一些代码优化技术。Code Review可以让他们在CR的过程中学到优化代码的技术。但是要注意，代码可读性应该优于性能，不应过早的优化代码性能。

如何做代码评审

准备工作

• 首先你应该仔细阅读公司的代码规范，一定要以公司的为主。代码规范就像一种技术，很多时候不是说一个技术好我们就要用，而是要根据公司的实际情况。不能说你刚加入公司，觉得你上一家的规范比这一家的好，你要按照自己的那一套规范来，这是不可取的，因为规范要统一才有作用，每个人都实施自己的规范那就是没有规范了
• 熟悉你们的CR工具平台，确保自己会熟练使用工具

准备完成后，你就可以去参与CR活动了

CR如何发现问题

为了避免在CR过程中被玩成了大家来找茬儿，我们对CR过程中应该关注的问题分了三个层次，组成了CR问题金字塔，分别是设计、实现和规范

设计

CR时，首先应该关注设计问题，为什么呢？因为如果本次实现的逻辑有设计问题，那么肯定是要大改，已经没有关注实现和规范的必要了，常见的一些设计问题

• 这个功能是否有更成熟的开源实现，不需要自己实现
• 软件架构设计问题，架构是否设计合理，有无过渡设计等等
• 代码分层，这个问题是最多的，很多年轻工程师设计经验不足，在分层时比较混乱。如何分层一两句话可能也讲不清楚，简单的项目可以直接按照MVC分层即可；如果时复杂项目，那么可以参照洋葱架构或者六边形架构
• 抽象设计问题，很多年轻工程师对抽象设计经验不足，常常体现在类职责划分不对

实现

当我们的代码设计没有问题时，那么就要开始关注实现了。那么常见的实现问题有哪些呢

• 实现复杂，经常体现在函数圈复杂度过高，常见的有if-else嵌套太多、if-else分支判断太多，这些可以用用策略模式来优化
• 方法职责不单一，这个也是常见的
• 逻辑错误，比如需求实现有误
• 安全问题，变量被并发修改、资源未关闭造成资源泄露等、SQL注入问题
• …

规范、一致性等

规范性的问题一般参照公司的规范性文档，一致性就是要保证团队编码风格、专有词汇、实体变量等等要求是一致的，不能存在理解误差，常见的规范和一致性问题有哪些呢

• 变量命名，驼峰还是下划线连接
• 方法命名，一般应该以动词开头表示一个动作
• 魔法值，应该用常量代替方便维护和理解
• 注释，虽然良好的代码应该是自解释的，但是如果我们技术水平达不到的话还是老实在关键地方加上注释
• 异常的处理，比如java中有些团队是在业务逻辑处理层抛checked异常，有些团队则是使用unchecked(RuntimeException)异常，这个就要和团队保持一致
• 当然规范性的东西很多，这里不一一列举了，而且每个公司的规范可能不一样

优秀的点

CR过程中也要发现好的地方，因为好的地方可以提取出来作为团队的知识库，团队成员可以共享知识共同成长

CR如何编写Comment

我们在找出问题之后我们应该怎么编写comment呢，编写comment是一门艺术。你的目的是提出自己的问题以及建议，甚至需要和开发人员讨论，这是一个双向协作的过程。为了和谐的展开CR，你应该做到

• 避免使用质问的语气，应该使用疑问或者建议的语气，避免开发人员觉得自己被质问而恼火，这样也不利于展开后续的沟通
• 解释建议的原因，如果给出了建议，应该附带给出这样建议的原因，而不是直接建议让开发人员疑惑
• 避免拐弯抹角，应该清晰明了的提出问题或建议，不要让开发人员去猜
• 接受解释，不能一味坚持自己的观点，应该和开发人员沟通，如果他的解释是合理的，那么Reviewer应该接受

代码评审工程实践

CR频率

很多公司也有CR机制，但是他们的集中式CR在我看来就是一种作秀。比如说有些团队一周一次或者是上线前一起做CR，这里就按一周一次，一个团队平均10个开发，假设平均每天100行有效代码，那么每次集中评审时将有5000行有效代码，注意这里是有效代码行，我相信这个数据肯定是少的，真实情况肯定比这个多。那么每周拿2小时左右看5000行代码，你觉得能看出啥，我估计都没看懂逻辑，更别说提Comment。还有那种上线前代码走查的一个版本的代码至少都是大几千行，这么大的代码量根本无法做有效CR
CR应该是我们日常开发的一部分，应该融入到日常的开发过程中，而不是等着代码堆成小山之后大家一起集中起来做。所以CR的频率应该就像你写代码一样，每天都要进行

CR代码量

提交代码请小伙伴帮忙CR时，代码量不宜过多，代码量多的话有两个问题

• 代码量大意味着理解难度大，短时间之内，Reviewer可能很难理解，这样会大大降低CR效率
• 每次实现很多功能之后再提交代码，如果CR过程中有设计问题，那么改动就会非常大，意味着前期的开发时间可能做了无用功，所以要小步快跑，快速迭代的方式
  
  上图横坐标是每次CR代码行数，纵坐标表示CR每小时发现问题数，可见当代码量越大时，CR效率下降的还是很明显的，所以你每次提交CR的代码应该控制在400行以内，你可能会说经常有自动生成的数据库操作代码动不动几千行，对于这类代码你应该单独发起MR，这类代码不需要CR。将自动生成的代码和你的业务逻辑代码分离开

MR的提交信息

提交信息非常关键，至于你代码提交信息倒不是很重要。但是发起MR时，这个提交信息一定要描述清楚，这里建议的结构

背景：描述做这个功能是要解决什么问题，可以在这里粘贴需求文档等，方便Reviewer理解你的需求，
	用来判断你的设计和实现是否合理，是否有现有的开源库已经实现
实现：描述清楚怎么实现这个功能

CR工具

如果你的公司有自主研发的工具，那么你应该没啥选择，如果你公司没有，那么这里有几个可以推荐的

• Gitlab和Github自带的CR功能
• Gerrit
  其他的看个人习惯，就我自己而言，喜欢Gitlab自带的

不要在代码扫描结束前发起CR

如果你们公司有代码规范扫描的流水线，那么请先扫描完成无明显规范性问题之后再发起CR

CR总结会

团队应该至少两周开一次CR周会，在周会上主要是解决平时CR中遗留的没有达成一致的问题，在CR周会上可以讨论达成一致，一定要将达成的规范落地到知识库作为团队的技术财产

不要害怕别人给你提出问题

很多人可能害怕CR，觉得别人看自己写的代码如果有问题会笑话自己。如果你有这种想法，那你真的想多了，我觉得程序员的性格大多都是比较纯粹，他们都喜欢帮助别人，并且从中获得成就感，所以当你的代码写得不好时，大胆的请他们帮你CR，你会成长的非常快，毕竟工程实践的东西在交流中学习比自己闭门造车高效多了

最后

最后要说的当然是大家要提升自己的技术，这里推荐几本书给大家《Effective Java》、《Clean Code》《重构：改善既有代码的设计》，网上很容易找到电子版，如果你找不到找我发给你