代码评审里最该盯的 3 类改动

一次代码评审记录里，一百二十条评论中有八十三条在讨论命名和空格，两条提到了并发写同一份数据的问题。三个月后线上出现数据错乱，回看那次评审，隐患当时就在那里。

代码评审的时间是有限的，而注意力分配决定了它能拦住什么。格式问题交给工具自动处理，人的精力应该留给人才能判断的部分。

第一类：边界与错误处理

这类问题最不容易在本地测试中暴露，却最容易在生产上变成事故。评审时重点看输入校验、空值处理、超时设置和重试逻辑。

• 外部接口调用是否设置了超时，是否有重试次数上限。
• 集合与数组访问是否考虑了空和越界的情况。
• 失败路径是否被吞掉，比如捕获异常后只打一行日志就继续执行。
• 降级与兜底是否有默认值，默认值本身是否安全。

其中「吞掉异常」最值得警惕。捕获之后不处理、不抛出，代码看起来更健壮，实际是把错误推迟到更难定位的时刻，届时现场信息已经丢失。

第二类：数据与状态变更

凡是涉及写数据库、改缓存、更新共享状态的改动，都值得逐行看。这类问题往往不体现在语法上，而体现在执行顺序和并发假设上。

需要确认的点包括：一次业务操作是否包在事务里，事务边界是否过大；多个写操作之间有没有隐含的顺序依赖；同一份数据会不会被并发修改；幂等性如何保证，尤其是消息重投和用户重复提交的场景。

某支付团队的错乱问题，根因正是回调处理与定时补偿任务同时更新同一条订单记录，两个流程都做了「先查后写」，中间的时间窗导致后写覆盖了先写。这类缺陷只能靠评审时的并发推演发现，测试环境很难复现。

第三类：可读性与测试覆盖

命名和结构属于可读性范畴，但关注点不是空格和引号风格，而是看这段代码解决的到底是什么问题。一段需要读三遍才能理解的逻辑，半年后大概率会被改错。

测试覆盖看两件事：新增逻辑有没有对应的用例，用例是否覆盖了失败路径和边界情况。只有成功路径的测试，保护作用非常有限。

还可以看一个指标：这次改动有没有让某个函数的职责变得更多。如果一个函数同时负责参数校验、业务计算和结果落库，即使这次只加了五行，也值得提出来讨论拆分。

评审本身的几个常见问题

第一个是单次改动过大。超过四百行的提交，评审人只能快速浏览，风险审查基本失效。有效做法是把大改动拆成可独立评审的小步，每个提交都保持可编译可测试。

第二个是评论语气。写「这个写法有问题」会引发作者辩解，写「这里如果并发写同一行会怎样」则只是提出一个待验证的问题。评审讨论的是代码，但沟通方式决定了讨论能否推进。

第三个是只给结论不给理由。要求别人改，至少说明为什么这样更好，否则作者只是机械照做，下一次仍会写出同样的问题。

还有一种隐性成本是审批太慢。改动挂三天才能合入，作者会倾向于攒更大的提交，评审质量进一步下降，形成循环。把响应时间控制在半天内，比提高单次评审强度更有效。

下次评审可以这样开始

先花两分钟搞清这次改动要解决什么问题，再看代码是否符合这个目标。按边界处理、数据变更、可读性与测试三类顺序过一遍，格式问题交给自动检查工具。

在评审意见里保留一条习惯：至少提出一个关于失败路径的问题。这个问题看似简单，却能在很长时间内持续拦住那些最难排查的线上故障。