代码审查时我在看什么——一个 Senior 工程师的 CR 清单

一、先说一个反直觉的事实

做了六年代码审查，我越来越觉得：CR 看的不是代码，是人。

每一行代码背后都站着一个活生生的开发者——他当时的状态、他对这个模块的理解深度、他对系统整体的认知边界，全藏在 diff 里。

所以我的 CR 清单分两层：技术层和人性层。技术层保证代码正确，人性层保证团队可持续。

二、技术层：我的五步扫描法

每次打开一个 PR，我按固定顺序扫五遍，每遍只看一件事。

第一遍：看结构，不看细节

先不读代码逻辑，只看：

• 改了几个文件？超过 10 个文件我就会警觉——是不是一次做了太多事？
• 文件之间的依赖方向对不对？有没有出现循环依赖或上层依赖下层？
• 新增了哪些类/模块？命名能不能让我不看实现就猜出它干什么？

这一步的目标是判断：这个 PR 的架构意图是否清晰。如果扫完第一遍我还说不清作者想干什么，这个 PR 就该打回重拆。

第二遍：看接口和契约

• 新增/修改的 API，参数和返回值有没有校验？
• 错误码定义是否一致？有没有随手扔个 500 Internal Server Error 敷衍了事？
• 数据库字段变更有没有考虑向后兼容？老数据怎么迁移？
• 如果这是公共库改动，有没有破坏现有调用方？

接口是承诺。代码可以改，接口一旦发布就是债务。

第三遍：看核心逻辑

• 边界条件处理了吗？空值、零值、超长输入、并发重复提交
• 有没有硬编码的魔法数字？（if retry > 3 里的 3 为什么是 3？）
• 循环里有没有隐藏的 O(n²) 甚至 O(n³)？
• 事务边界对不对？有没有"查完数据到写入之间"的竞态窗口？

这一步最费时间，但也最值钱。我通常会挑改动中最核心的一两个函数，逐行读。

第四遍：看测试

• 有没有测试？没有测试的 PR 我默认不合并（除非是纯配置或文档）
• 测试覆盖的是 happy path 还是异常路径？
• 测试数据是不是写死的？能不能随着代码演化自动适应？
• 有没有测试"不该发生的事"？比如"并发情况下不应该出现重复订单"

我特别看重一个指标：这个测试能不能在我改坏代码时抓住问题？​ 如果答案是"不能"，那这测试就是装饰品。

第五遍：看安全和可观测性

• 有没有把敏感信息打到日志里？（密码、token、身份证号）
• SQL 有没有注入风险？用参数化查询了吗？
• 有没有加必要的日志和监控埋点？出问题能不能快速定位？
• 外部调用有没有超时和熔断？

这一遍经常被忽略，但恰恰是线上事故的主要来源。

三、人性层：比技术更重要的三件事

1. 看作者是否"被逼着走了捷径"

有时候我看到一段明显不够优雅的代码，不会直接说"这样写不好"。我会先问：

"这个实现是有什么约束条件吗？时间紧？依赖没就绪？还是我漏掉了什么上下文？"

很多时候答案是："我知道这样不太好，但 deadline 是今天下午。"

这时候我的 CR 评论会变成：

"先合并，但开一个 follow-up issue，下周我们一起重构这块。我建议的方案是……"

好的 CR 不是挑刺，是帮作者找到他当时看不到的更好路径。

2. 看新人是否在"模仿坏模式"

新人写的代码里，如果复刻了仓库里已有的坏模式（比如全局变量、上帝类、复制粘贴的重复代码），我不会只改他这一处。

我会评论：

"这块逻辑确实可以这么写，但仓库里有个更好的模式在 xxx 模块，建议参考那个。另外我也开了个 issue 打算把旧代码统一重构一下。"

新人模仿坏模式，责任在 reviewer——因为你没给他更好的参照物。

3. 看评论本身是否在"建立文化"

我的每一条 CR 评论都在传递一个信号：

• "这里加个测试" → 我们重视质量
• "这个命名再想想" → 我们重视可读性
• "这个边界情况处理得漂亮" → 我们认可好工作
• "不确定我的理解对不对，你的意图是 xxx 吗？" → 我们保持谦逊

CR 是团队文化的放大器。你用什么标准审代码，团队就会用什么标准写代码。

四、我的 CR 评论分类法

为了减少"被挑刺"的挫败感，我把评论分成三类，用不同前缀标注：

这样作者一眼就知道哪些必须改、哪些可以商量、哪些只是我的个人偏好。

五、几个让我印象深刻的 CR 瞬间

那次一个实习生提交了一段"完美"的代码。

逻辑正确、测试齐全、命名规范。但我注意到他用了一种非常"学院派"的设计模式——在我们这个快速迭代的项目里，过度设计了。我在 CR 里写了一句：

"代码写得很漂亮，但这个场景其实一个 if-else 就够了。等我们真的需要扩展到三种以上策略时，再引入策略模式不迟。"

他后来告诉我，那句话让他第一次理解了"简单优于复杂"不是一句口号。

那次我漏掉了一个并发 bug。

代码 review 通过了，上线后在高并发下出现了重复数据。根因是 check-then-act 竞态。我后来在团队里立了个规矩：

所有涉及"先查后写"的逻辑，CR 时必须显式标注并发安全策略（数据库唯一索引 / 分布式锁 / 乐观锁），没标注的一律打回。

六、写在最后

好的代码审查，本质上是在做一件事：

在代码合入的那一刻，确保它不仅能跑，而且能被下一个接手的人理解和维护。

CR 不是考试，不是挑刺，不是权力展示。它是团队里最日常、也最有效的知识传递机制。

你审过的每一行代码，都在塑造团队的代码品味。你给的每一条评论，都在告诉作者："我们是谁，我们怎么做事，我们重视什么。"