Back to Details

code-review

Compare original and translation side by side

🇺🇸

Original

English
🇨🇳

Translation

Chinese

Code Review

代码审查

Review a change for correctness first, quality second. A review is not a formatting pass — it's a search for the ways this diff is wrong or will become wrong. Approve when you'd be comfortable being paged for it at 3am.
审查变更时，先关注正确性，再关注质量。代码审查不是格式检查——而是要找出该diff存在的问题或未来可能出现的问题。当你愿意在凌晨3点为这个变更被叫醒处理问题时，再批准它。

Scope the review before reading line-by-line

逐行阅读前先明确审查范围

Don't review the diff in isolation. A diff shows what changed, not whether the change is correct.
  1. Read the description / linked issue. What is this change claiming to do? You're verifying the claim, not just the syntax.
  2. Read the surrounding code, not just the red/green lines. A removed null check looks fine in the diff; whether it's a bug depends on the caller three functions up. Open the files.
  3. Reproduce the claim mentally (or actually). Trace one real input through the new path. If the PR says "fixes the retry loop," follow a failing request through the loop and confirm it now terminates. If you can run it, run it.
  4. Check the tests changed with the code. New behavior with no new test is a finding. A test that passes against both the old and new code tests nothing.
不要孤立地审查diff。diff仅展示了变更内容，无法体现变更是否正确。
  1. 阅读描述/关联的议题。该变更声称要实现什么功能？你需要验证这个声明，而不只是检查语法。
  2. 阅读关联代码，不局限于变更行。在diff中删除空值检查看起来没问题，但这是否是bug取决于上层三个调用函数的逻辑。请打开完整文件查看。
  3. 在脑中（或实际）复现声明的功能。追踪一个真实输入在新路径中的流转。如果PR声称“修复了重试循环”，请跟随一个失败请求遍历循环，确认它现在能正常终止。如果可以运行代码，就实际运行测试。
  4. 检查随代码变更的测试。新增行为但未添加测试是问题点。同时能通过新旧代码的测试等于没有测试效果。

What to actually look for

重点审查方向

Go in this order — correctness bugs are worth more than style nits.
CategoryConcretely
Logic bugsOff-by-one, inverted conditions, wrong operator (
&&
vs 
||
), 
=
vs 
==
, swapped args, fallthrough.
Edge casesEmpty list/string, zero, negative, 
null
/
None
/
undefined
, single element, max size, duplicate keys, unicode.
Error handlingSwallowed exceptions, ignored return/error values, partial failure leaving inconsistent state, no cleanup on the error path, error message that loses the cause.
ConcurrencyShared mutable state without a lock, check-then-act (TOCTOU), non-atomic read-modify-write, missing 
await
, deadlock-prone lock ordering, assuming callback order.
Resource safetyLeaked file/socket/connection, missing 
close
/
defer
/
finally
/context manager, unbounded growth (caches, queues).
API misuseCalling a function with wrong assumptions about its contract, ignoring documented preconditions, depending on undefined behavior or implementation details.
Boundaries / inputUntrusted input used unvalidated (defer real security to a dedicated security review, but flag the obvious).
TestsNew branch with no test, happy-path-only coverage, assertions that can't fail, flaky timing-dependent tests.
PerformanceN+1 queries, work inside a hot loop that could hoist out, accidental O(n²), a query with no index, loading a whole collection to count it. Flag only when it matters at expected scale.
Readability / reuseDuplicated logic that already exists elsewhere, a 6-deep nesting that an early return flattens, a name that lies about what the thing does.
按以下顺序检查——正确性问题比风格小问题更重要。
分类具体检查点
逻辑漏洞差一错误、条件反转、错误运算符（
&&
vs `
边界情况空列表/字符串、零值、负值、
null
/
None
/
undefined
、单元素、最大值、重复键、Unicode字符。
错误处理异常被吞、忽略返回/错误值、部分失败导致状态不一致、错误路径未清理资源、错误消息丢失根因。
并发问题共享可变状态未加锁、检查后执行（TOCTOU）、非原子性的读-改-写操作、缺失
await
、易导致死锁的锁顺序、假设回调执行顺序。
资源安全文件/套接字/连接泄漏、缺失
close
/
defer
/
finally
/上下文管理器、无限制增长（缓存、队列）。
API误用调用函数时违背其契约假设、忽略文档说明的前置条件、依赖未定义行为或实现细节。
边界/输入未验证的不可信输入（真正的安全问题交给专门的安全审查，但需标记明显问题）。
测试新增分支无测试、仅覆盖正常路径、断言无法失败、依赖时序的不稳定测试。
性能N+1查询、热循环内可提取的操作、意外的O(n²)复杂度、无索引的查询、加载全量集合仅为计数。仅在预期规模下会产生影响时标记。
可读性/复用性代码库中已存在的重复逻辑、可通过提前返回简化的6层嵌套、名称与实际功能不符。

Severity model

严重程度模型

Tag every finding so the author knows what blocks merge and what doesn't.
SeverityMeaningBlocks merge?
BlockerCorrectness bug, data loss, security hole, breaks the build/tests.Yes
MajorLikely-wrong under realistic conditions, missing error handling on a real failure path, missing test for new behavior.Usually
MinorWorks, but harder to maintain, slightly off-pattern, small inefficiency.No
NitPure style/naming preference. Prefix with 
nit:
so it's clearly optional.		No
If you can't articulate the failure mode, it's at most a Minor — don't inflate taste into a Blocker.
为每个问题标记严重程度，让作者清楚哪些会阻碍合并，哪些不会。
严重程度含义是否阻碍合并？
Blocker（阻塞）正确性bug、数据丢失、安全漏洞、破坏构建/测试。是
Major（重大）实际场景下可能出错、真实失败路径缺失错误处理、新增行为无测试。通常是
Minor（次要）功能正常，但维护难度更高、略微不符合规范、小范围低效。否
Nit（细节）纯风格/命名偏好。前缀加
nit:
明确表示为可选项。		否
如果你无法明确说明失败场景，那最多是Minor问题——不要把个人偏好升级为Blocker。

How to phrase feedback

反馈的表述方式

A useful comment points at a line, names the failure, and proposes a fix. Three parts:
payment.go:142
(Blocker) — If 
charge()
throws after 
markPaid()
runs, the order is marked paid but no charge exists; the next reconciliation will refund a payment that never happened. Move 
markPaid()
after the charge succeeds, or wrap both in a transaction.
  • Point at 
    file:line
     so it's locatable.
  • Explain the failure mode ("if X then Y goes wrong"), not just "this is wrong."
  • Suggest the fix — even a rough direction. A finding with no path forward stalls the PR.
  • Ask, don't assert, when unsure. "Is 
    items
    guaranteed non-empty here? If a caller can pass 
    []
    , this indexes out of bounds." — invites the author to confirm rather than starting a fight.
  • Praise sparingly and specifically when a tricky thing is done well; it calibrates the rest.
Avoid: vague drive-bys ("this seems off"), restating the diff, bikeshedding naming on a Blocker-laden PR, and demanding rewrites that don't change behavior.
有用的评论需包含三个部分：指向代码行、说明问题、提出修复建议。
payment.go:142
（Blocker） — 如果
charge()
在
markPaid()
执行后抛出异常，订单会被标记为已支付但实际未完成扣款；下次对账会退还一笔从未发生的支付。建议将
markPaid()
移到扣款成功后执行，或把两者包裹在事务中。
  • 指向
    文件:行号
    ，方便定位。
  • 说明失败场景（“如果X发生，Y会出错”），而非仅说“这是错的”。
  • 建议修复方向——哪怕是粗略的思路。没有解决路径的问题会导致PR停滞。
  • 不确定时用提问而非断言。“
    items
    是否保证非空？如果调用者可以传入
    []
    ，这里会出现索引越界。”——邀请作者确认，而非引发争执。
  • 对处理巧妙的部分给出具体的表扬；这能让作者更重视你的其他反馈。
避免：模糊的路过式评论（“这看起来有问题”）、重复diff内容、在有Blocker问题的PR上纠结命名、要求不改变功能的重写。

Approve vs request changes

批准 vs 请求修改

  • Approve — no Blockers or Majors; remaining items are Minors/Nits you're happy to see deferred. Approving with a couple of 
    nit:
    s the author can take or leave is normal and keeps things moving.
  • Request changes — any Blocker, or a Major that materially risks correctness. Be explicit about which findings gate the merge so the author isn't guessing.
  • Comment (no verdict) — you have questions that change your assessment depending on the answer, or you only skimmed and want to flag that.
Don't block a PR on preferences. Don't approve a PR you didn't actually understand — "LGTM" on a diff you skimmed is how bugs ship.
  • 批准 — 无Blocker或Major问题；剩余问题为Minor/Nits，你可以接受延后处理。批准时附带几个作者可选择采纳或忽略的
    nit:
    评论是正常操作，能推进流程。
  • 请求修改 — 存在任何Blocker，或对正确性有实质性风险的Major问题。明确说明哪些问题会阻碍合并，避免让作者猜测。
  • 仅评论（无结论） — 你有一些问题，答案会改变你的评估；或你只是略读，想标记这一点。
不要因个人偏好阻塞PR。不要批准你未真正理解的PR——对略读的diff说“LGTM”是bug流入生产的原因。

Review checklist

审查清单

  • I read the description and know what the change claims to do
  • I read the surrounding code, not just the diff lines
  • I traced at least one real input through the new path
  • Edge cases covered: empty / null / zero / negative / max / duplicate
  • Every error/failure path leaves state consistent and is observable
  • No shared mutable state touched without synchronization
  • Resources are released on every path (success and error)
  • New/changed behavior has a test that fails against the old code
  • No obvious N+1, accidental quadratic, or unindexed hot query
  • No duplicated logic that already exists in the codebase
  • Every finding is tagged with a severity and names a failure mode
  • My verdict (approve / request changes) matches the findings
  • 我已阅读描述，了解变更声称的功能
  • 我阅读了关联代码，而非仅变更行
  • 我至少追踪了一个真实输入在新路径中的流转
  • 边界情况已覆盖：空值/Null/零值/负值/最大值/重复值
  • 所有错误/失败路径都能保持状态一致且可观测
  • 共享可变状态的操作都有同步处理
  • 所有路径（成功和错误）都释放了资源
  • 新增/变更的行为有针对旧代码会失败的测试
  • 无明显的N+1查询、意外平方复杂度或无索引的高频查询
  • 代码库中无重复逻辑
  • 每个问题都标记了严重程度并说明失败场景
  • 我的结论（批准/请求修改）与问题情况匹配

On reviewing your own diff

自我审查代码差异

Self-review before you send is worth doing and this checklist applies to it. It lowers what a reviewer finds.
It does not make you the reviewer of record. The mistakes you cannot catch in your own diff are the ones that come from your own model of the code — you will re-derive the same wrong assumption reading it back, because it is the assumption that produced the line. That is precisely what a second reader is for. Self-review reduces the defect rate; it does not discharge the independent check.
提交前进行自我审查非常有必要，以上清单同样适用。这能减少评审者发现的问题。
但自我审查不能替代正式评审。你无法在自己的diff中发现的问题，往往源于你对代码的固有认知——回头看时，你会重复推导同样的错误假设，因为这个假设正是写出代码的原因。而这正是需要第二位评审者的原因。自我审查能降低缺陷率，但无法替代独立检查。

See also

参考

verify
 — the complementary skill. Use 
code-review
when you have a diff and the question is whether the change is any good; use 
verify
when you have a claim and the question is whether it is true, including claims with no diff to read (a backfill, a migration, a deploy).
verify
 — 互补技能。当你有diff并想评估变更质量时使用
code-review
；当你有一个声明并想验证其真实性（包括无diff的场景，如回填、迁移、部署）时使用
verify
。