Code review

Code review has two purposes that are often mixed up: finding errors and spreading knowledge. Both presuppose that the review is readable and concrete.

Review in this order — the most expensive error first:

1. Does the change solve the right problem? The wrong solution to the right problem is more expensive than a bug.
2. Are there correctness errors? Edge cases, race conditions, error handling, None.
3. Are there security or privacy risks? Injection, secrets, personal data logged.
4. Are there tests that catch what matters?
5. Is it readable? Names, structure, dead branches.
6. Style — should be handled by a formatter, not by people.

A reviewer who starts at point 6 has missed the point.

Comments that work are specific, justified and preferably come with a suggestion:

❌ «This is wrong.» ✅ «Line 47: if items is empty, sum/len becomes a ZeroDivisionError. Suggestion: return 0.0 or raise ValueError — what does the caller expect?»

Mark the severity so that the author knows what is blocking:

Prefix	Means
blocking:	has to be fixed before merging
suggestion:	should be considered, does not block
question:	I do not understand, please explain
nit:	a matter of taste, feel free to ignore

The size decides the quality. Reviews of PRs above about 400 lines find considerably fewer errors per line — the reviewer tires. Split large changes up.

Receiving it: answer every point, change it or justify why not, and say thank you. The code is not you. Someone who gets defensive gets worse reviews next time — and therefore more bugs in production.

What does not belong in a human review: formatting (a formatter), unused imports (a linter), simple type errors (mypy). Automate them and leave the person to what requires judgement.

Review this diff:

+def get_user(user_id):
+    q = f"SELECT * FROM users WHERE id = '{user_id}'"
+    row = db.execute(q).fetchone()
+    log.info(f"fetched user {row}")
+    return row

Three findings, in order of severity:

1. blocking (security): SQL injection — user_id is interpolated straight into the query. Use a parameterised query: db.execute("SELECT * FROM users WHERE id = %s", (user_id,)).
2. blocking (privacy): the whole user row is logged, probably including personal data. Log only an id.
3. suggestion: SELECT * makes the code depend on the column order and fetches more than necessary. State the columns.

All three are concrete, justified and actionable — and two of them a linter would never have found.