Code Review到底在关注些什么？

代码语言：javascript复制

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

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

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

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

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

基础篇

基本编码规范

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

日志规范

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

代码语言：javascript复制

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

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

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

代码语言：javascript复制

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

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

经验性检查

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

代码语言：javascript复制

    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对象创建，如果没有使用好，也可能出现问题。比如下面的示例：

代码语言：javascript复制

    //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能够满足。 

代码语言：javascript复制

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 都不满足，导致更新的范围扩大。

代码语言：javascript复制

  <if test="xxx !=null ">

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

安全渗透方面检查

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

接口保护检查

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

模式应用

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

代码语言：javascript复制

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

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

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

抽象类

代码语言：javascript复制

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);
}

服务请求参数

代码语言：javascript复制

import java.io.Serializable;
public class ServiceRequest implements Serializable {
  private static final long serialVersionUID = 4314529075012784996L;
  // 属性省略
}

决策流服务处理类

代码语言：javascript复制

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
  }
}

决策工具服务处理类

代码语言：javascript复制

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（本地缓存）结合使用，本地缓存更新的策略是什么？（集群下需要通过消息广播来达到快速更新各机器本地缓存的目的）
• 缓存存放的值是否为大对象，缓存个数多大？失败策略是什么？缓存雪崩/并发等场景是否有考虑等等
• ... ...

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

小结

本文主要从关注代码本身，对Code Review做了简易的说明，想到啥就写了点啥。

Code Review对代码的关注点，远不止这些，本文也算是一个简单的抛砖引玉，有兴趣的读者可以一起留言探讨。

缓存 云数据库Redis 编程算法 短信

0 人点赞