CodeReview 规范及实施

优质博文：IT-BLOG-CN

一、为什么需要CodeReview

随着业务压力增大，引发代码质量下降，代码质量的下降导致了开发效率的降低，维护成功高等问题，开发效率下降后又加重了业务压力，最终陷入了死亡三角的内耗之中。只要解决掉死亡三角内耗中的任一一角，就能终止恶性循环，比如精简业务需求、增加开发人员、重构项目架构等，很多时候可能是多管齐下的。这篇文章主要目的是：通过代码评审（Code Review，简称CR）整治代码不规范和代码安全性/健壮性低等问题，从而提高代码和产品的质量和确保代码库整体代码健康状况随着时间的推移而完善，最终降低系统的维护成本和提高开发效率。

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

代码评审（Code Review）可以带来以下好处： 不要吝啬你的赞赏和鼓励，如果你在CD中看到了不错的内容，也可以添加comments表示鼓励，并写清楚好的点并由开发者进行补充，或者直接告诉开发人员，尤其是当他们以出色的方式处理你的Comments时。Reviewers通常只关注错误，但他们也应该为良好实践提供鼓励和赞赏。有时，在指导方面，告诉开发人员他们做对了什么比告诉他们做错了什么更有价值。我们每周五都会组织会议对项目进行一次深度的Review，让大家工作对出现的问题和优秀的代码进行点评和学习，这里学习就达到了共同学习的目的。

【1】提高代码质量： 通过多个开发人员对代码进行审查，可以发现潜在的错误、缺陷和不规范的代码，从而改善代码质量。
【2】提升团队合作： 代码评审是团队成员之间的合作过程，可以促进团队成员之间的沟通和交流，增强团队合作意识。
【3】学习和知识分享： 代码评审是一个学习的机会，通过审查他人的代码，可以学习到新的编码技巧和最佳实践，同时也可以分享自己的知识和经验。
【4】减少维护成本： 通过及时发现和修复问题，可以减少后续维护和调试的成本，提高代码的可维护性。
【5】提高系统安全性： 代码评审可以帮助发现潜在的安全漏洞和风险，从而提高系统的安全性。
【6】增强代码一致性： 通过代码评审，可以确保代码符合团队或组织的编码规范和标准，提高代码的一致性和可读性。

总而言之，代码评审是一个重要的质量控制和团队协作的过程，可以提高代码质量、减少错误、促进团队合作，并提升整体开发效率。

二、CodeReview 标准

为什么需要指定规范： 当你尝试在团队中实施有效的代码审查流程时，会遇到许多挑战。例如：代码审查的comments是否被认同，是否是错的等，做不好甚至会损害你的人际关系。因此需要一套标准，Submitter和Reviewers都需要一个指南针来进行建设性和尊重的代码审查。

CodeReview规范推荐： 阿里巴巴开发手册【泰山版】
书籍推荐：《代码整洁之道》《重构》《代码审查之道》《代码审查实战》《代码审查艺术》

【1】代码可读性： 变量、函数和类的命名清晰、有意义：一个好的名称可以充分传达该项目是什么或做什么，而不会太长以至于难以阅读。

// Good example
String customerName = "John Doe";

// Bad example
String x = "John Doe";

代码缩进和格式一致：

// Good example
if (condition) {
    statement1;
    statement2;
}

// Bad example
if (condition) {
statement1;
statement2;
}

注释清晰明了：

// Good example
// Calculate the sum of two numbers
public int addNumbers(int a, int b) {
    return a + b;
}

// Bad example
public int addNumbers(int a, int b) {
    return a + b; // Add the numbers
}

【2】代码结构和组织： 模块化和功能分离：避免过大的类，一个类做太多事情，维护了太多功能，可读性变差，性能也会下降。Duplicated Code也就接踵而至了。

// Good example
// UserService.java
public class UserService {
    public void createUser() {
        // Create a new user
    }
}

// OrderService.java
public class OrderService {
    public void createOrder() {
        // Create a new order
    }
}

// Bad example
public class UserServiceAndOrderService {
    public void createUserAndOrder() {
        // Create a new user and order
    }
}

避免冗余代码和重复逻辑： 一般是因为需求迭代比较快，开发小伙伴担心影响已有功能，就复制粘贴造成的。重复代码很难维护的，如果你要修改其中一段的代码逻辑，就需要修改多次，很可能出现遗漏的情况。优化手段： 可以使用​​Extract Method提取公共函数，抽出重复的代码逻辑，组成一个公用的方法。

// Good example
public double calculateAverage(List<Double> numbers) {
    double total = numbers.stream().mapToDouble(Double::doubleValue).sum();
    return total / numbers.size();
}

// Bad example
public double calculateAverage(List<Double> numbers) {
    double total = 0;
    for (Double number : numbers) {
        total += number;
    }
    return total / numbers.size();
}

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

// Good example
public void getUserInfo（UserInfoParamDTO userInfoParamDTO){
  // do something ...
}

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

【3】错误处理和异常处理： 检查和处理可能的异常情况：

// Good example
public double divide(double a, double b) {
    if (b != 0) {
        return a / b;
    } else {
        throw new IllegalArgumentException("Cannot divide by zero");
    }
}

// Bad example
public double divide(double a, double b) {
    return a / b; // No handling for zero division
}

使用适当的错误处理机制：

// Good example
try {
    // Code that may throw an exception
} catch (IOException e) {
    // Handle the specific exception
}

// Bad example
try {
    // Code that may throw an exception
} catch (Exception e) {
    // Handle all exceptions generically
}

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

// Good example
int sum = 0;
for (int i = 0; i < array.length; i++) {
    sum += array[i];
}

// Bad example
int sum = 0;
for (int i = 0; i < array.length; i++) {
    for (int j = 0; j < array.length; j++) {
        sum += array[j];
    }
}

使用适当的数据结构和算法：

// Good example
List<Integer> numbers = new ArrayList<>();
numbers.add(1);
numbers.add(2);
numbers.add(3);
boolean contains = numbers.contains(2);

// Bad example
int[] numbers = {1, 2, 3};
boolean contains = false;
for (int i = 0; i < numbers.length; i++) {
    if (numbers[i] == 2) {
        contains = true;
        break;
    }
}

【5】安全性： 避免使用不安全的函数和方法：

// Good example
String sanitizedInput = input.replaceAll("<script>", "");

// Bad example
String sanitizedInput = input.replace("<script>", "");

防止代码注入和安全漏洞：

// Good example
PreparedStatement statement = connection.prepareStatement("SELECT * FROM users WHERE username = ?");
statement.setString(1, username);
ResultSet resultSet = statement.executeQuery();

// Bad example
String query = "SELECT * FROM users WHERE username = '" + username + "'";
Statement statement = connection.createStatement();
ResultSet resultSet = statement.executeQuery(query);

【6】可测试性： 编写可测试的代码单元：

// Good example
public int add(int a, int b) {
    return a + b;
}

// Bad example
public void doSomething() {
    // Perform some complex logic
}

使用适当的测试框架和工具进行单元测试：

// Good example (using JUnit)
@Test
public void testAdd() {
    Calculator calculator = new Calculator();
    int result = calculator.add(2, 3);
    assertEquals(5, result);
}

// Bad example
public void testAdd() {
    Calculator calculator = new Calculator();
    int result = calculator.add(2, 3);
    if (result != 5) {
        throw new AssertionError("Addition test failed");
    }
}

【7】可维护性： 遵循设计原则和最佳实践：

// Good example (using SOLID principles)
public interface Shape {
    double calculateArea();
}

public class Circle implements Shape {
    private double radius;

    public Circle(double radius) {
        this.radius = radius;
    }

    public double calculateArea() {
        return Math.PI * radius * radius;
    }
}

// Bad example
public class Circle {
    public double calculateArea(double radius) {
        return Math.PI * radius * radius;
    }
}

避免过于复杂和冗长的代码：

// Good example
public void processOrder(Order order) {
    validateOrder(order);
    calculateTotalPrice(order);
    updateInventory(order);
    sendConfirmationEmail(order);
}

// Bad example
public void processOrder(Order order) {
    // A long and complex method with multiple nested if-else statements
}

【8】可扩展性： 使用适当的设计模式和架构：设计模式全集

// Good example (using the Factory pattern)
public interface Animal {
    void makeSound();
}

public class Dog implements Animal {
    public void makeSound() {
        System.out.println("Woof!");
    }
}

public class Cat implements Animal {
    public void makeSound() {
        System.out.println("Meow!");
    }
}

// Bad example
public class Animal {
    public void makeSound(String type) {
        if (type.equals("dog")) {
            System.out.println("Woof!");
        } else if (type.equals("cat")) {
            System.out.println("Meow!");
        }
    }
}

【9】关注性能问题： 性能问题虽然不常见，可一旦暴雷往往就是大问题。下面是一些CR时通常需要关注的点。
  ■ 不要在循环中操作数据库或者调用远程服务。
  ■ 如果可以的话，使用数组实现的集合类型时预估大小。
  ■ 不要在循环中使用try…catch…，应该把其放在最外层。
  ■ 变量不要重复计算。

// Good example
for (int i = 0, length = list.size(); i < length; i++) {...} 

// Bad example
for (int i = 0; i < list.size(); i++) {...}

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

// Good example
Object obj = null;
for (int i = 0; i <= count; i++) {
    obj = new Object();
} 

// Bad example
for (int i = 1; i <= count; i++) {
    Object obj = new Object();
}

  ■ 异常只能用于错误处理，不应该用来控制程序流程。
  ■ 字符串拼接 尽量使用StringBuilder/StringJoiner。
【10】关注分布式事务： 涉及远程服务调用，或者跨库更新的场景，都应考虑是否存在分布式事务问题，以及适用何种处理方式，是依赖框架保证强一致性，还是记录异常数据保证最终一致性，抑或是直接忽略？
【11】关注架构设计： 代码有代码规范，架构有架构规范。面对一个新功能的CD，除了检查架构规范，还应推敲其架构设计，比如是否符合整洁架构三原则，无依赖环原则，稳定依赖原则，稳定抽象原则。如果阅读代码有些困难，减CR的速度，应该找到对应的作者，让其解释沟通清楚，然后再尝试审查它。如果你看不懂代码，其他开发人员很可能也不会。因此，当你要求开发人员对其进行解释时，你也在帮助未来的开发人员理解此代码。

常见CR缺陷：
可能死循环；
传递引用错误；
类型转换错误；
公式计算错误；
数组可能越界；
条件范围选择错误；
除数为0、整数溢出、精度损失；
字符串对比不能用==，使用equals；
在finally程序块中关闭或者释放资源；
锁的获取和释放是有序的，避免死锁情况的发生；
UT是否覆盖所有场景，测试案例是否存在遗漏场景；
异常未处理或提示不明确（没有catch异常，集合等没有判空和长度为0）；
日志埋点信息输出需有意义，需具有可读性，可供日常开发同学排查线上问题；
是否正确处理了共享数据的并发访问，确保对共享资源的操作是原子的，避免出现数据竞争问题；
对于连接池、线程池等共享资源，需要适当地设置和管理资源池的大小，避免资源耗尽或资源浪费的问题；
多个线程相互等待对方释放所持有的资源，导致程序无法继续执行。需要确保对锁的获取和释放是有序的，避免死锁情况的发生；
检查代码中对并发集合的使用，例如ConcurrentHashMap、ConcurrentLinkedQueue等。确保对并发集合的操作是线程安全的；
检查代码中对数据的读写操作，确定在多线程环境下是否会出现数据不一致的问题。需要使用适当的同步机制来保证数据的一致性；
避免在循环中频繁使用字符串拼接操作，尤其是在大型循环中。建议使用StringBuilder或StringBuffer来进行字符串拼接，以提高性能；
检查数据库查询和更新操作是否高效。避免频繁的数据库交互，可以考虑使用批处理操作、适当的索引以及缓存机制来提高性能；
检查代码中是否存在频繁的磁盘或网络I/O操作，这些操作通常是性能瓶颈之一。可以考虑合并操作、异步操作或使用缓冲机制来提高性能；

重点检查项：
结构性检查：程序的每个功能是否都作为一个可辨识的代码块存在；
可验证性检查：代码功能是否便于测试；单元测试覆盖度是否足够；
可预测性检查：变量初始化；方法稳定性；代码是否存在死循环；代码无穷递归检查；
高可用性检查：是否有预案（降级开发、限流配置、兜底策略）；补偿方案是否合理；
可追溯性检查：代码是否包括一个修订历史记录，记录中对代码的修改和原因都有记录；
完整性检查：服务层是否包含了所有业务功能；数据层是否包含所有需要的数据和操作；
正确性检查：计算逻辑等业务逻辑是否正确；变量是否被正确定义和使用；代码是否符合制定的标准；
健壮性检查：代码是否采取措施避免运行时错误（如数组边界溢出、被零除、值越界、堆栈溢出等）；
一致性检查：是否需求相关；是否和方案设计一致；代码风格、日志规范、异常处理等是否和统一规范一致；
可理解性检查：是否使用到不明确或不必要的复杂代码；代码中的算法是否符合开发文档中描述的数学模型；每个变量都定义了合法的取值范围；
可修改性检查：代码涉及到常量是否易于修改，比如使用配置，定义为常量类等；建议业务方法只有一个出口和一个入口（异常处理除外）；重要公共方法是否有交叉注释说明；重要的对外方法修改后影响多个下游接口；

三、如何编写CodeReview

【1】态度&位置： CodeReview者需要保持一个虚心请教的态度发表评论，因为彬彬有礼的人才能够让着团队向更好的方向发展。
  ■ 优秀评语： “你的代码整体结构清晰，逻辑合理，很容易理解。你在变量和方法命名上也很注重描述性，这有助于他人快速理解你的意图。不过，在这段代码中，我注意到了一个潜在的性能问题。在第15行的循环中，你每次都在循环内部调用了一个耗时的方法。建议你将这个方法的结果缓存起来，以避免重复计算。这样可以提高代码的效率。除此之外，你的代码注释也可以更加详细一些，帮助其他人更好地理解你的思路。继续保持优秀的工作！”
  ■ 坏的评语： “你的代码结构乱七八糟，根本看不懂。变量和方法命名也太随意了吧！这段代码简直就是垃圾。你在第15行的循环中做了一个非常愚蠢的错误，每次都在循环内部调用一个耗时的方法，你是怎么想的？你的代码注释也太少了吧，根本看不出你在想什么。你需要好好反思一下你的编码能力。”
【2】解释清楚为什么： 因为每个人的阅历和所想的不同，所以Reviewers应该尽量让开发者明白为何会有这一次Comments，Reviewers并不总是需要在Comments中包含这些信息，但有时对Comments的意图、遵循的最佳实践或建议进行更多解释是很有必要的。
【3】提出改进方案和该方案的优点： Reviewers应该在指出问题和提供直接指导之间取得适当的平衡。指出问题并让开发人员做出决定通常有助于开发人员学习，并使代码审查变得更容易。它还可以产生更好的解决方案，因为开发人员比审阅者更接近代码。

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

上面的问题，最主要的是先弄清楚谁是是对的，因为开发者比你更了解整个系统的业务逻辑和架构，你的意见可能不适合该系统或架构，因此需要友好的交流，让他们知道他们的代码是对的，继续保持，自己则更进一步的了解整个系统和架构，避免第二次犯同样的错误。如果交流后发现你是对的，问题在开发者，就需要搞明白为什么开发者不愿意修改。是因为修改Comment带来的代码质量改进需要花费额外的工作，还是因为项目需要积极发布，修改Comments影响过大，时间上不允许等等。都需要根据自己的团队给出合理的解决方案。

举个例子：稍后清理 开发者不想为了解决这个Comments而进行额外工作，而且需要今天紧急发布，Comment回复在后期的工作中进行清理工作。但是经验表明，清理工作会在其他工作的压力下丢失或被遗忘，这并不是因为开发人员不负责任。我们的解决办法是：如果Comments引入了新的复杂性，除非是紧急情况，否则必须在提交之前对其进行清理。如果Comments现在无法解决，开发人员应该提交一个bug并将其分配给自己，这样它就不会丢失。

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

建议：
【1】CR就像读书，先看目录（改动的文件列表），再精读重点章节（包含核心业务逻辑的代码），最后扫读剩余章节。
【2】如果改动的文件数量较多，可以打开IDE，切换到源分支，方便在CR过程中随时打开相关代码进行阅读。
【3】如果MR提交者对评审意见提出异议，评审者应找提交者当面讨论，避免在评论区互踢皮球。
【4】合并代码之前应确保所有评审意见都被妥善处理。

四、什么时候进行CodeReview

问题一：我们CodeReview工作者基本都是一个开发者，手里有很多工作在做，而且大部分需求都集中在发布日当天进行CodeReview，因此CR的时间和质量就成了一个新的挑战。
问题二：CodeReview慢时，会导致开发者新功能的错误修复延迟，从而增加了返工重构的工作量，降低开发效率。

规范：一个工作日是响应CR请求。越接近软件发布的最终期限，代码也就不能改得太多，因为此时没有测试再帮忙回归了。

五、Commit Message

GitLab Commit Message在CR时的作用也很重要，一个好的Git Commit Message应该能够清晰点明主题，是bug还是feature还是doc的完善，应该让Reviewers一目了然。我们在日常使用GitLab提交代码时经常会写commint message，否则就不允许提交。我们常用的模板如下：

feat(版本号):需求名 - 核心内容
feat: 更改的内容详细说明