Pull-request review · correctness, security, readability, tests, performance, dependencies
Review code for the problems that actually matter.
A practical checklist for reviewing a pull request — correctness and edge cases, security, readability, tests, performance, and dependencies. Use it to catch real defects and design problems instead of arguing about whitespace, and to make sure every reviewer on the team is looking for the same things.
Why a code review checklist
Code review is one of the highest-leverage habits a team has, and it is also where that leverage quietly leaks away. Without a shared idea of what a review is for, two reviewers look at the same diff and check completely different things — one rewrites the variable names, the other waves through a missing authorization check. The result is reviews that feel thorough but miss the defects that actually reach production. A checklist turns review from a matter of taste into a repeatable pass everyone runs the same way.
This is the checklist we apply on every change we ship, and the discipline that sits behind our QA and test automation work. None of it is exotic — it is the boring, ordered attention that keeps a codebase from accumulating technical debt one rushed merge at a time. The single biggest force multiplier is automation: let CI handle formatting and linting so humans spend their attention on the things a tool cannot judge.
1. Correctness & edge cases
- Confirm the change does what the pull request description and the linked ticket say it does — and only that. Scope creep hides bugs.
- Walk the edge cases: empty inputs, zero, negative numbers, very large values, null and undefined, and unicode or unusual strings.
- Check every error path, not just the happy path. What happens when the API call fails, the row is missing, or the timeout fires?
- Look for off-by-one and boundary mistakes in loops, slices, ranges, and pagination — the classic source of silent wrong answers.
- Verify concurrency assumptions: shared state, race conditions, and whether two requests hitting this code at once stay correct.
- Make sure failures fail loudly and safely — no swallowed exceptions, no error states that leave data half-written.
2. Security
- Treat all external input as untrusted: validate and sanitize anything from users, query params, headers, webhooks, or third-party APIs.
- Check authorization on every new or changed endpoint — confirm the code verifies the caller may act on this specific object, not just that they are logged in.
- Look for injection risks: parameterized queries instead of string-built SQL, escaped output instead of raw interpolation into HTML.
- Confirm no secrets, tokens, or credentials are hardcoded, logged, or committed — they belong in environment configuration, not the diff.
- Verify sensitive data is not leaked in responses, error messages, or logs, including stack traces and internal identifiers.
- Flag new dependencies and external calls for their security posture, and ensure anything handling auth uses a vetted library, not a homemade scheme.
3. Readability & maintainability
- Check that names say what they mean: variables, functions, and types should read clearly without a comment to decode them.
- Watch for functions doing too much — a single responsibility per unit keeps the change reviewable and the code testable.
- Prefer clarity over cleverness; a clever one-liner the next engineer has to puzzle over is a maintenance cost, not a win.
- Confirm comments explain why, not what. The code already says what it does; comments should capture the reasoning a reader cannot infer.
- Look for duplicated logic that should be shared, and premature abstraction that should not — both make the codebase harder to change.
- Make sure the change fits the surrounding conventions; consistency is what lets a reviewer trust the parts they did not read line by line.
4. Tests
- Confirm the change ships with tests, and that those tests would actually fail if the new behavior broke — assert on outcomes, not just that nothing threw.
- Check that the edge cases and error paths from the correctness section are covered, not only the happy path.
- For a bug fix, look for a regression test that reproduces the original bug, so it cannot quietly come back later.
- Watch for brittle tests tied to implementation details or wall-clock time; they create false failures and erode trust in the suite.
- Verify tests are deterministic and isolated — no shared state, no ordering dependencies, no reliance on a live external service.
- Make sure the tests are readable themselves; a test nobody understands is a test nobody will maintain.
5. Performance
- Scan for N+1 query patterns — a database call inside a loop is the single most common avoidable performance problem.
- Check that queries hitting large tables are indexed and bounded, and that list endpoints paginate rather than returning everything.
- Look for unnecessary work in hot paths: repeated computation that could be hoisted, or data fetched and then discarded.
- Confirm expensive or slow operations (large uploads, third-party calls, heavy reports) run async or in a background job, not in the request path.
- Weigh added caching for correctness, not just speed — a cache with no invalidation story trades a speed problem for a staleness bug.
- Right-size the concern: optimize what the workload actually exercises, and do not trade readability for micro-optimizations that never run hot.
6. Dependencies
- Question every new dependency: is it worth the maintenance, supply-chain, and bundle-size cost, or would a few lines of your own code do?
- Check that added packages are reputable and maintained — recent releases, real usage, and no obvious abandonment or single-maintainer risk.
- Confirm versions are pinned and the lockfile is updated and committed, so builds are reproducible across machines and CI.
- Review the license of anything new for compatibility with how the project ships and is distributed.
- Watch for dependencies that duplicate something already in the project, and for transitive bloat a single small package can drag in.
- Make sure new packages are scoped to where they are needed, and that nothing pulls a dev-only tool into the production bundle.
How to apply the checklist
Read the change in priority order. Start with correctness and security, because a defect or a vulnerability is the most expensive thing to let through; only then move to readability, tests, performance, and dependencies. Working top-down means that if a review runs out of time, the most important checks already happened. Keep pull requests small so the whole checklist stays fast — a focused diff gets a careful review, while a thousand-line change gets a rubber stamp because nobody can hold it all in their head.
Push everything mechanical into the pipeline. A formatter and a linter settle whitespace, import order, and style before a human ever opens the diff, so review comments stay about substance. Treat the checklist as a floor, not a ceiling: a review catches the obvious security mistakes, but it is not a substitute for a real penetration test or a security audit. Finally, keep the tone constructive — phrase comments as questions and reasons, not verdicts, so review stays a way the team learns rather than a gate it dreads.
How this connects to our work
This is not a document we wrote for a blog post — it is how we actually ship. Every change on a custom software engagement goes through review against these checks, backed by a CI/CD pipeline that runs the formatter, linter, and test suite before anyone merges. The same standard carries into our QA and test automation and web app penetration testing work, where the security section of this list becomes a much deeper audit.
If you have inherited a codebase with no review discipline and want help putting one in place, or you want a second set of eyes on the quality of work a vendor is shipping you, see how we scope and price the work or reach out to talk it through.
Frequently asked questions
What is the most important thing to check in a code review?
Correctness first: does the change actually do what it claims, including the edge cases, error paths, and boundary conditions the happy path skips? Style and naming matter, but a readable function that returns the wrong answer is still a bug. Lead with correctness and security, then move to readability, tests, and performance.
How do I stop code reviews from turning into style arguments?
Automate style. Run a formatter and a linter in CI so whitespace, import order, and formatting are settled before a human ever looks at the diff. That frees the review to focus on the things a tool cannot judge — correctness, security, design, and whether the tests actually prove the change works.
How long should a code review take?
Long enough to understand the change, which is why small pull requests get better reviews. A focused diff of a few hundred lines can be reviewed carefully in well under an hour; a thousand-line PR usually gets a rubber stamp because nobody can hold it all in their head. Keep changes small and the checklist stays fast.
Should reviewers check security on every pull request?
Yes, at least at the checklist level — untrusted input handling, authorization on new endpoints, secrets, and injection risks. A review is not a substitute for a real security audit or a penetration test, but catching the obvious issues in review is far cheaper than catching them in production or in an incident.
Related resources & reading
API Design Guidelines Checklist
The conventions a reviewer holds new API endpoints to.
Web App Pentest Checklist
Go far deeper on the security section than a review can.
Software Project Estimate Worksheet
Budget review and test time into the plan from the start.
QA & Test Automation
How we build the test discipline this checklist relies on.
Want better reviews on your codebase?
Whether you are setting up review discipline for the first time or want a second opinion on the quality of work being shipped to you, we can help you put the process, the CI checks, and the test coverage in place. See how engagements are priced or book a call.