Code you did not write needs a reader
Reviewing code you did not write means reading it closely, running it and asking what it assumes before you accept it. It matters most with a coding assistant, which produces a lot of code quickly and rarely says where it is unsure.
A reviewer is the last person who can stop a fault reaching users.
By the end of this lesson
Note
I can review code I did not write.
Key words
| Word | What it means |
|---|---|
| Review | reading code to find faults before it is used |
| Diff | the lines a change removes and the lines it adds |
| Assumption | something the code takes for granted |
| Edge case | an unusual input where code often fails |
| Trace | to follow code line by line with one sample input |
The cells students fill in are left blank in this preview.
Read the code before you run it
Start with the question "does this do what I asked, and only that?" Then look for what it takes for granted. Then try the odd inputs: empty, zero, negative and very large.
Write down what you find as a question for the author. A reviewer's job is to ask, even when the author is a program.
Worked example: five questions for any code
| Question | What it catches |
|---|---|
| Does it do what I asked? | Code that solves a different problem |
| Does it do anything else? | Extra changes nobody asked for |
| What does it assume? | An input that is never empty or never negative |
| What if the input is odd? | Crashes on empty, zero or huge values |
| Would I understand it next month? | Clever code nobody can maintain |
The cells students fill in are left blank in this preview.
Read the function that formats a price
docker exec -w /home/node/project agent grep -n -A2 "function asPounds" shop.jsShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Try an odd input: a negative amount
docker exec -w /home/node/project agent node -e "console.log(require('./shop').asPounds(-5))"Shown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Read the function that works out change
docker exec -w /home/node/project agent grep -n -A2 "function change" shop.jsShown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Try an odd input: paying too little
docker exec -w /home/node/project agent node -e "console.log(require('./shop').change(50, ['juice']))"Shown here, not run. Create a free account to press Run and type it into your own lab’s terminal.
Two odd inputs, two questions for the author
The first function printed £-0.05 for a negative amount. The second printed -35 when 50 pence was paid for an 85 pence juice. Neither crashed, and neither refused.
A reviewer does not call these faults yet. They write them as questions: what should a negative price show, and what should the till do when the customer has not paid enough?
A trace shows what the code really does
To trace code, pick one input and follow it line by line, writing down each value. Do it by hand first, before you run anything.
The function below is the kind of code an assistant writes. Trace it with two inputs and see what each gives.
A function to trace
# Run it, then change the list to [] and run it again.
def average(numbers):
total = 0
for n in numbers:
total = total + n
return total / len(numbers)
print(average([4, 8]))
Worked example: tracing average with two inputs
| Input | Steps | Result |
|---|---|---|
| [4, 8] | total goes 0, then 4, then 12. Then 12 divided by 2 | 6.0 |
| [] | The loop never runs, total stays 0. Then 0 divided by 0 | An error: division by zero |
The cells students fill in are left blank in this preview.
A diff shows what a change took away
A diff marks removed lines with a minus sign and added lines with a plus sign. Lines with neither are unchanged context.
Read the added lines first. Then read the removed lines and ask what each used to do. A fix can quietly delete a check.
A small diff
- return paid - total(basket)
+ due = total(basket)
+ return paid - due
# the till prints this
A common mistake
Warning
Many people read only the added lines of a change. The removed lines matter just as much. A tidy rewrite can delete a check, and then the code looks cleaner and is less safe.