Code Review：提升代码质量与团队能力的利器
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。
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