Code review

Code Review

概述

对代码做有重点、分层次的审查——先抓正确性和安全问题，再管可读性，风格交给工具。核心：好的 review 让作者知道"哪里有真问题、该先改什么"；坏的 review 是一堆 nit 淹没重点，或一句"看起来不错"放过错误。

何时使用

• 提交前审查自己或别人的代码（自我 review 见下文专门技巧）
• 同事让你看他的 PR
• 不确定 review 该看什么、怎么反馈

不该用：纯风格/格式问题（交给 linter/formatter 自动化，不该占用人 review）；想确认"功能对不对"——那是 verify-and-fix（实际跑）的事，review 是静态审查、替代不了运行验证。

与相邻 skill 的边界：code-review 是审查视角（找问题、分严重度、给建议），verify-and-fix 是修复视角（改对、验证）。code-review 发现的问题，需要修的进入 verify-and-fix 流程；review 中发现的"行为不对/报错"可能要先用 debugging 定位根因。三者接力：review 找问题 → debugging 定位 → verify-and-fix 修+验证。

核心内容

正确性优先，别被风格带偏

review 最常见的错是逐行挑风格（命名、缩进、注释多少）而放过正确性（逻辑对不对、边界处理了吗、有没有竞态）。正确性问题会让程序出错，风格问题只是不好看——前者 critical，后者 nit，优先级天差地别。

按重要性排序，review 该查的维度：

1. 正确性：逻辑对吗？能跑通预期路径吗？有没有逻辑漏洞？（最该花时间）
2. 边界与异常：空值/空集合/零/负数/超大输入怎么处理？外部依赖失败（网络/DB）呢？
3. 安全：有没有注入（SQL/命令/XSS）？密码/密钥处理对吗？权限检查到位吗？敏感信息泄露吗？
4. 并发：共享状态有竞态吗？异步顺序依赖对吗？资源泄漏（未关闭的连接/锁未释放）？
5. 可维护性：命名清不清楚？结构是否过度复杂？有没有重复？（这里才是 nit 的领地）
6. 测试：有测试吗？测的是行为还是实现？覆盖了关键路径和边界吗？

前 4 类是"会让程序出错或出事"的，必须查；第 5 类是"让人难受"的，次要；风格细节（缩进/格式）不该人查，交给工具。

按改动性质调整重点：上面是通用清单，但不同代码该重点查的不同——别对所有代码平均用力。涉及钱/库存/计数的，重点查原子性和一致性（中途失败会不会凭空产生/消失）；涉及外部输入的（用户输入、API、文件），重点查安全（注入、越权）；涉及共享状态/异步的，重点查竞态和资源泄漏；涉及配置/迁移的，重点查回滚和兼容。先识别"这段代码的风险面在哪"，把 review 力量集中投到那里。

分严重度，别把 nit 和 critical 混着

review 反馈必须分严重度，让作者知道先改什么：

• 阻塞（blocking / critical）：必须改才能合并——逻辑错、安全漏洞、会崩溃、数据丢失风险。
• 重要（important）：强烈建议改——边界没处理、缺少测试、设计有隐患，但不阻塞本次合并。
• 建议（nit / suggestion）：可选——命名、可读性、小重构。改了更好，不改也能过。

不分层的 review 有两种失败：把 nit 当 critical（作者被一堆小事压垮，反而漏改真问题）、把 critical 当 nit（真问题被淹没在风格意见里）。nit 要克制——堆 15 条 nit 是在浪费作者时间，把同类 nit 归并成一条"风格建议"，或直接交给 linter。

反馈要带依据和建议，不只是"这不好"

每条 review 意见应该让作者能理解问题 + 知道怎么改：

• 指出问题 + 为什么是问题：不说"这写得不好"，说"user.name 在 user 为 null 时会抛 TypeError——db.find 找不到时返回 null"。
• 给方向或示例：不只说"改一下"，给"加个 null 检查，找不到时抛业务错误或返回 null，看调用方期望"。
• 区分事实和偏好："这里有 null 风险"（事实，基于代码）vs "我觉得该用 early return"（偏好，标注是建议）。

带依据的反馈让作者能判断对错、学到东西；空泛的"不好"让作者只能盲从或抵触。

别只看 diff，要看整体

review 容易陷入"逐行看 diff"，但很多问题在整体层面才看得出来：

• 这个改动和系统的其他部分一致吗（命名约定、错误处理风格、架构分层）？
• 改动有没有破坏既有契约（改了函数签名，调用方都更新了吗）？
• 缺了什么（diff 里只有 happy path，错误处理呢？只有功能代码，测试呢？）？

diff 告诉你"改了什么"，但要结合"整体该怎么"才能看出"改对了吗、改全了吗"。

大 PR：分块 + 抓主线，别试图一口气读完

大改动（几百行、多文件）的 review 容易陷入"看不完、看到后面忘前面"。策略：

• 先读 PR 描述/commit message：作者说这次改了什么、为什么——这是主线，review 时随时对照"改动是否服务这个主线、有没有跑题"。
• 按逻辑分块，不按文件：把改动按"功能模块"分组（如"认证部分""数据迁移""UI"），一块一块审，每块审完总结"这块改了啥、有没有问题"，再下一块。
• 抓主线正确性，细节分层：先确认主线逻辑通（核心路径对不对），再下沉到边界/异常。主线错了一切白搭；主线对了再逐块找细节问题。
• 太大就要求拆：如果一个 PR 改了互不相关的多件事，要求作者拆成几个 PR——大而杂的 PR 几乎无法有效 review，这不是 reviewer 的问题，是 PR 的问题。

自我 review：换视角，先跑后看

提交前自查自己的代码，有两个反直觉但有效的技巧：

• 放一会儿再看 / 换视角：刚写完时代码在大脑的"短期记忆"里，你会自动补全没写好的地方、跳过自己的盲点。放几小时（或睡一觉）再看，或假装在 review 别人的代码——视角一换，自己的问题就显形了。
• 先用工具跑，再用人眼看：先跑测试/lint/类型检查，让工具抓住机械问题（编译错、类型错、明显的 lint）；人眼集中查工具抓不到的——逻辑对不对、边界处理了吗、命名清不清楚。工具和人各查擅长的，别让人干工具的活。
• 对照 commit message 自查：你这次提交说"修了 X"，那 diff 里应该只有修 X 相关的改动——混进去的无关改动（顺手重构、调试代码）会被这个对照揪出来。

自我 review 的产出标准同对外 review：说出查了什么维度、发现什么，而不是"我看过了没问题"。

常见错误

问题	修法
表面附和"看起来不错"	列出实际查了哪些维度、发现什么，空 review 等于没 review
逐行挑 nit 淹没正确性	正确性/安全/边界优先，nit 克制并归并
不分严重度，nit 和 critical 混着	分阻塞/重要/建议三层，让作者知道先改什么
只说"不好"不说怎么改	每条带"为什么是问题 + 改的方向/示例"
只看 diff 逐行	结合整体：一致性、契约破坏、缺了什么
风格问题占用人 review	交给 linter/formatter，人查判断性问题
只看功能不看测试	查测试是否存在、测的是行为还是实现、覆盖关键路径
大 PR 一口气读，读到后面忘前面	先读描述抓主线，按逻辑分块审，太大就要求拆 PR
自我 review 刚写完就看	放一会儿/换视角，先用工具跑再人眼看，对照 commit message 查混入的无关改动