Code Review Checklist #
Code review without a clear structure produces two problems that look opposite but are equally dangerous. The first: reviews that are too loose — reviewers approve almost every PR with minimal comments because there’s no guide about what needs checking. The second: reviews that are too strict on the wrong things — lots of energy spent debating naming conventions and formatting while logic bugs and security holes slip through unnoticed.
A checklist is the solution to both problems. Not because checklists make reviews mechanical — quite the opposite. A good checklist frees reviewers from the worry of “did I miss anything?” so they can focus on thinking, not remembering. It ensures the same standards apply regardless of who the reviewer is, how busy their day is, or how familiar the reviewer is with the code area under review.
The important thing to understand: a checklist is a thinking guide, not an obligation list that must all be checked. Not every item is relevant to every PR. The reviewer’s job is using the checklist to determine which items need deep checking for the PR in front of them — not mechanically checking all items.
How to Use This Checklist #
This checklist is split into two perspectives: author (before requesting review) and reviewer (during review). Both matter — a good review is a shared responsibility.
flowchart LR
subgraph Author["Author — before requesting review"]
A1[PR Readiness Checklist]
A2[Self-Review Checklist]
end
subgraph Reviewer["Reviewer — during review"]
R1[Functional & Business Logic]
R2[Code Quality]
R3[Architecture & Design]
R4[Error Handling]
R5[Security]
R6[Performance]
R7[Testing]
R8[Ops & Observability]
end
Author --> |PR ready| Reviewer
Reviewer --> |All areas checked| Decision{Decision}
Decision --> |No blocking issues| Approve
Decision --> |Blocking issues exist| RequestChanges[Request Changes]Author Perspective: Before Requesting Review #
Before adding reviewers, the author is the first reviewer of their own PR. This checklist helps ensure the PR is in a review-worthy condition — not wasting reviewers’ time on issues that should have been found independently.
PR Readiness Checklist #
PR READINESS — basic conditions that must be met:
□ CI is green — linting, tests, and build all pass
✗ "Will fix after review" — red CI means the PR isn't ready
✓ All pipelines green before requesting review
□ PR title uses a consistent format (Conventional Commits)
✗ "Update code" / "Fix stuff" / "WIP"
✓ "feat(auth): add rate limiting to login endpoint"
□ PR description filled in — background, changes, how to test
✗ Empty description or only "see commit messages"
✓ Explains context, technical decisions, and verification approach
□ PR has one purpose summarizable in one sentence
✗ A PR containing refactor + feature + bug fix at once
✓ "This PR adds rate limiting to the login endpoint"
□ PR size is reasonable — not exceeding one-session review ability
✗ A 1500+ line PR without a strong reason
✓ Considered whether it can be split into smaller pieces
Self-Review Checklist #
SELF-REVIEW — read the entire diff from a reviewer's perspective:
□ No leftover debug code
✗ console.log("debug"), fmt.Println("test"), print("DEBUG HERE")
✓ All debug output removed
□ No TODOs without ticket numbers
✗ // TODO: fix this later
✓ // TODO(JIRA-456): optimize this query after load test
□ No commented-out code without a reason
✗ // old_function(x) — code no longer used
✓ Just delete it — git history exists if it's ever needed back
□ No accidentally included files
✗ .env, credentials, binary files, files unrelated to the PR
✓ git diff shows only intentional changes
□ All thought-of edge cases handled or commented
□ Tests run locally and results are green
Reviewer Perspective: Eight Review Areas #
1. Functional & Business Logic #
This is the most important area and the most common source of bugs slipping into production. Code with perfect syntax can still be wrong if its business assumptions are incorrect.
FUNCTIONAL & BUSINESS LOGIC:
Correctness:
□ Does this change do what the PR describes?
□ Is the logic correct for all possible cases?
□ Are there implicit assumptions that aren't documented?
Anti-pattern: a function assuming input is never null
without documentation or assertions ensuring that
Edge Cases:
□ Is behavior correct for empty/null/zero input?
□ Is concurrent access to the same resource safe?
□ Could a race condition occur between two operations?
Anti-pattern: SELECT → check condition → UPDATE without locking
(lost updates can occur if two requests arrive simultaneously)
Backward Compatibility:
□ Is existing, relied-upon behavior preserved?
□ Is there a breaking change not mentioned in the PR description?
□ Are other consumers of this function/endpoint affected?
Guiding question for reviewers:
"If I were a user using this feature in an unusual way,
what could I do to make it fail?"
2. Readability & Maintainability #
Code is read far more often than it’s written. The engineer who wrote this code may not be on the team in a year or two — the code must be understandable by someone without its context.
READABILITY & MAINTAINABILITY:
Naming:
□ Do variable, function, and class names describe their intent?
Anti-pattern: d, tmp, data, result, helper — names giving no information
✓ userRegistrationDeadline, paymentGatewayTimeout, isEligibleForDiscount
□ Is naming consistent with existing codebase conventions?
Anti-pattern: mixing camelCase and snake_case in the same language
Anti-pattern: getUserData() in one place, fetchUserInfo() in another
for identical operations
Functions and Methods:
□ Does each function do one thing only (SRP)?
Anti-pattern: a 200-line function doing validation, calculation,
database operations, and sending emails all at once
✓ Split into smaller functions with clear names
□ Is there duplicated logic that should be extracted?
Anti-pattern: the same validation block appearing in 3 different endpoints
✓ Extract into a shared validateInput() function
Comments:
□ Do comments explain WHY, not WHAT?
Anti-pattern: // increment counter by 1 — the code already shows this
✓ // Using exponential backoff to prevent a thundering herd
// after gateway recovery (see RFC-123)
3. Architecture & Design #
A PR’s architectural quality isn’t always visible from its diff. Understanding how the system works as a whole is needed to judge whether a change is consistent with the agreed direction.
ARCHITECTURE & DESIGN:
Pattern Consistency:
□ Does the change follow the team's agreed architecture patterns?
Anti-pattern: adding business logic directly in a controller
when the team agreed all logic lives in the service layer
□ Are layer boundaries properly maintained?
Anti-pattern: a repository layer aware of HTTP request/response
Anti-pattern: a controller querying the database directly
Dependencies:
□ Is the new dependency truly necessary?
Anti-pattern: adding a 50KB library for one utility function
that could be written in 10 lines
□ Does this change create tighter coupling than before?
Anti-pattern: a previously independent service A now directly
imports and calls service B — unnecessary coupling
Extensibility:
□ Will this design hinder future changes?
(but don't over-engineer for non-existent cases — YAGNI)
□ Are there hardcoded values that should be configurable?
Anti-pattern: timeout hardcoded as 5000ms inside a function
✓ Read from configuration or accept as a parameter
4. Error Handling & Reliability #
Poor error handling is one of the most common sources of production incidents. Code that “works” in normal conditions can fail unexpectedly when an error occurs outside the happy path.
ERROR HANDLING & RELIABILITY:
Error Handling:
□ Are all fail-capable operations' errors handled?
Anti-pattern (Go): ignoring returned error values
result, _ := doSomething() // ← error ignored
✓ result, err := doSomething(); if err != nil { ... }
□ Are error messages informative enough for debugging?
Anti-pattern: return errors.New("error")
✓ return fmt.Errorf("failed to process payment for order %d: %w", orderID, err)
□ Are errors logged at the right level?
Anti-pattern: log.Error for something expected (user not found)
Anti-pattern: log.Info for something needing immediate attention
Resilience:
□ Could one component's failure cause cascading failure?
Anti-pattern: no timeout on HTTP calls to external services
→ one slow service can hang the entire request chain
□ Is a retry mechanism needed?
Anti-pattern: fail once → return error immediately without retry
on operations where transient errors are common (network timeouts)
□ Are resources (connections, files, locks) always released?
Anti-pattern: resource leaks because there's no defer/finally for cleanup
✓ defer conn.Close() immediately after opening the connection
Swallowed errors (caught but nothing done) are among the hardest bugs to find in production. Make sure there’s nocatch (e) {}orif err != nil { return }without logging or proper handling. If deliberately ignored, add an explicit comment explaining why.
5. Security & Data Safety #
Security holes are almost always easier to prevent at code review than to fix after production. Missing one SQL injection or improper authorization check once can become a security incident affecting all users.
SECURITY & DATA SAFETY:
Input Validation:
□ Is all user/external input validated?
Anti-pattern: using input directly without sanitization
✓ Validate type, format, length, and range before processing
□ Is there potential for injection attacks?
SQL anti-pattern: "SELECT * FROM users WHERE email = '" + email + "'"
✓ Parameterized query: "SELECT * FROM users WHERE email = ?" + [email]
XSS anti-pattern: innerHTML = userInput
✓ textContent = userInput, or sanitize with the right library
Sensitive Data:
□ Are credentials, tokens, or PII not logged or exposed?
Anti-pattern: log.Info("User login: email=%s, password=%s", email, pass)
Anti-pattern: returning the entire user object (including password hash)
in an API response
□ Is sensitive data handled per regulations (GDPR, etc.)?
Anti-pattern: storing credit card numbers in plaintext
Anti-pattern: logs storing PII without a clear retention policy
Authorization:
□ Is authorization checked in the right place?
Anti-pattern: authorization only in the UI, not the backend
Anti-pattern: assuming that if a user can log in, they can
access all resources
□ Is there an Insecure Direct Object Reference (IDOR)?
Anti-pattern: GET /orders/{id} without verifying the order
belongs to the currently logged-in user
✓ Always verify ownership before granting resource access
6. Performance & Scalability #
Performance bugs often hide inside code that looks clean and runs well. They only show up when data grows or traffic increases — often at the worst possible time.
PERFORMANCE & SCALABILITY:
Database Queries:
□ Is there an N+1 query problem?
Anti-pattern: queries inside loops
for user in users:
orders = db.query("SELECT * FROM orders WHERE user_id = ?", user.id)
✓ Use JOIN or batch loading:
orders = db.query("SELECT * FROM orders WHERE user_id IN (?)", user_ids)
□ Do queries use existing indexes?
Anti-pattern: WHERE LOWER(email) = ? — functions on columns make indexes unusable
✓ WHERE email = LOWER(?) — function on the value, not the column
□ Does SELECT only fetch needed columns?
Anti-pattern: SELECT * when only 3 of 30 columns are needed
✓ SELECT id, name, email FROM users
Algorithm Complexity:
□ Are there avoidable O(n²) or worse operations?
Anti-pattern: nested loops growing quadratically with data volume
✓ Use hash maps or sorted lists to reduce complexity
□ Are there expensive operations inside hot paths (request handlers, loops)?
Anti-pattern: reading configuration from disk on every request
✓ Load configuration once at startup, cache in memory
Resources:
□ Is there potential for memory leaks?
Anti-pattern: event listeners added but never removed
□ Is caching needed to reduce the load of frequently repeated operations?
7. Testing & Validation #
Tests are executable documentation. Good tests explain what code should do, prove it does it, and act as a guard preventing future regressions.
TESTING & VALIDATION:
Coverage:
□ Are there tests for newly added logic?
Anti-pattern: a PR adding 200 lines of logic without a single new test
✓ Every important path has a test verifying it
□ Do tests cover the happy path AND edge cases?
Anti-pattern: only testing "user logs in successfully" without testing
"user doesn't exist", "wrong password", "account locked", etc.
✓ Tests cover all business-relevant scenarios
Test Quality:
□ Could the test actually fail if the logic breaks?
Anti-pattern: tests only checking that a function runs without error
without verifying output or side effects
✓ Assert specific values, not just "no exception"
□ Are tests easy to understand — clear what's tested and expected?
Anti-pattern: tests with 100 lines of setup without comments about what
is being tested
✓ Descriptive test names + AAA pattern (Arrange, Act, Assert)
TestProcessPayment_WhenBalanceSufficient_ShouldDeductAndReturnSuccess
□ Are tests independent of execution order or global state?
Anti-pattern: test B fails if test A isn't run first
✓ Each test can run alone and produce the same result
8. Ops & Observability #
Code running in production needs to be observable, debuggable, and safely deployable. Problems that can’t be detected or debugged are problems that can’t be fixed.
OPS & OBSERVABILITY:
Logging:
□ Is the added logging enough for production debugging?
Anti-pattern: no logging at all on critical operations
Anti-pattern: overly verbose logging in hot paths (fills the disk)
✓ Useful logs: "Payment processed: order_id=%d, amount=%d, duration=%dms"
□ Are logs using the right levels?
DEBUG: detailed information for development
INFO: normal events worth recording
WARN: something isn't ideal but the system still runs
ERROR: something failed and needs attention
Deployment:
□ Is there a database migration that needs running?
If yes: can the migration be rolled back? Is it backward compatible?
□ Are there new environment variables or configurations required?
□ Is this change safe to roll back if needed?
Anti-pattern: schema changes not backward compatible with old code
Feature Flags:
□ Should high-risk features be wrapped in feature flags?
✓ Feature flags enable gradual rollout and instant rollback without redeploy
Ready-to-Use Checklist Template #
This is a condensed version that can go directly into the team’s PR template. Adapt it to your context and technology stack.
## Code Review Checklist
### Author (before requesting review)
- [ ] CI green (linting, tests, build)
- [ ] PR description complete (background, changes, how to test)
- [ ] PR focused on one purpose
- [ ] Self-review: no debug logs, TODOs without tickets, commented-out code
### Reviewer — Functional
- [ ] Logic correct for all relevant cases
- [ ] Edge cases (null, empty, concurrent) handled
- [ ] No dangerous implicit assumptions
### Reviewer — Code Quality
- [ ] Naming expressive and consistent with the codebase
- [ ] No unnecessary logic duplication
- [ ] Comments explain WHY, not WHAT
### Reviewer — Architecture
- [ ] Follows the agreed architecture patterns
- [ ] Layer boundaries maintained
- [ ] New dependencies justified
### Reviewer — Error Handling
- [ ] All error paths handled properly
- [ ] Error messages informative for debugging
- [ ] Resources always released (connections, files, locks)
### Reviewer — Security
- [ ] User input validated before processing
- [ ] No injection vulnerabilities
- [ ] Sensitive data not logged or exposed
- [ ] Authorization checked in the right place
### Reviewer — Performance
- [ ] No N+1 queries
- [ ] Queries use existing indexes
- [ ] No expensive operations in hot paths
### Reviewer — Testing
- [ ] Tests exist for new logic
- [ ] Tests cover relevant edge cases
- [ ] Tests can fail if logic breaks
### Reviewer — Ops
- [ ] Added logs useful for debugging
- [ ] Migration and deployment notes exist (if needed)
- [ ] Change safe to roll back
How to Adapt the Checklist to Your Team’s Context #
The checklist above is a generic one. Different teams have different needs — backend, frontend, mobile, and data engineering each have unique concerns. The most effective checklist is one adapted to the team’s reality.
Checklist adaptation guidance:
Add stack-specific items:
→ Go backend: "Can goroutines leak? Is context propagated?"
→ React frontend: "Are there unnecessary re-renders? Is accessibility considered?"
→ Mobile: "Does the UI stay responsive? Is battery usage considered?"
→ Database migration: "Can the migration run in production without downtime?"
Remove irrelevant items:
→ Teams with strict linters don't need reviewers commenting on formatting
→ Teams whose APIs are all read-only don't need a SQL injection checklist
Review and update periodically:
→ After every production incident, ask: "Is there a checklist item that
could have prevented this?" If yes, add it.
→ Every quarter, review whether any items are never relevant
and can be removed.
Separate the core checklist from the contextual one:
→ Core checklist: always used for all PRs
→ Specialized checklist: only for PRs touching specific areas
(payment, authentication, data migrations, etc.)
Overly long checklists tend to be ignored — reviewers go through them mechanically without truly considering each item. A shorter checklist that’s actually thought through is better than a 50-item checklist all checked in 30 seconds. Start with a short core checklist and add items only when there’s a clear reason.
Summary #
- A checklist is a thinking guide, not an obligation list — not all items are relevant to all PRs. Use the checklist to determine which areas need deep checking, not to check off mechanically.
- Eight critical review areas — Functional & Business Logic, Readability, Architecture, Error Handling, Security, Performance, Testing, and Ops & Observability. Each has different problem types needing different lenses.
- The author is the first reviewer — a self-review checklist before requesting review removes issues others shouldn’t need to see: debug logs, TODOs without tickets, red CI.
- Functional & Business Logic is the most important area — dangerous logic bugs often aren’t visible from syntax, but from wrong business assumptions or unhandled edge cases.
- Security must not be deferred — security holes are far easier to prevent at code review than to fix after incidents. Watch for injection, authorization, and sensitive data in every PR.
- Poor error handling is an incident source — swallowed errors, unreleased resources, and missing timeouts on external calls are patterns to always watch for.
- N+1 queries are the most common performance bug — easy to miss but impactful when data grows. Always check for queries inside loops.
- Tests that can fail are more valuable than tests that always pass — verify tests truly test important behavior, not just that functions run without errors.
- Observability is part of good code — informative logs, right metrics, and clear deployment notes are part of an engineer’s responsibility, not an afterthought.
- Adapt the checklist to your team’s context — a generic checklist is a starting point. Add items after every incident, remove never-relevant items, and separate the core checklist from the contextual one.