Tutorial 2: Prioritize What to Comment On

Review everything = review nothing. Comment on what matters; let the rest go.

Step 1: What Matters Most (10 min)

In order:

1. Correctness — does it work? Does it match the spec?

2. Security — any vulnerabilities introduced?

3. Data integrity — DB / state corruption risk?

4. Performance — obvious problems?

5. Maintainability — readable, testable, evolvable?

6. Style — formatting, naming, organization

Comment from top down. Style nits last — and sometimes not at all.

Step 2: Critical Path Focus (10 min)

For a 500-line PR, 50 lines are critical:

• The core algorithm

• Auth / permissions

• DB writes

• External API calls

• Concurrency / locking

Spend 80% of your time on these 50 lines.

Skim the rest unless something jumps out.

Step 3: Distinguish Blocking from Optional (10 min)

Mark feedback clearly:

[blocking] This will lose data — see comment below.
[suggestion] Consider renaming this to `userId`.
[nit] Trailing whitespace.
[question] Why this branch? Couldn't see a case for it.
[praise] Nice approach to handling the race condition!

Author knows what's required vs. nice-to-have.

Some teams use conventional comments: nit:, suggestion:, issue:, question:.

Step 4: Style Nits — Automate (5 min)

Don't comment "missing semicolon" or "rename to camelCase."

Configure:

• Prettier / Black / gofmt (formatting)

• ESLint / Ruff / golangci-lint (linting)

• pre-commit hooks

• CI checks

Tools enforce style consistently. Humans don't.

If a nit isn't automatable, ask: does it matter? If yes, automate it. If no, drop it.

Step 5: Don't Bikeshed (10 min)

A 100-comment debate over a variable name is a bikeshed.

Pick your battles:

• Important: data corruption, security, design choices

• Bikeshed: variable names, whitespace, function ordering

If a nit could go either way, leave it (or accept the author's choice).

Step 6: Look for What's Missing (10 min)

Diff shows what was added/changed. Review also asks:

• Tests — does this need them? Are they here?

• Error handling — what happens on failure?

• Edge cases — empty list? Null? Concurrent?

• Documentation — does API doc need updating?

• Migrations — DB schema implications?

The diff doesn't always surface what's not there. Look anyway.

Step 7: Don't Re-Architect in Review (10 min)

If you'd have written this completely differently:

• Is the current approach wrong? (Push back.)

• Is it suboptimal? (Comment, but don't block.)

• Is it just different? (Let it go.)

Wholesale rewrites in review are exhausting and rude. Reserve for genuinely bad approaches.

For early designs: review the design doc, not the implementation.

Step 8: Praise the Good (5 min)

Reviews aren't only criticism:

• "Nice approach to caching this"

• "Good test for the edge case"

• "Clever — I wouldn't have thought of this"

Authors appreciate. Patterns spread. Builds team morale.

Cost: 30 seconds. Worth it.

Step 9: Time Box (5 min)

Spend more than 30-60 minutes per PR? Reconsider.

Either:

• The PR is too big — request a split

• The work is unfamiliar — ask for context

• You're nitpicking — refocus on what matters

Long reviews exhaust both sides. Iterate fast.

Step 10: When in Doubt, Discuss (5 min)

Long comment threads happen. After 3-4 back-and-forths:

• Hop on a call

• Discuss in Slack

• Pair on the fix

Async review for clear feedback. Sync for nuance.

Faster resolution. Less defensiveness.

What You Just Did

Review prioritization: severity order, critical path, blocking vs. nit, automation, anti-bikeshed, missing pieces, no rewrites, praise, time box, sync when needed. Effective reviews.

Common Failure Modes

Comment on everything. Author drowns; real issues lost.

Ignore style; comment in reviews. Inconsistent; wastes time.

Long bikeshed threads. Drain both sides; ship slower.

Only criticism; no praise. Demoralizing over time.

Async review on nuanced design. Multi-day cycles; better as a call.