Code Review 到底在关注些什么？

作者：孟君的编程札记

2022-12-28
浙江
本文字数：3406 字
阅读完需：约 11 分钟

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

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

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

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

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

基础篇

基本编码规范

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

日志规范

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

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

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

复制代码

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

#合并前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 是否被定义成一个全局变量，如下代码，多线程将出现问题。


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

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

小结

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

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

发布于: 刚刚阅读数: 4

孟君的编程札记

关注

脚踏实地，仰望星空 2019-09-13 加入

还未添加个人简介

发布

暂无评论

创作场景

Code Review 到底在关注些什么？

基础篇

进阶篇

高阶篇

小结

孟君的编程札记

评论