Code Review 指南

PPT: https://www.yuque.com/itguang/mweb/code-review-zhi-nan

文章目录

CR 中需要关注什么？

降低圈复杂度

关注性能问题

关注分布式事务

关注架构设计。

语境

不要吝啬你的赞赏和鼓励

如何编写 CR 评论(Comments)

如何处理 CR 评论（Comments）

代码审查的速度

Git Commit Message 应该怎样写

总结

前言

先来看一个令无数开发者闻风丧胆的项目“死亡”三角：

业务压力引发代码质量下降，代码质量下降引发开发效率下降，开发效率下降又加重了业务压力，最终导致业务压力山大，乃至项目烂尾。

如何破解？方法有很多，像精简业务需求、增加开发人手、升级技术架构等，很多时候需要多管齐下，但凡打掉这个“死亡”三角中的任何一角，就能终止这个恶性循环，甚至逆转为良性循环。

代码评审（Code Revew，简称CR）的首要打击目标显然是“烂代码”。避免“烂代码”的最好时机是写代码的时候，其次是代码评审的时候。

CR 是什么？

来看下 Google Code Review Developer Guide 对其的定义：

“A code review is a process where someone other than the author(s) of a piece of code examines that code.”
Code Review 是一段代码作者以外的其他人审查该代码的过程。

在 Google，工程师们使用 CR 来维护代码和产品的质量。代码审查的主要目的是确保代码库的整体代码健康状况随着时间的推移而改善。代码审查的所有工具和过程都是为此目的而设计的。

还记得初次接触 Code Review ，那时刚从学校毕业，编程都是自学，走的都是野路子。毕业后入职，公司老板和leader都是外企出来的，工程师文化比较浓厚，其中 CR 也是必不可少。入职没多久就提交了自己的第一个需求，第二天一来，打开 Gitlab 的 Merge Request，收到了 34 个 Comments，平均每个类 5 个 Comments，可想而知，当时我的内心是有多崩溃，但是仔细看每个 Comments 提的建议都非常中肯，有些甚至是 BUG 级别的 Comments。后来我痛定思痛，把别人对我的所有的 Comments 都整理记录了下来，分类总结，吸取教训，过了几个月之后，我的 Comments 数量显著减少，也真正体会到了 CR 机制带给我的变化和好处。初次接触 CR 的人，肯定刚开始是不适应的，但是当了解了 CR 的好处后，你会非常享受这种机制。

一张图先来看看 CR 在开发流程中的位置:

CR 整体流程:

下面 8 条有关 CR 的阐述，你觉得哪些是正确的？

1. 搞形式主义，存粹是浪费时间
2. CR 是保证程序正确性的手段
3. CR 是保证代码规范的手段
4. CR 是 Leader 的事，跟我没关系
5. 我只看指给我的 CR，其他 CR 跟我没关系
6. 没有时间 CR，直接 Merge
7. CR 必须一行不落从头看到尾
8. CR 必须一次完成

---

请仔细思考 60 秒

3…2…1…时间到，你的答案是几条？很抱歉，在我看来，没有一条是正确的。
1、4、5、6 是送分题，显然都是错误的。
7 是眉毛胡子一把抓，CR 就像读书，不是所有的书都适合精度，也不是所有的代码都需要评审。
8 是任务心态，为了 CR 而 CR，CR 的目的不是完成 CR，而在于提升代码质量，你写代码时也不会一次完成所有功能。
比较有争议的是 2 和 3，诚然，正确性和代码规范都是 CR 要关注的方面，但这并不意味着 CR 要保证正确性和代码规范（CR 也没法保证），保证正确性的主要手段是测试（单元测试，集成测试，契约测试，功能测试，自动化测试等），而保证代码规范主要依靠代码规范检查工具（像常用的 checkstyle 和 PMD）。

为什么要做 Code Review ？

IBM 的 Orbit 项目有 50 万行代码，使用了 11 级的代码检查（其中包含代码评审），结果是项目提前交付，并且只有通常预期错误的 1%。一份对 AT＆T 的一个 200 多人组织的研究报告显示，在引入代码评审后，生产率提高了 14％，缺陷减少了 90％。

《左耳听风》作者陈皓对代码质量的评级有过这样的描述：

我个人认为代码有这几种级别：1）可编译，2）可运行，3）可测试，4）可读，5）可维护，6）可重用。通过自动化测试的代码只能达到第3）级，而通过CODE REVIEW的代码少会在第4）级甚至更高。

我认为，Code Review 可以为我们带来三大好处。代码质量提升，知识交流，学习。

编程是一种技巧，是可以通过训练提高的。

那到底什么是代码评审？如何进行代码评审？业界有很多 CodeReview 规范，本文我们介绍 Google 的 CodeReview 是如何做的？

CR 的标准

一个 CL¹ 什么情况下可以被批准，google 给出了一条准则：

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
通常，一旦CL处于能改善所处理系统的整体代码健康状况的状态，即使CL并不完美，Reviewers 也应该倾向于批准它。

当然，这是有局限性的。例如，如果 CL 在其系统中添加了 Reviewers （审阅者）不想要的功能，那么即使代码设计良好，Reviewers 也可以拒绝批准。

这里的一个关键点是 没有“完美”的代码——只有更好的代码 。Reviewers 不应要求作者在批准之前对 CL 的每一小块进行润色。相反，Reviewers 应该平衡项目的进展与他们建议之间的重要性。Reviewers 不应该追求完美，而应该追求的是 持续改进。一个整体上提高了系统的可维护性、可读性和可理解性的 CL 不应该因为它不是“完美的”而延迟数天或数周。

进行 CR 的一些原则

代码审查的重要功能是向开发人员传授有关语言、框架或一般软件设计原则的最佳途径。留下有助于开发人员学习新知识的评论总是好的。下面是 google 提到的一些指导原则:

CR 中需要关注什么？

代码审查是提高代码质量、建立最佳实践和传播知识的强大工具。但是，当你尝试在团队中实施有效的代码审查流程时，会遇到许多挑战。

此外，代码审查如果做错了，可能会一事无成，甚至会损害你的人际关系。因此，重要的是要注意代码审查的人为方面。Submitter 和 Reviewers 都需要一个指南针来进行建设性和尊重的代码审查。

下面列举 CR 时需重点关注的几个方面，并辅以相应的例子便于理解。

关注代码规范

代码坏味道

Duplicated Code （重复代码）

编程法则第一条，Don’t repeat yourself. 重复代码是万恶之首，重复代码人人得而诛之！

重复代码就是不同地点，有着相同的程序结构。一般是因为需求迭代比较快，开发小伙伴担心影响已有功能，就复制粘贴造成的。重复代码很难维护的，如果你要修改其中一段的代码逻辑，就需要修改多次，很可能出现遗漏的情况。

如何优化重复代码呢？分三种情况讨论：

1. 同一个类的两个函数含有相同的表达式

class A {public void method1() {doSomething1doSomething2doSomething3}public void method2() {doSomething1doSomething2doSomething4}
 }
 

优化手段：可以使用 Extract Method(提取公共函数) 抽出重复的代码逻辑，组成一个公用的方法。如下：

class A {public void method1() {commonMethod();doSomething3}public void method2() {commonMethod();doSomething4}public void commonMethod(){doSomething1doSomething2}
 }
 

2. 两个互为兄弟的子类内含相同的表达式

class A extend C {public void method1() {doSomething1doSomething2doSomething3}
 }class B extend C {public void method1() {doSomething1doSomething2doSomething4}
 }
 

优化手段：对两个类都使用Extract Method(提取公共函数)，然后把抽取出来的函数放到父类中。

class C {public void commonMethod(){doSomething1doSomething2}
 }
 class A extend C {public void method1() {commonMethod();doSomething3}
 }class B extend C {public void method1() {commonMethod();doSomething4}
 }
 

3. 两个毫不相关的类出现重复代码

如果是两个毫不相关的类出现重复代码，可以使用Extract Class将重复代码提炼到一个类中。这个新类可以是一个普通类，也可以是一个工具类，看具体业务怎么划分吧。

Long Method (长函数)

长函数是指一个函数方法几百行甚至上千行，可读性大大降低，不便于理解。反例如下：

public class Test {private String name;private Vector orders = new Vector();public void printOwing() {//print bannerSystem.out.println("****************");System.out.println("*****customer Owes *****");System.out.println("****************");//calculate totalAmountEnumeration env = orders.elements();double totalAmount = 0.0;while (env.hasMoreElements()) {Order order = (Order) env.nextElement();totalAmount += order.getAmout();}//print detailsSystem.out.println("name:" + name);System.out.println("amount:" + totalAmount);......}
 }
 

可以使用Extract Method，抽取功能单一的代码段，组成命名清晰的小函数，去解决长函数问题，正例如下:

public class Test {private String name;private Vector orders = new Vector();public void printOwing() {//print bannerprintBanner();//calculate totalAmountdouble totalAmount = getTotalAmount();//print detailsprintDetail(totalAmount);}void printBanner(){System.out.println("****************");System.out.println("*****customer Owes *****");System.out.println("****************");}double getTotalAmount(){Enumeration env = orders.elements();double totalAmount = 0.0;while (env.hasMoreElements()) {Order order = (Order) env.nextElement();totalAmount += order.getAmout();}return totalAmount;}void printDetail(double totalAmount){System.out.println("name:" + name);System.out.println("amount:" + totalAmount);}}
 

Large Class (过大的类)

一个类做太多事情，维护了太多功能，可读性变差，性能也会下降。Duplicated Code 也就接踵而至了。

Class A{public void printOrder(){System.out.println("订单");}public void printGoods(){System.out.println("商品");}public void printPoints(){System.out.println("积分");}
 }
 

解决方法： 按照 单一职责 原则，使用 Extract Class 把代码划分开。

Class Order{public void printOrder(){System.out.println("订单");}
 }Class Goods{public void printGoods(){System.out.println("商品");}
 }Class Points{   public void printPoints(){System.out.println("积分");}}
 }
 

Long Parameter List (过长参数列)

方法参数数量过多的话，可读性很差。如果有多个重载方法，参数很多的话，有时候你都不知道调哪个呢。并且，如果参数很多，做新老接口兼容处理也比较麻烦。

public void getUserInfo（String name,String age,String sex,String mobile){// do something ...
 }
 

如何解决过长参数列问题呢？将参数封装成结构或者类，比如我们将参数封装成一个DTO类，如下：

public void getUserInfo（UserInfoParamDTO userInfoParamDTO){// do something ...
 }class UserInfoParamDTO{private String name;private String age; private String sex;private String mobile;
 }
 

更多代码坏味道可阅读 《重构》这本书。

降低圈复杂度

什么是圈复杂度？简单来说就是代码中 if/case/for/while 出现的次数。圈复杂度越高，BUG 率越高。如果一个方法的圈复杂度达到 5 或者更高，那么 CR 时就要多看两眼。太复杂通常意味着“无法被开发者快速理解”。这也可能意味着“开发人员在尝试调用或修改此代码时可能会引入错误”。

    public static void testCase(Integer num){switch (num){case 1:System.out.println(num);break;case 2:System.out.println(num);case 3:System.out.println(num);}}
 

    public Integer testCase(String numStr) {Integer num = null;if (StringUtils.isEmpty(numStr)) {return 0;} else {num = Integer.valueOf(numStr);}return num;}
 

优化后

    public Integer testCase(String numStr) {if (StringUtils.isEmpty(numStr)) {return 0;}return Integer.valueOf(numStr);}

public void test() {if(条件1成立){if(条件2成立){执行xxx逻辑;}}
 }
 

public void test() {if(!条件1成立){return;}if(!条件2成立){return;}执行xxx逻辑;
 }
 

关注性能问题

性能问题虽然不常见，可一旦暴雷往往就是大问题。下面是一些 CR 时通常需要关注的点。

不要在循环中操作数据库或者调用远程服务。
如果可以的话，使用数组实现的集合类型时预估大小。
不要在循环中使用 try…catch…，应该把其放在最外层。
变量不要重复计算。
循环内不要不断创建对象引用。
异常只能用于错误处理，不应该用来控制程序流程。
字符串拼接 尽量使用 StringBuilder/StringJoiner。
…
下面举两个例子：

变量的重复计算。

for (int i = 0; i < list.size(); i++) {...}优化后for (int i = 0, length = list.size(); i < length; i++) {...}
 

循环内不要不断创建对象引用。

for (int i = 1; i <= count; i++){Object obj = new Object();
 }优化后Object obj = null;
 for (int i = 0; i <= count; i++) {obj = new Object();
 }
 

关注分布式事务

涉及远程服务调用，或者跨库更新的场景，都应考虑是否存在分布式事务问题，以及适用何种处理方式，是依赖框架保证强一致性，还是记录异常数据保证最终一致性，抑或是直接忽略？

关注架构设计。

代码有代码规范，架构有架构规范。面对一个新功能的 CL，除了检查架构规范，还应推敲其架构设计，比如是否符合整洁架构三原则，无依赖环原则，稳定依赖原则，稳定抽象原则。

如果阅读代码有些困难，减 CR 的速度，应该找到对应的作者，让其解释沟通清楚，然后再尝试审查它。如果你看不懂代码，其他开发人员很可能也不会。因此，当你要求开发人员对其进行解释时，你也在帮助未来的开发人员理解此代码。

语境

通常，代码审查工具只会显示正在更改的部分周围的几行代码。有时必须查看整个文件以确保更改确实有意义。例如，你可能会看到只添加了四个新行，但是当你查看整个文件时，你会发现这四行位于一个 50 行的方法中，需要将其分解为更小的方法。

不要吝啬你的赞赏和鼓励

如果你在 CL 中看到了不错的内容，请告诉开发人员，尤其是当他们以出色的方式处理你的 Comments时。Reviewers 通常只关注错误，但他们也应该为良好实践提供鼓励和赞赏。有时，在指导方面，告诉开发人员他们做对了什么比告诉他们做错了什么更有价值。

如何编写 CR 评论(Comments)

上面我们介绍了 CR 时需要的关注点，那么如果发现有问题，需要提 Comments 时，我们该注意些什么呢？

礼貌

一般来说，礼貌和尊重是很重要的，彬彬有礼的人总是受人喜欢。做到这一点的一种方法是确保你总是对代码发表评论，而不是对开发人员发表评论。

坏： “在对并发性显然没有好处的情况下，为什么要使用线程”。
好： “这里的并发模型增加了系统的复杂性，而我看不到任何实际的性能优势。因为没有性能优势，所以最好让这段代码是单线程的，而不是使用多线程。”

解释为什么

从上面的例子中可以看出，Reviewers 应该尽量让开发者明白为何会有这一次 Comments ，Reviewers 并不总是需要在 Comments 中包含这些信息，但有时对 Comments 的意图、遵循的最佳实践或建议进行更多解释是很有必要的。

给予指导

一般来说，解决 Comments 是开发人员的责任，而不是 Reviewers 的责任。你不需要为开发人员进行解决方案的详细设计或编写代码。

不过，这并不意味着 Reviewers 不需要进行帮助。一般来说，Reviewers 应该在指出问题和提供直接指导之间取得适当的平衡。指出问题并让开发人员做出决定通常有助于开发人员学习，并使代码审查变得更容易。它还可以产生更好的解决方案，因为开发人员比审阅者更接近代码。

如何处理 CR 评论（Comments）

有时开发人员会推迟代码审查。他们要么不同意 Reviewers 的建议，要么抱怨 Reviewers 总体上过于严格。

谁是对的？

当开发人员不同意你的建议时，请先花点时间考虑一下它们是否正确。通常，开发人员比你更接近代码，因此他们可能对代码的某些方面有更好的了解。如果是这样，让他们知道他们是对的，让问题消失。

但是，开发人员并不总是正确的。在这种情况下，你应该耐心礼貌的对开发人员进行解释。一个好的解释表明了对开发人员回复的理解。

特别是，当你认为解决 Comments 带来的代码质量改进证明所要求的额外工作是合理的，他们应该继续坚持。 改善代码健康往往是在小步骤中发生的。

惹恼开发者

Reviewers 有时认为，如果自己坚持要求改进，开发人员会感到不安。有时开发人员确实会感到沮丧，但这通常是短暂的，他们稍后会非常感谢你帮助他们提高代码质量。通常，如果你的评论有礼貌，开发人员实际上根本不会生气。不安通常是因为 Comments 的编写方式，而不是 Reviewers 对代码质量的坚持。

稍后清理

一个常见的回击来源是开发人员想要完成任务。他们不想为了解决这个 Comments 而进行额外工作。所以他们说他们会在以后的 CL 中清理一些东西。

然而，经验表明，随着开发人员编写原始 CL 后的时间越长，这种清理工作发生的可能性就越小。事实上，通常除非开发人员立即进行解决现在的 Comments ，否则它永远不会发生。这并不是因为开发人员不负责任，而是因为他们有很多工作要做，而清理工作会在其他工作的压力下丢失或被遗忘。因此，通常最好坚持让开发人员现在解决他们的 Comments。让开发者“稍后清理”是代码库退化的常见方式。

如果 Comments 引入了新的复杂性，除非是紧急情况，否则必须在提交之前对其进行清理。如果 Comments 现在无法解决，开发人员应该提交一个 bug 并将其分配给自己，这样它就不会丢失。

关于严格性 CR 的一般投诉

如果你之前的 CR 相当松懈，而你转而进行严格的审查，那么一些开发人员会非常大声地抱怨。提高 CR 的速度通常会导致这些抱怨消失。

有时这些抱怨可能需要几个月的时间才能消失，但最终开发人员往往会看到严格 CR 的价值，因为他们看到了他们帮助编写了哪些出色的代码。有时，最响亮的抗议者甚至会成为你最坚定的支持者，让他们真正看到你通过严格 CR 增加的价值。

代码审查的速度

上面在 关于严格性 CR 的一般投诉 一节中我们提到，提高 CR 的速度有助于减少 开发人员的抱怨，但是对于 Reviewers 来说，本身可能也是一个开发者，手里有很多工作在做。这个 CR 的速度如何保证呢？

为什么代码审查要快？

首先，我们要明确的是：我们的目标是优化开发人员团队共同生产产品的速度，而不仅仅是优化单个开发人员编写代码的速度。个人发展的速度固然是是非常重要的，它只是不作为整个团队的速度很重要。
有时 Reviewers 为了速度直接通过开发者的 CL，看似提高了效率，但是未经严格审查的代码可能会隐藏巨大的不确定性，影响以后的阅读和扩展性，得不偿失。

当代码审查缓慢时，会发生几件事：

代码审查应该多快？

收到 CL 之后，先判断一下 CL 的性质，如果是 Bug Fix 类型的 CL，应尽快评审，如果是新功能 CL，则可以等待下一个 CR 窗口。

一个工作日是响应 CR 请求（即第二天早上的第一件事）应该花费的最长时间。

遵循这些准则意味着典型的 CL 应该在一天内获得多轮审查。

速度与中断

如果你正忙于一项重点任务，例如编写代码，请不要打扰自己进行 CR 。 研究表明，开发人员在被中断后可能需要很长时间才能恢复平稳的开发流程。因此，在编码时打断自己对团队来说实际​​上 比让另一个开发人员等待 CR 更昂贵。

相反，在你回复 CR 请求之前，请等待你的工作出现断点。这可能是你当前的编码任务完成时、午餐后、从会议回来、从休息室回来等。

Git Commit Message 应该怎样写

Git Commit Message 在 CR 时的作用也很重要，一个好的 Git Commit Message 应该能够清晰点明主题，是 bug 还是 feature 还是 doc 的完善，应该让 Reviewers 一目了然。

我们在日常使用Git提交代码时经常会写 commint message，否则就不允许提交。一般来说，commit message 应该清晰明了，说明本次提交的目的。目前，社区有多种 Commit message 的写法规范。推荐使用 Angular 规范，这是目前使用最广的写法，比较合理和系统化，并且有配套的工具。

可以参考IDEA 中 Git Commit message 编写 来学习 IDEA 中如何方便的编写 Commit Message

总结

如果你遵循这些准则并且严格执行 CR，你应该会发现整个 CR 过程会随着时间的推移变得越来越快。开发人员了解健康代码的编写手段，并从一开始就向你发送出色的 CL，从而需要越来越少的 CR 时间。Reviewers 学会快速响应，而不是在 CR 过程中增加不必要的延迟。长此以往，你的团队会成为一个人人都喜欢 CR 的团队。在那里，每个人都能写出质量非常好的代码。

---

参考资料：

---

---

1. CL：代表“ChangeList”，意思是已经提交给版本控制或正在进行代码审查的一个独立的更改。其他组织通常将此称为“更改”、“补丁”或“拉取请求”，如果你使用 GitLab， CL 就是你的 Merge Request 简称 MR，如果你使用 GitHub ，CL就是你的 Push Request 简称 PR。 ↩︎

2. 风格指南（Style Guide）：每个主要的开源项目都有自己的风格指南：一组关于如何为该项目编写代码的约定（有时是任意的）。当其中的所有代码都采用一致的样式时，理解大型代码库会容易得多。一个公司也应该有自己的风格指南，国内一般都爱参考 《阿里巴巴编码规约》。最好结合一些静态代码检查工具如 Checkstyle，PMD 等制定出符合本公司本团队的风格指南。重点是自己的团队要都认同并遵守。 ↩︎