Skip to main content

​
Overview

Code review is the practice of having someone other than the author examine a proposed change before it merges, and decide — based on that examination — whether the change is ready to become part of the codebase. This page starts from the value review brings an organization, works through what a reviewer actually checks, then looks at how tooling has taken over more of that checking over time, and closes with three practical habits: reviewing your own change first, approving on improvement rather than perfection, and writing comments that are easy to act on. It assumes the pull request itself — its granularity, its template, its title and commit conventions, and the basic request-to-merge cycle — is already in place; see Pull Requests for that layer, and Test Code for what makes a change verifiable in the first place.

​
The value review brings

Catching defects before they ship is the obvious reason to review a change, and it is real — but a team that stops there is leaving most of review’s value on the table.

Shared understanding

Examining a change forces someone besides the author to understand it, so knowledge of why it exists and how it works is never trapped in one person’s head.

Growing conventions

A convention starts as a one-off comment in a review — “we don’t do it this way” — and, repeated across enough reviews, becomes a rule the next author already knows. The team’s shared standard grows one review at a time.

Verifying non-functional requirements

Response time, security exposure, and compliance with rules outside the codebase rarely show up as a failing test. Review is often the only checkpoint that asks about them at all.

Quality improvement

A change that already works can still be reviewed toward a better one — a reviewer who suggests a cleaner approach raises the bar past “does it work,” not just enforces it.

​
What review verifies

Functional requirements are usually explicit — an issue’s acceptance criteria, a ticket’s description — so checking them is mostly a matter of comparing the diff against what was actually asked, not just what compiles. Non-functional requirements are easy to miss precisely because they rarely announce themselves: a slow query does not fail the build, and a compliance gap does not throw an exception. Design fit, the test suite, and readability round out the check: design fit protects the codebase the requirement is being added to, the test suite protects whoever touches this code next, and readability keeps the change’s true cost — the cost of maintaining it — from staying hidden until long after it merged.

​
How tooling has taken on review’s mechanical burden

Historically, every row in the table above was checked by a human, and review meant catching all of it by eye. A review drags on for several reasons, and one of the least essential is when it re-derives things a machine could already have told the author. Lint violations, missing tests, and a naming mismatch a style guide already settles are the typical cases. Worse, re-deriving the easy items alongside the hard ones splits a reviewer’s attention, making the judgment calls that actually need it more likely to slip through. Linters and formatters became the standard fix for exactly this: enforcing naming, formatting, and a growing set of known-bug patterns automatically, before a human reviewer ever opens the diff. That is the premise most teams build on today. A reviewer’s time no longer goes to catching a missing semicolon or a naming mismatch, because a tool already did. AI extends the same idea further. Where a linter can only check against fixed, generic rules, an AI-assisted pass can also check a change against a team’s own coding conventions and context — the kind of judgment that used to require someone who already knew the codebase.

Check mechanically

Correct answers a rule, or a team’s own conventions, can decide.
  • Adherence to naming and coding conventions
  • Test coverage for the changed lines
  • Known bug and vulnerability patterns matched against a fixed rule set

Keep for the human reviewer

Judgments whose correct answer depends on context.
  • Whether the design and architecture fit the surrounding system
  • Whether the change actually satisfies the requirement
  • Security risk acceptance and the final merge decision
At Findy, this mechanical pass is delegated to AI-assisted self-review, run before the pull request is even opened. What matters is keeping the rule-checkable part separate from the part that needs human judgment. See Agentic Workflow for how the same principle governs AI-generated code more broadly.
The boundary is not fixed forever. As conventions solidify and the checks a team trusts grow more reliable, more rows can move from the human side to the mechanical side — but that migration should be a decision a team makes on purpose, not something that drifts unexamined.

​
Reviewing your own change first

Before requesting a reviewer, read the diff as if you had never seen it. Distance changes what you notice: a mistake invisible while writing becomes obvious once you view the change as a stranger would, and every one you catch here costs a local fix instead of a round trip through someone else’s attention. Reading the diff in the order the reviewer will see it — top to bottom, file by file, without editing along the way — surfaces the same rough edges a reviewer would notice, instead of just the first thing you trip over.
Example self-review checklist
Keep the checklist itself under version control and revise it like any other convention. When a self-review pass misses something a human later catches, add the missed case so the same gap does not recur.

​
The bar for approval: better, not perfect

A reviewer does not need to hold approval until a change is perfect. The question that decides approval is “does merging this change leave the codebase better than it is now?” There is no such thing as perfect code — only better code. Holding out for perfection makes a change grow with every round of feedback, multiplies the round trips, and delays the value it was meant to deliver. A steady stream of small changes merged quickly improves the codebase more reliably than a few changes polished indefinitely. This does not mean lowering the bar. It means separating what blocks a merge from what does not.

Blocks the merge

Things that would leave the codebase or its users worse off once merged.
  • The change does not meet the requirement, or behaves incorrectly
  • The changed behavior is not backed by tests
  • It harms performance or security
  • It adds a feature that does not belong in the system at all

Does not block the merge

Things a follow-up pull request can handle after merge.
  • A better way of writing it that came to mind afterward
  • Design or naming differences that fit the existing structure and come down to preference
  • Tidying or refactoring in preparation for the next change
Mark blocking points with the must label described below. Leave non-blocking points with imo or nits, and let the author decide whether to apply them. Improvements deferred this time go into a refactoring-only pull request or an issue, so they do not keep the change under review open. For keeping structural changes separate from behavioral ones, see Refactoring. Deferring improvements is safe because of tests. When behavior is pinned down by tests, structural improvements can be made later, at any time, without changing behavior. Conversely, merging an untested change “for now” makes fixing it later harder, not easier.
At Findy, the rule of thumb is “clean code later; application behavior first.” If tests cover the change and CI passes, it merges unless it harms performance or security. When a better way of writing it comes to mind afterward, it goes out as a separate refactoring-only pull request.

​
Writing review comments

A comment’s job is to get the right change made with the fewest round trips, which means the author must be able to act on it without guessing what it means or how urgent it is. Vague reactions push that guessing work onto the author: “this looks off” does not say what is wrong, where, or whether it blocks the merge. Naming the specific location and the reasoning — not just the reaction — turns a comment into something the author can act on directly instead of a puzzle to solve first. Prefixing a comment with a short label that states its weight closes the other gap. A common set of labels:
Where possible, propose the fix rather than only naming the problem — it turns a round of discussion into a round of confirmation. Where the right answer genuinely is not clear, prefer a question over an assertion; framing an uncertain point as a question invites the context the author has and the reviewer does not, instead of forcing a debate over a claim that may already be wrong.

Pull Requests

Where review is requested and applied — granularity, template, and merge mechanics.

Test Code

What makes a change verifiable in the first place, and the patterns review checks test code against.

Agentic Workflow

The broader AI/human responsibility split for AI-generated code that the mechanical/judgment split above draws on.

Dependency Updates

The same mechanical/judgment split applied to automated update pull requests, with an AI agent doing the first pass.