ninehills 代码审查 (Code Review)
Overview
基于 Google 工程实践整理的代码审查指南。首要目的：确保代码库的整体代码健康状况随时间推移而改善。 不存在"完美"代码，只有更好的代码——追求持续改善，而非追求完美。
评审者应在 CL 明确改善系统代码健康状态时予以通过，即使 CL 并不完美。
三大原则：
- 技术事实和数据压倒意见和个人偏好。
- 风格问题上，风格指南是绝对权威。 任何不在风格指南中的纯风格问题属于个人偏好，应保持与现有代码一致。
- 软件设计几乎从来不是纯风格问题或个人偏好。 基于底层原则衡量，而非仅凭个人意见。
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 删除或废弃代码，考虑文档是否也应删除。
评论写法
三项要点
- 解释原因——让开发者理解评论的意图、遵循的最佳实践、或建议如何改善代码健康。
- 平衡直接指导和指出问题让开发者自己决定——指出问题让开发者决策，但有时直接指令、建议甚至代码更有帮助。
- 鼓励开发者简化代码或添加代码注释，而非依赖于在审查工具中做解释。审查工具中的解释对未来代码读者没有帮助。
评论严重程度标签
| 标签 | 含义 |
|---|---|
| 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
- 只关注表面差异——应审查变更的意图和设计，而非只是 diff 的语法正确性。
- 跳过测试审查——测试的质量和覆盖率同等重要，测试也是代码。
- 接受"以后再清理"——几乎不会发生。坚持现在处理。
- 风格偏好冒充设计问题——区分风格（遵循风格指南）和设计（基于原则权衡）。不确定时用 Nit 标签。
- 忽略过度工程——"以后可能需要"不是添加抽象的理由。解决现在的问题。
- 大 CL 一把审查——超过 1000 行应要求拆分，否则审查质量无法保证。
- 功能和重构混在一起——格式/重构变更应独立于功能变更，分开提交。
Verification Checklist
- CL 描述清晰传达了做什么和为什么
- 审查了全部 10 个维度（设计、功能、复杂度、测试、命名、注释、风格、文档、每一行、上下文）
- 没有降低代码健康状态的变更被接受
- 复杂度和过度工程已评估
- 测试覆盖充分、测试设计合理
- 安全信号已检查（密钥、注入、路径穿越等）
- 评论带有严重程度标签（Nit/Optional/FYI）
- "以后再清理"已被拒绝（除非有立即的后续 CL）
- 输出格式遵循标准模板（审查总结 → 关键问题 → 建议 → 好的实践 → 问题）