The Code Review Checklist I Built After 2,000 Pull Requests

I categorized 2,147 PR comments I left over 18 months. 34% were about error handling. 22% were about naming. Only 8% about performance. This wasn't what I expected when I started tracking my reviews. I thought I'd be catching architectural issues and complex bugs. Instead, I was repeatedly pointing out the same fundamental problems: missing null checks, vague variable names, and error messages that told users nothing useful. After seeing the same issues surface in production incidents, I realized these weren't minor nitpicks—they were the difference between systems that fail gracefully and systems that wake you up at 3 AM.

Why Most Code Review Checklists Miss the Point

Everyone tells you to check for code style, test coverage, and documentation. That's fine, but it's not where production breaks. The checklists I see shared on engineering blogs focus on what's easy to measure rather than what actually matters. They'll tell you to verify that functions are under 50 lines, but they won't tell you to check if the error handling actually helps someone debug the issue at 2 AM when logs are your only friend.

The best code reviews don't just prevent bugs—they prevent the kind of bugs that cascade into incidents. A missing null check isn't just a potential crash; it's a potential data corruption event when that crash happens mid-transaction.

I started tracking my PR comments because I was frustrated. I'd review code, approve it, and then see the same developer make the same mistake in the next PR. The feedback wasn't sticking. So I built a spreadsheet. Every comment I left got categorized: error handling, naming, testing, security, performance, architecture, or other. After six months, I had enough data to see patterns. After eighteen months, those patterns had hardened into a checklist that actually worked.

The surprising part wasn't just what topped the list—it was what didn't. Performance optimization comments made up only 8% of my reviews. Security issues were 6%. These are the things we obsess over in architecture discussions, but in day-to-day PRs, they're rare. What's common? Developers assuming the happy path, naming things poorly, and writing error messages for themselves instead of for the person who'll be debugging at midnight.

The Error Handling Patterns That Keep Breaking Production

Here's my numbered list of error handling issues, ranked by how often they cause actual incidents:

1. Silent failures in background jobs. The code catches an exception, logs it, and continues. Seems reasonable until you realize that job was supposed to send a payment confirmation email. Now your customer thinks they weren't charged, but they were. I see this pattern in 40% of background job PRs. The fix is simple: if the operation is critical, don't catch the exception—let it bubble up and trigger your alerting. If it's not critical, document why it's okay to fail silently.
2. Generic error messages that hide context. "An error occurred" tells me nothing. "Failed to process payment" is better but still useless. "Failed to charge card ending in 4242: insufficient funds (error code: card_declined)" actually helps. I flag this in about 30% of PRs. The test I use: if you saw this error in production logs at 3 AM, could you diagnose it without adding more logging and redeploying?
3. Catching Exception instead of specific exceptions. This is controversial because some style guides recommend it, but I've seen it hide bugs too many times. When you catch Exception, you're also catching NullPointerException, IllegalStateException, and all the other exceptions that indicate programmer error rather than runtime conditions. Catch what you expect to handle. Let programmer errors crash—that's how you find them.
4. Not validating external data at the boundary. API responses, user input, file contents—if it comes from outside your process, validate it immediately. I see developers validate in the business logic layer, which means invalid data has already propagated through several functions. By then, you're debugging why a null value made it into your database. Validate at the boundary, fail fast, return clear errors.
5. Retry logic without exponential backoff. The service is down, so you retry immediately. It's still down, so you retry immediately again. Congratulations, you've just turned a service degradation into a complete outage by hammering it with retries. I see this in 15% of PRs that add retry logic. Always use exponential backoff with jitter. Always have a maximum retry count. Always consider if retrying even makes sense for this operation.
6. Not handling partial failures in batch operations. You're processing 1,000 records. Record 500 fails. What happens to records 501-1000? In most code I review, they never get processed. The batch fails, gets retried, fails again at record 500, and you're stuck. Handle partial failures explicitly: track what succeeded, what failed, and why. Make your batch operations resumable.
7. Assuming database transactions will always succeed. You start a transaction, do some work, and commit. But what if the commit fails? What if you lose the database connection mid-transaction? I see code that doesn't handle this in 25% of PRs touching database code. The result: your application state and database state diverge, and you spend hours debugging why the data doesn't match what the logs say happened.

The Day a Variable Name Caused a $50,000 Incident

It was a Tuesday morning when I got the Slack message: "We just refunded $50,000 to the wrong customers." I pulled up the incident channel and started reading. A developer had pushed a fix the night before for a bug in our refund processing system. The fix was one line. The PR had been approved by two senior engineers. The tests passed. Everything looked fine.

The bug was in a variable name. The original code had a variable called `refundAmount` that represented the amount to refund in cents. The developer added a new variable called `refundAmount` that represented the amount in dollars. They forgot to rename the original variable. The code compiled fine—they were both integers. The tests passed because the test data happened to use amounts where cents and dollars were close enough that the assertions didn't catch it.

In production, we processed 200 refunds that morning. Half of them were for the wrong amount. A $10.00 refund became a $1,000 refund. A $5.00 refund became a $500 refund. By the time someone noticed, we'd overpaid by $50,000. We had to manually review every refund, contact customers, and in some cases, ask for money back. It took three days to clean up.

The root cause wasn't the developer's mistake—everyone makes mistakes. It was that we had two variables with the same name representing different units in the same scope. The code review didn't catch it because the reviewers were focused on the logic, not the naming. After that incident, I added a rule to my checklist: if a variable represents a quantity with units, the unit must be in the name. Not `amount`, but `amountCents` or `amountDollars`. Not `duration`, but `durationSeconds` or `durationMilliseconds`.

This seems pedantic until you've debugged a production incident caused by unit confusion. Then it seems obvious. I now flag this in every PR where I see quantities without units in the name. Some developers push back, saying it's verbose. I show them the incident report. They stop pushing back.

What the Data Actually Shows About Code Review

Here's the breakdown of my 2,147 PR comments by category, with the percentage that led to changes before merge and the percentage that were related to production incidents in the following 6 months:

The most interesting column is "% Related to Incidents." Error handling comments had a 42% correlation with production incidents. That means nearly half of the error handling issues I flagged in code review would have caused problems in production if they'd been merged unchanged. Naming issues were 18%—lower than I expected, but still significant. The $50,000 refund incident is in that 18%.

Performance comments had a 15% incident correlation, which surprised me. I thought performance issues would be more likely to cause incidents, but most performance problems degrade gracefully. They make things slow, not broken. The incidents that did occur were usually timeout-related: a slow query that worked fine in testing but timed out under production load.

Security comments had only a 5% incident correlation, but that's misleading. Security issues don't always manifest as incidents—they manifest as breaches, which are rarer but more severe. The 95% change rate before merge shows that developers take security feedback seriously. When I flag a potential SQL injection or XSS vulnerability, it gets fixed.

The "Avg Time to Fix" column shows why developers sometimes resist feedback. Architecture changes take 3 hours on average. That's a significant investment, and if the developer doesn't agree with the feedback, they'll push back. Error handling changes take 15 minutes. Naming changes take 8 minutes. These are easy wins, which is why I focus on them in reviews.

The "Everyone Knows" Myth About Code Coverage

Everyone knows you should aim for 80% code coverage, right? Wrong. This is one of those engineering truisms that sounds good but falls apart under scrutiny. I've reviewed PRs with 95% coverage that had critical bugs, and PRs with 60% coverage that were rock solid. Coverage measures what code is executed by tests, not whether the tests are any good.

Code coverage is a measure of test execution, not test quality. A test that executes every line but asserts nothing has 100% coverage and 0% value.

Here's what I actually check instead of coverage percentage: Does the test verify the error cases? Does it test the boundaries? Does it check what happens when external dependencies fail? Most tests I see only verify the happy path. They call a function with valid input and assert it returns the expected output. That's fine, but it's not where bugs hide.

Bugs hide in the error paths. They hide in the edge cases. They hide in the interactions between components when one of them is failing. I've seen production incidents caused by code that had 100% test coverage because the tests never checked what happened when the database was down, or the API returned a 500, or the input was null.

So I don't ask for 80% coverage. I ask for error case coverage. Show me tests that verify your error handling works. Show me tests that check boundary conditions. Show me tests that simulate failures in dependencies. If you have 60% coverage but you've tested all the error paths, that's better than 95% coverage of just the happy path.

The other problem with coverage targets is that they incentivize the wrong behavior. Developers write tests to hit the coverage number, not to verify correctness. I've seen tests that call a function and don't assert anything, just to get the coverage up. I've seen tests that mock out all the interesting logic, so they're testing nothing but the mocks. Coverage goes up, confidence goes down.

How to Review Code When You Don't Understand the Domain

You're reviewing a PR that touches a part of the codebase you've never worked on. Maybe it's a payment processing flow, or a machine learning pipeline, or some legacy system that predates your time at the company. You don't understand the domain. How do you review it effectively?

The best code reviews happen when you admit what you don't know. Ask questions. Make the author explain their reasoning. If you can't understand the code after reading it twice, that's feedback in itself.

First, I read the PR description. If it doesn't explain what the change does and why, I ask for that before reviewing the code. A good PR description should tell me: what problem this solves, what approach was chosen, what alternatives were considered, and what risks exist. If the description is "fix bug" or "add feature," I send it back. I can't review code if I don't know what it's supposed to do.

Second, I look at the tests. Tests are documentation. They show me what the code is supposed to do in concrete terms. If the tests are clear, I can understand the behavior even if I don't understand the implementation. If the tests are unclear or missing, that's a problem regardless of domain knowledge.

Third, I focus on the things I can evaluate without domain expertise: error handling, naming, code structure, and potential edge cases. I don't need to understand payment processing to see that a function doesn't handle null inputs. I don't need to understand machine learning to see that an error is being caught and ignored. These are universal code quality issues.

Fourth, I ask questions. Lots of them. "Why did you choose this approach?" "What happens if this API call fails?" "How does this handle concurrent requests?" The author knows the domain better than I do. Make them explain it. If they can't explain it clearly, that's a sign the code might be too complex or poorly structured.

Finally, I look for missing documentation. If I needed to ask questions to understand the code, future developers will too. Ask the author to add comments explaining the non-obvious parts. Not comments that restate what the code does—comments that explain why it does it that way.

I've reviewed hundreds of PRs in domains I didn't understand. The reviews were still valuable because I focused on code quality fundamentals rather than domain-specific logic. The domain experts can verify the business logic is correct. I verify the code is maintainable, testable, and handles errors properly.

The Patterns That Predict Future Bugs

After reviewing 2,000 PRs, I've started to recognize patterns that predict future bugs. These aren't bugs yet—the code works, the tests pass, everything looks fine. But I know from experience that this pattern will cause problems later. Here are the patterns I watch for:

Code that works today but will break tomorrow is worse than code that breaks today. At least when it breaks today, you know about it and can fix it before it reaches production.

Tight coupling between components. When I see a PR that adds a direct dependency between two modules that were previously independent, I flag it. Not because it's wrong today, but because it makes future changes harder. Six months from now, someone will want to modify one of those modules and discover they can't without breaking the other. The fix is to introduce an interface or event bus to decouple them.

Magic numbers and strings scattered through the code. A PR adds a check for `status === "pending"` in five different places. Today, that works fine. Tomorrow, someone adds a new status and forgets to update all five places. Or they typo it as "pendng" in one place and spend an hour debugging why the logic doesn't work. The fix is to define constants or enums and use them consistently.

Implicit assumptions about data format or ordering. The code assumes the API always returns results sorted by date. It assumes the database query returns rows in a specific order. It assumes the array has at least one element. These assumptions are true today, but they're not enforced. When they become false—and they will—the code breaks in subtle ways. The fix is to make assumptions explicit: sort the data yourself, check array length, validate the format.

Copy-pasted code with minor variations. Someone needs similar functionality in two places, so they copy the existing code and modify it slightly. Now you have two versions of the same logic that will drift over time. When you fix a bug in one, you'll forget to fix it in the other. The fix is to extract the common logic into a shared function and parameterize the differences.

State management that relies on execution order. The code sets a variable in one function and reads it in another, assuming the first function always runs before the second. Today, that's true. Tomorrow, someone refactors the code or adds a new code path, and suddenly the second function runs first. The fix is to pass state explicitly as parameters rather than relying on implicit ordering.

These patterns don't cause bugs immediately, which is why they often slip through code review. But I've seen each of them cause production incidents multiple times. When I spot them, I leave a comment explaining the future risk and suggesting a fix. About half the time, the developer agrees and makes the change. The other half, they push back, saying it's not a problem today. I document the concern and move on. When the bug happens six months later, I link back to the PR comment. That usually convinces them for next time.

The 5-Minute Review That Catches 80% of Issues

You have five minutes to review a PR. What do you check? Here's my process, refined over 2,000 reviews. This catches about 80% of the issues I would find in a deep review, in a fraction of the time.

First minute: Read the PR description and look at the file list. Does the description explain what changed and why? Do the files touched make sense for the stated change? If someone says they're fixing a bug in the payment flow but they modified 15 files across 5 different modules, something's wrong. Either the bug is more complex than described, or they're bundling multiple changes into one PR. Ask them to split it up or explain the scope.

Second minute: Look at the tests. Are there tests? Do they cover the main functionality and at least one error case? If there are no tests, ask why. If the tests only cover the happy path, ask for error case coverage. If the tests are all mocked out, ask for at least one integration test. Tests tell you what the author thinks the code should do, which is often more valuable than the code itself.

Third minute: Scan the code for error handling. Look for try-catch blocks, null checks, and validation. Are errors handled or ignored? Are error messages useful? Are there obvious cases where things could go wrong that aren't handled? This is where 34% of my comments come from, so it's worth the time.

Fourth minute: Check the naming. Are variables, functions, and classes named clearly? Can you understand what they do without reading the implementation? Are there magic numbers or strings that should be constants? Are there abbreviations or acronyms that aren't obvious? Good naming makes code self-documenting. Bad naming makes it unmaintainable.

Fifth minute: Look for the patterns I mentioned earlier: tight coupling, copy-pasted code, implicit assumptions, state management issues. These are quick to spot once you know what to look for. If you see them, flag them. If you don't, approve the PR.

This process isn't comprehensive. It won't catch architectural issues or subtle logic bugs. But it catches the common problems that cause most production incidents: missing error handling, poor naming, insufficient testing, and code patterns that will cause future bugs. And it takes five minutes, which means you can do it for every PR without becoming a bottleneck.

The key is consistency. Do this for every PR, even the small ones. Especially the small ones. Small PRs are where developers cut corners because "it's just a quick fix." But quick fixes cause incidents too. The $50,000 refund bug was a one-line change. The five-minute review would have caught it.

Disclaimer: This article is for informational purposes only. While we strive for accuracy, technology evolves rapidly. Always verify critical information from official sources. Some links may be affiliate links.