The Anatomy of a Good Code Review: What Authors and Reviewers Owe Each Other

By Sergey Nosov

1 October 2026

In April 1942, the philosopher Simone Weil wrote to the poet Joe Bousquet that “attention is the rarest and purest form of generosity.” She was not writing about software, but her sentence describes a code review well: a colleague’s attention, given to your work.

The most generous thing you can do with a colleague’s pull request is not to approve it quickly. It is to read slowly enough to see what the author can no longer see: the null path, the swallowed exception, the name that made sense at eleven at night. Attention is finite, though, and both sides of a pull request have to earn it.

This article covers both halves. First comes what the author owes the reviewer before anyone opens the diff, and then where the reviewer should spend attention once they do. In the middle is a short C# method with four bugs for you to find, followed by the same diff reviewed twice, once badly and once well. The last sections cover how to phrase comments, when to approve, and how fast to respond.

Why Review at All?

Start with why, because if the why is wrong, the how will be too. I see four reasons. Listed in descending order of how often people cite them, they also run in ascending order of how much they matter.

Research at Microsoft and Google points the same way. Alberto Bacchelli and Christian Bird reported on code review at Microsoft in a 2013 paper. They found that “while finding defects remains the main motivation for review, reviews are less about defects than expected.” The additional benefits they found included knowledge transfer, increased team awareness, and alternative solutions to problems. A 2018 case study at Google found something similar in what developers there expect from review: “Defect finding is welcomed but not the only focus.”

So a review is not a gate. It is the last moment at which a mistake is cheap.

The Author’s Half: Make It Reviewable

A good review starts before anyone clicks “Create pull request,” so the author’s half comes first.

Keep one intent per pull request. A few hundred lines gets read. Two thousand lines gets skimmed and approved, and an approval on a skim is worth nothing. The evidence points the same way from several directions:

One intent also means one kind of change. If you have a refactor and a feature, that is two pull requests. Google’s guide gives an example: “moving and renaming a class should be in a different CL from fixing a bug in that class.” The same rule applies when an AI agent writes the code. My article on spec-driven development argues for reviewing an agent’s work one focused pull request at a time.

Describe it. Say what changed, why, how to verify it, and what the risk and the rollback are. Link the work item. Here is a hypothetical description for a small refactor:

## What
Move the shipping-rate rules
from CheckoutController into a
new ShippingRates class.

## Why
The rules could not be tested
inside the controller, and the
last rate change broke quietly.

## How to verify
- dotnet test: 12 new unit tests
- Staging: a 2 kg parcel to
  Canada still quotes $18.40.

## Risk and rollback
Behavior-preserving refactor.
Revert the single commit.

Work item: #482

Look at the shape: four headings, a verification step a reviewer can actually run, and a rollback that takes one sentence. A reviewer who reads that description walks into the diff knowing what to look for.

Review it yourself first, in the pull request view, not in your editor. There you see the diff the way your reviewer will. You will find the debug logging, the leftover TODO, and the file that does not belong before your reviewer spends attention on them.

Ship the tests in the same pull request, not in a follow-up that never comes. Google’s guide for reviewers sets the same default: “In general, tests should be added in the same CL as the production code unless the CL is handling an emergency.”

Say where you want eyes. “I am least sure about the retry logic” is the single most useful sentence an author can write.

The Reviewer’s Half: Spend Attention in Order

Attention is finite, so the question is where to spend it. My answer: in order.

  1. Intent. Before you read a line of code, read the description and ask whether the diff matches it. A pull request that does more than it says is an easy way for a surprise to reach production. Google’s reviewer guide starts in the same place: “Does this change even make sense?”
  2. Correctness. Check edge cases, error paths, concurrency, and security: the null that arrives, the exception that is swallowed, the input that is trusted. This is where most of your attention belongs.
  3. Design. Does the change fit the code around it, and will it be hard to change later?
  4. Tests. Do they test behavior or implementation? A test that breaks when you rename a private method is a test of the wrong thing.
  5. Readability. Look at the names, the structure, and the comment that explains why.
  6. Style, last. Brace placement, whitespace, and import order are the linter’s job. If no linter does that job yet, the fix is a linter, not a comment.

The rule behind the order is to spend attention where a mistake costs the most. A naming nit on a broken null check is attention spent in the wrong place. It is also the fastest way to teach an author that reviews are about formatting. The Google case study says automated analysis lets reviewers focus on understandability and maintainability “instead of getting distracted by trivial comments (e.g., about formatting).”

Exercise: Read This as a Reviewer

Here is a short C# method. Assume that _repo wraps a table of customers and _cache holds a cached list of them. Read the method as a reviewer, in the order above. A good review finds four problems. Give it thirty seconds before you read on.

public async void Save(Customer c)
{
    var key = c.Email.ToLower();
    var found = _repo.Find(key);
    if (found != null)
        found.Name = c.Name;
    else
        _repo.Add(c);

    try
    {
        await _repo.SaveAsync();
    }
    catch (Exception) { }

    _cache.Remove("customers");
}

On a good day, the method works: it updates the customer if the email exists, adds one if not, saves, and clears the cache.

Same Diff, Two Reviews

Here is the same diff, reviewed twice. First come the comments that do not help. I have seen every one of them on a real pull request.

Here are the same four findings, written so the author can act on each one without asking what the reviewer meant:

Each comment says what is wrong, why it matters, and which way to go. Each also carries a label that sets its weight: two must change before merge, one needs an answer, and one is polish. The findings themselves deserve a closer look.

The first blocker is worse than it looks. Microsoft’s documentation is blunt about async void: “The caller of a void-returning async method can’t catch exceptions thrown from the method. Such unhandled exceptions are likely to cause your application to fail.” A fifth finding, if you spotted it, shows how. Nothing checks that c.Email is not null. I ran the method in a .NET 10 console app with a null email. The NullReferenceException never reached the caller’s catch. The runtime rethrew it on a thread pool thread, and the process terminated.

The second blocker hides failures. With the empty catch in place, a save can fail on every call and nothing reports it. When I made the save throw, the caller saw no error, the cache was cleared, and the change was gone.

The question is a question because the reviewer might be wrong. Whether Find matches a stored Ann@Example.com against the key ann@example.com depends on how the repository compares strings. With a case-insensitive database collation, it matches. With a case-sensitive one, it does not. In my test against a case-sensitive store, saving the same address twice produced two rows. The reviewer cannot see the collation from the diff, so the honest comment asks.

The nit is labeled as one so that nobody mistakes it for a blocker. The bug behind it is the one Microsoft’s guide to comparing strings singles out: “The canonical example is the Turkish-I problem.” Under Turkish culture settings, "INFO@EXAMPLE.COM".ToLower() returns "ınfo@example.com", which matches nothing. For an application that never runs under Turkish culture settings, nit is the right weight; if yours might, promote it. The same guide recommends ToUpperInvariant() over ToLowerInvariant() when you normalize strings for comparison.

Better still, let a tool leave this comment. The .NET analyzer rule CA1311 flags a ToLower() call that names no culture. It is off by default in .NET 10, and one line in .editorconfig turns it on: dotnet_diagnostic.CA1311.severity = warning.

Same attention, two results: one review the author can act on in ten minutes, and one that starts an argument.

How to Say It

A correct finding delivered badly gets ignored or resented, and either way the defect ships. Six habits help.

Label the weight. Blocking, suggestion, question, nit, praise: the author should know at a glance what must change before merge and what may. Much of the friction in reviews comes from an author treating a nit as a demand, or a reviewer treating a blocker as optional. Google’s guidance on comments warns that “without comment labels, authors may interpret all comments as mandatory.” If your team wants a shared format, Conventional Comments, published by Paul Slaughter, defines one. Each comment starts with a label such as praise, nitpick, suggestion, issue, or question, optionally followed by a decoration such as (blocking) or (non-blocking).

Say what and why. The reason teaches; the instruction alone does not, and the next pull request will repeat the problem.

Offer a direction, not a rewrite. The author owns the fix. A reviewer who pastes the replacement code has taken both the work and the learning. Google’s guide makes the same argument: “Pointing out problems and letting the developer make a decision often helps the developer learn.”

Comment on the code, not the coder. Write “this method swallows the exception,” never “you swallowed the exception.” It sounds like a small thing. It is the difference between a review and a performance review. Google’s guide recommends “always making comments about the code and never making comments about the developer.” My article on empathy as an engineering skill covers the wider habit behind this: feedback that uplifts instead of discourages.

Ask when you might be wrong. A question costs nothing; a wrong command costs trust. Conventional Comments gives the same advice: “If you are not sure if a problem exists or not, consider leaving a question.”

Treat praise as a review comment too. If the author did something you want to see again, say so in the thread, where it stays. Conventional Comments suggests at least one praise comment per review, with a caveat: “Do not leave false praise (which can actually be damaging).”

One test covers every comment you write: could the author act on it without asking what you meant?

Approving, Requesting Changes, and the Clock

The last mechanics decide whether reviews help a project or slow it down.

Respond within one business day. A pull request waiting on review is the most expensive kind of idle. The author has moved on, the context is cooling, and every day it waits, the branch drifts further from main. If you cannot get to it today, say when you can; “this afternoon” is a review comment. Google’s guidance on speed sets the same limit. It says that “one business day is the maximum time it should take to respond to a code review request (i.e., first thing the next morning).”

Speed pays off beyond the single pull request. DORA’s 2023 Accelerate State of DevOps Report, which drew on a survey of nearly three thousand professionals, states: “Teams with faster code reviews have 50% higher software delivery performance.” The report adds a fair caveat. Software delivery performance, it says, “is unlikely to improve if your code reviews are already fast but speed is constrained elsewhere in the system.”

Approve with comments when nothing is blocking. Do not hold a pull request hostage for nits; the author can address them and merge. Google calls this “LGTM With Comments,” where LGTM stands for “Looks Good to Me.” Its guide recommends it, for example, when “the reviewer is confident that the developer will appropriately address all the reviewer’s remaining comments.”

Request changes only for blocking findings, and say which comments are the blockers, so the author knows when they are done.

Too big to read? Ask for a walkthrough or a split, not a rubber stamp. For a change too large to review soon, Google’s guide gives the same answer. The typical response, it says, “should be to ask the developer to split the CL into several smaller CLs that build on each other.”

Disagree on the thread, then on a call, then decide, and record the decision. When consensus gets especially difficult, Google’s guide suggests a face-to-face meeting or a video conference instead of more comments. It then asks you to “record the results of the discussion as a comment on the CL, for future readers.” Closing a thread without a fix deserves one line of why, too. The next reader of the thread needs the reasoning, including any automated reviewer that may raise the same point again.

Treat automated and AI findings like a colleague’s. Fix the finding, or close it with a reason the next reader can follow. Dismissing a finding without a reason is the same as approving without reading. On GitHub, the mechanics already match. Its documentation says that “Copilot’s review comments work like comments from human reviewers.”

Approval itself is changing too. On 1 September 2026, GitHub announced that Copilot code review can approve pull requests, as a public preview. The ability is off by default. Administrators control it at the enterprise, organization, and repository levels, and a repository can limit the file paths Copilot may approve. Once it is on, GitHub says, “Copilot can submit an approval that counts toward the repository’s required-approvals rule.” Whether to turn it on is each team’s decision.

Reviewing work you did not write is part of what my book Code You Did Not Write calls technical direction. Whether the contributor is a junior developer, a senior contractor, or an AI agent, the core challenge is the same.

That brings us back to Weil. Approving a pull request you have not read is not generosity. It is a signature on someone else’s risk.

Takeaways

As the author:

As the reviewer:

One exercise to finish: on your next review, leave one comment that starts with praise: and one that explains a why.

Code review also has its own entry in my Software Development Principles series, with indicators of proper application and of common violations.

Further Reading