Code Review

Course 2 · Ch 7
Code Review
Practical guidance for both sides of the review — as the person whose code is being reviewed, and as the person reviewing it

Course 1, Chapter 7 covered the mechanics of leaving and replying to PR comments. This chapter is about doing review well — the judgment calls that separate a review that genuinely improves code from one that just creates friction, from both sides of the table.

As the Author — Making Your PR Easy to Review

1
Keep PRs small and focused
A 50-line PR doing one thing gets reviewed carefully. A 1,500-line PR doing five things gets skimmed and rubber-stamped — reviewers genuinely cannot hold that much context at once. Split unrelated changes into separate PRs.
2
Write a description that explains WHY, not just what
The diff already shows what changed. The description's job is the part the diff can't show: why this approach, what alternatives were considered, what's intentionally out of scope.
3
Review your own diff before requesting review
Catching your own leftover debug code, commented-out blocks, or typos before a reviewer does saves a review round-trip and signals care.
4
Don't take feedback personally — it's about the code
A comment on a function isn't a comment on you. The healthiest review cultures treat critique of code as completely separate from judgment of the person who wrote it.

As the Reviewer — Giving Feedback That Actually Helps

Less useful
"This is wrong."
More useful
"This will throw if `user` is null — looks like that's possible after the change in `getCurrentUser()`. Maybe an early return or optional chaining here?"
Less useful
"Why did you do it this way?"
More useful
"Curious about the reasoning here — would a Set be faster than this array.includes() check for the larger lists we expect? Not blocking, just want to understand the tradeoff."

Distinguishing severity — not every comment is equally important

🚫 Blocking
A genuine bug, security issue, or something that breaks functionality. Must be addressed before merge — say so explicitly.
💭 Suggestion
A better approach exists, but the current one isn't wrong. Worth raising, not worth blocking on — many teams prefix these with "nit:" or "suggestion:" to signal this.
❓ Question
Genuinely seeking to understand the reasoning, not implying disagreement. Often surfaces a real issue, but framed as curiosity rather than correction.
Labelling severity explicitly saves a lot of back-and-forth
A reviewer who prefixes every comment with "nit:", "blocking:", or "question:" makes it immediately obvious to the author what genuinely needs addressing before merge versus what's optional polish — without that, authors often can't tell if a comment is a hard requirement or a passing thought, and either over-react or under-react to it.

What to actually look for as a reviewer

  • Correctness first. Does this do what it claims to do? Are there edge cases (empty input, null, the boundary conditions) that aren't handled?
  • Security and safety. Any of the OWASP-style concerns — unsanitised input, exposed secrets, injection risks — covered more deeply in the security-focused parts of this curriculum elsewhere.
  • Consistency with the existing codebase. Does this match established patterns, or introduce a new one without reason? Inconsistency compounds into real maintenance cost over time.
  • Tests. Does new behaviour have coverage? Does an existing test actually verify the fix, or just exercise the code path without asserting anything meaningful?
  • What you're NOT reviewing. Pure style preferences a linter could enforce automatically aren't worth a human reviewer's comment — that's what automated formatting/linting in CI (Chapter 8) is for.

Approving, Requesting Changes, or Commenting

GitHub's review submission offers three distinct outcomes, each sending a different signal:

  • Comment — feedback with no formal verdict either way. Useful for early-stage or partial reviews.
  • Approve — "I'm satisfied this is ready to merge." Doesn't mean zero feedback was given, just that none of it is blocking.
  • Request changes — explicitly blocks merging (if branch protection requires review approval — Course 3, Chapter 6) until addressed and re-reviewed.
"Request changes" for a minor nit is usually overkill
Reserving "Request changes" for things that are genuinely blocking (bugs, security issues, real correctness problems) — and using plain comments or an approval-with-suggestions for everything else — keeps the review process from feeling adversarial over small stuff. Overusing the hard "block" signal trains authors to dread review rather than welcome it.

Review Culture — The Part That's Not About Syntax

  • Assume good intent. The author wrote what seemed reasonable to them at the time — feedback framed as curious rather than corrective tends to land better and surfaces the same information.
  • Praise good decisions, not just flag problems. A review that's 100% criticism, even when every point is valid, reads as harsher than intended. A genuine "nice use of early returns here" costs nothing and balances the tone.
  • Respond to review requests reasonably promptly. A PR sitting unreviewed for days blocks the author's progress and discourages future small, frequent PRs — exactly what Chapter 1's "keep PRs small" advice depends on working well.

Chapter 7 Quick Reference

  • As author: keep PRs small, explain WHY in the description, self-review before requesting, don't take feedback personally
  • As reviewer: be specific and explain reasoning, not just "this is wrong"
  • Label severity: blocking vs suggestion ("nit:") vs genuine question — removes ambiguity about what must change
  • Focus review on: correctness, security, consistency, test coverage — not style a linter could catch
  • Approve / Comment / Request changes — reserve "Request changes" for genuinely blocking issues
  • Culture matters: assume good intent, balance criticism with genuine praise, review promptly
  • Next chapter: GitHub Actions basics — your first automated CI workflow