What I look for in a code review

I spent my first year of reviewing pull requests leaving comments about naming and formatting, because those were the things I could see. They were also the things a formatter should have been doing, and the reviews were not catching bugs.

Here is the order I go in now.

1. Does it do what the description says

Before reading any code: read the description, then check the diff does that.

The failures this catches are not subtle. A PR titled “fix crash on empty state” that also refactors the networking layer. A bug fix that changes the API contract. A feature that quietly deletes a behaviour someone else depends on.

If the description does not say what the change is for, that is the first comment, and everything else waits. I cannot review a change whose purpose I am guessing at, and neither can the person reading it in a year’s time.

2. What happens when it fails

The happy path is usually right — it is the one the author ran. The interesting questions are all about the other paths:

• What if the network is offline? Not slow — off. Airplane mode mid-request.
• What if this array is empty? First, last, index zero, division by count.
• What if this runs twice? A double tap, a retry, a notification arriving during a retry.
• What if the user leaves the screen mid-operation? Is the work cancelled, and does the completion handler still write to a deallocated thing?
• What if this optional is nil in production but never in your test data?

Most of the real bugs I have caught in review were one of those five, and they are all askable without deeply understanding the feature.

3. Is the state model right

This is the highest-leverage thing to get right and the hardest to change later, so it is worth spending review time on.

The specific thing I look for is states that can be represented but should not exist:

struct ViewState {
    var isLoading: Bool
    var items: [Item]?
    var error: Error?
}

Eight combinations, three of which are meaningful. isLoading == true with a non-nil error is representable, so somewhere there is a branch handling it, or there is not and it happens in production.

enum ViewState {
    case loading
    case loaded([Item])
    case failed(Error)
}

Three states, all meaningful, and the compiler makes sure every switch handles them. Suggesting this in review is a real change to ask for, and it is worth asking for, because the alternative is a class of bug that persists for the lifetime of the screen.

4. Will this be readable in a year

Not “is this clever” — clever is usually the problem. The questions:

• Would someone who has not read the ticket understand why this is here? If the reason is non-obvious, it needs a comment, and the comment should say why rather than what.
• Are the names honest? A function called validate that also saves is a bug waiting to happen when someone calls it expecting validation.
• Is there a comment explaining what the code does? Delete it and make the code say that instead.
• Is there commented-out code? Delete it. Git remembers.

What I deliberately do not comment on

Anything a formatter or linter should catch. If I am commenting on spacing, the project needs a formatter, and that is a separate conversation with the whole team rather than a note on someone’s PR.

Style preferences. guard versus if let, where the else goes, whether to use a trailing closure. If both forms are readable, the author’s choice wins. Every one of these comments costs goodwill and buys nothing.

Things I would have done differently but which are not worse. This is the discipline that took me longest. “I would have used a dictionary here” is not a review comment unless the array is actually a problem.

How I write the comments

Ask rather than instruct, where I am not certain. “What happens if items is empty here?” gets a better outcome than “this will crash on empty”, partly because I am wrong perhaps a third of the time and the question does not require anyone to lose face.

Say which comments are blocking. I prefix optional ones with nit: or non-blocking:. Without that, the author has to guess whether a stylistic note is a merge condition, and they usually guess that it is.

Approve with comments when nothing is blocking. Holding up a PR for three nits wastes a day and teaches people that review is an obstacle.

The thing I try to remember

A review is about the code, and it is read by a person who spent a day writing it. Approving something with a small flaw is nearly always cheaper than a review round trip that leaves someone feeling their work was picked over — and the small flaw can be fixed in the next PR, by either of us, in two minutes.