Code Review

Code Review

Review code the way a principal engineer would — not just "does it work?" but "will this cause problems at 3am?"

Review Dimensions

Every review should evaluate these dimensions, in order of importance:

Use an LSP if one is connected. If LSP tools are available (check your tools for a language server — TypeScript, Pyright, gopls, rust-analyzer), use them instead of grep-guessing. find references on a changed function tells you the real blast radius; go to definition confirms a call signature actually matches; rename/diagnostics surface type errors the eye misses. A review that verifies call sites with an LSP catches breakage that a text-only review ships. If no LSP is connected, fall back to Grep/Glob and say so.

1. Correctness

The code must do what it claims to do.

• Does the logic match the requirements/spec?
• Are edge cases handled? (empty input, null, max values, concurrent access)
• Are error paths tested, not just happy paths?
• Does it handle the "what if this is called twice?" scenario?
• Are race conditions possible? (async operations, shared state, database transactions)

2. Security

Think like an attacker for every piece of new code.

• Input validation: Is user input validated before use? (URL params, request body, query strings)
• IDOR: Can User A access User B's data by changing an ID? (check every route with :id params)
• Injection: Is user input ever interpolated into SQL, shell commands, or HTML?
• Auth: Are new endpoints protected by auth middleware? Are permissions checked, not just authentication?
• Secrets: Are any credentials hardcoded? Any new env vars documented?
• Data exposure: Do API responses leak internal fields? (password hashes, internal IDs, other users' data)

3. Performance

Will this work at 10x the current load?

• N+1 queries: Database calls inside loops. The #1 performance killer in web apps.
• Missing indexes: New columns used in WHERE/JOIN without index.
• Unbounded queries: findMany() without take/limit. Will return 1M rows when the table grows.
• Sync I/O: readFileSync, execSync in request handlers. Blocks the event loop.
• Sequential awaits: Independent awaits that should be Promise.all().
• Memory leaks: Module-scoped arrays with .push(), event listeners added in request handlers.
• Over-fetching: Selecting all columns when only 2 are needed. Returning full objects when IDs suffice.

4. Maintainability

Will the next person (including future-you) understand this in 6 months?

• Naming: Do variable/function names describe what they do, not how they do it?
• Complexity: Can any function be broken into smaller, testable pieces?
• Abstraction level: Is the code at a consistent level of abstraction? (mixing HTTP parsing with business logic is a smell)
• DRY violations: Is the same logic duplicated in multiple places?
• Dead code: Are there unused functions, imports, or variables?
• Comments: Are they explaining "why," not "what"? Comments that restate the code are noise.