Published on

代码评审实战指南：打造高质量、可维护的项目

Authors
  • avatar
    Name
    NoOne
    Twitter

What？什么是 CR

Code Review（代码评审）指的是让第三者来阅读作者修改的代码，以发现代码中存在的问题。

Why？为什么要做 CR

  • 保证代码的质量
  • 团队代码规范性的保证
  • 技术的交流和分享
  • 便于了解需求整体的进度

How ? 如何进行 CR

Design：程序设计、架构设计是否设计合理

  • Controller 里面直接传入的是 DO 对象
反例： Controller 里面直接传入的是 DO 对象
public ResultDTO<String> savePersonalFinance(@ApiParam @RequestBody AccountEcomFinanceAuditDO entity, @RequestHeader(value = "Accept-Language") String language) {
    String bizId = ecomFinanceAuditService.savePersonalFinance(entity, language);
    return ResultDTO.ok(bizId);
}
  • 全都是 Service，没有良好的分层和分包
  • 过大的类和方法

Functionality：代码功能是否符合作者预期，代码行为是否用户友好

  • 加锁示例
反例：
Lock lock = new ReentrantLock();
try {
    lock.lock();
    System.out.println("hello");
} finally {
    lock.unlock();
}

正例：
Lock lock = new ReentrantLock();
lock.lock();
try {
    System.out.println("hello");
} finally {
    lock.unlock();
}
  • 事务里面不允许调用外部接口
在事务中调用外部接口可能会产生以下问题：  
性能问题：外部接口的响应时间可能会很长，这会导致事务的执行时间增加，从而影响系统的性能。  
事务回滚问题：如果外部接口的调用失败，可能需要回滚事务。但是，如果外部接口的操作不能回滚，那么就可能导致数据的不一致。  
事务超时问题：如果外部接口的响应时间过长，可能会导致事务超时。
错误回滚问题：如果外部接口超时且没有正确的判断异常，在使用 @Transactional(rollbackFor = Exception.class) 注解的情况下会错误的回滚数据。

Complexity：实现是否能简化，代码可读性是否良好，接口是否易用

如何减少 if else？
  • Return Early
优化前：
public String findUser(String userId) {
    if (userId == null) {
        return null;
    } else {
        User user = database.findUserById(userId);
        if (user == null) {
            return null;
        } else {
            return user.getName();
        }
    }
}

优化后：
public String findUser(String userId) {
    if (userId == null) {
        return null;
    }

    User user = database.findUserById(userId);
    if (user == null) {
        return null;
    }

    return user.getName();
}
  • 表驱动
优化前：
if (param.equals(value1)) {
    doAction1(someParams);
} else if (param.equals(value2)) {
    doAction2(someParams);
} else if (param.equals(value3)) {
    doAction3(someParams);
}
// ...

优化后：
Map<?, Function<?> action> actionMappings = new HashMap<>();
actionMappings.put(value1, (someParams) -> { doAction1(someParams)});
actionMappings.put(value2, (someParams) -> { doAction2(someParams)});
actionMappings.put(value3, (someParams) -> { doAction3(someParams)});
actionMappings.get(param).apply(someParams);
  • 职责链模式
优化前：
public void handle(request) {
    if (handlerA.canHandle(request)) {
        handlerA.handleRequest(request);
    } else if (handlerB.canHandle(request)) {
        handlerB.handleRequest(request);
    } else if (handlerC.canHandle(request)) {
        handlerC.handleRequest(request);
    }
}

优化后：
public void handle(request) {
    handlerA.handleRequest(request);
}

public abstract class Handler {
    protected Handler next;

    public abstract void handleRequest(Request request);

    public void setNext(Handler next) {
        this.next = next;
    }
}

public class HandlerA extends Handler {
    public void handleRequest(Request request) {
        if (canHandle(request)) doHandle(request);
        else if (next != null) next.handleRequest(request);
    }
}
  • Tests：是否提供了正确、设计良好的自动化测试、单元测试
反例：
@Test
public void testAddWhiteIpList_KjEnterpriseExtProviderReturnsNoItems() {
    // Setup
    final SsoUser ssoUserDO = new SsoUser();
    ssoUserDO.setSysChannel("sysChannel");
    ssoUserDO.setAuthCode("authCode");
    ssoUserDO.setIp("ip");
    ssoUserDO.setLoginName("loginName");
    ssoUserDO.setId(0L);

    when(mockEnterpriseExtProvider.getUserWhiteConfig("loginName")).thenReturn(Collections.emptyList());

    // Run the test
    whiteIpListServiceUnderTest.addWhiteIpList(ssoUserDO);

    // Verify the results
}
  
正例：
public void testBasicCatcherDistribution() {
	StringCatcher catcher = new StringCatcher();
	bus.register(catcher);
	bus.post(EVENT);
	
	List<String> events = catcher.getEvents();
	assertEquals("Only one event should be delivered.", 1, events.size());
	assertEquals("Correct string should be delivered.", EVENT, events.get(0));
}

Naming：变量名、类名、方法名等字面量的选择是否清晰、精炼

  • 方法应该清晰，精炼
反例1：
// flag 不需要
boolean flag = false;
for (Long assetId : assetIdList) {
    // 在循环里面套数据库查询会有性能问题，最好是通过 id 列表批量查询
    MpAssetInfoDO assetInfoDO = assetInfoService.getById(assetId);
    if (distributePrefilter(assetInfoDO)) {
        flag = true;
        break;
    }
}
if (flag) {
    throw new CommonException("Asset has lapsed, please refresh and try again");
}
  • 对于常用的工具和方法要统一
// 优化前
private List<FundsPartyConfigDetailDTO> underConfigIntersection(List<FundsPartyConfigDetailDTO> a, List<FundsPartyConfigDetailDTO> b) {
    if (ListUtils.isEmpty(a) || ListUtils.isEmpty(b)) {
        return new ArrayList<>();
    }
    return a.stream().filter(item ->
    {
        for (FundsPartyConfigDetailDTO configDetailDTO : b) {
            if (configDetailDTO.getId().equals(item.getId())) {
                return true;
            }
        }
        return false;
    }).collect(Collectors.toList());
}

// 优化后
private List<FundsPartyConfigDetailDTO> underConfigIntersection(List<FundsPartyConfigDetailDTO> a, List<FundsPartyConfigDetailDTO> b) {
    if (CollectionUtils.isEmpty(a) || CollectionUtils.isEmpty(b)) {
        return Collections.emptyList();
    }

    Set<Long> idSet = b.stream()
            .map(FundsPartyConfigDetailDTO::getId)
            .collect(Collectors.toSet());

    return a.stream()
            .filter(item -> idSet.contains(item.getId()))
            .collect(Collectors.toList());
}

Comments：是否编写了清晰的、有用的注释

  • 注释是给别人看的，目标是让别人知道自己这么写的原因

Style：代码风格是否符合规范

  • 新增代码要做格式化
  • 不要直接全量格式化之前的代码

Documentation：修改代码的同时，是否同步更新了相关文档

  • 代码要和设计文档保持一致

实施注意

  • 小批量：每次 Review 的代码量要少，单次 Review 的代码太多，占用评审人的评审时间太长，很容易沦为走过场，建议为 200 行左右，最多不超过 1000 行。
  • 多批次：Review 要频繁发生，小步快跑，建议开发同学一周有 3-4 次 CR。
  • 快速响应：提交完的 CR，评审人尽量在当前完成当次评审，提交 MR 的时候也尽量选取对相关代码比较熟悉的评审人。
  • Release 分支只允许合并，不允许直接推送。
  • Release 分支合入到 master 分支由模块负责人操作。

参考

Share this content