★ FEATURED ARTICLE 前阵子把这两三年Review过的PR记录翻出来做了次复盘数量刚好过了一千。说实话刚开始带团队做Code Review那会儿我总以为会看到各种“花式错误”——复杂的并发问题、精妙的算法缺陷、诡异的架构失误。真正把这堆记录看完之后我反而有点失落高频出现的永远是那几张熟面孔。空指针、被吞掉的异常、循环里的数据库查询、不知所云的变量名、散落在代码各处的魔法数字、看似绿油油实则没有断言的测试以及让人根本没法集中精力看的巨型PR。它们几乎出现在每一个项目里和语言无关和框架无关和团队资历深浅也没想象中那么大关系。认真把这些问题拆开看你会发现背后其实只有两类原因一类是写代码时只顺着happy path走没把输入空间和失败路径当回事另一类是提交之前根本没想着“让别人看懂”。这两类问题不解决团队技术建设做得再好也白搭。这篇文章就把这七个高频错误逐个摊开说说它们的现场、根因、修复方式以及我现在是怎么在评审阶段快速把它们揪出来的。1. 1000多个PR看下来我发现的规律1.1 这些错误有一个共同点先说个反直觉的结论这七个错误没有一个是“能力问题”。能把这些PR写出来的工程师都能独立完成需求、都能把功能跑通有些人的方案设计能力还相当强。但一到代码评审环节同样的问题就像复读机一样反复出现。我复盘后的判断是它们的共同点在于**“默认别人不需要理解我的代码”**。写代码的时候大脑里装的是一条从输入到输出的直线路径所有的假设都是隐性的这个参数一定不为空、这个文件一定存在、这个接口一定成功、这个变量以后一定会被改。这些隐性假设在作者自己眼里理所当然但在评审人眼里、在三个月后的维护者眼里、在线上异常堆栈里统统都是定时炸弹。另一个共同点是**“能跑就行”**的及格线思维。很多开发者把“代码能跑通测试”当成完成的标志而不是“这段代码在任何输入下都不会出问题”。于是边界分支不写、异常处理不写、命名不推敲、测试不写断言等到线上出了事故再回头补代价翻了十倍不止。1.2 为什么我专门挑这七个其实我整理一开始不只总结了七个列了二十多个但后来我筛了一遍留下这七个的标准很明确出现频率最高、影响范围最大、并且完全可以通过评审流程拦截住。像“并发下的竞态条件”“分布式事务不一致”这类问题确实严重但它们是低频事件而且对评审者的经验要求极高普通团队很难靠Checklist拦住。而这七个错误不一样它们有非常清晰的识别特征看到空catch块就知道有问题、看到循环里有查询就知道会慢、看到测试没有断言就知道是白写。只要你愿意在提交前多看自己一眼或者在评审时说一句“这里补个边界判断”就能挡下大部分线上事故和后续维护成本。所以这篇文章不只是列错误更想给一套“可执行”的排查方法。下面逐个来。2. 线上事故的三张熟面孔空值、异常、N1查询2.1 空值边界一大半问题都从这里来如果只允许我保留一条评审铁律我会选这条每个入参都可能为空每条数据都可能缺失每个集合都可能为空集合。这个错误最常见的现场是这样的public BigDecimal calcDiscount(Order order) { if (order.getAmount().compareTo(new BigDecimal(100)) 0) { return order.getAmount().multiply(new BigDecimal(0.9)); } return order.getAmount(); }这段代码在“订单一定存在”“金额一定有值”的前提下没有任何问题测试也能过。但一旦调用方传了个不完整的Order对象或者数据库里读出一条amount为NULL的记录这里就是一个NPE。更隐蔽的是amount为负或为零的情况你明明没有达到优惠门槛结果算出来一个负折扣。后来我让团队做了一个习惯写任何方法之前先把入参的取值空间穷举一遍。把这个方法当成一个黑盒问自己四个问题如果这个值是null会怎样如果是0或负数会怎样如果是超大值会怎样如果是空字符串或空集合会怎样只要其中一个回答不是“没问题”那就得写防御逻辑。修复并不复杂加个空值校验、对边界值做显式判断即可public BigDecimal calcDiscount(Order order) { Objects.requireNonNull(order, order must not be null); BigDecimal amount order.getAmount(); if (amount null || amount.compareTo(BigDecimal.ZERO) 0) { throw new IllegalArgumentException(amount invalid: amount); } if (amount.compareTo(new BigDecimal(100)) 0) { return amount.multiply(new BigDecimal(0.9)); } return amount; }我自己的评审习惯是看到方法签名之后先不急着看逻辑自己默念一遍“这个方法哪些地方会炸”然后再在diff里找对应的防御逻辑。如果找不到直接在评论里问作者一句“调用方传null怎么处理”多数情况下作者会愣一下然后默默补上。这一问的价值经常比后面看十行代码都大。2.2 异常被吞系统最危险的“沉默杀手”比起空值异常被吞的后果更隐蔽也更致命。空值好歹会在调用栈里留个NPE异常被吞则是把问题藏进了数据里线上可能几天后才会以“数据对不上”的形式爆发出来。典型的现场是这两种try { paymentClient.charge(order); } catch (Exception e) { // 什么也不干 }try { paymentClient.charge(order); } catch (Exception e) { log.info(charge failed); return null; }第一种是空catch第二种是catch之后只打个info日志然后返回一个看似“正常的默认值”。如果调用方拿到null之后也没有做空值判断那这个失败就彻底被“消化”掉了——订单没有扣款但业务继续往下走库存照常扣减最终对账的时候发现钱货对不上。我让团队记一句话catch块里只写日志等于没处理异常。日志只是记录它不是恢复策略。一个合格的异常处理至少要回答三个问题这个异常可恢复吗如果可恢复当前上下文要怎么补偿如果不可恢复我应该抛出去还是切到降级路径修复方案要按业务性质区分。像ocr识别失败这种可降级的场景应该明确降级策略并记录失败数据像支付扣款这种不可乱降级的场景必须向上抛出并触发事务回滚try { paymentClient.charge(order); } catch (PaymentException e) { log.error(charge failed, orderId{}, account{}, order.getId(), order.getAccountId(), e); throw new BizException(PAYMENT_FAILED, 扣款失败, e); }评审时我有几个条件反射动作看到catch块先看里面有没有除了日志之外的行为看到catch后返回常量或null直接标记看到printStackTrace()直接要求改成日志框架。另外catch块里至少要带上业务上下文信息否则线上看日志根本没法定位是哪个订单出了问题。2.3 循环里的数据访问N1查询的典型现场N1问题在评审中出现的频率高得惊人而且我见过不止一次在互联网大厂的核心链路里出现。表现形式很简单ListOrder orders orderService.listByUser(userId); for (Order order : orders) { ListItem items itemMapper.selectByOrderId(order.getId()); // 循环里查库 // 业务处理 }单看代码逻辑没问题。但你把数量代进去算一下假设一个用户有1000条订单这条代码会产生1条查询订单列表的SQL再加上1000条查询订单明细的SQL总共1001次数据库往返。如果数据库连接池只有50个连接这1000多条SQL足够把连接池打满紧接着整个服务的接口全部排队。更隐蔽的变体是循环里调远程HTTP接口、循环里读文件、循环里调外部RPC。有一次我评审一个批量导出功能作者在循环里对每个订单调一次供应商接口查询物流状态导出1000个订单就要调1000次供应商接口每次耗时300毫秒总耗时5分钟起步用户早就把页面关掉了。修复原则只有一个循环里不允许出现任何IO操作。批量数据要么一次性查出再在内存里匹配要么走批量接口要么用缓存ListOrder orders orderService.listByUser(userId); ListLong orderIds orders.stream().map(Order::getId).toList(); MapLong, ListItem itemMap itemMapper.selectByOrderIds(orderIds) .stream().collect(Collectors.groupingBy(Item::getOrderId));我评审时专门盯两点一是diff里出现for、foreach、while关键字代码块内必须扫一遍有没有数据库访问、远程调用、文件读写二是看一个接口的数据库访问次数是否和入参数量级成正比。只要是直接指出来并要求改成批量方案。这个错误的成本是可以量化的讲道理比讲情怀好使。3. 让代码在半年后变成废墟的三个习惯3.1 命名不表达意图代码在快速腐烂的第一信号如果说空值和异常是线上炸出的雷那命名问题就是埋在代码库里的慢性毒药。它不会让系统今天挂掉但会在半年后让每一个修改需求都变成考古现场。这种变量名我每个月都会看到public void handle(int a, int b) { ListObject temp getSomeData(); for (Object data : temp) { if (1.equals(data.getStatus())) { flag true; } } if (flag) { // ... } }这段代码里的a、b、temp、data、flag没有任何表达力。以data为例它到底是用户列表、订单列表还是配置项以flag为例它表示的是“处理成功”“需要重试”还是“存在缓存”变量名存在的意义是让读者在不需要阅读全部上下文的情况下就能理解代码现在完全成了摆设。我自己评审时有一条很主观但有效的标准看到一个变量名如果需要在脑子里转一个弯才能猜到它的含义我就会请作者把它改掉。例如data可以改成unpaidOrdersflag可以改成needsRetrytemp可以改成pendingItems。Boolean类型的命名应该给人一种天然的判断感用is、has、can、should开头金额类命名要带上单位amountInCents比money清楚得多集合命名用复数一眼就知道是多个。修复成本其实极低改个名字而已但收益是整个代码库的可读性。我经常和团队说命名是你能做的成本最低的代码优化没有之一。如果作者解释不清为什么叫这个名字那基本等于他也没想清楚这段代码的真正职责。3.2 魔法值与硬编码需求变更时最难改的地方魔法值的经典现场是if (order.getStatus() 2) { // 已支付状态 }Thread.sleep(5000); // 等待回调String url http://internal-service.example.com/api/v1/query;这些数字和字符串在写出来的那一刻只有作者知道含义而且大概率没有任何注释。更麻烦的是同一个魔法值往往散落在多个文件里。我遇到过一次改订单超时时间业务方把超时时间从3600秒改成1800秒开发者只改了订单模块里的判断但定时关单任务里还硬编码了一个3600结果线上出现超时未关单的订单。排查了一下午最终定位到就是那个漏改的常量。修复方向很清楚业务相关状态用枚举固定参数用常量或配置跨环境的配置放到配置中心。拿上面的例子来说public enum OrderStatus { CREATED(0), PAID(1), CANCELLED(2); private final int code; // ... }Value(${order.timeout.seconds:3600}) private int orderTimeoutSeconds;评审时我会把diff里所有肉眼可见的裸数字、裸字符串全部过一遍筛子。当然数组下标、算数常数这种不算魔法值Arrays.copyOf(arr, 8)里的8如果是BUFFER_SIZE那倒是值得建议提取成常量。这里不需要矫枉过正标准是“这个值脱离上下文之后是否依然清晰”。3.3 测试覆盖“看起来有效”绿色CI也会骗人我见过最坑的一类PR是测试全绿、覆盖率很高但合入之后功能明显有问题。后来我仔细看测试代码才发现问题出在“测试根本没做事”。以下是我拆穿这种无效测试的三大典型现场Test void testCalcDiscount() { Order order new Order(); order.setAmount(new BigDecimal(200)); service.calcDiscount(order); // 没有断言 }第一个现场是没有断言。测试跑完绿灯亮起但没有任何输出校验。这个测试的意义约等于“调用一次看看会不会抛异常”。我经常在评审里问作者一句如果把calcDiscount的实现删掉这个测试会失败吗如果答案是“不会”那这个测试就是不达标的。Test void testCalcDiscount() { Order order new Order(); order.setAmount(new BigDecimal(200)); BigDecimal result service.calcDiscount(order); Assertions.assertTrue(result.compareTo(BigDecimal.ZERO) 0); }第二个现场是断言强度不足。断言“结果大于0”和断言“结果等于180”是两个完全不同的强度前者无法发现计算结果多扣一倍的回归。第三个现场是只测正常路径不测失败路径。空值、异常、超时这些分支完全没有覆盖而这些恰恰才是生产环境最容易出问题的地方。评审测试时我现在有固定的动作先看测试方法有没有断言再看断言是否和被测试方法的真实业务结果挂钩最后在心里做一次变异测试推演——“把某一行实现改成错的测试能不能红”。如果推演不出来我会直接在评论里把对应的变异体列出来请作者补断言。测试的目的不是凑覆盖率数字而是让代码的每一次重构都能被安全网接住。4. 一个最容易被忽略的团队协作错误PR本身一团糟4.1 巨型PR与混乱提交评审人根本没法聚焦代码层面的问题说完了剩下这个是流程层面的但它的破坏力一点不比前面几个小。我发现很多团队的PR体积大得惊人一个PR动辄改30个文件、几千行diff里面同时混着新功能、重构、格式化、依赖升级和配置调整。评审人打开这样的PR第一反应就是放弃治疗。几百个文件密密麻麻摆在那儿根本不知道从哪看起最后只能象征性地点几个文件看看然后在评论区说一句“看起来不错”。等合入之后出了问题想回滚又因为混了太多改动而无法干净回滚想用git bisect定位问题也因为提交粒度过粗而无从下手。我自己对团队的硬性要求很简单一个PR只解决一个问题文件数超过10个必须拆分。新功能PR不应该包含无关的格式调整重构PR不应该夹带依赖升级。Commit也最好是按逻辑小步提交保持每个提交都可以独立通过测试。功能做大了就先提Draft PR把方向确认了再逐步完善而不是攒到最后一次性抛出一个巨型怪物。评审时碰到“看起来不错”这个回复我反而会提高警惕。说明评审人已经放弃了细节审查而这个PR要么太大、要么太乱才导致了这种集体摆烂。正确的做法是拆小PR、分批合入让每一次评审都在可控的上下文里进行。4.2 PR描述缺失评审变成猜谜游戏还有一种情况代码本身写得不错但PR描述空荡荡。标题是一个模糊的“fix bug”描述只有一两行没有背景、没有改动清单、没有测试说明。评审人拿到这样的PR需要先根据diff反推业务背景再猜作者当初为什么这么改效率极低。我自己评审时习惯先看PR描述再看代码。描述是作者的思维导图它应该回答三个问题这次改动是为了解决什么问题动机和背景具体改了哪些模块、哪些行为改动清单怎么验证的测试、自测结果、相关的日志截图一个合格的PR描述应该让评审人在不看代码的情况下就能判断“这个改动的方向对不对”。我见过不少团队引入了PR模板后评审效率直接上一个台阶。下面这个模板我可以直接分享## 背景 这个PR为什么存在解决了什么业务问题 ## 改动内容 - 模块A新增XXX能力 - 模块B调整XXX逻辑 - 模块C修复XXX边界情况 ## 测试验证 - [ ] 单测覆盖了XX和XX分支 - [ ] 自测用XX数据跑通了XX场景 - [ ] 联调与XX服务接口联调通过 ## 风险点 这个改动可能影响哪些下游是否有兼容性风险回滚方案是什么我甚至会让作者在描述里加“测试步骤”一节写出评审人可以自己复现的路径这对快速验证改动有效性帮助极大。许多团队代码review效率低不是评审人不够认真而是PR信息本身就不足够评审人连读懂都费劲更别提提出有价值的意见了。5. 把这些错误挡在merge之前团队层面的三个抓手5.1 用PR模板和Checklist固化评审标准前面说了这么多落到执行层面我建议团队把这几条整理成一份可勾选的Review Checklist放在PR模板里。这样作者提交之前就能自己检查一遍评审人也能按清单逐项核对不靠记性靠流程。我们团队目前用的是这张精简版检查项对应错误通过标准入参和边界值是否校验空值边界对每个入参能回答“null/0/负数/超长会怎样”异常是否处理且上下文完整异常被吞catch块有恢复或抛出逻辑日志带业务上下文循环内是否有IO/DB调用N1查询循环体内无DB访问、无远程调用、无文件IO变量名是否表达意图命名不清晰变量名可读业务含义明确无需猜测是否引入裸数字/裸字符串魔法值硬编码业务状态用枚举参数用常量或配置测试是否有有效断言无效测试删除被测实现后测试能红覆盖失败路径PR是否小而聚焦、描述完整巨型PR/描述缺失单PR解决一个问题描述含背景、改动、验证这张表不需要很长盯住这七个就行。执行过程中我的体会是Checklist的价值不在于“多”而在于“稳”。固定的检查项能降低评审人之间的水平方差新人上手评审时也有一个兜底的引导不至于从零开始摸索。5.2 用自动化工具拦截能拦截的错误人工精力是有限的所以我的原则是能让机器拦的绝对不浪费人眼。现在团队的CI流水线上跑着几道固定检查执行下来的感受是很值的。第一道是静态检查。不同语言的工具链不同Java那边可以用Checkstyle或SpotBugs前端用ESLintPython用Ruff或Pylint。静态检查拦截的是格式、明显的空指针隐患、未使用的变量这类机械问题把这些交给机器评审人就能把注意力放到逻辑语义上。第二道是安全扫描。如果PR里涉及依赖升级流水线上会跑一遍依赖漏洞扫描比如GitHub Dependabot或者Sonatype的检查。很多硬编码密钥、高危依赖版本这一层就能挡下来。第三道是覆盖率门槛。单测覆盖率不达标不让合入这个门槛不需要设太高核心模块80%以上重点盯分支覆盖率而不是行覆盖率。行覆盖率95%但分支覆盖只有30%说明边界情况根本没测这种绿油油最坑人。但我要提醒一句自动化只是兜底它拦不住“命名毫无表达力”“循环里调远程接口”这类语义问题。一个清晰的命名机器永远无法替你判断好坏所以自动化越强人工评审的精力越发要集中在那些真正需要思考的地方。5.3 控制评审节奏与合并门禁最后说点团队管理层面的经验。评审不是越严格越好而是要有节奏。我自己踩过“过于苛刻”的坑早期我带团队时每个PR都要反复打回动辄好几个来回最后大家提PR前都胆战心惊反而把评审当成一种惩罚而不是协作。后来我调整了节奏单次评审的diff行数控制在200~300行以内超过就请作者拆分。人一次能消化多少代码是有限的硬看只会走神走了神就会漏掉真正的问题。评审响应时间控制在一个工作日内。迟到的评审意见价值会大幅缩水别人可能已经基于这个PR继续铺了好几个功能这时候再提建议改造成本已经很高。合并门禁设为两票通过制至少一个资深评审人看过加上CI全绿才能合入。这个门禁不是为了设卡而是防止“一个人拍板”造成的信息盲区。这些结合起来团队的PR周期其实并没有变长反而是因为每个PR更小、描述更清楚、机器先过滤了一遍人工评审单次用时大大缩短整体效率是提升的。我自己的另一个体会是评审最核心的心态是帮作者把代码从“我能跑”推到“别人能维护”。保持这个心态七个错误至少能消灭一半——因为你会开始在写代码的时候考虑边界、考虑异常、考虑命名、考虑看代码的人。剩下的那一半靠Checklist和自动化兜底就已经足够了。
阅读完成 · 觉得有帮助？
