Reviews, feedback, and thick skin
Code review is the cheapest place to catch a bug and the fastest way to spread how a codebase works. It is not a verdict on you.
- You can prepare a change that is easy to review, which is most of the battle.
- You can give and receive a comment without either person getting defensive.
Reviews fail in a predictable way: the change is 1,200 lines across nine files, the reviewer skims it in four minutes, approves, and the bug ships. Or the opposite — the reviewer rewrites the design in comments, the author defends every line, and the review takes a week.
Both are fixed upstream. Review quality is decided mostly by the size of the change, which is decided by the author, before anyone reads a line.
8.1Make it reviewable
- One change, one purpose. A refactor and a behavior change in the same PR makes both unreviewable — split them.
- Small. If reviewing takes more than twenty minutes, it will be skimmed. Break it into a stack of small changes if needed.
- Describe the why. Two lines: what problem, what approach. The diff shows the what.
- Self-review first. Read your own diff before requesting. You will catch the debug print, the commented-out code, the accidental file.
- Tests in the same change. A reviewer should not have to imagine how this is verified.
8.2What reviewers actually look for
| Priority | Question | Example comment |
|---|---|---|
| Correctness | Does this do what it claims, including at the edges? | "What happens if items is empty here?" |
| Tests | Would a regression be caught? | "Can we get a test for the expired-token case?" |
| Security | Input validated? Authorization checked server-side? Secrets out of logs? | "This is authorized on the client only — move the check server-side." |
| Operability | What happens when it fails in production? Is it observable? | "If this call times out, we retry forever. Add a cap?" |
| Clarity | Will a stranger understand this in six months? | "Could this name say what it returns rather than how?" |
| Style | Formatting, naming conventions | Mostly the formatter's job — don't spend review attention here |
"This is wrong."
Better: "If the order is already refunded, this path runs twice — I think we'd double-pay. Am I reading that right?"
Name the condition, the consequence, and leave room for you being wrong. It costs one sentence and removes the sting.
"Why not use a WeakMap here?"
Better: "I'd have reached for a WeakMap — is there a reason this holds the reference? If it's deliberate, worth a comment so the next person doesn't 'fix' it."
Questions invite context; directives invite defense. If it's genuinely a preference, say so and move on.
You got twelve comments. The useful response is not to defend every line — it's to sort them:
- Bugs: fix now, add a test.
- Questions: answer in the thread, then change the code so the next reader doesn't need to ask.
- Preferences: pick one, politely, and note it in the team's conventions so it stops recurring.
- Scope creep: "good idea, separate ticket" is a complete and professional answer.
A review comment is about a diff, which is a draft of an idea, not a measure of your worth. The healthiest teams make this explicit: critique the code, assume competence, and prefer a question to a verdict. If a comment stings, wait ten minutes before replying — the reply you write second is almost always better.
Reviews are a queue, and queues have a cost. A change that waits two days for a first look is a change whose author has moved on, whose context has evaporated, and whose rebase will hurt. Small and frequent beats thorough and late — a twenty-minute review today is worth more than a perfect one on Thursday.
"LGTM" on everything is worse than no review, because it creates false confidence and no shared understanding. If a change is too big to review properly, that is the finding: ask for it to be split. Reviewing is a skill you practice, not a box you tick.
Your change is 900 lines across 12 files. What should you do before requesting review?
60-second recap
- Small, single-purpose changes get real review. Size is the author's decision.
- Review correctness, tests, security, operability, clarity — let the linter handle style.
- Ask questions, name consequences, and critique the code rather than the person.
- A rubber stamp is worse than nothing. Too big to review is itself a finding.