Back to Details

engineering-code-review

Compare original and translation side by side

🇺🇸

Original

English
🇨🇳

Translation

Chinese

Engineering Code Review

工程代码评审

Use code review to protect and improve long-term code health while still allowing useful changes to land. The standard is not perfection; the standard is that the change improves the system overall and does not introduce unacceptable risk.
Adapted from Google Engineering Practices Documentation, especially the code review overview, reviewer guide, standard of review, navigation, speed, small changes, and emergency guidance. Sources: https://google.github.io/eng-practices/, https://google.github.io/eng-practices/review/reviewer/standard.html, https://google.github.io/eng-practices/review/reviewer/navigate.html, https://google.github.io/eng-practices/review/reviewer/speed.html, https://google.github.io/eng-practices/review/developer/small-cls.html, and https://google.github.io/eng-practices/review/emergencies.html. License: CC-BY 3.0.
代码评审的目的是在允许有用变更落地的同时，保护并提升代码的长期健康度。评审标准并非追求完美，而是确保变更从整体上改进系统，且不会引入不可接受的风险。
本文改编自谷歌工程实践文档，尤其是代码评审概述、评审者指南、评审标准、导航、速度、小型变更及紧急情况指南。来源：https://google.github.io/eng-practices/, https://google.github.io/eng-practices/review/reviewer/standard.html, https://google.github.io/eng-practices/review/reviewer/navigate.html, https://google.github.io/eng-practices/review/reviewer/speed.html, https://google.github.io/eng-practices/review/developer/small-cls.html, 以及 https://google.github.io/eng-practices/review/emergencies.html。许可证：CC-BY 3.0。

Review Standard

评审标准

  • Favor approval when the change definitely improves the code health of the touched system, even if it is not perfect.
  • Do not approve changes that make the system worse unless this is a true emergency and the follow-up cleanup is explicit.
  • Prefer technical facts, data, existing project standards, and durable design principles over taste.
  • Treat style guides and automated formatters as authorities. Personal style preferences are nits.
  • If several designs are defensible, accept the author's preference unless it harms code health.
  • Ask for another qualified reviewer when the change touches specialized areas such as security, privacy, concurrency, accessibility, internationalization, infrastructure, data migration, or domain logic you cannot validate.
  • 若变更确实能提升所涉及系统的代码健康度，即便不够完美，也应倾向于批准。
  • 除非是真正的紧急情况且后续清理计划明确，否则不得批准会导致系统状况变差的变更。
  • 优先依据技术事实、数据、现有项目标准和持久化设计原则，而非个人偏好。
  • 将风格指南和自动格式化工具视为权威，个人风格偏好属于细枝末节的问题。
  • 若多种设计方案均合理，除非会损害代码健康度，否则应接受作者的选择。
  • 当变更涉及你无法验证的专业领域（如安全、隐私、并发、无障碍、国际化、基础设施、数据迁移或领域逻辑）时，请求其他合格评审者参与。

Review Workflow

评审工作流

  1. Establish scope.
    • Identify the diff, requested review scope, target branch, and whether you are doing a full or partial review.
    • If partial, state exactly which files, behaviors, or concerns you reviewed.
  2. Gather review material.
    • For a GitHub PR, inspect the PR description, linked issue/design docs, changed files, existing review comments, CI status, and relevant commit history before commenting.
    • For a local review, inspect 
      git status --short
      , the target branch or base SHA when known, and the local diff.
    • If there is no visible diff or PR context, ask for it instead of inventing a review.
  3. Take the broad view first.
    • Read the description, issue, tests, and surrounding context.
    • Decide whether the change should exist in this codebase now.
    • If the direction is wrong, say so early and suggest the better path.
    • If the change is too large to review thoroughly, request a split or a reviewer-approved large-change plan before attempting a shallow full review.
  4. Review the main design before details.
    • Find the core files or APIs that define the change.
    • Review architecture, ownership boundaries, data flow, compatibility, and rollback risk before naming or formatting comments.
    • Send major design concerns immediately if they invalidate the rest of the review.
  5. Review every human-written line in your assigned scope.
    • Generated files, large data files, or mechanical output can be sampled when appropriate.
    • If code is too hard to understand, request simplification or clearer structure before approval.
  6. Check the change against the review checklist below.
  7. Check verification evidence.
    • Prefer existing CI and targeted tests when the user did not ask you to run commands.
    • Run local tests only when the environment and request make that appropriate; otherwise state which tests are missing or unverified.
    • Do not claim tests passed unless you saw the command output or CI result.
  8. Make a verdict.
    • APPROVE
      : code health improves and remaining comments are non-blocking.
    • APPROVE_WITH_COMMENTS
      : remaining comments are minor, optional, or the author can be trusted to handle them without another review round.
    • REQUEST_CHANGES
      : correctness, design, maintainability, test, documentation, or risk issues remain.
    • NEEDS_SPECIALIST
      : another reviewer must cover a domain you cannot responsibly assess.
  1. 确定评审范围。
    • 明确diff、请求的评审范围、目标分支，以及你要进行全面评审还是部分评审。
    • 若为部分评审，需明确说明你评审了哪些文件、行为或关注点。
  2. 收集评审材料。
    • 针对GitHub PR，在发表评论前，先查看PR描述、关联的问题/设计文档、变更文件、已有的评审评论、CI状态及相关提交历史。
    • 针对本地评审，查看
      git status --short
      、目标分支或已知的基准SHA，以及本地diff。
    • 若没有可见的diff或PR上下文，应索要相关材料，而非凭空进行评审。
  3. 先从宏观视角入手。
    • 阅读描述、问题、测试及相关背景信息。
    • 判断该变更当前是否应存在于代码库中。
    • 若方向错误，应尽早指出并建议更优路径。
    • 若变更过大无法全面评审，应要求拆分或制定经评审者认可的大型变更计划，再尝试进行粗略的全面评审。
  4. 先评审核心设计，再关注细节。
    • 找到定义变更的核心文件或API。
    • 在关注命名或格式问题之前，先评审架构、所有权边界、数据流、兼容性及回滚风险。
    • 若存在重大设计问题会影响后续评审，应立即提出。
  5. 评审你负责范围内的每一行人工编写的代码。
    • 生成文件、大型数据文件或机械输出可酌情进行抽样检查。
    • 若代码难以理解，应要求简化或优化结构后再批准。
  6. 根据下方的评审检查清单验证变更。
  7. 检查验证证据。
    • 若用户未要求你执行命令，优先依赖现有CI和针对性测试。
    • 仅当环境和请求合适时才运行本地测试；否则说明缺少哪些测试或哪些内容未经验证。
    • 除非你看到命令输出或CI结果，否则不要声称测试已通过。
  8. 给出评审结论。
    • APPROVE
      ：代码健康度得到提升，剩余评论不影响批准。
    • APPROVE_WITH_COMMENTS
      ：剩余评论为次要、可选内容，或作者无需再次评审即可自行处理。
    • REQUEST_CHANGES
      ：仍存在正确性、设计、可维护性、测试、文档或风险方面的问题。
    • NEEDS_SPECIALIST
      ：需要其他评审者覆盖你无法负责任评估的领域。

Review Checklist

评审检查清单

  • Design: Does the change belong here? Is the abstraction appropriate for the current system?
  • Functionality: Does it do what the author intends, and is that behavior good for users and future developers?
  • Edge cases: Are failures, empty states, retries, idempotency, ordering, time, permissions, and concurrency considered?
  • Complexity: Is it understandable quickly? Is it solving today's known problem rather than a speculative future one?
  • Tests: Are unit, integration, or end-to-end tests appropriate for the risk? Would the tests fail if the code were broken?
  • Test quality: Are assertions meaningful, focused, deterministic, and maintainable?
  • Naming: Do names communicate purpose without being vague or noisy?
  • Comments: Do comments explain why, constraints, or non-obvious algorithms instead of restating the code?
  • Documentation: Are READMEs, API docs, migration notes, runbooks, or generated docs updated when behavior or usage changes?
  • Style and consistency: Does the change follow project conventions without mixing unrelated formatting churn into functional work?
  • Context: Does the change improve the system around it, or does it add another small piece of long-term complexity?
  • Operations: Are deployment, migration, observability, rollback, feature flags, and failure handling adequate for the change?
  • Security: Are authorization, authentication, secrets, injection, dependency, deserialization, and privilege-boundary risks handled?
  • Privacy: Does the change avoid unnecessary collection, logging, exposure, retention, or cross-boundary movement of sensitive data?
  • Accessibility: For user interfaces, are keyboard access, semantics, focus, contrast, labels, and assistive-technology behavior preserved?
  • Internationalization: Are locale, timezone, encoding, text expansion, pluralization, sorting, and translated strings handled where relevant?
Escalate to a specialist when you cannot confidently assess security, privacy, accessibility, internationalization, concurrency, infrastructure, data migration, or domain-specific correctness.
  • 设计：该变更是否属于此处？抽象程度是否适合当前系统？
  • 功能：是否实现了作者的预期？该行为对用户和未来开发者是否有益？
  • 边缘情况：是否考虑了故障、空状态、重试、幂等性、顺序、时间、权限和并发问题？
  • 复杂度：是否易于快速理解？是否解决的是当前已知问题而非推测性的未来问题？
  • 测试：针对该风险，单元测试、集成测试或端到端测试是否合适？若代码出现问题，测试是否会失败？
  • 测试质量：断言是否有意义、聚焦、确定且易于维护？
  • 命名：命名是否能传达用途，既不模糊也不冗余？
  • 注释：注释是否解释了原因、约束或非显而易见的算法，而非重复代码内容？
  • 文档：当行为或用法变更时，README、API文档、迁移说明、运行手册或生成文档是否已更新？
  • 风格与一致性：变更是否遵循项目约定，未在功能性工作中混入无关的格式调整？
  • 上下文：变更是否改进了周边系统，还是增加了长期的小复杂度？
  • 运维：部署、迁移、可观测性、回滚、功能标志和故障处理是否能满足变更需求？
  • 安全：是否处理了授权、认证、密钥、注入、依赖、反序列化和权限边界风险？
  • 隐私：是否避免了不必要的敏感数据收集、日志记录、暴露、保留或跨边界传输？
  • 无障碍：针对用户界面，是否保留了键盘访问、语义化、焦点、对比度、标签及辅助技术行为？
  • 国际化：相关场景下是否处理了区域设置、时区、编码、文本扩展、复数形式、排序及翻译字符串？
当你无法自信评估安全、隐私、无障碍、国际化、并发、基础设施、数据迁移或领域特定正确性时，请寻求专家协助。

Comment Severity

评论严重程度

  • BLOCKER
    : must be fixed before approval.
  • IMPORTANT
    : should be fixed before approval unless there is a written rationale.
  • QUESTION
    : answer needed before final verdict.
  • NIT
    : polish that must not block approval.
  • OPTIONAL
    : suggestion the author may decline.
  • FYI
    : learning or future consideration, not a requested change.
  • BLOCKER
    ：批准前必须修复。
  • IMPORTANT
    ：除非有书面理由，否则批准前应修复。
  • QUESTION
    ：给出最终结论前需要得到答案。
  • NIT
    ：优化问题，不得阻碍批准。
  • OPTIONAL
    ：作者可拒绝的建议。
  • FYI
    ：仅供学习或未来参考，不要求变更。

Output Format

输出格式

Lead with findings, highest severity first. Use file and line references when available.
markdown
undefined
先列出发现的问题，按严重程度从高到低排序。如有可用的文件和行号引用，请一并使用。
markdown
undefined

Findings

发现的问题

  • [BLOCKER] path/file.ext:123 - Problem. Why it matters. Required change.
  • [IMPORTANT] path/file.ext:45 - Problem. Why it matters. Suggested fix.
  • [BLOCKER] path/file.ext:123 - 问题描述。说明影响。要求的变更。
  • [IMPORTANT] path/file.ext:45 - 问题描述。说明影响。建议的修复方案。

Open Questions

待解决问题

  • ...
  • ...

Verdict

评审结论

REQUEST_CHANGES | APPROVE_WITH_COMMENTS | APPROVE | NEEDS_SPECIALIST
REQUEST_CHANGES | APPROVE_WITH_COMMENTS | APPROVE | NEEDS_SPECIALIST

Notes

备注

  • Scope reviewed:
  • Tests/commands checked:
  • Follow-ups:

If no issues are found, say that clearly and still mention any unverified scope or residual risk.
  • 评审范围：
  • 检查的测试/命令：
  • 后续工作：

若未发现问题，需明确说明，同时提及任何未验证的范围或残留风险。

Common Mistakes

常见错误

  • Blocking approval on preference rather than code health.
  • Reviewing only the changed lines when the surrounding function or module makes the change unsafe.
  • Accepting complex code because the author explained it only in the review thread.
  • Letting "clean it up later" pass when the change itself introduces the complexity.
  • Requesting large rewrites late instead of flagging design problems early.
  • Delaying the author when a quick broad response would unblock useful work.
  • 因个人偏好而非代码健康度阻碍批准。
  • 仅评审变更行，忽略周边函数或模块可能导致的变更风险。
  • 因作者仅在评审线程中解释了复杂代码就予以接受。
  • 当变更本身引入复杂度时，允许“以后再清理”的理由通过。
  • 后期才要求大幅重写，而非尽早指出设计问题。
  • 本可快速给出宏观反馈以推进工作，却拖延作者进度。