Code review
What to look for, how to give and take feedback, and automating the routine checks.
Create a free account Try part of it here
Why it matters
On most teams, every change is read by someone else before it merges. Code review catches bugs, spreads knowledge of the codebase, and keeps it consistent. Done badly, it's slow, petty or hurtful, and people learn to dread it. Done well, it's one of the fastest ways to grow as an engineer, on both sides. Reviewing well is also how you'll check the work of AI coding tools, which write more and more code that people approve.
What you'll be able to do
- Review a change in a useful order: purpose, correctness, tests, design, security, then style
- Write review comments that are specific, kind and labeled by how much they matter
- Receive feedback well, and make your own pull requests easy to review
- Automate the routine checks by reading a unified diff in code
What it covers
- What to look for, in order
- A bug the tests didn't catch
- Writing comments
- Being reviewed
- A check so reviewers don't have to
- Parsing a hunk header
- Passing tests aren't a review
What to look for, in order
An editor reads a manuscript for the story first and for commas last. There's no point polishing a chapter that's about to be cut. Read code in the same spirit:
- Purpose. Does it solve the right problem? Read the description and the linked issue first.
- Correctness. Edge cases: empty input, None, very large values, time zones, concurrency, errors from other services.
- Tests. Is the new behavior tested? Would the tests fail if the code were wrong?
- Design. Does it fit the codebase? Is there a simpler way? Will the next person understand it?
- Security. Untrusted input, SQL built from strings, secrets, permissions, logging private data.
- Performance, where it matters: a query inside a loop, loading a whole table into memory.
- Readability. Names, comments where the why isn't obvious, dead code.
Formatting and simple style rules belong to tools (formatters and linters in CI), not people. Arguing about spaces in review wastes everyone's time.
A bug the tests didn't catch
This function passes its test. A reviewer who asks "what about empty input?" finds what the test missed.
def average_order(orders): return sum(orders) / len(orders) print(average_order([10, 20, 30])) # the only test try: print(average_order([])) # a new customer has no orders except ZeroDivisionError as err: print("crash:", err) def average_order(orders): return sum(orders) / len(orders) if orders else 0 print(average_order([]))
Empty, None, huge, duplicate and negative are the first inputs to think about in review. Then ask whether 0 is really the right answer for no orders, or whether it hides a mistake. That's a question for the author, so it makes a good review comment.
Writing comments
A good coach says "keep your elbow in", not "that was bad". Review comments work the same way:
- Be specific. Not "this is confusing" but "
process()does three things; could the retry logic be its own function?" - Explain why. "This query runs once per order, so 1,000 orders means 1,000 queries."
- Label severity. Prefixes make expectations clear: blocking (must fix), suggestion (worth doing, your call), nit (tiny, optional), question (help me understand).
- Comment on the code, not the person. "This function could..." rather than "you forgot...".
- Say what's good. A clear test or a neat fix deserves a word.
- Be quick. Review within a day; waiting blocks your teammate.
In plain words: say what you noticed, why it matters and how much it matters.
Look at purpose, correctness and tests first, and style last.
Show the answer
blocking: amount comes from the request and goes straight into the SQL string, so it's open to injection. Use a parameter.
It says how much it matters, what the problem is, why, and what to do instead.
Being reviewed
- Review your own diff first. It's like rereading an email before you send it. You'll catch half the comments before anyone else sees them.
- Keep it small. Small pull requests get faster and better reviews. Split refactors from behavior changes.
- Write the description: what, why, how tested, what's risky, screenshots for UI.
- Assume good intent. Comments are about the code. Reply to each one: fix it, or explain why not.
- Disagree with reasons, and move long debates to a quick call.
Reading a diff
Reviews happen on diffs, which work like "track changes" in a word processor. The unified format shows each file, then hunks, the separate passages that changed:
--- a/shop/cart.py
+++ b/shop/cart.py
@@ -10,4 +10,5 @@ def total(items):
subtotal = sum(i.price for i in items)
- return subtotal
+ print("debug", subtotal)
+ return round(subtotal * 1.2, 2)
@@ -10,4 +10,5 @@ means this hunk covers 4 lines from line 10 of the old file and 5 lines from line 10 of the new one. Lines starting with - were removed, + added, and a space means unchanged context. Counting from the hunk header gives each added line's number in the new file, which is where a review comment goes.
Automate the routine
You run the spell-checker before you bother an editor. Bots can catch what humans shouldn't spend attention on: leftover debug prints, new TODOs, possible secrets, missing tests, huge pull requests. AI review tools can give a useful first pass too. People still own the review: design, intent and "is this the right change?" need judgment.
A check so reviewers don't have to
If a rule can be checked by a tool, it shouldn't take up a person's attention. This runs over the added lines in a diff.
import re RULES = [(re.compile(r"\bprint\("), "left-over print"), (re.compile(r"(api_key|password)\s*=\s*['\"]"), "possible secret in code"), (re.compile(r"\bTODO\b(?!.*#\d+)"), "TODO with no issue number")] diff = """+++ b/app.py +def charge(order): + api_key = "sk-live-123" + print(order) + # TODO handle refunds - old_line() + # TODO(#412) retry on 503""" for n, line in enumerate(diff.splitlines()): if line.startswith("+") and not line.startswith("+++"): for pattern, why in RULES: if pattern.search(line): print(f"line {n}: {why}: {line[1:].strip()}")
It checks only added lines, so nobody gets blamed for old code. And a TODO that names an issue passes. Run this in CI, and reviewers can spend their attention on design and correctness.
@@ -20,3 +22,4 @@. On which line of the new file does the hunk start?Show the answer
22
The + part is the new file: start at line 22 and cover 4 lines. The - part describes the old file.
Parsing a hunk header
A regular expression pulls out where the new lines start.
import re header = "@@ -10,4 +10,5 @@ def total(items):" m = re.match(r"@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@", header) old_start, old_len, new_start, new_len = m.groups() print("new file starts at line", int(new_start), "for", int(new_len or 1), "lines")
The counts are optional in the format: a missing count means 1.
import re m = re.match(r"@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@", "@@ -7 +7,2 @@") print(m.group(2), m.group(4))
Show the answer
None 2
The old side has no count, so its group is None, which means 1 line. The new side covers 2 lines.
Passing tests aren't a review
A car can pass its inspection and still be the wrong car for your family. The tests tell you the code does what the tests check. They can't tell you it's the right thing to build, that it handles a case nobody thought of, or that the next person will understand it. So don't approve only because CI is green. Read the code asking what could go wrong that no test covers.
Spend attention where it counts
Reviewers have limited attention, and it's easy to use it up on spacing and variable names. A formatter and a linter can handle those. If you find yourself writing the same style comment for the third time, add a tool.
Write comments people can act on
"This is confusing" leaves the author guessing. Compare it with this: "I had to read this twice. Could process be named apply_discounts, since that's all it does?" The second says what happened, why it matters and what might help. Try to phrase things as questions when you're unsure, since the author may know something you don't. And mark what's optional: starting with "nit:" tells the author it shouldn't block the change.
Taking feedback
Comments are about the code, even when they don't feel like it. It helps to assume good intent, and to ask for an explanation rather than defending the change at once. If a discussion goes back and forth more than twice, switch to a quick call, since tone gets lost in text. Reviewers often see a problem you can't see any more because you've been looking at it for days. That's the point of having one.
Practice what you just read
- 3 code examples you can edit and run
- a practice exercise that checks your code
- a 5-question quiz
- 4 review cards for spaced repetition
Try a quiz question
Who should enforce formatting and simple style rules?