Code review

Sam's loyalty pull request is open, and Jordan has been asked to review it. Jordan hesitates: Sam studies computing at college, and Jordan learned Python from a course. Who is Jordan to judge Sam's code? But Jordan knows things Sam does not: that the till exports refunds as negative amounts, that Mei wants points shown on the receipt, that Anita needs totals in a particular format. Twenty minutes of reading turns up two real problems, a refund would earn points, and a customer at exactly 100 points stays in the Bean tier. Neither shows up in Sam's tests. That is code review working as intended.

This lesson explains why teams review code, what reviewers look for, how to read a change with diffs, how to write comments that help rather than hurt, how to receive feedback well, how approval rules decide when a pull request may be merged and how automation lets people focus on what only people can judge.

Why review code

Code review is the practice of having someone other than the author read a change before it is merged. Its most obvious benefit is catching defects early, when they are cheapest to fix: a bug found in review costs a comment and a small commit, while the same bug found by Mei at the till on a busy Saturday costs much more. Studies of software teams have long found that careful reading catches a large share of defects, including many that testing misses, because a reader questions assumptions that the author's tests share.

The other benefits matter as much. Review spreads knowledge: after reading the loyalty code, Jordan can maintain it when Sam is at college. It keeps the codebase consistent, since reviewers notice when a change ignores the project's conventions. It is also one of the best ways to learn, in both directions: the author learns from the reviewer's suggestions, and the reviewer learns from reading someone else's approach. And it leaves a record, on the pull request, of why the code is the way it is.

Review is not about catching people out. Every programmer writes bugs, and the point of review is to find them before users do. A team where people feel safe asking for review early, and where comments target the code rather than the person, gets all of these benefits. A team where review feels like an exam gets defensive authors and large, late pull requests that are hard to review well.

What reviewers look for

Not every change needs every question. A one-line typo fix needs a glance; a change to how money is calculated needs slow, careful reading. A useful order is to start with the pull request description to understand the intent, then look at the tests to see what the author believes the code should do, then read the code itself. Reading the tests first often reveals missing cases straight away, as it did for Jordan with the boundary between the Bean and Roast tiers.

Reading a change

The hosting service shows a pull request's diff file by file, with removed lines in one color and added lines in another, and lets reviewers comment on any line. Reviewing locally is sometimes easier for larger changes: fetch the branch, check it out, run the program and the tests, and read the code in your editor with all its navigation features. Python's difflib module produces the same unified diff format that Git and the hosting services use, which shows exactly what a diff contains.

import difflib

before = """def points_for(total):
    return int(total)

def tier(points):
    if points > 100:
        return 'Roast'
    return 'Bean'
""".splitlines()

after = """def points_for(total):
    if total <= 0:
        return 0
    return int(total)

def tier(points):
    if points >= 300:
        return 'Reserve'
    if points >= 100:
        return 'Roast'
    return 'Bean'
""".splitlines()

for line in difflib.unified_diff(before, after, 'a/loyalty.py', 'b/loyalty.py', lineterm=''):
    print(line)

--- a/loyalty.py
+++ b/loyalty.py
@@ -1,7 +1,11 @@
 def points_for(total):
+    if total <= 0:
+        return 0
     return int(total)
 
 def tier(points):
-    if points > 100:
+    if points >= 300:
+        return 'Reserve'
+    if points >= 100:
         return 'Roast'
     return 'Bean'

The diff shows Sam's fix after review: refunds and zero totals now earn no points, the boundary uses >= so 100 points reaches Roast, and the Reserve tier exists. The header lines name the old and new files, and each @@ line gives the starting line and length of a hunk in the old and new versions. Unchanged lines around each change, called context, help the reader see where it sits. Diff statistics, the counts of added and removed lines, give a quick sense of a change's size before reading it.

import difflib

def diff_stats(old, new):
    lines = list(difflib.unified_diff(old.splitlines(), new.splitlines(), lineterm=''))
    return (sum(1 for l in lines if l.startswith('+')), sum(1 for l in lines if l.startswith('-')))

print(diff_stats(OLD, NEW))

Writing helpful comments

How a comment is written decides whether it is acted on or resented. Comment on the code, not the person: this function returns early for negative totals rather than you forgot negative totals. Be specific about the problem and explain why it matters: a refund is exported as a negative total, so int(-18.5) gives -18 points and the customer loses points they earned. Suggest a fix when you have one, but leave room for the author to find a better one. Ask genuine questions when you are unsure; sometimes the author had a reason you had not considered.

It helps to say how much each comment matters. Many teams label comments with a short prefix. A blocking comment, or issue, describes something that must change before merging, such as a bug. A suggestion is an improvement the author may take or leave. A question asks for an explanation. A nit, short for nitpick, is a minor point of style or wording that should not hold up the merge. Praise belongs in reviews too: pointing out a clever test or a clear function tells the author what to keep doing.

issue (blocking): A refund arrives as a negative total, so int(-18.5)
gives -18 and the customer loses 18 points. Can points_for return 0
for totals at or below zero? A test for a -18.50 refund would cover it.

question: Should 100 points already count as Roast? The tier table in
issue #21 says Roast starts at 100, but this uses > 100.

nit: tier_name might read better than tier, since it returns a string.

praise: The table-driven tests for points_for are really easy to follow.

def label_counts(comments):
    return {}

def blocking(comments):
    return False

print(label_counts(COMMENTS))

Receiving review

Being reviewed is a skill too. The comments are about the code, and even experienced developers receive many of them; treat each one as free help. Respond to every comment, either by making the change or by explaining why you think the code should stay as it is, and keep the discussion on the pull request so the reasoning is recorded. If a thread goes back and forth more than twice, talk instead, then write the outcome in the thread.

Make the requested changes as new commits on the same branch and push them; the pull request updates automatically, and the reviewer can see exactly what changed since their last look. Mark threads as resolved when they are handled. Then ask for another review. Sam pushes two commits, one for the refund case with a new test and one for the tier boundary, and replies to the nit: good idea, renamed.

Approval rules

A reviewer finishes by submitting a review with one of three outcomes. Approve means the change can be merged. Request changes means something must be fixed first. Comment gives feedback without a decision. Branch protection, from lesson 6, can require a number of approvals before the merge button works, and hosting services apply sensible rules: authors cannot approve their own pull requests, a reviewer's latest review is the one that counts, and new commits can optionally dismiss earlier approvals so that reviewers see the final version.

def can_merge(author, reviews, required=1):
    return any(state == 'approved' for _, state in reviews)

print(can_merge('sam', [('jordan', 'changes_requested'), ('jordan', 'approved')]))

Letting machines do the routine checks

Reviewers' attention is limited and valuable, so it should not be spent on things a program can check. Continuous integration, or CI, is a service that runs automatically on every pull request: it installs the project, runs the linter and formatter checks and runs the tests, then reports the results on the pull request as passing or failing checks. Later courses in this program set up a full CI pipeline. With CI in place, reviewers know the tests pass and the formatting is right before they start, and they can spend their time on correctness, design and naming.

Two more conventions help larger teams. A CODEOWNERS file lists who must review changes to which parts of the project, so a change to the payment code automatically asks for the person who knows it best. And a size limit, enforced by habit or by a check, keeps pull requests small enough to review well: when a change grows past a few hundred lines, split it.