ninehills/review-code

代码审查 (Code Review)

Overview

基于 Google 工程实践整理的代码审查指南。首要目的：确保代码库的整体代码健康状况随时间推移而改善。 不存在"完美"代码，只有更好的代码——追求持续改善，而非追求完美。

评审者应在 CL 明确改善系统代码健康状态时予以通过，即使 CL 并不完美。

三大原则：

1. 技术事实和数据压倒意见和个人偏好。
2. 风格问题上，风格指南是绝对权威。 任何不在风格指南中的纯风格问题属于个人偏好，应保持与现有代码一致。
3. 软件设计几乎从来不是纯风格问题或个人偏好。 基于底层原则衡量，而非仅凭个人意见。

When to Use

使用场景：

• 审查代码变更、PR、diff、commit、patch
• 用户说"审查"、"review"、"CR"、"看看这段代码"、"检查这个 PR"、"审一下"
• 审计现有代码库或模块的代码质量
• 作为 requesting-code-review 审查阶段的参考标准

不适用：

• 纯文档变更、配置调整（除非涉及安全配置）
• 生成代码、数据文件、自动格式化的代码

与 requesting-code-review 的区别： 本技能定义审查标准和方法（审查什么、怎么看）。requesting-code-review 定义提交前的自动化流水线（安全扫描、基线对比、auto-fix 循环）。

审查总览

审查应覆盖以下维度：

| 维度 | 核心问题 | |------|---------| | 设计 | 代码设计是否合理、与系统适配？ | | 功能 | 行为是否符合作者意图？对用户是否有益？ | | 复杂度 | 能否更简单？未来其他开发者能否轻松理解和修改？ | | 测试 | 是否有正确、设计良好的自动化测试？ | | 命名 | 变量、类、方法等命名是否清晰？ | | 注释 | 注释是否清晰有用？注释应解释为什么而非是什么。 | | 风格 | 是否遵循风格指南？ | | 文档 | 是否同步更新了相关文档？ | | 每一行 | 人工编写的每行代码都要看。数据文件、生成代码可略过。 | | 上下文 | 在更广阔上下文中审视——整个文件、整个系统。不接受降低代码健康状态的 CL。 |

审查流程

三步法浏览 CL

第一步：整体视角

• 阅读 CL 描述，理解变更做什么、为什么做。
• 这个变更本身是否合理？如果根本不应该做，立即回复说明原因并建议替代方案。

第二步：审查核心部分

• 找到变更量最大的"主"文件，先审查这些核心部分。
• 如果发现重大设计问题，立即发送评论——不等其他部分审查完。如果设计问题严重，大量后续代码可能被废弃。

第三步：按合理顺序审查其余部分

• 确认无重大设计问题后，按逻辑顺序浏览剩余文件。
• 有时先读测试代码再读主代码有助于理解变更意图。

审查标准详解

设计审查（最重要）

• CL 中各代码模块的交互是否合理？
• 变更应该放在当前代码库还是抽到库中？
• 是否与系统其余部分良好集成？
• 现在是添加此功能的合适时机吗？

功能审查

• CL 是否实现了开发者意图？意图对用户是否有利？
• 思考边界情况、并发问题、站在用户角度思考。
• 特别关注：UI 变更（难以仅靠读代码判断影响）、并行编程（可能导致死锁或竞态条件）。

复杂度审查

逐级检查：单行 → 函数 → 类是否过于复杂？

"过于复杂" = 代码读者无法快速理解、调用或修改时容易引入 bug。

特别警惕过度工程：代码做得比需要的更通用，或添加了当前系统不需要的功能。解决现在已知的问题，而非猜测未来可能需要解决的问题。

测试审查

• 要求与变更匹配的单元测试、集成测试或端到端测试。测试应和产品代码在同一个 CL 中。
• 确保测试正确、合理、有用：
  • 代码损坏时测试会真的失败吗？
  • 底层代码变更时测试会误报吗？
  • 断言是否简单有用？
  • 不同测试方法之间是否合理分离？
• 测试也是需要维护的代码，不接受测试中的复杂度。

命名审查

好名字足够长以完全传达该项是什么/做什么，但不至于长到难以阅读。

注释审查

• 注释通常应解释为什么而非是什么。如果代码本身不够清晰，应该简化代码而非加注释。
• 例外：正则表达式和复杂算法通常需要注释解释它们在做什么。
• 查看 CL 之前的注释：是否有可移除的 TODO、是否有反对此变更的旧注释。
• 注释 ≠ 文档（类/模块/函数的文档应表达目的、用法和行为）。

风格审查

• 确保 CL 遵循相应风格指南。
• 想改进风格指南之外的内容，用 "Nit:" 前缀标注，表示非强制性。
• 不要仅基于个人风格偏好阻止提交。
• CL 不应将大规模风格变更与功能变更混在一起——格式化和功能应分开提交。

一致性

• 如果现有代码与风格指南不一致：风格指南是绝对权威，CL 应遵循指南。
• 如果风格指南是建议而非要求：倾向于遵循风格指南，除非局部不一致会造成更大混淆。

文档审查

• 如果 CL 改变了用户构建、测试、交互或发布代码的方式，检查是否同步更新了 README、文档页面和生成的参考文档。
• 如果 CL 删除或废弃代码，考虑文档是否也应删除。

评论写法

三项要点

1. 解释原因——让开发者理解评论的意图、遵循的最佳实践、或建议如何改善代码健康。
2. 平衡直接指导和指出问题让开发者自己决定——指出问题让开发者决策，但有时直接指令、建议甚至代码更有帮助。
3. 鼓励开发者简化代码或添加代码注释，而非依赖于在审查工具中做解释。审查工具中的解释对未来代码读者没有帮助。

评论严重程度标签

| 标签 | 含义 | |------|------| | Nit | 小事，技术上应该做但影响不大 | | Optional / Consider | 可能是个好主意，但不严格要求 | | FYI | 不期望在本 CL 中修改，但未来可考虑 |

使用标签让审查意图明确，帮助作者确定优先级。

关于解释

如果要求开发者解释一段代码，通常结果应该是他们重写代码使其更清晰。偶尔添加代码注释也是合适的回应（前提是不是仅仅解释过于复杂的代码）。审查工具中写的解释对未来代码读者没有帮助。

处理"以后再清理"

开发者常表示会在后续 CL 中清理——经验表明，除非在立即写清理 CL，否则很少真正发生。

• 通常最好坚持开发者在代码进入代码库之前现在清理。"以后再清理"是代码库退化的常见方式。
• 如果 CL 引入了新的复杂度，必须在提交前清理（除非紧急情况）。
• 如果 CL 暴露了周围问题且现在无法解决，开发者应提交 bug 并分配给自己，可选在代码中写 TODO 引用该 bug。

CL 指南

写好 CL 描述

CL 描述应传达：做了什么变更？ + 为什么做这些变更？

第一行规则： 简短摘要（祈使句），独立存在让未来搜索代码历史的人能快速理解。第一行后空一行。

坏的描述： "Fix bug." / "Fix build." / "Add patch." / "Phase 1."

好的描述：

RPC: 移除 RPC 服务器消息空闲列表的大小限制。

像 FizzBuzz 这样的服务器有非常大的消息，能从复用中受益。
增大空闲列表，并添加一个 goroutine 随时间缓慢释放空闲列表条目，
使空闲服务器最终释放所有空闲列表条目。

写小 CL

一个 CL = 一个自包含的变更，只解决一件事。通常 100 行合理，1000 行太大。文件分散程度也影响"大小"——一个文件中的 200 行可能 OK，散在 50 个文件中通常太大。

拆分策略：

• 基于堆叠：写完一个小 CL 发出审查后，立即基于此 CL 写下一个。
• 按文件拆分：需要不同评审者的文件分组，各自自包含。
• 水平拆分：在技术栈各层之间创建共享代码或桩，隔离变更。
• 垂直拆分：按功能拆分，每个功能是独立的全栈实现。
• 重构分离：重构独立 CL，不与功能变更或 bug 修复混在一起。
• 测试在同一个 CL：逻辑变更应包含新的或更新的测试。

AI 审查执行指南

审查准备

git diff --cached
# 如果为空
git diff HEAD~1 HEAD

审查输出格式

## 审查总结
[1-2 句整体评估]

## 设计层面
- [设计相关评论]

## 关键问题（必须修复）
- 问题 1：[描述 + 建议修复方案]
- 问题 2：...

## 建议（可选）
- 建议 1：[描述]

## 好的实践（肯定）
- [开发者做得好的地方]

## 问题
- [需要开发者澄清的问题]

常见问题速查

Python

# 差: SQL 注入风险
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# 好: 参数化查询
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

# 差: Shell 注入
os.system(f"ls {user_input}")
# 好: 安全子进程
subprocess.run(["ls", user_input], check=True)

# 差: 危险 eval/exec
eval(user_input)
# 好: 使用安全的解析方式

JavaScript

// 差: XSS 风险
element.innerHTML = userInput;
// 好: 安全文本内容
element.textContent = userInput;

需要特别关注的信号

• 硬编码的密钥、API Key、凭证
• 缺少输入验证
• SQL 查询使用字符串拼接
• 文件操作未验证路径（路径穿越风险）
• 外部调用缺少错误处理
• 遗留的调试 print/console.log
• 被注释掉的代码
• 新代码没有对应测试
• 过度工程（做了当前不需要的抽象/功能）
• 并发代码没有正确处理竞态条件
• 重构和功能变更混在同一个 CL 中
• 大规模格式化变更和逻辑变更混在一起

Common Pitfalls

1. 只关注表面差异——应审查变更的意图和设计，而非只是 diff 的语法正确性。
2. 跳过测试审查——测试的质量和覆盖率同等重要，测试也是代码。
3. 接受"以后再清理"——几乎不会发生。坚持现在处理。
4. 风格偏好冒充设计问题——区分风格（遵循风格指南）和设计（基于原则权衡）。不确定时用 Nit 标签。
5. 忽略过度工程——"以后可能需要"不是添加抽象的理由。解决现在的问题。
6. 大 CL 一把审查——超过 1000 行应要求拆分，否则审查质量无法保证。
7. 功能和重构混在一起——格式/重构变更应独立于功能变更，分开提交。

Verification Checklist

• [ ] CL 描述清晰传达了做什么和为什么
• [ ] 审查了全部 10 个维度（设计、功能、复杂度、测试、命名、注释、风格、文档、每一行、上下文）
• [ ] 没有降低代码健康状态的变更被接受
• [ ] 复杂度和过度工程已评估
• [ ] 测试覆盖充分、测试设计合理
• [ ] 安全信号已检查（密钥、注入、路径穿越等）
• [ ] 评论带有严重程度标签（Nit/Optional/FYI）
• [ ] "以后再清理"已被拒绝（除非有立即的后续 CL）
• [ ] 输出格式遵循标准模板（审查总结 → 关键问题 → 建议 → 好的实践 → 问题）