三个人都点了 Approve，上线还是出了事故
9 月底的一次线上故障，起因是个不到 200 行的 PR：修改优惠券核销逻辑。三个人 review 过，两个 LGTM 加一个 Approved，上线两小时后，监控发现优惠券超发了 1.7 万张。
复盘时我把三个人的评论翻出来看，发现一个规律：
评审人 A：字段名建议改成 camelCase
这个 if 可以合并
评审人 B：日志用 log.debug 吧
加个注释说明下
评审人 C：LGTM
三个人看的全是命名、格式、注释这些表层问题。真正的 bug 藏在并发控制里——原代码用 synchronized 锁的是 String 常量，新代码改成了从数据库读出来的配置对象，锁对象变了，等于没锁。
这件事让我意识到：我们缺的不是评审，是评审的着力点。
我现在用的清单，按五层排
我不再从上往下逐行读代码了，改成按下面五层依次过。顺序有讲究，先看会出事的，再看好不好看。
第一层：正确性
这层出错直接导致线上故障，必须逐条看。
- 边界条件：空集合、单元素、null 参数、超长字符串。我见过最多的 bug 是
list.get(0)没判空。 - 异常吞掉了没：
catch (Exception e) {}这种直接打回，至少要log.error("xxx", e)。空 catch 是最难排查的问题来源。 - 金额和浮点：所有金额用
BigDecimal，比较用compareTo不用equals（new BigDecimal("1.0").equals(new BigDecimal("1"))是false）。 - 时间处理：
SimpleDateFormat非线程安全，用了 static 就是埋雷。统一用DateTimeFormatter。 - 幂等：接口重复调用会怎样。特别是 MQ 消费者和支付回调，没做幂等的一律打回。
第二层：并发与事务
这次事故就栽在这层，现在我最花时间的地方。
- 锁对象是不是稳定的。
synchronized锁String、锁每次new出来的对象、锁Integer（超过 127 就出缓存），都是无效锁。 - 事务范围对不对。事务里调 RPC、发 MQ、写文件，这些都不会回滚，还会拉长事务持有时间。我见过一个方法加了
@Transactional，里面调了个耗时 8 秒的外部接口，数据库连接池直接被拖死。 - 事务会不会失效。
@Transactional在同一个类内部调用、方法是private或final、异常被catch掉了，这三种情况事务都不生效。 - 线程池参数。用
Executors.newCachedThreadPool()的一律打回，队列是无界的，流量一来就 OOM。
第三层：性能
- 循环里查数据库。这个太常见了，见过最夸张的是循环 2000 次每次查一次库，接口耗时 47 秒。
- 大集合操作。
List.contains()在循环里用，10 万数据就是 100 亿次比较，改成HashSet。 - 慢 SQL。看有没有走索引，
EXPLAIN一下。字段上加函数、隐式类型转换、like '%xxx'开头这三种会让索引失效。 - 返回体大小。列表查询没分页，或者把整个大对象序列化返回。
第四层：可维护性
这层不出事故，但影响团队长期效率。
- 方法超过 80 行就该拆。不是说长方法一定错，而是它没法被有效 review。
- 重复代码出现第三次就抽。一两次可以容忍，第三次说明模式已经稳定了。
- 魔法值。状态码
if (status == 3)这种，要么用常量，要么用枚举。 - 日志够不够排查。关键分支有没有日志，日志里有没有 traceId、业务主键。
第五层：安全
- SQL 注入：用
${}而不是#{}的 MyBatis 语句。 - 越权：查数据带没带租户 ID 或用户 ID。我们踩过一次，订单查询接口没带用户 ID，改一下 URL 就能看别人的订单。
- 敏感信息脱敏：日志里打了手机号、身份证、密码。
能用机器查的，别让人看
上面清单里"第四层可维护性"和一部分"第一层"，机器做得比人好。我们现在流水线里挂了这些：
| 工具 | 查什么 | 阻断策略 |
|---|---|---|
| Checkstyle | 命名、格式、import 规范 | error 级别阻断 |
| SpotBugs | 空指针、资源未关闭、equals 错误 | 高危阻断 |
| SonarQube | 圈复杂度、重复代码、覆盖率 | 新增代码覆盖率 < 60% 阻断 |
| ArchUnit | 分层依赖约束 | 阻断 |
ArchUnit 是我后来加的，用来卡分层，比如 Controller 不能直接调 Mapper：
@AnalyzeClasses(packages = "com.example.order")
class LayerTest {
@ArchTest
static final ArchRule controller_should_not_access_mapper =
noClasses().that().resideInAPackage("..controller..")
.should().dependOnClassesThat().resideInAPackage("..mapper..");
@ArchTest
static final ArchRule service_should_not_use_servlet =
noClasses().that().resideInAPackage("..service..")
.should().dependOnClassesThat().haveFullyQualifiedName(
"jakarta.servlet.http.HttpServletRequest");
}
加了自动化之后，评审意见里"命名不规范""没加日志"这类评论从 60% 降到了 12%，人的注意力终于能集中到逻辑和并发上。
评审文化比清单更难搞
清单再好，没人认真看也是废纸。我们做了几条约定，效果比工具明显：
- PR 控制在 400 行以内。超过 400 行的 PR，缺陷发现率会断崖下跌。我们的数据显示：200 行以内的 PR，评审平均能发现 1.3 个问题；800 行以上的 PR，平均发现 0.4 个，因为没人看得完。遇到大改动就拆成多个 PR，按"重构 PR + 功能 PR"分开提交。
- 4 小时响应。PR 挂超过一天，作者已经切到别的事情上了，回来改上下文切换成本高。我们在飞书群里做了机器人提醒，超过 4 小时没人看就 @ 一下。
- 区分评论语气。我们约定在评论前加标记：
[必须]表示不改不能合，[建议]表示可以讨论，[ nit ]表示吹毛求疵可改可不改。这个小小的约定解决了"提意见怕得罪人"的问题。 - 作者自己先过一遍 diff。提交前自己看一遍 diff，能发现一半以上的低级错误。我现在养成了习惯，提交前把 diff 从头读一遍，经常能发现自己写的半成品代码。
- 事故 PR 要复盘。出了线上故障，把那个 PR 的评审记录翻出来看，问一句"为什么当时没发现"。这不是追责，是为了更新清单。上面"锁对象不稳定"这一条就是这么加进去的。
小结
评审这事儿，我最大的体会是：人脑的带宽是稀缺资源，要花在机器判断不了的地方。格式、命名、空指针这些交给工具，人去想"这个并发场景对不对""这个异常会不会导致数据不一致""这个接口被恶意调用会怎样"。
至于清单本身，它不是一成不变的，每次事故复盘都是往里加一条的机会。我们这份清单从最初的 12 条涨到现在的 31 条，每一条背后都有一次踩坑。