Lesson 5 of 6
Your capstone works. That's not the same as being finished.
Code review is reading code deliberately, looking for what's wrong with it — before someone else finds out the hard way. On a team, someone else does it for you. Alone, you do it for yourself, and it's the single fastest way to improve.
Do it on a different day from writing. You can't see your own code clearly an hour after writing it.
The checklist
Does it actually work?
- [ ] The normal case produces the right answer — checked against something you computed by hand
- [ ] Empty input: empty list, empty file, empty string, zero
- [ ] One item, and a lot of items
- [ ] Bad input: wrong type, missing key, malformed file, negative number
- [ ] Running it twice in a row does the right thing
- [ ] Nothing is left behind — temp files cleaned up, files closed
Can someone read it?
- [ ] Every name says what the thing is (
total_spent, nott) - [ ] Functions do one thing, and their name says which
- [ ] Nothing is nested more than three levels deep
- [ ] No function longer than about 30 lines
- [ ] No number appears with no explanation — name it (
MAX_RETRIES = 3) - [ ] Comments say why; the code says what
- [ ] Public functions and classes have docstrings
- [ ] PEP 8 clean (lesson 9.8) — let
rufforblackdecide
Does it fail well?
- [ ] Every
exceptnames the exception it's catching — never bareexcept: - [ ] No
except: passthat hides a real problem - [ ] Error messages tell the user what to do, not what Python thinks
- [ ] The user never sees a raw traceback
- [ ] Files, connections, and locks are opened with
with - [ ] Data is validated at the boundary, where it comes in
Is it safe?
- [ ] No passwords, API keys, or tokens in the source — environment variables only (lesson 8.3)
- [ ] Nothing secret in the git history either
- [ ] No
eval()orexec()on anything a user typed - [ ] Nothing overwrites or deletes a file without checking first
- [ ] Destructive operations have a dry-run mode (lesson 10.3)
Can you change it in six months?
- [ ] Tests for the core logic, and for every bug you've fixed (lesson 9.9)
- [ ] A
README.md: what it is, how to install it, how to run it - [ ] A
requirements.txt(lesson 7.6) - [ ] A
.gitignore—__pycache__/,.venv/,*.jsondata files,.env - [ ] Configuration is separate from logic — no hardcoded paths
- [ ] Logic is separate from interface: the class computes, the CLI prints
Review this
Here's a function that works, reviewed and rewritten. Same answer, different code.
Everything the checklist caught in six lines:
| Problem | Fix |
| --- | --- |
| p, d, f, x, y, t | Names that say what they hold |
| File never closed | with open(...) |
| No encoding | encoding="utf-8" — the default varies by platform |
| for i in range(len(y)) | Loop over the items |
| t = t + ... accumulator | sum(...) with a generator |
| Prints and returns | Return the value; let the caller print |
| "total is " + str(t) | f-string |
| No docstring | One line saying what it returns |
That's not style pedantry. The version with with open can't leak a file handle. The version
that only returns can be tested and reused. The sum version has nowhere for an off-by-one to
hide.
Three traps a checklist catches
These are real bugs, they don't raise errors, and every Python programmer has shipped at least one.
Mutable default argument. basket=[] is evaluated once, when the function is defined — so
every call shares one list. Look at that output: the basket keeps filling up across separate
calls. Default to None and build the real value inside.
Bare except. It catches everything, including the NameError from your typo three lines
down. You'll spend an hour on a bug the interpreter would have handed you instantly. Name your
exceptions, and return what you rejected instead of dropping it silently.
Float equality. 0.1 + 0.2 == 0.3 is False. Compare with a tolerance, or use integer
cents (lesson 10.2).
Let tools do the boring half
pip install ruff pytest
ruff format . # rewrite to a consistent style
ruff check . # unused imports, shadowed builtins, bare except, and more
pytest # run the tests
ruff check catches a real fraction of the checklist above, in under a second, without
opinions. Add mypy if you want type checking, and pytest --cov to see which lines your tests
never touch.
Run these before every commit. What's left for your human review is the part tools can't do: is this the right thing to build, and will the next person understand it?
The README
Nobody can use a project they can't run. Yours needs, at minimum:
# Expense Tracker
A command-line tool for recording expenses and reporting on them by month.
## Install
python -m venv .venv
source .venv/bin/activate
pip install -r requirements.txt
## Use
python tracker.py add --amount 12.50 --category food
python tracker.py report 2026-03
Data is stored in `expenses.json` in the working directory.
## Run the tests
python -m unittest
Write it for yourself in six months, because that's who'll read it.
Reviewing other people's code
Once you're working with other people, the rules change:
- Review the code, never the person. "This loop could be a comprehension," not "you always do this."
- Ask, don't decree. "What happens if
itemsis empty?" invites a fix. "This is broken" invites a fight. - Say what's good. A review that's only criticism is exhausting to receive, and you learn from good code too.
- Separate must-fix from nice-to-have. Prefix the optional ones: "nit: extra blank line."
- Approve when it's better than what's there. Perfect is not the bar; better is.
And when you're on the receiving end: it's not about you. Every review comment is someone spending their time making your code better.
Checkpoint
- Review on a different day from writing.
- Five questions: does it work, can it be read, does it fail well, is it safe, can it be changed?
- Watch for mutable defaults, bare
except, and float equality. ruffandpytestdo the mechanical half — run them before every commit.- A README and a
.gitignoreare part of "done". - Review code, not people; ask questions; say what's good.
Questions about this lesson
Log in to ask a question.