Code Review：提升代码质量与团队能力的利器

2024-07-18
北京
本文字数：6822 字
阅读完需：约 22 分钟

引言 Code Review（下文简称 CR），即代码审查，是一种通过评审代码以发现并修正错误的实践。它不是一个新概念，但在软件开发中，它的重要性毋庸置疑。首先，它可以显著降低软件中的缺陷比例；其次，它促进了知识共享，通过评审的过程，团队成员可以相互学习，增强对系统的整体理解；最后，CR 是一种预防措施，它有助于维护代码的清晰和统一，减轻技术债务，提升系统的稳定性。

尽管 CR 有诸多好处，实际操作中却面临不少挑战。例如，交付压力可能导致 CR 被忽视或流于形式；另一方面，缺乏有效技巧和工具支持，可能会使 CR 变得低效，甚至引发团队内的冲突；此外，一些团队可能会遇到参与度不足的问题，团队成员不愿意投入必要的时间和精力。

在接下来的内容中，我们将探讨如何克服这些挑战，优化流程，并分享一些实战经验，以帮助读者在自己团队中实施有效的 CR。

在此特别感谢 JDL 平台技术部王鑫、刘建设、刘风、杨宏强、鞠万奎等对本文撰写的帮助。

Code Review 的核心目标和基本原则 2.1 核心目标首先，CR 并不是走马观花，也并不需要面面俱到，我们先要明确以下几个核心目标。

2.1.1 提高代码质量 CR 的首要目标是提高代码质量。这包括识别缺陷、识别性能问题、确保代码遵循一致的设计原则、提高代码的可读性和可维护性。

2.1.2 风险管理 CR 的次要目标是发现潜在风险。通过 CR 尽早发现并解决潜在的代码问题，以降低未来的修复成本，降低大型项目返工及上线失败的风险。

2.1.3 促进知识共享最后，通过 CR 促进团队知识共享。CR 过程鼓励团队成员之间的交流和协作，让团队成员相互学习对方的代码和设计思路。这种交流有助于提高团队的整体技能水平，同时减少代码库中知识的单点问题。

2.2 基本原则对应 CR 的核心目标，遵循以下几个基本原则有助于做好 CR。

2.2.1 专注于代码质量 CR 的核心目的是提升代码质量。这包括但不限于代码的清晰性、可维护性、性能、安全性和可测试性等，在评审过程中应时刻专注于这些方面。

2.2.2 保持一致性的标准遵循团队或项目的编码标准、风格指南和最佳实践。CR 应该确保代码更改都符合这些标准，以便于团队成员理解和维护代码，保持一致性还有助于减少错误和提高代码质量。

2.2.3 保持尊重/建设性沟通沟通是 CR 过程中的核心元素。所有的反馈都应该是建设性的，目的是改进代码而不是批评个人。作为评审者应针对代码给出具体、有用的反馈，并在表达时考虑代码作者的感受。

Code Review 的实践步骤与技巧 3.1 实践步骤 CR 的实践步骤总体分为三步：准备、评审、修改及完成。

3.1.1 准备在提交 CR 之前，应该先自行检查代码，以确保基本的代码质量且遵循代码规范。可以通过单元测试、静态分析插件（例如 SonarLint、JD EOS）、借助 AI 分析插件（例如 Copilot、JD JoyCoder）等来完成。

如果更改较大，考虑将其分割成几个小的、逻辑上独立的 commit。这样不仅能使每次评审过程更高效，也便于追踪和管理更改。

提交评审的时机，越早进行 CR 则修改的代价越小，至少应保证在提测前提交 CR 及完成修改。

最后，确定适合的评审者，建议选择具有业务经验及较为资深的研发人员。

3.1.2 执行评审在评审过程中，聚焦在代码质量方面（可参考下文提供的 checklist）。控制好每次的时长，如果一次评审时间过长，则考虑是否应在准备阶段就拆分成多次 commit，进行多次评审，而不是在提测前进行一次大型评审。

3.1.3 修改及完成开发者根据收到的反馈进行代码调整，改动较大时可能会进行多次反复评审，当修改完成后，由具有权限的负责人将代码合并至相应分支。

3.2 CR 的最佳实践技巧遵循以下的最佳实践技巧，有助于解决 CR 中遇到的各种问题，并保持高效。

3.2.1 有一份明确的 checklist 每次评审时，评审者应该检查哪些内容？对照一份明确的 checklist，有助于我们专注于代码质量，并保持一致性的标准。以下是一份可供参考的 checklist。

•设计：主要评审整体设计，例如，API 设计简单清晰，代码交互、系统交互恰当，技术组件、中间件使用得当等。

•功能性/非功能性：评审代码的行为是否符合预期？大多数时候，仅靠评审并不能发现每一行代码是否如期运行，我们应特别关注一些异常的极端情况，例如，边界处理、异常死循环、非法的输入输出、大报文处理、兼容性问题、线程安全/并发问题、Exception 处理等。

•性能/稳定性：对于一些高吞吐量的系统，响应性能尤其重要。例如，确保依赖服务 SLA 符合预期，超时和重试配置得当，避免产生慢 SQL、大量锁等待、线程阻塞/耗尽等。

•可观测性：是否在上线后可观测代码运行的行为，发生异常时可及时感知？例如，确保方法添加了必要的监控埋点、有正确的日志级别及日志内容。

•复杂度：代码实现足够简单吗？是否有过度设计？作为评审者应让代码尽量保持简洁，以便让其他的开发者可以快速理解，降低未来修改时引入新错误的风险。

•命名：是否为变量、类、方法等选择了清晰的名称？命名应遵守代码规范，且能够准确表达代码的意图，而又不至于过长难以阅读。

•注释：注释清晰无歧义，应解释代码“为什么”，而不是“是什么”。注释更应解释一些代码外的隐含信息，例如，设计的取舍、业务背景、某些看起来很 tricky 的实现，以及解释正则表达式、特定算法等内容。

•测试：是否有适当的单元测试？需要修改已有的单元测试？

•风格：是否遵循一致的代码风格？风格无所谓好坏，但保持一致性的风格，会让其他团队成员更容易理解。

•文档：是否需要更新相关 API 说明、Readme 等文档？

3.2.2 避免完美主义在评审中发现问题固然重要，但也应结合实际约束及现状进行权衡，并非所有代码均要达到理论上的最优解及最佳实践。只要这次修改让代码有所改善，或是向着正确的方向前进，那么代码就是可以接受的。（调研报告显示 61%的 CR 没有发现缺陷）

3.2.3 拆分为小型 MR/PR/Commit 小型的 changelist，拥有降低评审难度、缩短评审时间、减少引入错误的可能性、易于合并等诸多好处。通常认为将 changelist 控制在只解决一件事（可以只是 feature 的一部分），视作合适的大小。我们可以按层进行水平拆分、按功能进行垂直拆分，亦或是结合两者，有兴趣的读者可以阅读文章最后引用的 google 关于 CR 工程实践文章。

3.2.4 一次不要评审过多的代码建议将每次评审的代码控制在 100~300 行，最多不超过 500 行，每次评审时间不超过 1.5 小时（调研报告显示超过这些阈值会导致 CR 质量及效率大幅降低）。不过根据实际场景不同，读者可以根据代码实际的复杂度进行调整。

3.2.5 尽早进行小而频繁的评审尽早评审有助于提前发现问题，减少后期修正的成本。编码阶段，在 IDE 环境安装静态代码检查工具，提前预先检查代码风格、格式等基本错误，可减少人工评审的工作量。面对大型代码变更，将代码分为更小而独立的多次 commit，尽早进行多次评审，也可提升评审质量，减少返工成本。

3.2.6 保持尊重保持开放的心态，抛开自负，不要将个人偏好带入到 CR 中。作为代码审查者，应意识到代码作者更了解其编写的代码，并不是每次评审都需要进行代码调整。基于事实及代码规范来提出改进建议，会使代码作者更容易接受。作为代码提交者，提交高质量的代码，是对评审者和团队最基本的尊重。保持开放的心态，将评审当做自我学习和提升的过程。

3.2.7 度量和改进设定一些度量指标，并持续追踪趋势，有助于我们持续不断改进 CR 过程。以下是一些可以用作度量的指标，例如，审查时长、缺陷密度、CR 率等。

案例分享以下是身边真实发生的一些 CR 案例，与 3.2.1 章节中的 checklist 都有相应的对照，供大家参考。为了便于阅读，部分代码进行了删除简化。

4.1 案例 1-异常及并发情况处理不周问题：静态缓存先 clear，再进行加载，如果解析过程异常会导致配置丢失、在高并发访问时读取到错误的配置。

改善：应使用覆盖更新的方式。

public class ReverseSwitch {private static Map<String, Boolean> multiConfigAddress = new HashMap<>();

public void setMultiConfigAddress(String multiConfigAddress){    ReverseSwitch.multiConfigAddress.clear();    // 以下是解析字符串配置映射到Map配置中，省略具体过程    for (/*.....*/) {        ReverseSwitch.multiConfigAddress.put(/*.....*/);    }}
public static boolean isMultiConfigSwitch() {    // .....}

复制代码

}

CR 修改后：

public void setMultiConfigAddress(String multiConfigAddress){log.info("ReverseSwitch.setMultiConfigAddress {}", multiConfigAddress);Map<String, Boolean> newAddress = new HashMap<>();// 省略解析过程 for () {newAddress.put();}// 使用覆盖更新的方式 ReverseSwitch.multiConfigAddress = newAddress;}

4.2 案例 2-设计问题、可观测性不足问题：1. 本地缓存每小时失效一次，会集中产生大量 RPC 请求加载数据（容器数量*外部请求数），当依赖的 RPC 服务抖动时有可能导致雪崩；2. do while 语句在远程数据异常时，可能循环次数超出预期或产生死循环，导致 tp99 超时、阻塞或 OOM；3. 缺少必要的日志及监控埋点。

改善：1. 使用 redis 缓存并预加载；2. while 内设置最大分页次数进行 break；3. 上层调用增加监控埋点及日志。（由于修改不止一处文件，未一一列出修改后的代码）

@CacheMethod(key = "vrs.SpareQueryProxyCache.getAllSpareInfo",cacheBean = "localGuavaCacheBean60m",timeout = Constants.REDIS_KEY_TIMEOUT_MINUTES_60)public List<BaseStoreInfoDto> getAllSpareInfo() {int pageNum = 0;PageDto<List<BaseStoreInfoDto>> page;List<BaseStoreInfoDto> returnList = new LinkedList<>();do {page = basicPrimaryWS.getBaseStoreInfoByPage(++pageNum);if (page != null && CollectionUtils.isNotEmpty(page.getData())) {// 省略对 page 内容进行筛选等逻辑处理代码// ......returnList.addAll(page.getData());}}while (page != null && page.getCurPage() < page.getTotalPage());return returnList;}

4.3 案例 3-代码复杂度问题：代码不够内聚，可读性不好，开发追加需求时将多个校验的逻辑写到了校验方法外。

改善：将校验逻辑放到对应的校验方法内，保持代码整洁，降低理解难度。

public void buildWaybillCodeList(AfterSaleOrderReceiveContext afterSaleOrderContext) {boolean useServiceCode = true;// 条件 1if (condition_1) {useServiceCode = false;}// 其他条件 if (!canUseServiceCode(afterSaleOrderContext)) {useServiceCode = false;}// 条件 2if (condition_2) {useServiceCode = false;}List<String> waybillCodeList = new ArrayList<>();if (useServiceCode) {// 场景 1：单号规则 waybillCodeList.add(WAYBILLCODE_PREFIX + afterSaleOrderContext.getAfterSaleOrderReceiveDTO().getServiceCode());} else {// 场景 2：单号规则 waybillCodeList.add(this.preDeliveryId(afterSaleOrderContext));}// ......}

private boolean canUseServiceCode(AfterSaleOrderReceiveContext afterSaleOrderContext) {List<ProductDetailDTO> productDetailDTOList = buildMainGiftProductList(afterSaleOrderContext);// 只针对一单一品一个数量的返回 truereturn productDetailDTOList.size() == 1 && Objects.equals(productDetailDTOList.get(0).getProductCount(), 1);}

CR 修改后：

public void buildWaybillCodeList(AfterSaleOrderReceiveContext afterSaleOrderContext) {List<String> waybillCodeList = new ArrayList<>();// 将多次需求变更的逻辑点聚合到职责明确的方法内 if (canUseServiceCode(afterSaleOrderContext)) {// 场景 1：单号规则 waybillCodeList.add(WAYBILLCODE_PREFIX + afterSaleOrderContext.getAfterSaleOrderReceiveDTO().getServiceCode());} else {// 场景 2：单号规则 waybillCodeList.add(this.preDeliveryId(afterSaleOrderContext));}// ......}

private boolean canUseServiceCode(AfterSaleOrderReceiveContext afterSaleOrderContext) {// 条件 1if (condition_1) {return false;}// 条件 2if (condition_2) {return false;}// 条件 3List<ProductDetailDTO> productDetailDTOList = buildMainGiftProductList(afterSaleOrderContext);// 只针对一单一品一个数量的返回 truereturn productDetailDTOList.size() == 1 && Objects.equals(productDetailDTOList.get(0).getProductCount(), 1);}

4.4 案例 4-增加灰度策略控制问题：CR 过程中发现无法评估改动影响的业务范围，如有问题可能会影响 100%的流量。

改善：增加灰度策略开关。

public void setConsigneeAddress(WaybillAddress targetAddress) {// 100%修改至新逻辑：A// ......}

CR 修改后：

public void setConsigneeAddress(WaybillAddress targetAddress) {// 选取可控的特征属性，例如按流量比例或商家切量 if (ThreadLocalRandom.current().nextInt(1000) < ducc.getAddressMontageSwitchRate()) {

// 新逻辑：A，使用灰度策略，控制本次改动影响范围// ......} else {// 原逻辑：B// ......}}

4.5 案例 5-善用工具在提交人工 CR 前，善用工具进行代码扫描，可能有意想不到的收获。

对如下代码使用 JD JoyCoder 进行自动评审建议。

if (response != null&& response.getCode() != 0&& String.valueOf(response.getCode()).length() > 2&& (KK_PARAM_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2)))|| KK_BIZ_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2))) {throw new BusinessException(StringUtils.isNotBlank(response.getSubMsg()) ? response.getSubMsg() : response.getMsg());}

JD JoyCoder 给出的评审意见：“代码中存在一个潜在的逻辑错误，这是由于条件判断中的逻辑运算符 &&和||的优先级没有被明确区分。在 Java 中，&&操作符的优先级高于||，这意味着在没有适当的括号的情况下，&&绑定的条件会先被评估，然后才是||绑定的条件。”

修改后：

if (response != null&& response.getCode() != 0&& String.valueOf(response.getCode()).length() > 2&& ((KK_PARAM_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2))|| KK_BIZ_PREFIX_CODE.equals(String.valueOf(response.getCode()).substring(0, 2)))) {throw new BusinessException(StringUtils.isNotBlank(response.getSubMsg()) ? response.getSubMsg() : response.getMsg());}// 此段代码可以进一步优化，将 if 里面的条件提前抽取到有明确业务语义的变量中，提升可读性

除目前流行的基于 LLM 实现的 AI 扫描工具外，使用传统代码扫描也可以发现潜在问题。

以下代码通过静态扫描工具发现问题：直接使用“==”进行包装类型 Integer 的比较，当遇到[-128, 127]范围外时比较结果会不符合预期。

if (!(request.getSkuList().stream().allMatch(sku -> sku.getPreProduce() != null &&sku.getPreProduce() == request.getSkuList().get(0).getPreProduce()))) {throw new DOSException(ResultEnum.PRE_PRODUCE_UN_SAME.getCode(), ResultEnum.PRE_PRODUCE_UN_SAME.getMessage());}

修改后：

if (!(request.getSkuList().stream().allMatch(sku -> sku.getPreProduce() != null &&sku.getPreProduce().equals(request.getSkuList().get(0).getPreProduce())))) {throw new DOSException(ResultEnum.PRE_PRODUCE_UN_SAME.getCode(), ResultEnum.PRE_PRODUCE_UN_SAME.getMessage());}

Code Review 的成果收益笔者所在团队没有单独统计数据来佐证 CR 与线上缺陷的直接关联。线上质量与 CR、单元测试、质量测试、SRE 等各方面息息相关，CR 并非银弹，但是做好 CR 非常有助于降低缺陷数量。

通过搜索公开数据显示，行业中使用 CR 的项目，潜在缺陷发现率约在 50%~60%之间，大部分的测试，潜在缺陷发现率约在 30%左右。同时，数据显示约 75%的 CR 评审意见影响着软件的可维护性/可演化性，这表明 CR 利于软件系统的长期演化。

总结与展望本文探讨了 CR 的重要性，它可以提前发现缺陷，有助于知识共享及团队能力提升，同时分享了 CR 实践步骤、技巧、案例等内容。当然，本文仅是一份参考指南，每个团队根据其所处现状的不同，可以根据本文调整优化各自的实践流程。

如今，软件开发的格局在不断变化，围绕 CR 的实践也在不断发展。随着技术的进步，更智能的工具和 AI 辅助平台在不断涌现，这些工具能够提供更高级的静态分析、模式识别，甚至预测分析，在潜在问题出现之前识别它们。这种 AI 上下文感知的能力，将能够根据项目特定的编码风格、功能模块以及依赖关系，提供针对性的 CR 反馈，甚至不再需要人工评审的介入。

未来，CR 将继续发挥其关键作用，我们期待 AI+CR 成为一个更加强大和智能的伙伴，使团队将能够保持竞争力，持续提升软件质量和交付速度。