Code Review到底在关注些什么？

孟君

发布于 2022-11-21 20:29:43

2950

发布于 2022-11-21 20:29:43

Code Review是软件开发过程中非常重要的一个环节，
其主要的目的是在项目早期找到和修正错误、提升软件质量。

本文主要关注的是在做Code Review的时候，我们主要在关心代码的哪些方面来进行说明。

每个人的关注点不尽相同，于我而言，我的关注点一般在下面的几个部分上：

基础篇 - 包括编码规范、风格、日志规范、内存泄漏等
进阶篇 - 包括是否有较好的抽象、数据库变更检查等
高阶篇 - 包括应急方案、失败性考虑等

接下来，我们逐一来看一下。

基础篇

基本编码规范

类、方法命名是否规范、清晰
方法参数是否过多（比如6个以上）、方法体是否过大
重复代码
是否有“魔数”，比如 == 1 处理什么逻辑？(建议有良好的注释说明，定义常量或枚举代替)
... ...

日志规范

日志打印是否有意义，比如像如下几种，这种日志对问题排查等没有啥意义

logger.info("方法xxx执行");
... ...


logger.error("方法xxx出错～");

日志打印可以合并的合起来打印

#合并前s
logger.info("name={}", name);
logger.info("type={}", type);
logger.info("timeCosted={} ms", timeCosted);

#合并后
logger.info("name={},type={},timeCosted={}", name,type,timeCosted);

日志是否有用MDC，如有需要检查是否合理的调用了remove / clear方法，防止线程间日志打印干扰等问题。
... ...

经验性检查

SimpleDateFormat是否被定义成一个全局变量，如下代码，多线程将出现问题。

    private static final SimpleDateFormat DEFAULT_FOMAT
     = new SimpleDateFormat("yyyy-MM-dd"); 

    public static Date formatDefaultDate(String date) {
        try {
            return DEFAULT_FOMAT.parse(date);
        } catch (ParseException e) {
            return null;
        }
    }

SimpleDateFormat更多的信息请参考《SimpleDateFormat线程不安全示例及其解决方法》

金额处理是否使用BigDecimal类型，不能使用float和doube。

另外，BigDecimal对象创建，如果没有使用好，也可能出现问题。比如下面的示例：

    //0.299999999999999988897769753748434595763683319091796875
    BigDecimal v1 = new BigDecimal(0.3d);
    System.out.println(v1);

    //0.3
    BigDecimal v2 = new BigDecimal("0.3");
    System.out.println(v2);

    //0.3
    BigDecimal v3 = BigDecimal.valueOf(0.3d);
    System.out.println(v3);

文件流使用后是否正常关闭。我们需要finally关闭流，以防止内存泄漏。
... ...

超时问题

新增的服务，如Dubbo服务，需要有超时设置
分布式锁需要有超时处理
调用下游接口或者调用各种中间件需要有超时的机制
... ...

代码职责和调用关系

代码职责是否清晰，比如UserService写了很多权限资源相关的逻辑处理方法就不好。
层次调用是否正确，一般Controller -> Service -> DAO 。如果Controller -> DAO就不好
Dubbo服务不要调用调用自己的服务。这个怎么理解呢？比如有一个UserRpcService，其有update和query的接口，假设，update里面需要做用户查询的操作做判断，刚好query能够满足。

public class UserRpcServiceImpl implements UserRpcService {

  public int update(xxx) {

      //调用query，而query也是一个rpc接口 
      query(xxx);
  }

  public int query(xxx) {


  }

}

不建议在rpc接口，再调用自身的rpc接口，如本示例的update和query。

... ...

线程池创建线程

对一些多线程逻辑，使用线程池创建线程，而不是通过new Thread等方式创建。
... ...

进阶篇

数据库方面检查

对表增加字段Alter table xxx add column xxxx ... ，需要检查下当前表中的记录数，如果数据量很大这个是不能做的。需要同DBA进行沟通。或者自己通过建一个新表（比原表多一个字段），用操作新表替换旧表来完成。这里需要完成原表数据迁移等其他内容。
索引添加是否合适
是否存在危险SQL，如update / delete 语句中的变量是否在业务能够保证有必要的值，不能出现很多值没有，导致if test 都不满足，导致更新的范围扩大。

  <if test="xxx !=null ">

查看是否有循环单个插入记录的情况，改成批量插入。
... ...

安全渗透方面检查

文件上传是不是只判断了文件后缀？只判断后缀，攻击方可以将一个jsp等文件伪装成jpg等格式的文件，从而成功上传到服务，导致服务器信息泄漏。
短信验证码对一个手机号，是否调用接口就能给用户发一个新的短信验证码，从而造成短信轰炸？我们需要在短信产生的有效期内保证不重新产生短信验证码。
接口是否存在越权查看等风险？比如A可以通过id查看属于B的设备信息？
... ...

接口保护检查

列表查询是否有pageSize的限制（如最多一次查询100条）。如果不限制，那么假设pageSize可以为5000条，那真的是简直了，对吧？
如果接口调用需要有次数限制，我们还要考虑是否对方法等有限流的措施？
... ...

模式应用

如果使用了D.C.L（Double-Check Lock），那么看是否有volatile修饰
抽象是否充分？比如，有不同类型的服务接口调用，主要有如下几个步骤：

1、参数校验
2、检查是否有流量
3、执行业务逻辑
4、记录调用日志
5、流量扣减
6、... ...

如果每个服务都自己写一遍，不是很合适，也不不容易维护和扩展。在这种情况下，不同的服务调用主要是步骤#1和步骤#3有个性化的处理，可以抽象出来。

比如写一个简单的模版，步骤#1和步骤#3使用abstract方法，由子类具体实现。

抽象类


public abstract class AbstractServiceHandler<T extends ServiceRequest> {

  /**
   * 模版方法
   */
  public void handle(T param) {
    // 1、验证
    this.doValidate(param);

    // 2、校验是否有流量

    // 3、执行业务逻辑
    doBusiness(param);

    // 4、记录调用日志
    // 5、流量扣减
    // 6、 ... ...
  }

  // 子类做校验
  protected abstract void doValidate(T param);

  // 子类做业务逻辑
  protected abstract void doBusiness(T param);

}

服务请求参数

import java.io.Serializable;

public class ServiceRequest implements Serializable {

  private static final long serialVersionUID = 4314529075012784996L;

  // 属性省略

}

决策流服务处理类

public class DecisionFlowHandler extends AbstractServiceHandler<ServiceRequest> {

  @Override
  protected void doValidate(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

  @Override
  protected void doBusiness(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

}

决策工具服务处理类

public class DecisionToolHandler extends AbstractServiceHandler<ServiceRequest> {

  @Override
  protected void doValidate(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

  @Override
  protected void doBusiness(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

}

可以考虑一些设计原则，比如单一职责/接口隔离等。

可以参考《设计模式几大原则》

... ...

高阶篇

在这个篇章部分，我们要对一些“失败”的场景，方案&应急有一定的考虑。

重试和幂等

针对系统间的数据同步，如果失败？我们是否有重试机制？
针对计费等场景，失败后重试调用，我们的接口是否支持幂等？
文件导入任务，如中断，我们是否有重启任务的机制，继续完成？
... ...

方案和应急

缓存对象增加属性（比如User加一个type），发布上线的时候，缓存的Key是否有做版本调整。如果升级后Key不变，可能导致Redis的value是由原服务更新，导致新改的内容上线后，可能还是取的原来的值（不包含type）。
比如在某些场景中，Redis缓存添加是不是有开关（一般由配置中心推送设置），以防止在缓存不是很正确的场景下，用数据库来保底
比如涉及数据迁移或者Redis集群升级（由5.0改成6.0）, 切流的计划是否合理？
有时候为了减少RPC调用或者少走网络，会结合Redis（分布式） + Guava（本地缓存）结合使用，本地缓存更新的策略是什么？（集群下需要通过消息广播来达到快速更新各机器本地缓存的目的）
缓存存放的值是否为大对象，缓存个数多大？失败策略是什么？缓存雪崩/并发等场景是否有考虑等等
... ...

更多缓存的一些知识，读者可以从之前的文章《啊哈！缓存》了解更多内容。