Code Review
By the end of this lesson
Give and receive feedback that improves the change.
Code review exists to catch what the author could not see. Not typos — a machine finds those. The things a second reader notices: a case that was not considered, a name that means something different elsewhere in the system, a decision that will be expensive in six months.
It also spreads knowledge. After a review, two people understand the change instead of one, which matters on the day the author is unavailable.
What is worth a reviewer's attention, roughly in order:
- Correctness — does it do what the description says, including at the boundaries?
- Missing cases — empty input, nothing found, two requests at once, the operation failing halfway
- Design — does this belong here, and does it fit how the rest of the system works?
- Naming and clarity — will the next reader understand this without asking the author?
- Tests — do they cover the behaviour that matters, or only the path that was easy to test?
- Security and data handling — anything unvalidated, over-trusted, or logged that should not be
The same observation, phrased two ways. The difference is not politeness — it is how much the author has to guess:
| Hard to act on | Easy to act on | |
|---|---|---|
| A design concern | This feels wrong. | This method now does validation and sending. Splitting them would let us test validation without a mail server — worth it here? |
| A missing case | What about empty lists? | If order.Lines is empty, Sum returns zero and we create a zero-value invoice. Should that be rejected instead? |
| A question about approach | Why did you do it this way? | I would have put this in the repository layer — was there a reason to keep it in the controller? |
| A minor preference | Rename this. | Minor, take it or leave it: totalValue reads as money to me, and this is a count. |
Giving feedback that helps:
- Be specific and point at the line. "Consider extracting this" with no location is work for the author
- Say what you would do differently, not only that something is wrong
- Mark severity. "This will break on empty input" and "minor, optional" should not look identical
- Ask rather than assert when you might be missing context — you usually are
- Comment on the code, never the person. "This misses the empty case", not "you always forget"
- Say what is good, briefly and genuinely. A review with no positive signal is hard to read fairly
- Approve when it is good enough, not when it matches what you would have written
Receiving feedback without defensiveness. This is a skill and it takes practice:
- Read everything before replying. The first comment often becomes less annoying in the light of the fifth
- Assume the reviewer is trying to improve the change, because they almost always are
- A comment on your code is not a comment on you. This is easy to say and takes real practice
- If you disagree, explain the reasoning and the constraint. Disagreement handled in writing is how teams settle conventions
- If a reviewer misread something, that is a signal the code is unclear, even when their conclusion was wrong
- Push a fix or reply, but do not leave a comment unanswered. Silence reads as dismissal
- Thank them for the one that caught a real bug. That is the review working
# Get the branch without merging anything into your own work
git fetch origin
git switch reject-negative-quantities
# What does this change, measured from where it diverged?
git diff origin/main...HEAD
# Read the commits in order, with their messages
git log --oneline origin/main..HEAD
# Build and run the tests yourself
dotnet test
# Back to your own work when you are done
git switch -- Switching to the branch gives you the working code, not a diff in a web page. Some problems are only visible when you run it.
- The three-dot form shows the branch's own changes, excluding anything that arrived on main since it started.
- Reading the commits in order follows the author's reasoning. A well-sequenced branch is much easier to review this way than as one combined change.
- git switch - returns to the branch you were on before, which saves remembering its name.
Summary
- Review is for correctness, missing cases and design — the things only a second reader notices
- Formatting belongs to a formatter and a linter in the automated checks, never to a review comment
- Useful comments are specific, located, marked for severity, and about the code rather than the person
- Receiving feedback well means reading it all, assuming good intent, and treating confusion as a signal about clarity
- Run the branch and read the commits in order — reviewing the diff alone misses interactions
Practice
Attempt each one before opening the solution. Getting it wrong first is how the idea sticks.
Think about it
Rewrite the comment
A reviewer leaves: "this is not how we do things here."
Rewrite it so the author can act on it without asking a follow-up question. What does the rewritten version have to contain?
Show solution
It needs the convention, where the convention lives, and why it exists. Something like: "We put database queries in the repository layer rather than the controller — see OrderRepository for the pattern. It keeps the controller testable without a database."
The original comment is not rude, it is incomplete. The author cannot comply without a conversation, so the review takes an extra day and the reviewer looks like a gatekeeper.
The general test for any review comment: could the author act on this without replying to you? If not, add what is missing.
Challenge
Review your own change as a stranger
Take a branch you are about to propose. Run git diff against the main branch and read it as though someone else wrote it, writing down every comment you would leave.
Then check: how many of your comments were about formatting, and how many about behaviour? If formatting dominates, your project needs a formatter more than it needs reviewers.
Show solution
Most people find two or three real issues in their own change when they read it cold as a diff. The change of perspective from writing to reading is what does it.
The formatting count is the useful measurement. Anything a tool could have decided should be decided by a tool, running automatically, so human attention goes to the parts that need judgement.
Saved in this browser only.