一文梳理Code Review方法论与实践总结

434次阅读  |  发布于1年以前

作为卓越工程文化的一部分,Code Review其实一直在进行中,只是各团队根据自身情况张驰有度,松紧可能也不一,这里简单梳理一下CR的方法和团队实践。

一、为什么要CR

二、他山之石

2.1 某大厂A

非常重视Code Review,基本上代码需要至少有两位以上Reviewer审核通过后,才会让你Check In。

2.1.1 代码评审准则

2.1.2 代码评审原则

2.1.3 代码审核者应该看什么

2.2 某大厂B

2.3 某大厂C

三、我们怎么做CR

3.1 作为代码提交者

3.2 作为代码评审者

3.2.1 评审范围

主要从两方面来评审:

3.2.2 评审注意事项

我们主要是通过交叉CR、集中CR相结合的方式,由应用Owner+SM+架构师+TL完成。

四、CR怎么避免流于形式

CR流于形式的因素很多,大概如下:

五、CR实践中发现的几个常见代码问题

笔者对个人CR评论问题做了个大概统计,Bug发现数占比约4%(直接或潜在Bug),重复代码数占比约5%,其他还有规范、安全、性能、设计等问题。在CR代码质量时,可以参考《重构:改善既有代码的设计》,书中所列的22种坏味道在CR中基本都会遇到。而此处我们主要聚焦以下几个常见问题:

5.1 DRY

DRY是Don't Repeat Yourself的缩写,DRY是Andy Hunt 和 Dave Thomas's 在《 The Pragmatic Programmer 》一书中提出的核心原则。DRY 原则描述的重复是知识和意图的重复,包含代码重复、文档重复、数据重复、表征重复,我们这里重点讲讲代码重复

5.1.1 代码重复

《重构》中对“Duplicated Code(重复代码)”的描述:坏味道行列中首当其冲的就是Duplicated Code。如果你在一个以上的地点看到相同的程序结构,那么可以肯定:设法将它们合而为一,程序会变得更好。 最单纯的Duplicated Code就是“同一个类的两个函数含有相同的表达式”。这时候你需要做的就是采用Extract Method (110)提炼出重复的代码,然后让这两个地点都调用被提炼出来的那一段代码。 另一种常见情况就是“两个互为兄弟的子类内含相同表达式”。要避免这种情况,只需对两个类都使用Extract Method (110),然后再对被提炼出来的代码使用Pull Up Method (332),将它推入超类内。如果代码之间只是类似,并非完全相同,那么就得运用Extract Method (110)将相似部分和差异部分割开,构成单独一个函数。然后你可能发现可以运用Form Template Method (345)获得一个Template Method设计模式。如果有些函数以不同的算法做相同的事,你可以选择其中较清晰的一个,并使用Substitute Algorithm (139)将其他函数的算法替换掉。 如果两个毫不相关的类出现Duplicated Code,你应该考虑对其中一个使用Extract Class (149),将重复代码提炼到一个独立类中,然后在另一个类内使用这个新类。但是,重复代码所在的函数也可能的确只应该属于某个类,另一个类只能调用它,抑或这个函数可能属于第三个类,而另两个类应该引用这第三个类。你必须决定这个函数放在哪儿最合适,并确保它被安置后就不会再在其他任何地方出现。

代码重复的几种场景:

CASE:

反例


private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    Money cost = billDTO.getCost();
    if (cost != null && cost.getAmount() != null) {
        billVO.setCostDisplayText(String.format("%s %s", cost.getCurrency(), cost.getAmount()));
    }
    Money sale = billDTO.getSale();
    if (sale != null && sale.getAmount() != null) {
        billVO.setSaleDisplayText(String.format("%s %s", sale.getCurrency(), sale.getAmount()));
    }
    Money grossProfit = billDTO.getGrossProfit();
    if (grossProfit != null && grossProfit.getAmount() != null) {
        billVO.setGrossProfitDisplayText(String.format("%s %s", grossProfit.getCurrency(), grossProfit.getAmount()));
    }
    return billVO;
}

正例

private static final String MONEY_DISPLAY_TEXT_PATTERN = "%s %s";

private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    billVO.setCostDisplayText(buildMoneyDisplayText(billDTO.getCost()));
    billVO.setSaleDisplayText(buildMoneyDisplayText(billDTO.getSale()));
    billVO.setGrossProfitDisplayText(buildMoneyDisplayText(billDTO.getGrossProfit()));
    return billVO;
}

private String buildMoneyDisplayText(Money money) {
    if (money == null || money.getAmount() == null) {
        return StringUtils.EMPTY;
    }
    return String.format(MONEY_DISPLAY_TEXT_PATTERN, money.getCurrency(), money.getAmount().toPlainString());
}

5.1.2 DYR实践忠告:- 不要借用DRY之名,过度提前抽象,请遵循 Rule of three 原则

5.2 Primitive Obsession

《重构》中对“Primitive Obsession(基本类型偏执)”的描述:大多数编程环境都有两种数据:结构类型允许你将数据组织成有意义的形式;基本类型则是构成结构类型的积木块。结构总是会带来一定的额外开销。它们可能代表着数据库中的表,如果只为做一两件事而创建结构类型也可能显得太麻烦。 对象的一个极大的价值在于:它们模糊(甚至打破)了横亘于基本数据和体积较大的类之间的界限。你可以轻松编写出一些与语言内置(基本)类型无异的小型类。例如,Java就以基本类型表示数值,而以类表示字符串和日期——这两个类型在其他许多编程环境中都以基本类型表现。 对象技术的新手通常不愿意在小任务上运用小对象——像是结合数值和币种的money类、由一个起始值和一个结束值组成的range类、电话号码或邮政编码(ZIP)等的特殊字符串。你可以运用Replace Data Valuewith Object (175)将原本单独存在的数据值替换为对象,从而走出传统的洞窟,进入炙手可热的对象世界。如果想要替换的数据值是类型码,而它并不影响行为,则可以运用Replace Type Code with Class (218)将它换掉。如果你有与类型码相关的条件表达式,可运用Replace Type Codewith Subclass (213)或Replace Type Code with State/Strategy (227)加以处理。 如果你有一组应该总是被放在一起的字段,可运用Extract Class(149)。如果你在参数列中看到基本型数据,不妨试试IntroduceParameter Object (295)。如果你发现自己正从数组中挑选数据,可运用Replace Array with Object (186)。

给我们的启示主要有两点:

CASE:

反例


@Data
public class XxxConfigDTO implements Serializable {

  private static final long serialVersionUID = 8018480763009740953L;

  /**
   * 租户ID
   */
  private Long   tenantId;
  /**
   * 工商税务企业类型
   */
  private String companyType;
  /**
   * 企业名称
   */
  private String companyName;
  /**
   * 企业纳税人识别号
   */
  private String companyTaxNo;
  /**
   * 审单员工工号
   */
  private String auditEmpNo;
  /**
   * 审单员工姓名
   */
  private String auditEmpName;
  /**
   * 跟单员工工号
   */
  private String trackEmpNo;
  /**
   * 跟单员工姓名
   */
  private String trackEmpName;
}

正例

@Data
public class XxxConfigDTO2 implements Serializable {

  private static final long serialVersionUID = 8018480763009740953L;

  /**
   * 租户ID
   */
  private Long   tenantId;
  /**
   * 企业信息
   */
  private Company company;
  /**
   * 审单员工信息
   */
  private Employee auditEmployee;
  /**
   * 跟单员工信息
   */
  private Employee trackEmployee;

}

@Data
public class Company {
  /**
   * 工商税务企业类型
   */
  private String companyType;
  /**
   * 企业名称
   */
  private String companyName;
  /**
   * 企业纳税人识别号
   */
  private String companyTaxNo;
}

@Data
public class Employee {
  /**
   * 员工工号
   */
  private String empNo;
  /**
   * 员工姓名
   */
  private String empName;
}

其实就是怎么去抽象,对于特定领域的对象可以参考DDD里面的Domain Primitive(DP)。5.3 分布式锁

5.3.1 未处理锁失败


private void process(String orderId) {
    // do validate
    try {
        boolean lockSuccess = lockService.tryLock(LockBizType.ORDER, orderId);
        if (!lockSuccess) {
            // TODO 此处需要处理锁失败,重试或抛出异常
            return;
        }
        // do something
    } finally {
        lockService.unlock(LockBizType.ORDER, orderId);
    }
}

分布式锁的目的是为了防止并发冲突和保证数据一致性,锁失败时未处理直接返回,会带来非预期结果的影响,除非明确失败可放弃。

5.3.2 手写解锁容易遗漏

上面的加锁和解锁都是手动编写,而这两个动作一般是成对出现的,在手动编写时容易发生遗漏解锁而导致线上问题,推荐封装一个加解锁的方法来实现,会更加安全和便利。


private void procoess(String orderId) {
    // do validate
    Boolean processSuccess = lockService.executeWithLock(LockBizType.ORDER, orderId, () -> doProcess(orderId));
    // do something
}

private Boolean doProcess(String orderId) {
    // do something
    return Boolean.TRUE;
}

// LockService
public <T> T executeWithLock(LockBizType bizType, String bizId, Supplier<T> supplier) {
    return executeWithLock(bizType, bizId, 60, 3, supplier);
}

public <T> T execteWithLock(LockBizType bizType, String bizId, int expireSeconds, int retryTimes, Supplier<T> supplier) {
    // 尝试加锁
    int lockTimes = 1;
    boolean lock = tryLock(bizType, bizId, expireSeconds);
    while(lockTimes < retryTimes && !lock) {
        try {
            Thread.sleep(10);
        } catch (Exception e) {
            // do something
        }
        lock = tryLock(bizType, bizId, expireSeconds);
        lockTimes++;
    }
    // 锁失败抛异常
    if (!lock) {
        throw new LockException("try lock fail");
    }
    // 解锁
    try {
        return supplier.get();
    } finally {
        unlock(bizType, bizId);
    }
}

5.3.3 加锁KEY无效

private void process(String orderId) {
    // do validate
    try {
        // 此处加锁类型与加锁KEY不匹配
        boolean lockSuccess = lockService.tryLock(LockBizType.PRODUCT, orderId);
        if (!lockSuccess) {
            // TODO 重试或抛出异常
            return;
        }
        // do something
    } finally {
        lockService.unlock(LockBizType.PRODUCT, orderId);
    }
}

注意加锁类型与加锁KEY在同一个维度,否则加锁会失效。

5.4 分页查询

5.4.1 完全没有分页

反例


private List<OrderDTO> queryOrderList(Long customerId) {
    if (customerId == null) {
        return Lists.newArrayList();
    }

    List<OrderDO> orderDOList = orderMapper.list(customerId);
    return orderConverter.doList2dtoList(orderDOList);
}

正例


private Page<OrderDTO> queryOrderList(OrderPageQuery query) {
    Preconditions.checkNotNull(query, "查询条件不能为空");
    Preconditions.checkArgument(query.getPageSize() <= MAX_PAGE_SIZE, "分页size不能大于" + MAX_PAGE_SIZE);
    // 分页size一般由前端传入
    // query.setPageSize(20);
    long cnt = orderMapper.count(query);
    if (cnt == 0) {
        return PageQueryUtil.buildPageData(query, null, cnt);
    }
    List<OrderDO> orderDOList = orderMapper.list(query);
    List<OrderDTO> orderDTOList = orderConverter.doList2dtoList(orderDOList);
    return PageQueryUtil.buildPageData(query, orderDTOList, cnt);
}

没有分页的列表查询对DB性能影响非常大,特别是在项目初期,因为数据量非常小问题不明显,而导致没有及时发现,会给未来留坑。

5.4.2 分页size太大

反例


private Page<OrderDTO> queryOrderList2(OrderPageQuery query) {
    Preconditions.checkNotNull(query, "查询条件不能为空");
    query.setPageSize(10000);
    long cnt = orderMapper.count(query);
    if (cnt == 0) {
        return PageQueryUtil.buildPageData(query, null, cnt);
    }
    List<OrderDO> orderDOList = orderMapper.list(query);
    List<OrderDTO> orderDTOList = orderConverter.doList2dtoList(orderDOList);
    return PageQueryUtil.buildPageData(query, orderDTOList, cnt);
}

分页size的大小并没有一个固定的标准,取决于业务需求、数据量及数据库等,但动辄几千上万的分页size,会带来性能瓶颈,而大量的慢SQL不但影响客户体验,对系统稳定性也是极大的隐患。

5.4.3 超多分页慢SQL

反例


<!-- 分页查询订单列表 -->
<select id="list" parameterType="com.xxx.OrderPageQuery" resultType="com.xxx.OrderDO">
    SELECT
        <include refid="all_columns"/>
    FROM t_order
        <include refid="listConditions"/>
    ORDER BY id DESC
    LIMIT #{offset},#{pageSize}
</select>

正例


<!-- 分页查询订单列表 -->
<select id="list" parameterType="com.xxx.OrderPageQuery" resultType="com.xxx.OrderDO">
    SELECT
        <include refid="all_columns"/>
    FROM t_order a
    INNER JOIN (
        SELECT id AS bid
        FROM t_order
            <include refid="listConditions"/>
        ORDER BY id DESC
        LIMIT #{offset},#{pageSize}
    ) b ON a.id = b.bid
</select>

以上 bad case 的SQL在超多页分页查询时性能极其低下,存在多次回表甚至Using Filesort的问题,在阿里巴巴编码规范中也有明确的规避方案,此处不展开。

最后,我们工程师的智慧结晶都尽在代码之中,而Code Review可以促进结晶更加清莹通透、纯洁无瑕、精致完美,值得大家一起持续精进!

Copyright© 2013-2020

All Rights Reserved 京ICP备2023019179号-8