What a code review is for
A code review is when a teammate reads your change before it joins the main code. It catches faults you cannot see in your own work, and it spreads knowledge of the code around the team.
A useful review is a skill. It looks at what the change does, not only how it looks.
By the end of this lesson
Note
I can review a small code change by running it and writing comments that help.
Key words
| Word | What it means |
|---|---|
| Diff | a list of the lines a change removes and adds |
| Code review | a teammate reading a change before it is merged |
| Edge case | an unusual input, such as an empty list, that code must still handle |
| Review comment | a note on one place in a change, saying what to look at again |
| Leftover | something a developer forgot to remove, such as a debug print |
The cells students fill in are left blank in this preview.
A change to review
A teammate wants to tidy a small function. It averages the ratings customers gave a product. Make two folders: before holds the code today, and after holds the code with their change.
Your job is to review it, as you would on a pull request.
Make the two folders
mkdir -p ~/reviewtest/before ~/reviewtest/afterShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Go into the project
cd ~/reviewtestShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Write the code as it is today
printf 'def average_rating(ratings):\n if len(ratings) == 0:\n return 0.0\n total = 0\n for rating in ratings:\n total = total + rating\n return total / len(ratings)\n' > before/ratings.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Write the teammate's version
printf 'def average_rating(ratings):\n print("ratings", ratings)\n return round(sum(ratings) / len(ratings), 1)\n' > after/ratings.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Read the diff before anything else
A diff shows only what changed. A line that starts with - was removed. A line that starts with + was added. A line that starts with a space is context, left as it was.
The @@ line says where the change sits. Here the old file runs from line 1 for 7 lines, and the new file from line 1 for 3.
Show the change as a diff
diff -u before/ratings.py after/ratings.py | tail -n +3Shown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Ask four questions of every change
Does it do what it says? The change says it tidies the function.
What happens at the edges: an empty list, one item, a very large one? Is there a test that would notice? Is there anything left over, or unclear, such as a debug line or a vague name?
Run the change, do not only read it
Reading shows what looks wrong. Running shows what is wrong. Try the same calls on both versions, including the empty list. PYTHONPATH tells Python which folder to import from.
Try the old code
PYTHONPATH=before python3 -B -c "from ratings import average_rating; print(average_rating([4, 5]), average_rating([]))"Shown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Try the new code on two ratings
PYTHONPATH=after python3 -B -c "from ratings import average_rating; print(average_rating([4, 5]))"Shown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Try the new code on an empty list
PYTHONPATH=after python3 -B -c "from ratings import average_rating; print(average_rating([]))" 2>&1 | grep ErrorShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Search the new code for leftovers
grep -n "print(" after/ratings.pyShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Write a comment that helps
A useful comment names the place, says what you saw, says why it matters, and offers a way forward. It speaks about the code, not the person.
A comment that only says "bad" or "fix this" gives the author nothing to act on.
Worked example: weak comments and useful ones
| Weak comment | Useful comment |
|---|---|
| This is wrong | Line 1: average_rating([]) now raises ZeroDivisionError. The old code returned 0.0. Could we keep that case and add a test? |
| Remove this | Line 2: this print will write to the logs on every call. Was it left from debugging? |
| Why round? | Line 3: rounding to 1 decimal changes 4.33 to 4.3 for every caller. Is that wanted? If so, say it in the name or the title. |
| Needs tests | No test changed. A test for an empty list and one for a single rating would protect the next edit. |
The cells students fill in are left blank in this preview.
A common mistake
Warning
Many reviewers check only names and spacing, then approve. Style matters least. Run the change, try the empty case, and ask whether a test would notice if it broke.
Going further: review your own change
Make a small change to one of your own functions, such as adding a feature. Make a copy of the old file first.
Run diff -u on the two, then answer the four questions as a reviewer would. Write one comment you would be glad to receive.