Reading code, and reviewing it without being unbearable

Code review is the part of the job nobody teaches and everybody does. For a junior engineer it is also the single fastest way to get better — in both directions.

Reading code you did not write

You will join a team with a codebase of a hundred thousand lines and be asked to fix something in week one. Nobody will explain all of it, because nobody can.

Start from a behaviour, not from the top. Do not read the codebase. Pick something it does — "what happens when an expense is added?" — and follow it, depth-first, one path at a time.

Find the entry point. if __name__ == "__main__":, a CLI command, a route, a test. Read it, note what it calls, go one level down. Stop when you have your answer, not when you have read everything.

Use the debugger as a reading tool.

breakpoint()          # built in since 3.7 — drops you into pdb right here

Then n to step, s to step into, l to list the code around you, p expr to print, w for the stack, c to continue. Ten minutes of stepping beats an hour of guessing, and the stack tells you the structure faster than any diagram.

Read the tests first when you can. A test says what the code is for, in the author's words, with a worked example. test_expense.py tells you the validation rules faster than expense.py does.

git log on a confusing file. "Why is this here?" is often answered by the commit that added it — a strong argument for writing commit messages that explain why.

Resist rewriting on sight. Code you do not understand looks worse than it is. Assume there is a reason and go looking. Sometimes there is not, and by then you can say so with evidence.

Reviewing somebody else's change

The point is finding problems while they are cheap, and spreading knowledge of the codebase. It is not proving you are clever.

In rough order of value:

Does it do what it says? Read the description, then the diff. A change doing more than it claims is the commonest real problem and the easiest to miss.

What happens when it fails? The happy path is usually fine. Ask about the empty list, the None, the missing key, the file that is not there.

Is it tested, and does the test assert anything? A test that calls a function and asserts nothing is coverage, not a test.

Would you be able to debug this at 2am? Names, error messages, log lines.

Is there a simpler version? Ask, do not assert — there is often a reason.

What not to spend review on: formatting, import order, line length. Ruff and a formatter settle those before the diff is opened, and arguing about them in review is how review gets a reputation as an obstacle. That is the real argument for ruff format from module 10 — not that it is prettier, but that it ends a category of conversation.

How to word it

The difference between a review people act on and one they resent is almost entirely tone.

"This is wrong."
"Why didn't you use a dict here?"
"I would never write it this way."

versus

"If `expenses` is empty, does this return 0 or raise? I think the report
would show a total of zero, which might read as 'nothing spent'."

"A dict keyed by category here would mean the report loop stops scanning —
worth it, or are there never enough rows to matter?"

"Nit: `e` is fine in the comprehension, but `expense` in the function above
would help."

Three habits do most of the work:

Ask rather than assert. You are frequently missing context. A question gets the reason; a statement gets defensiveness.

Mark the small stuff as small. "Nit:" tells the author it is optional and separates it from the thing that matters.

Say what is good. Genuinely. Review that is only ever criticism is review people start avoiding.

And the most important: review the code, not the person. "This function does two things" rather than "you did two things in this function". A smaller difference on the page than it is in the reading.

Taking a review

Harder than giving one, especially early on.

It is not about you. A comment on your code is a comment on your code. Every engineer you admire has had their work pulled apart in review; that is where they learned it.

Assume good faith even when the wording is blunt. Plenty of good engineers write terse reviews. Terse is not hostile.

Answer every comment, even if only "done" or "good catch". Silence reads as disagreement.

Disagree when you are right. "I tried the comprehension and it used 2GB on the real file, which is why the loop is there" is a complete answer, and a reviewer who gets it learns something. Reviewers are not always right, and a junior who never pushes back is not learning to think.

Ask when you do not understand. "I do not know what you mean by idempotent here" costs nothing and is how vocabulary gets into your head.

What makes a change reviewable

Half of a good review is the author's doing.

Small. A 60-line diff gets real review. A 2,000-line diff gets "looks good to me", which is a rubber stamp with your name on it.

One thing. Do not mix a refactor with a behaviour change. The reviewer cannot tell which lines changed what, and neither can git blame later.

A description that says why. What changed is in the diff. Why it changed is not, and that is what a reviewer needs to judge the approach.

Already reviewed by you. Read your own diff before sending it. You will find the print() you left in, the commented-out block and the badly named variable yourself — and every one you catch buys attention for the things only another person can see.

A print() left in a diff is the most common thing a Python reviewer finds, and it is the easiest to catch yourself: search the diff for print( and breakpoint() before you send it.

What this is really for

An intern joining a team is often surprised that review is where most of the learning happens. Not the tutorials — the fifteen comments on your first pull request, each a piece of context nobody would have thought to tell you otherwise.

So: send small changes early, ask why rather than defending, and review other people's work even when nobody asked. Reading other people's code attentively is the fastest way to get good at writing your own.

Check your work

How to read an unfamiliar codebase: start from a behaviour, follow one path depth-first, stop when you have the answer.

What breakpoint() gives you: pdb at that line, and a stack that shows structure faster than a diagram.

Why read tests first: they say what the code is for, with worked examples.

What review is for: finding problems while they are cheap, and spreading knowledge.

The order to look in: does it do what it says, what happens when it fails, is it really tested, could you debug it at 2am, is there a simpler version.

What not to review: formatting — that is what the formatter is for, and ending that conversation is the point of it.

The three tone habits: ask rather than assert, mark nits as nits, say what is good.

The framing: review the code, not the person.

How to take a review: answer everything, disagree when you are right, ask when you do not understand.

What makes a change reviewable: small, one thing, a description saying why, and read by you first.

Practice

1. Open your capstone and trace what happens when an expense is added, from entry point to storage. Write the call chain down.
2. Do it again with breakpoint() and n/s. Compare how long each took.
3. Read one of your test files before its module. Write down the rules you learned.
4. Find a confusing line in any project and run git log -p on that file until you find the commit that added it.
5. Review a pull request in an open-source Python project. Write three comments you would leave, then check them against the tone habits.
6. Rewrite this as something you would send: "this is wrong, use a dict".
7. Review your own last commit as a stranger. List what you find.
8. Search a diff of yours for print( and breakpoint().
9. Split a change into two commits — one refactor, one behaviour.
10. Ask somebody to review something of yours, and answer every comment even if only to agree.

Next: the capstone — building the whole thing to the standard this module just set.