S16-B06 · 代码评审入门：怎么提、怎么收

评审意见提了一堆，作者全改了，合并后发现改错了方向：评审者想说的是"这个查询在循环里会放大到 N+1"，作者理解成"循环写法不好看"，于是把 for 换成 stream，问题原地不动。评审的失效点很少在"看不出来"，多数在意见没被转成可执行的动作。这篇讲怎么提、怎么收这两件事的手艺。

1. 一条模糊意见的解剖

❌ "这里性能不好，建议优化一下"
作者能接收到的信息：这里有性能问题（不可定位、不可验证、不可判定完成）

✅ "OrderService.java:78，循环里每次迭代调 orderMapper.selectById，
   100 单就是 100 次往返（典型量级：单次 1-3ms，合计 0.1-0.3s）。
   建议改成 selectByIds 批量查一次再在内存里 groupBy。
   验证：本地造 100 条明细，看 SQL 日志条数从 100 降到 1。"

要素	模糊意见	可执行意见
位置	无	文件:行
机制	"不好"	"循环内单查，放大成 N 次"
后果	无	"100 单 → 100 次往返"
替代方案	无	批量查 + 内存聚合
验证方式	无	SQL 条数从 100 降到 1

五项缺一项，作者就得猜；缺三项，作者就会自己发明一个改法。

2. 评审要看四件事，不看两件事

看什么	具体问句	典型问题
正确性	边界、空值、并发、异常路径完整吗	分页 offset 溢出、`null` 未处理
可维护性	半年后的人能只读这个函数就懂吗	200 行的方法、魔法数字
一致性	和现有约定一致吗	别处用 `Result<T>`，这里直接返 `Map`
风险	失败后果与可回滚性	新增外部调用没超时、DDL 不可逆

不该在评审里争	为什么	该去哪
个人风格（命名偏好）	无客观判据，纯消耗	交给格式化/lint 自动裁决
已达成共识的架构	评审不是重新开会的场合	设计评审阶段已定，改动走新提案
与本次改动无关的旧代码	会把 PR 变成重构工程	单独开 issue

3. 顺序：先看意图，再看结构，最后逐行

第 1 遍（2 分钟）读 PR 描述与 commit message
  → 回答：这次改动想解决什么？范围是否单一？
  → 描述缺失或范围混乱，直接退回补充，不必逐行看

第 2 遍（5 分钟）只看改动涉及的文件清单与 diff 的"骨架"
  → 回答：分层对吗？有没有跨层调用、绕过既有抽象？
  → 结构错的话，逐行意见全是白写

第 3 遍（按行数）逐行看逻辑
  → 只在这一遍提具体行级意见

顺序颠倒的代价很实在：先逐行抠细节，最后发现整个函数该删，前面的意见全废。

4. 意见必须带级别

级别	标记写法	含义	作者可否不采纳
阻塞	`[blocking]`	合并前必须处理	否，除非给出反证
建议	`[suggestion]`	更好，但不挡合并	可以，说明理由即可
提问	`[question]`	我没看懂，不是批评	回答即可，不必改代码
赞赏	`[nit+]`	这段写得好，值得别人学	—

不带级别的后果有一个固定模式：作者把提问当阻塞、把建议当命令，于是 PR 延期三天、代码被改得面目全非，而真正阻塞的那条反而被埋在二十条评论里。

5. 怎么写才可执行

模板四句：
1. 位置：OrderService.java:78（create 方法内 for 循环）
2. 现状与后果：每次迭代一次 selectById，100 条明细 = 100 次往返
3. 替代方案：selectByIds 一次查回，内存 groupBy 后组装
4. 验证方式：SQL 日志里该方法的执行次数 100 → 1

第 4 句是分水岭。没有验证方式，意见就只是观点；有验证方式，意见可以被证伪——作者也能拿着它反驳你。

6. "为什么不"与"为什么不这样"

问法	隐含前提	对方的反应
"为什么不加缓存？"	你应该加，你没加	防御：解释一堆理由
"为什么不这样：在 X 处加缓存？"	我提一个具体方案	讨论方案本身
"这里加缓存会引入什么？"	我不确定取舍	一起算代价
"你是怎么权衡的？"	承认对方可能有信息	愿意讲真实约束

差别不在礼貌，在是否把结论预设进去。带预设的问句会触发辩护，不带预设的问句才会换来信息。

7. 小 PR 的价值

PR 规模	评审行为	缺陷检出
< 200 行	能逐行读完，能本地跑	高
200-400 行	开始跳跃式阅读	中
400-1000 行	只看关键文件	低
> 1000 行	"看起来没问题，通过"	接近零

200 行不是规矩，是注意力预算：一次能维持深度阅读的 diff 大概这个量级（团队经验，非精确阈值）。巨型 PR 的真实成本不是评审慢，而是评审变成盖章——盖了章，风险就转移到线上。

8. 评审者的时间分配

改动类型	时间占比建议	关注点
核心链路、并发、事务	高	逐行 + 自己跑一遍
数据迁移、DDL	高	可逆性、锁、回滚脚本
配置、依赖升级	中	版本兼容、默认值变化
样板代码（getter、DTO、注释）	极低	扫一眼即可
格式化、重命名	极低	相信工具

原则：按风险分配注意力，不按行数。一个 20 行的锁改动值得看十分钟，一个 800 行的格式化 PR 值得看十秒。

9. 作者的义务

义务	具体动作	不做会怎样
自审	提交前自己通读一遍 diff	评审者替你做第一遍，浪费最高成本的时间
描述意图	写"为什么改"，不是"改了什么"	评审者从 diff 反推意图，经常推错
拆小	一个 PR 一个目的	巨型 PR 无人深读
回复每条	采纳说改了什么，不采纳说为什么	评审者不知道自己的意见是否被理解

PR 描述最小模板（四行足够）：
为什么：deduct 重试导致库存重复扣减（issue #412）
改什么：create 内加 orderNo 幂等占位，失败路径释放
怎么验：并发 20 请求同 orderNo，库存只减 1；OrderServiceTest 全绿
风险：Redis 不可用时降级为直接放行，需确认业务可接受

10. 分歧怎么收

第 1 步 回到判据：有没有测试、指标、日志能裁决？
        有 → 用数据，不用嗓门
第 2 步 无判据 → 拉第三人（不是拉领导，是拉熟悉这块的人）
第 3 步 仍不一致 → 按团队既有约定执行，并记录"这是约定，不是最优"
第 4 步 事后补文档：把这次分歧与结论写进 ADR / 代码注释

最常见的坏收尾是"算了，你怎么写都行"。它省钱但亏信息：同样的分歧三个月后会再来一次，而且没人记得上次为什么那么定。

11. 评审不替代测试

手段	能发现	发现不了
人工评审	意图偏差、结构问题、约定不一致	边界条件、并发时序、性能拐点
单元测试	逻辑分支、回归	跨服务契约、真实数据分布
集成/契约测试	接口兼容、序列化差异	代码可读性
静态检查	空指针模式、资源泄漏、风格	业务语义错误

评审与测试是互补的两张网，孔的大小不同。用"我评审过了"替代测试，等于用肉眼替代量具。

12. 异步评审的时效

PR 等待时长	现实状况	建议
< 4 小时	上下文还在，能一次评完	理想区
1 天	作者已切到别的任务	可接受上限
3 天	作者忘了细节，改动开始叠加	风险区
> 1 周	diff 与主干严重偏离，合并冲突频发	基本要重做

超一天的 PR 会"腐烂"，腐烂的不是代码，是双方的上下文。对策不是催，而是把大改动切成当天能评完的尺寸，并约定评审响应时间（如工作时间内 4 小时首响）。

13. 最小示例：一条模糊意见改成三条

原始意见（不可执行）
"这个事务边界有问题，建议再想想。"

改写（三条，各带级别与验证）
[blocking] OrderService.java:52 事务注解在 private 方法上，
  Spring 代理不拦截同类内部调用，事务实际未生效。
  验证：在事务内抛异常，检查库存是否回滚（当前不会回滚）。
  方案：把 @Transactional 提到对外方法 create 上，或注入自身代理。

[suggestion] 建议把 Redis 幂等占位的释放放到 finally，
  当前 catch 分支漏了释放，异常时 key 会留到过期。

[question] 这里的 retry 次数 3 是拍的吗？有没有依赖下游超时配置？
  如果下游超时 2s，3 次重试最坏 6s，可能超过网关超时。

三条意见分别对应：能挡住合并的缺陷、可以商量的改进、需要对方信息的问题。作者看一眼就知道先处理哪个。

14. 排错

现象	原因	处理
意见多但作者没改对	意见缺位置或替代方案	按四句模板重写
作者改了无关文件	没写范围约束	明确"只改 X"
PR 卡在评审三天	尺寸过大或无人认领	拆分 + 指定评审人 + 约定首响
评审变成盖章	diff 太大，注意力耗尽	硬性限制 PR 行数
同一问题反复出现	只改了代码没沉淀规则	写进 lint 规则或团队约定
作者与评审者僵住	没有可裁决的判据	补测试/基准，或拉第三人
评论里互相误解	文字缺语气	关键分歧转语音，结论回写评论

15. 边界

情况	更合适的做法
架构方向本身有问题	回到设计评审，不在 PR 里掰
需要全量重构才合理	单独立项，PR 只做可合并的一步
紧急线上修复	先止血，事后补一次完整评审并留记录
生成代码、第三方代码	只审生成规则与引入理由，不逐行审产物
纯格式改动	交给格式化工具，人不看

一句话：代码评审管的是这次改动能不能安全合并，不管"这个系统该不该这么设计"。

16. 自测

一条可执行的评审意见最少要包含哪几项？
为什么不带级别的意见会让 PR 延期？
先逐行看细节、再看改动意图，代价是什么？
"为什么不加缓存"和"为什么不这样加缓存"，差别在哪？
200 行这个数字背后是什么约束？
评审和测试各自网眼的大小差在哪，为什么不能互相替代？