How I Review Code: 12 Specific Patterns I Look For
TL;DR: Code review without a checklist is a vibes-based meeting. Here's the 12-item checklist I run on every PR — patterns that catch real bugs before they ship.
Why specific patterns beat 'general feedback'
Most code review feedback is too abstract to act on. 'This could be cleaner' doesn't tell anyone what to change. After reviewing thousands of PRs, I narrowed my mental checklist to twelve specific patterns. Each one has a name, a smell, and a fix.
What follows isn't an exhaustive list — it's the patterns that catch the most bugs per minute of review time.
1. Boolean parameters that should be enums
If a function takes (user, true, false), the call site is a riddle. Replace boolean parameters with named values when the meaning isn't obvious from the function name.
// before
sendEmail(user, true, false);
// after
sendEmail(user, { html: true, attachments: false });
// even better
sendEmail(user, { format: "html", attachments: "none" });2. Implicit any
TypeScript's any is a hatch through the type system. Look for it in catch blocks (catch (e) defaults to unknown in strict mode but to any otherwise), in third-party type holes, and in the right-hand side of assignments.
3. Mutation across boundaries
If a function takes a list and modifies it in place AND returns it, that's a trap. Either mutate without returning, or return without mutating. Mixing the two confuses every caller.
# bad — caller can't tell what's safe
def normalize(items):
items.sort()
return [i.lower() for i in items]
# good — return-only
def normalize(items):
return sorted(i.lower() for i in items)4. Magic numbers without a name
if retries < 3 tells you nothing. const MAX_RETRIES = 3 tells the next reader what the number means and where to change it.
5. Errors swallowed silently
try { ... } catch {} is a time bomb. If the error is recoverable, log it. If it's not, propagate. Empty catch blocks mean 'I gave up debugging this' and the next person can't tell.
6. Tests that test the implementation, not the behavior
If renaming a private method breaks a test, the test is testing the wrong thing. Tests should describe behavior the user/caller cares about, not the internals.
7. Database queries inside loops
The N+1 problem is the most common performance bug in web apps. Look for any for ... in ... with a query inside. Fix with a single batched query plus an in-memory join.
# bad — N+1
for user in users:
posts = db.query("SELECT * FROM posts WHERE user_id = ?", user.id)
print(user.name, len(posts))
# good — one query, joined in memory
user_ids = [u.id for u in users]
all_posts = db.query("SELECT user_id, ... FROM posts WHERE user_id = ANY(?)", user_ids)
by_user = {}
for p in all_posts:
by_user.setdefault(p.user_id, []).append(p)
for user in users:
print(user.name, len(by_user.get(user.id, [])))8. Names that lie
getUser() that mutates the database. parseDate() that throws on invalid input but isn't called parseDateOrThrow. Names are the cheapest documentation. Make them tell the truth.
9. Unbounded recursion or unbounded loops
Any loop or recursion needs a clear termination guarantee. If you can't articulate why it always halts, the reviewer shouldn't approve it.
10. Fields that should be private
Public-by-default leaks implementation. If a field has no caller outside the class, mark it private. The compiler/linter will flag the day someone tries to depend on it.
11. Comments that paraphrase the code
// increment counter next to counter++ wastes everyone's time. Comments should explain why, not what. If the what isn't obvious, fix the code.
12. PRs larger than 400 lines
Reviewer attention falls off a cliff after about 400 lines of diff. Big PRs are reviewed worse, not better. Push back on size; ask for the change to be split into a refactor PR and a feature PR.
How to actually use this
I print this list and tape it next to my monitor. When a PR opens, I scan top-to-bottom. The fix isn't to nag the author about all twelve — it's to find the two or three that matter most for this change and write actionable feedback.