说透代码评审

发表于 3年以前 | 总阅读数：259 次

参考资料：

《互联网大厂如何玩转代码评审》梁松华京东高级开发工程师
《学习Facebook真正发挥代码审查的提效作用》葛俊前Facebook内部工具团队Tech Lead
《代码审查哪种方式更适合我的团队》葛俊前Facebook内部工具团队Tech Lead
《聊一聊代码审查》熊燚（四火）Oracle首席软件工程师
《代码审查普遍存在的 6 大问题》松花皮蛋me InfoQ
《代码评审：寄望与哀伤》胡峰京东成都研究院技术专家

代码如熵，不加外力很容易就会随着代码的不断堆积而发生腐烂和溃败。我们不是不知道代码问题，而是对既成事实难有改变。但是如果站在迭代的角度思考下一次升级，如何确保新的代码质量就显得很有必要，特别是你的代码需要重写的时候，我想你一定会遇到和我一样的问题，我们究竟应该如何保证我们的代码的质量？于是就有了这篇工具型的文章。

以下内容架构是我摘录觉得比较重要的纬度，很多方法论借鉴了以上几篇文章思想，对代码评审的各种情况基本都谈得比较到位了，反复精读基本上就可以采用拿来主义，希望对你的团队评审有所帮助。

为什么要评审

不审查的坏处

代码的质量反应了我们的产品质量，产品不稳定、老是出现BUG，直接影响客户满意度和口碑。

同时，代码的好坏决定了未来运维的成本，如果因为一时疏忽和妥协，回头又没有及时修改，中间又出现人员变动，那么这份代码的后患是无穷的。

因为不规范，可读性差，对交接人来说从心态上是本能反抗的，但是又不得不改，于是就一通乱改，能贴膏药就贴膏药，能运行就可以，管他规范不规范。这样导致的后果是，代码从不规范走向更加不规范，很难想象经过5-10年持之以恒的不规范，这个产品还能活着。

技术债务的危害怎么形容都不为过，轻则系统局部异常，中等的会导致修改困难，严重的需要推翻重来。

从物理学上看，熵让我们理解了一件事，如果不施加外力影响，事物永远向着更混乱的状态发展，所以规范和审查就显得弥足珍贵了。

从软件设计看，软件设计要关注长期变化，需要应对需求规模的膨胀。如果腐烂的代码日积月累，这些在不断流变腐烂的东西又怎能支撑起长期的变化呢！

做产品，不审查不足以长久。

评审的好处

在软件工程师日常的开发工作中，如果要挑出一项极其重要，却又很容易被忽视的工作，在我看来代码审查几乎是无可争议的第一。代码审查是一个低投入、高产出的开发活动，就个人而言，从其中学到的习惯、方法和知识，获益匪浅。

代码评审的作用很多，主要表现在 6 个方面。

好处 1，尽早发现 Bug 和设计中存在的问题。我们都知道，问题发现得越晚，修复的代价越大。代码审查把问题的发现尽量提前，自然会提高效能。

好处 2，提高个人工程能力。不言而喻，别人对你的代码提建议，自然能提高你的工程能力。事实上，仅仅因为知道自己的代码会被同事审查，你就会注意提高代码质量。

好处 3，团队知识共享。一段代码入库之后，就从个人的代码变成了团队的代码。代码审查可以帮助其他开发者了解这些代码的设计思想、实现方式等。另外，代码审查中的讨论记录还可以作为参考文档，帮助他人理解代码、查找问题。

好处 4，针对某个特定方面提高质量。一些比较专业的领域，比如安全、性能、UI 等，可以邀请专家进行专项审查。另外，一些核心代码或者高风险代码，也可以通过团队集体审查的方式来保证质量。

好处 5，统一编码风格。这，也是代码审查的一个常见功能，但最好能通过工具来实现自动化检查。

好处 6，社会性功用。如果你在编程，而且知道一定会有同事将检查你的代码，那么你编程的姿势和心态就会完全不同。这之间的微妙差异正是在于会不会有人将对你的代码做出反馈与评价。

评审的困境和争议

大多数的争议，都不是在否认代码审查的好处，而是聚焦在不进行代码审查的那些“原因” 或者 “借口”上。

1）代码评审费时费力：

项目期限 Deadline 已定，时间紧迫，天天加班忙成狗了，谁还愿意搞代码评审？这是一个最常见的客观阻碍因素，因为 Deadline 很多时候都不是我们自己能确定和改变的。

改来改去无非是一些格式、注释、命名之类不痛不痒的问题。

2）代码审查不利于团建：

因为经常有程序员因为观点不同在代码审查的时候吵起来。

3）主观意愿

评审人用自己主观意见看待别人的代码。觉得别人的代码风格不够好，或者把个人的喜好强加到别人身上。

4）团队人员结构搭配不合理。

新人没经验的多，有经验的少。新人交叉评审可能效果不好，老是安排经验多的少数人帮助 Review 多数新人的代码，新人或有收获，但对高级或资深程序员又有多大裨益？

一个好的规则或制度，总是需要既符合多方参与者的个体利益又能满足组织或团队的共同利益，这样的规则或制度才能更顺畅、有效地实施和运转。

5）有人就是不喜欢别人 Review 他的代码，他会感觉是在找茬。

比如，团队中存在一些自信超强大的程序员，觉得自己写的代码绝对没问题，不需要别人来给他 Review。

6）效果非常差、形式主义

代码评审做得不好确实像形式主义，纯粹走个过场。

搞清楚评审形式

落地之前，我们先搞清楚评审形式是什么样子的。

一般来说人工检查才叫审查，机器进行的检查一般叫作代码检查或者代码自动检查。常见的办法是人工评审和机器检查同时进行。

代码评审形式可以分为：

机器检查
人工评审
纯线上评审
线下投屏评审(推荐)

这里推荐做法是线下投屏评审，或许传统、保守，但是利大于弊。根据我的经验，线上问题处理和后续工作计划调整，所产生的一次性成本远远大于一次性线下评审。

评审对象有哪些

我们应该约定以下几种操作都要进行评审，一般变动比较大的需要多人评审，一般性修改只需要其他同事二次检查即可。

评审参与人员

在这个问题上，原则是业务密切相关的人员都要参加，比如说：

开发人员的导师
负责这个业务流转链路的下一个环节的同事

大家参与进来能够及时同步信息，避免信息的不对称，也就是说可以避免其他环节需要人员兼容改动的，可是实际上没有人考虑到，而导致漏改问题。

问题：为什么说要坚持密切相关的人员都参加这个原则？

答案：大部分人对于自己不感兴趣或者听不懂的事务时，非常容易走神。所以组织业务密切相关的一线开发，组长，及领导参与评审就足够了。

评审应该关注哪些

最后还有一个需要我们搞懂的问题，弄清楚之后，我们才能很好的把代码评审落地。

代码评审容易犯的错误：评审人用自己主观意见看待别人的代码。

觉得别人的代码风格不够好，或者把个人的喜好强加到别人身上，有这种想法固然好，如果觉得在团队内应该统一使用这种风格，最好申请把他纳入到团队的代码规范文档中，以避免代码评审的重点产生偏移。

代码评审的目的：最大化维护团队的利益。

为了软件的良好发展和团队的高效协作，每个人的代码最好看上去都差不多，这样修改别人的代码就比较亲切。

代码评审应该关注的重点：
是否明显的逻辑错误
是否落实了代码规范
代码的可读性和可维护性是否良好
代码是否有违背基本的设计模式理念
代码评审不应该过多关注：
代码是否能正常运行
代码是否满足业务需求
是否覆盖业务场景

这些应该由编写代码的人和测试人员共同保证

评审的流程有哪些

每个想要高效工作的程序员，都不希望自己的计划被频繁打断，大多数情况下按部就班，跟着规划走才是最稳妥的。

下面是评审中的六个流程：

约定规范文档

为了避免审查当中的主观喜好，制定有章可循的约定是前提条件。

制定排期：

我们可以约定每周一发起代码评审，由提交人根据逻辑变动情况，给出一个评审大概需要花费的时间，同时结合需求的提测时间，上线时间，确定好什么时候进行评审，避免过多杂乱的时间线。

补充资料

在评审工期申请后，还需要补充资料，交代清楚需求背景、编码内容以及功能影响的范围，评审人就可以根据这些资料，评估应该重点关注哪些代码隐患。

比如需求涉及到金钱往来，那么评审人就应该更加仔细留意每一行代码，检查是否有明显的逻辑错误，避免上线后导致用户或者公司的利益受到损害。

代码评审

代码评审中发现的问题，根据严重性决定是否进行二次评审。

完善必备要素

发现问题并不可怕，可怕的是相同的问题不同的同事重复犯，所以必须在评审后完善代码评审必须具备的元素清单，比如监控、注释等。如果不满足任何一个，直接驳回，毕竟重复提及相同的问题，会大家参与者的积极性。

完善代码规范约定

另外也需要定期回顾之前的代码评审，完善团队代码规范约束，让软件质量和团队能力都想着更好的方向走。

审查步骤和方法

从我的经验来看，要成功引入代码审查，首先要在团队内达成一些重要的共识，然后选择试点团队实行，最后选择合适的工具和流程。

个人觉得不是所有的团队都适合做代码审查，一定要慎用。特别是那种项目类型的团队，救火类型的团队，团队规模很小，业务优先的团队。

1、代码审查应该计入工作量

代码审查如果没有为它预留时间，结果是大家没有时间做审查，效果自然也就不好。于是，形成恶性循环，代码审查要么被逐渐废弃，要么流于形式。

预估工作量的时候需要考虑代码审查的时间。比如，平均每天大约预留 30-60分钟用于代码审查，大概占写代码总时间的 1/5。

一般的代码审查速度约是一小时 150 行，对于一些关键的软件，一小时数百行代码的审查速度太快，可能无法找到程序中的问题。

同时，代码审查的情况会作为绩效考评的一个重要指标。

另外，平时需要给审查者关于审查质量的实时反馈。比如，刚加入公司的时候，对代码审查不够重视，做得不够好。主管应该对给反馈意见，让新人提高审查质量。

其次，让新人多给同事做审查，另外，是让新人多给一些结构上的建议，不用太重视语法细节。

总之，管理者要明确代码审查是开发工作的重要组成部分，并记入工作量和绩效考评。这，是成功引入代码审查的前提，再怎么强调都不为过。

2、选择试点团队

为什么要选择试点团队，说白了就是要避免纸上谈兵的不足，在小范围内做实验可以有效降低风险，尽可能得收集负面效果，并及时改进。否则大规模出现负面效果是很影响大家信心，甚至出现反面效果。

3、选择工具，融合机器审查和人工审查

关于工具，如果你的团队本来已经在使用 GitLab、GitHub、Gerrit、Phabricator 管理代码的话，那么很容易上手代码审查。因为，GitHub、GitLab 有基于 PR 的审查。而 Gerrit 和 Phabricator 本身就主打代码审查的功能。

第一，将代码提交到本地 Git 仓库或者用于审查的远端 Git 服务器的分支上；第二，把 commit 提交给代码审查工具；第三，代码审查工具开始进行机器审查和人工审查；第四，如果审查通不过就打回重做，开发者修改后重新提交审查，直到审查通过后代码入库。

关于工具集的适用：使用 GitLab、Jenkins 和 SonarQube 进行配置。具体使用 GitLab 管理代码，代码入库后通过钩子触发 Jenkins，Jenkins 从 GitLab 获取代码，运行构建，并通过 Sonar 分析结果。这里有一篇不错的文章供你参考

评审的关键操作

提高提交的原子性

每次提交的代码粒度至关重要，我们可以反过来思考：

如果提交的是半个功能的代码会怎么样？
如果提交的是一周的代码会怎么样？

所以原子性就是合适的粒度，大功能要拆分来提交，一周的代码的代码要按天来提交。否则对于评审人员来说是很反感的，因为只会增加审查的难度。

提高提交说明的质量

我们应该经常见到很多的提交是这样描述的：

BUG
FIX
更新

下面是葛俊给出的三个改进步骤：

第一步，规定提交说明一定要包括标题、描述和测试情况三部分，但暂时还不具体要求必须写多少字。比如，测试部分可以简单写一句“没有做测试”，但一定要写。如果格式不符合要求，审查者就直接打回。这个格式要求工作量很小，比较容易做到，两个星期后整个团队就习惯了。虽然只是在提交说明里增加了简单描述，也已经为审查者和后续工作中进行问题排查提供一些必要信息，所以大家也比较认可这个操作。

第二步，要求提交说明必须详细写明测试情况。如果没有做测试一定要写出具体理由，否则会被直接打回。这样做，不但为审查者提供了方便，还促进了开发人员的自测。整个团队在一个多月后，也养成了详细描述测试情况的习惯。

第三步，逐步要求提交的原子性。我要求每一个提交要在详细描述部分描述具体实现了哪些功能。如果一个提交同时实现了多个功能，那就必须解释为什么不能拆开提交；如果解释不合理的话，提交直接被打回。

提交说明是提高代码审查的利器，好的格式应该包含以下几个方面：

标题，简明扼要地描述这个提交。这部分最好在 70 个字符之内，以确保在单行显示的时候能够显示完整。比如，在命令行常用的 git log --oneline 输出结果要能显示完全。

详细描述，包括提交的目的、选择这个方法的原因，以及实现细节的总结性描述。这三个方面的内容最能帮助审查者阅读代码。

测试情况，描述的是你对这个提交做了什么样的测试验证，具体包括正常情况的输出、错误情况的输出，以及性能、安全等方面的专项测试结果。这部分内容，可以增加审查者对提交代码的了解程度以及信心。

与其他工具和系统相关的信息，比如相关任务 ID、相关的冲刺（sprint，也可翻译为“迭代”）链接。这些信息对工具的网状互联提供基础信息，非常重要。这里还有一个 Git 的技巧是，你可以使用 Git 的提交说明模板（Commit Message Template），来帮助团队使用统一的格式。

评审的关键原则

虽说评审需要规范文档做指导，但是很多规范其实无法做的那么细，特别是规范早期，很多规范是缺失的。在评审过程中难免会出现争论或者相持不下的情况。这个时候有必要把评审的原则提前做一个说明，可以消除未来不必要的麻烦。

相互尊重

从编码作者的角度来看，审查者要花费时间去阅读他并不熟悉的代码，来帮助你提高，应该尽量为审查者提供方便。

比如，提高提交说明的质量，就是对审查者最基本的尊重。
还有，如果你的代码都没有进行自测就提交审查，你觉得审查者心里会怎么想呢？
又如，如果你提交的一个审查有一万行代码，让审查者怎么看呢？

所以，代码作者一定要提审查者着想，帮助审查者能够比较轻松地完成审查。

从审查者的角度来看，在提出建议的时候，一定要考虑代码作者的感受。最重要的一点是，不要用一些主观标准来评判别人的代码。

基于讨论

代码审查的目的是讨论，而不是评判，这个原则至关重要。

讨论的心态，有助于放下不必要的自尊心，从而顺利地进行技术交流，提高审查效率。

另外，讨论的心态也能促进大家提早发出审查，从而尽早发现结构设计方面的问题。

另外，我还有一些关于讨论的建议：审查者切记不要说教，说教容易让人反感，不是讨论的好方法。

审查者提意见即可，不一定要提供解决方法。给定答案也可能会引起不必要的反感。

代码评审的常见问题

这些是代码评审过程中发现的共同的问题，可以一起放在代码规范文档中。

缺少注释和变更说明

比如下面三个方法名称，参数，返回值相似，为什么会出现这种情况？

大半是开发者发现原来的代码没有注释，所以不敢修改，于是拷贝一份后只是稍微修改了一下，这样才出现了相似冗余的代码。重复代码最可怕的地方就是错该或者漏改


publi Article GetArticleById(long id)
{
  return null;
}
publi MaterialPO GetMaterialById(long id)
{
  return null;
}
publi ArticleVO GetArticleByIdWithoutStatus(long id)
{
  return null;
}

过渡相信第三方

对请求参数没有限制
对请求参数没有校验

//错误示范
Boolean SaveError(List<ShareDetail> list)
{
   //错误原因：只判断非空，但是无法保证list中的detail是非空的
   if(list.IsEmpty())
   {
      return false;
   } 
   //错误原因：无法保证list中的id都是一样的
   Int id = list.get(0).getId();
   //保存数据
   return Save(id,list)
}

//正确示范
Boolean SaveRight(Int id,List<ShareDetail> list)
{
   //深度判断非空情况
   if(id==null || CollectionUtil.hasNull(list))
   {
      return false;
   } 
   //id从参数中获取，避免二义性
   return Save(id,list)
}

变量作用域过大


public static void main(Sstring[] args){
  ShareDetail shareDetail1 =new ShareDetail();
  //错误示例：对象在多个方法间进行值地址引用传递
  varErrorStep1(shareDetail1); 
  //正常示例
  ShareDetail shareDetail2=varRight();
}

public static varErrorStep1(ShareDetail shareDetail)
{
   //其他复杂操作
  shareDetail.setId(111);
  var ErrorStep2(shareDetail);
}

public static void varErrorStep2(ShareDetail shareDetail)
{
  //其他复制操作
  shareDetail.setName("hello");
}

public static ShareDetail varRight()
{
  ShareDetail shareDetail =new ShareDetail();
  shareDetail.setId(111);
  shareDetail.setName("hello");

缺少阶段性结果

对于过多的if-else判断，优化方法：先进性异常判断，如果有异常就快速返回结果。


//错误示范
public void SetpError()
{
  //标记位
  Boolean flag=false;
  List<Integer> list=new ArrayList<>();
  for(Integer detail:list)
  {
    //如果列表中有值大于10则将标记设置为true
    if(detail>0)
      {
        flag=true;
      }
    }
  }
  //
  if(flag)
  {
    return；
  }
}
//正确示范
public void SetpRight()
{
  //步骤一：校验参数
  List<Integer> list=new ArrayList<>();
  for(Integer detail:list)
  {
    //如果列表中有值大于10则将标记设置为true
    if(detail>0)
      {
        return; //移除多余的标记，直接中断；
      }
    }
  }
  //步骤二：

  //步骤三：
}

同时为了增加代码的层次感，可以让代码阶段性的输出结果，比如编写时注释好每一个步骤要做什么。

日志打印问题


//错误示范
public Boolean logError(Integer id,List<ShareDetail> list)
{
  if(id==null || CollectionUtil.hasNull(list))
  {
      return false;
  }
  logger.info("logError run");

  try{
    return shareDao.save(id,list);
  }catch(Exception e)
  {
    //错误一：不记录异常上下文
   logger.error("save error");
   //错误二：只记录了有限的上下文
   logger.error("save error e:{0}",e.getMessage());
  }
  return false;
}

//正确示范
public Boolean logError(Integer id,List<ShareDetail> list)
{
  if(id==null || CollectionUtil.hasNull(list))
  {
      return false;
  }
  //info级别在非线上环境很容易
  logger.info("logError run");

  try{
    return shareDao.save(id,list);
  }catch(Exception e)
  {
    //正确：充分记录错误上下文
    logger.error("save error");
    //正确：规避记录日志时出现控制着，规避内存OOM
    logger.error("save error id:{},e:{},list:{},id,list==null?list:JSONObject.toJSON(list),e);
  }
  return false;
}