说透代码评审

参考资料：

• 《互联网大厂如何玩转代码评审》 梁松华 京东高级开发工程师
• 《学习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. 开发人员的导师
2. 负责这个业务流转链路的下一个环节的同事

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

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

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

评审应该关注哪些

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

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

　　以为别人的代码风格不够好，或者把我的的喜爱强加到别人身上，有这种想法当然好，若是以为在团队内应该统一使用这种风格，最好申请把他归入到团队的代码规范文档中，以免代码评审的重点产生偏移。

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

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

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

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

评审的流程有哪些

　　每一个想要高效工做的程序员，都不但愿本身的计划被频繁打断，大多数状况下循序渐进，跟着规划走才是最稳妥的。

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

　　

• 约定规范文档

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

• 制定排期：

　　咱们能够约定每周一发起代码评审，由提交人根据逻辑变更状况，给出一个评审大概须要花费的时间，同时结合需求的提测时间，上线时间，肯定好何时进行评审，避免过多杂乱的时间线。

• 补充资料

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

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

• 代码评审

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

• 完善必备要素

　　发现问题并不可怕，可怕的是相同的问题不一样的同事重复犯，因此必须在评审后完善代码评审必须具有的元素清单，好比监控、注释等。若是不知足任何一个，直接驳回，毕竟重复说起相同的问题，会你们参与者的积极性。

• 完善代码规范约定

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

审查步骤和方法

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

　　我的以为不是全部的团队都适合作代码审查，必定要慎用。特别是那种项目类型的团队，救火类型的团队，团队规模很小，业务优先的团队。

一、代码审查应该计入工做量

　　代码审查若是没有为它预留时间，结果是你们没有时间作审查，效果天然也就很差。因而，造成恶性循环，代码审查要么被逐渐废弃，要么流于形式。

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

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

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

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

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

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

二、选择试点团队

　　为何要选择试点团队，说白了就是要避免纸上谈兵的不足，在小范围内作实验能够有效下降风险，尽量得收集负面效果，并及时改进。不然大规模出现负面效果是很影响你们信心，甚至出现反面效果。

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

　　关于工具，若是你的团队原本已经在使用 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;
}

总结

　　想让团队从内心接受代码评审的理念，承认它是平常开发过程必不可少的步骤，那么就要提升代码评审的质量，不然评审的时候，你们都在搞私事，容易流于形式，而要提升代码评审的质量，咱们必须明确代码评审的形式，对象，参与者，关注点，流程，常见问题。

　　我但愿你们能记住一件事，代码评审要从团队利益出发，让每一个人都可以从里面持续获得正反馈，这才是最重要的。若是时间充足的话最好邀请测试人员一块儿参与，让测试人员充分了解代码改动点，有助于对测试场景边界的把控。