写点什么

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

  • 2024-07-18
    北京
  • 本文字数:7208 字

    阅读完需:约 24 分钟

1. 引言

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


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


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


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

2. 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 过程中的核心元素。所有的反馈都应该是建设性的,目的是改进代码而不是批评个人。作为评审者应针对代码给出具体、有用的反馈,并在表达时考虑代码作者的感受。

3. 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 率等。

4. 案例分享

以下是身边真实发生的一些 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;    // 条件1    if (condition_1) {        useServiceCode = false;    }    // 其他条件    if (!canUseServiceCode(afterSaleOrderContext)) {        useServiceCode = false;    }    // 条件2    if (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); // 只针对一单一品一个数量的返回true return 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) { // 条件1 if (condition_1) { return false; } // 条件2 if (condition_2) { return false; } // 条件3 List<ProductDetailDTO> productDetailDTOList = buildMainGiftProductList(afterSaleOrderContext); // 只针对一单一品一个数量的返回true return 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());}
复制代码

5. Code Review 的成果收益

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


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

6. 总结与展望

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


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


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

7. 参考资料

《Google Engineering Practices Documentation》:https://google.github.io/eng-practices/review/


《Code Review at Cisco Systems》:https://static1.smartbear.co/support/media/resources/cc/book/code-review-cisco-case-study.pdf


Wikipeida:https://en.wikipedia.org/wiki/Code_review

发布于: 刚刚阅读数: 5
用户头像

拥抱技术,与开发者携手创造未来! 2018-11-20 加入

我们将持续为人工智能、大数据、云计算、物联网等相关领域的开发者,提供技术干货、行业技术内容、技术落地实践等文章内容。京东云开发者社区官方网站【https://developer.jdcloud.com/】,欢迎大家来玩

评论

发布
暂无评论
Code Review:提升代码质量与团队能力的利器_京东科技开发者_InfoQ写作社区