Reviewing Django code

Code review is where a codebase's quality is actually maintained — not by rules written in a document, but by another engineer reading your change before it ships and asking the questions this course has been teaching you to ask. This lesson is a Django-specific review checklist: what to look for in a pull request, in roughly the order of how much damage each issue causes. It works whether you are reviewing someone else's code or — just as importantly — reviewing your own before you open the PR.

Why review, and the mindset

The point of review is not to prove the author wrong; it is to catch, cheaply and early, the problems that are expensive later — a security hole, a data-loss bug, an N+1 that will crawl on production data. A good reviewer is a second pair of eyes on exactly the things a tired author misses. And on this course, where merging is delegated and changes go live, you are often your own reviewer — so the checklist is a discipline to run on your own diff before you ship, not only on others'. Review the change, not the person; ask questions ("what happens if this is called concurrently?") rather than issue verdicts.

The security pass — first, because it is the most dangerous

Security issues cause the worst outcomes, so look for them first:

• Is untrusted input validated? Form/serializer validation on anything from a user; no mark_safe/ |safe on user data (XSS); no string-formatted SQL (injection).
• Are permissions enforced on the server? Every protected view has login_required/permission_required or the DRF equivalent — not just hidden in the template. Object-level access is scoped to the user for sensitive records.
• Are secrets out of the code? No SECRET_KEY, password, or API key committed; config from the environment.
• Is DEBUG handling correct and are security defaults intact (no needless @csrf_exempt)?

A single missed permission check or a committed secret is worse than any number of style issues, which is why this pass comes first.

The data-integrity pass — second, because it is the hardest to undo

Bugs that corrupt or lose data are the next most costly:

• Are related writes wrapped in transaction.atomic? A multi-step operation that could leave a half-state must be atomic.
• Is there a read-modify-write that should be F() or a lock? A counter or balance adjusted in Python is a lost-update race under concurrency.
• Do on_delete choices match intent? CASCADE where deletion should cascade, PROTECT where history must survive — a wrong choice silently deletes or blocks.
• Are invariants enforced by constraints, not just Python checks that a raw query could bypass?

Data bugs often cannot be fixed after the fact (the data is already wrong), so scrutinise anything that writes.

The performance pass — third, because it fails silently at scale

The issues that pass tests and demos but die on production data:

• Any N+1? A loop crossing a relationship without select_related/prefetch_related. Check every template loop and view that iterates objects. Ask for the query count.
• Summaries done in Python that should be aggregate/annotate.
• Missing pagination on a list endpoint or view that could return unbounded rows.
• A filtered/sorted column without an index where the table will grow large.

These never show up with ten rows in development, so they must be reasoned about in review — "what does this do with 50,000 appointments?" is the reviewer's job to ask.

The design and correctness pass

Then the everyday quality questions:

• Is logic in the right layer? Business rules on the model/service, not crammed in the view; queries on the manager, not scattered. A fat view is a smell.
• Are the failure cases handled and tested? Not just the happy path — invalid input, missing objects, unauthorised access. Are there tests, and do they cover the failures?
• Do names say what things are? A view, model, method or variable whose name matches its job; British clarity over cleverness.
• Is anything needlessly complex? A hand-rolled query where a manager method exists; a CBV maze where a function view would read better; a |safe or except Exception that signals a wrong turn.
• Are exceptions handled honestly (narrow catches, nothing swallowed) and is there logging where a production failure would otherwise be invisible?

A practical review checklist

Pulling it into a list you can actually run against a Django PR, in priority order:

1. Security — input validated, permissions enforced server-side, secrets not committed, defaults intact.
2. Data integrity — atomic multi-writes, no lost-update race, correct on_delete, constraints for invariants.
3. Performance — no N+1, DB-side summaries, pagination, indexes where needed.
4. Design — logic in the right layer, thin views, named queries.
5. Correctness & tests — failure cases handled and tested, not just the happy path.
6. Clarity — good names, no needless complexity, honest error handling and logging.
7. Migrations — included and committed; no data-destroying migration slipped in unnoticed.

Run these in order and stop worrying about item 6 on a PR that fails item 1 — fix the dangerous things first. This checklist is the whole course, compressed into what to look for: every item traces back to a lesson, and being able to apply them to a real diff — your own or a colleague's — is what "worth hiring" actually means.

Check your work

Why review, and the mindset. To catch expensive problems cheaply and early; review the change not the person, ask questions; on this course you are often your own reviewer, so run it on your own diff.

The priority order. Security first (most dangerous), then data integrity (hardest to undo), then performance (fails silently at scale), then design, correctness/tests, and clarity.

The security pass. Validated input, server-side permissions, no committed secrets, defaults intact.

The data pass. atomic multi-writes, F()/locks for read-modify-write, correct on_delete, constraints for invariants.

The performance pass. N+1, Python summaries, missing pagination, missing indexes — reasoned about for production scale, since demos hide them.

The rest. Logic in the right layer, failures handled and tested, clear names, no needless complexity, honest error handling, and migrations committed.

Practice

1. Take a PR (or your own recent change) and run the seven-item checklist in order; write one comment per issue found.
2. Review a diff specifically for N+1: find every loop crossing a relationship and ask whether it is eager-loaded.
3. Review for security: check every new view for a server-side permission check and every user input for validation.
4. Find a piece of business logic in a view during review and suggest where it should move.
5. Review your own next change before opening the PR, catching at least one issue you would have shipped.
6. For a change that writes to several models, verify it is atomic and reason about the concurrent case.

Official documentation

• Django — Security in Django — The security items to check.
• Django — Database optimization — The performance items.
• Django — Coding style (contributing) — Django's own conventions, a useful baseline.

Next: Django already encodes the patterns.