前言

最近一段时间进行了部分代码的code review。其中有 review 的，也有被review的。在这过程中发现了许多问题，而其中就包含许多本不该发生的。同样的，这些不该发生的问题如果携带上线，随之而来的则是一个个的生产事故。对于金融系统来说，会直接造成资产损失，而对于医疗软件而言，甚至涉及生命安全。经过分析分类后，以下几点是最为常见的问题：

代码顺序倒置

代码顺序，这一点太容易忽略了，以至于有时让我们觉得这根本不是问题。话不多说，我们先来看例子：

反例：

public void createUser(String telephone, String name, Integer age){
    //1. 校验手机号
    if (StringUitls.isBlank(telephone)) {
        ...
    }
    //2. 查询用户
    User uesr = queryUser(telephone);

    //3. 校验name ...
    if (StringUitls.isBlank(name)) {
        ...
    }
    //4. 校验年龄
    if (null == age) {
        ...
    }

    //5. 保存用户
    saveUser(telephone, name, age);
}

上述代码中存在若干问题，有以下几点：

  1. 代码顺序混乱，其中 查询用户 不应该在第二步，为什么呢？因为如果 name 为空 或 age 为空时，则会白白浪费一次数据库查询，而这次查询，原本是不该发生的。
  2. 校验逻辑和核心逻辑混在一起，造成红花与绿叶不分。

正例：

public void createUser(String telephone, String name, Integer age) {
    //检查参数
    checkCreateUserParams(telephone, name, age)
    // 保存用户
    saveUser(telephone, name, age);
}

//校验参数
private void checkCreateUserParams(String telephone, String name, Integer age) {
    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

//保存用户
private void saveUser(String telephone, String name, Integer age) {
    User uesr = queryUser(telephone);
    if (user == null) {
       saveUser(telephone, name, age);
    }
}

建议：实际上，工作中编写的逻辑会比上述复杂很多，但道理是相通的。代码合理有效顺序，也会带来诸多好处，例如：减少IO操作(调用DAO方法)，较少数据传输(调用RPC方法)。而合适的顺序应该遵循：参数校验 优先于 核心流程处理。

无意的 NullPointerException

我们都知道 NPE (NullPointerException) 的判断是调用者的职责。而在方法实现中，有很多同学稍微不注意，就会导致一些低级的 NPE (NullPointerException) 产生。例如：

反例：

public void processUserInfo(String telephone) {
    User uesr = queryUser(telephone);
    if (user != null) {
       ...
    }
    Long uerId = user.getOid;
}

问题：

此时当 telephone 查询的用户不存在时，则会产生 NullPointerException，你还别笑，这类问题发生的频率还不低，非常值得我们注意。

正例：

public void processUserInfo(String telephone) {
    User uesr = queryUser(telephone);
    if (user == null) {
       return;
    }
    ...
}

职责模糊

在学习设计模式时，第一条设计原则就是：单一职责。这给我们的启示是：在定义方法时，应该尽量原子，让其承担单一职责。但在实践过程中，则出现许多比较怪的实现，以下面代码为例：

反例：

//1. 调用创建用户
public void invokerCreateUser(String telephone, String name, Integer age) {

    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
    createUser()

}

//2. 创建用户原子方法
public void createUser(String telephone, String name, Integer age) {
    User uesr = queryUser(telephone);
    if (user == null) {
       saveUser(telephone, name, age);
    }
}
  • invokerCreateUser 为创建用户调用方。
  • createUser 为创建用户提供方。

问题：

  1. invokerCreateUser 作为 调用方承担了 本该 createUser 提供方承担的职责。

建议：

无论在什么场景下，方法的提供方都需要负责进行参数的校验，而不是将这些职责委托到调用方。否则诸多调用方都需要进行参数验证，代码坏味道随即而来！同样的道理，思维发散开来。我们在定义方法时：属于该方法的职责，我们一个也不能少的承担，不属于该方法的职责，一个也不需要承担。

方法过长

抽取方法这点非常重要，好处包括但不限于以下几点：

  1. 方法越小，职责越单一，其复用性越高，从而减少重复代码的产生。
  2. 代码逻辑清晰，可以使主干代码更清晰，易于维护。

…

有很多同学，习惯在一个方法中编写成百上千行的代码彰显自己的能力。但事实上这样的代码及其不易维护，且阅读都非常吃劲。那多少行才是最合适的呢？这一点，在阿里巴巴的《Java手册》中提到：

推荐：单个方法的总行数不超过 80 行。(除注释之外的方法签名、 左右大括号、方法内代码、空行、回车及任何不可见字符的总行数不超过 80 行。)

同样也给出了这样做的原因：代码逻辑分清红花和绿叶，个性和共性，绿叶逻辑单独出来成为额外方法，使主干代码更加清晰；共性逻辑抽取成为共性方法，便于复用和维护。

以下述方法为例：

反例：

public void createUser(String telephone, String name, Integer age){
    //
    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
    //用户不存在则保存
    User uesr = queryUser(telephone);
    if (user == null) {
       saveUser(telephone, name, age);
    }
}

正例：

public void createUser(String telephone, String name, Integer age) {
    //检查参数
    checkCreateUserParams(telephone, name, age)
    // 保存用户
    saveUser(telephone, name, age);
}

//校验参数
private void checkCreateUserParams(String telephone, String name, Integer age) {

    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

//保存用户
private void saveUser(String telephone, String name, Integer age) {

    User uesr = queryUser(telephone);
    if (user == null) {
       saveUser(telephone, name, age);
    }
}

无用代码引用

去除无用代码，其实有很多方面都是需要注意的，其中包括但不限于以下几点：

  1. 去除未使用的 import。(window 可用快捷键：Ctrl+Alt + O 进行格式化)。
     

java Code Review AI工具_User

 

  1. 去除未使用的局部变量。

反例：

private void checkCreateUserParams(String telephone, String name, Integer age) {
    //1. 未使用的局部变量 sex
    Integer sex = "";
    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

正例：

private void checkCreateUserParams(String telephone, String name, Integer age) {

    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

3. 移除未使用的方法

 

反例：

private void checkCreateUserParams(String telephone, String name, Integer age) {

    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

// 未使用的私有方法
private void check(){
    return ;
}

正例:

private void checkCreateUserParams(String telephone, String name, Integer age) {

    if (StringUitls.isBlank(telephone)) {
        ...
    }
    if (StringUitls.isBlank(name)) {
        ...
    }
    if (null == age) {
        ...
    }
}

建议：

  1. 代码中未使用到的代码，及时删除，特别是私有方法。已经上线的RPC方法，则采取版本递进的方法进行移除。未上线且未使用的RPC方法及时删除，否则上线完后删除的风险就会非常大。

挂羊头卖狗肉

这一类问题，其实在接口定义上出现的比较多。其简单来说：就是表里不一，方法中返回对象，方法名，方法请求对象三者不统一。这样的接口提供给调用者，调用者会非常疑惑，甚至想过来怼你两句！以下述代码为例：

反例：

private User saveUserInfo(String telephone) {
    return queryUser(telephone);

}

问题： 方法名为 saveUser，其字面意思是：保存用户信息，但其内部实现则为查询用户信息，明显的挂羊头卖狗肉。

正例：

private User getUser(String telephone) {
    return queryUser(telephone);

}

建议：

  1. 定义接口时，接口返回对象，方法名，请求参数 三者语义要统一，避免上述问题产生。
  2. 方法名尽量能够表达一个行为，例如：get / remove / put 等，而不是用名词来代替。

小结

上述几种问题是最近 code review 过程中出现最多的几类问题以及解决办法。还有一些诸如 code style 类的问题，没有在本文中描述，不过还是建议大家一定要抽时间看看《Java手册》，每次看都有新的收获！还有就是一些提升代码质量的途径，诸如：

  1. 回看一周前，一月前自己写的代码，对不满意的代码进行重构。
  2. 参加开源项目，给开源项目提交 Pull Request，提交的代码会有多人进行code review，直至满足规范后，才会进行 merge。