# What to look for in a code review

The reviewer half of Google's public eng-practices documentation. It lists what a reviewer should be checking, in rough order of importance, and is meant to be read alongside their Standard of Code Review page. Ten or so headings, with an argument buried in the ordering: design comes first and style comes near the end.

## Design, functionality, complexity

Design is called the most important thing in the review. Do the pieces of the change interact sensibly, does this belong in your codebase or in a library, does it integrate with what's already there, is now a good time to add it at all. None of that is visible line by line.

Functionality splits into two questions: does the change do what the author intended, and is what they intended good for the people who use this code. Users here means both end users and the developers who will have to call the code later. Reviewers aren't expected to re-test — the author is expected to have done that — but they are expected to think about edge cases and read for bugs. Two cases justify actually running things. User-facing changes, especially UI, are hard to judge from a diff, so ask for a demo if patching the change in is inconvenient. And parallel programming, where deadlocks and races usually can't be found by running the code and need someone to reason through them deliberately. The doc adds an aside worth keeping: this is itself a reason to avoid concurrency models where races and deadlocks are possible, since they make review and comprehension expensive.

Complexity gets a working definition rather than a feeling. Too complex means it can't be understood quickly by readers, or that developers are likely to introduce bugs when they call or modify it. Check it at every level: lines, functions, classes. Over-engineering is named as its own failure — code made more generic than it needs to be, or functionality nobody presently needs. The instruction is to solve the problem you know you have now, and let the future problem be solved once its actual shape is visible.

## Tests, names, comments

Tests belong in the same change as the production code, with emergencies as the only exception. The reviewer's job is to check that the tests are correct, sensible, and useful, because tests don't test themselves and nobody writes tests for tests. The specific questions: will these fail when the code breaks, will they start producing false positives when the code beneath them changes, does each test make simple and useful assertions, are the cases separated across test methods properly. Test code is code that has to be maintained, so complexity there isn't free just because it never ships in the binary.

A good name is long enough to say what the thing is or does without becoming hard to read. Comments should mostly explain why code exists rather than what it does — if what it does needs explaining, simplify the code, with regular expressions and complex algorithms as the acknowledged exceptions. The good habit here is reading the comments that were already there: a TODO that this change makes removable, or a comment warning against exactly what the change does. Documentation of a class, module or function is treated as a separate thing from comments, expressing purpose, intended use, and behavior. Separately, if a change alters how people build, test, use or release the code, the associated READMEs and reference docs move with it, and deleted code means deleting its documentation too.

## Style, consistency, and the two habits at the end

Where the style guide requires something, it wins outright. Where it only recommends, it's a judgment call between the recommendation and consistency with the surrounding code, biased toward the guide unless local inconsistency would be too confusing. With no rule to apply, match the existing code, and encourage a bug plus a TODO for cleanup instead of doing it inline. Personal preferences that aren't in the guide get a "Nit:" prefix and never block submission. Large reformats go in their own change, separate from functional ones, so the diff, merges and rollbacks stay readable.

Two things close the document out. Look at every line you were assigned — skimming is allowed for data files, generated code and large data structures, but not for a human-written function on the assumption it's fine. If the code is too hard to read and that's slowing the review down, that's a finding, not a personal failure: say so and wait for the author to clarify, because other developers will hit the same wall. Pull in a qualified reviewer for privacy, security, concurrency, accessibility or internationalization when you're not one. When several reviewers split a change, note in a comment which parts you covered.

And look at the change in context. The tool shows a few lines around the diff, but four added lines may sit inside a fifty-line method that now needs breaking up, which the diff can't show you. The system-level version of the same point is the one blunt rule in the document: don't accept changes that degrade the code health of the system, because systems get complex through many small changes that individually looked acceptable. The last section asks reviewers to say what was done well, on the grounds that telling someone what they got right is often worth more as mentoring than another correction.

## What it assumes

The checklist is written for blocking, pre-merge review by a human with time. Most of what it asks for — design fit, over-engineering, whether tests would actually fail — is not defect-hunting, which lines up with [[code-review-knowledge-transfer]]: under 15% of review comments are about bugs, and review's real product is shared understanding. "Look at every line" collides with the measured ceilings in [[code-review-throughput-limits]] once the volume of code goes up, which is the whole difficulty in [[reviewing-ai-code]] and the reason [[code-review-principal-agent]] argues agent-authored changes break the process rather than just enlarging it. [[ship-show-ask]] and [[stop-using-pull-requests]] are the responses that reduce how much work has to pass through this checklist at all.