Code review is one of the highest-leverage activities a developer does. A good reviewer catches real problems before they reach production, helps less experienced engineers grow faster, and raises the quality bar for the whole team over time. A poor reviewer creates friction, misses the things that matter, and makes people dread opening pull requests. The difference isn’t seniority — it’s craft. Reviewing code well is a skill, and it can be learned.

Start With the Why, Not the What

---

Before reading a single line of code, read the PR description. What problem does this change solve? What approach was taken, and why? If there’s no description, ask for one before reviewing — you can’t evaluate whether code is correct if you don’t know what it’s trying to do.

Most code review mistakes happen because the reviewer jumped straight to implementation details without understanding intent. You end up leaving comments like “this could be a map” on code that’s correct but stylistically different, while missing the architectural issue two files away that would cause an incident in three months.

The review order that works:

1. Understand the intent — read the description, the linked issue, the acceptance criteria
2. Evaluate the approach — is this the right solution to the right problem?
3. Check correctness — does the implementation do what it says, including edge cases?
4. Review quality — naming, structure, tests, clarity
5. Style and nits — only after everything else

Reversing this order — starting with style, ending with design — wastes everyone’s time and produces the kind of review that makes authors feel their work wasn’t actually read.

The Taxonomy of Review Comments

---

Not all feedback carries equal weight, and not making that distinction explicit is one of the most common reviewer mistakes. When every comment looks the same, authors can’t tell what needs to be addressed before merging and what’s optional context.

A practical taxonomy:

• Blocking — must be addressed before merge (correctness, security, data integrity, missing tests for critical paths)
• Suggestion — worth considering, author’s call (better naming, alternative approach, simplification)
• Nit — cosmetic or minor (trailing whitespace, minor style preference, debatable naming)
• Question — genuine curiosity, not blocking (asking why a choice was made, clarifying intent)
• FYI — share knowledge, no action needed (“you could also use filter_map here, just so you know”)

Label your comments. “Blocking: this has a race condition when two requests arrive simultaneously” is unambiguous. “Nit: I’d name this user_sessions for clarity, but fine either way” signals that you’ve left a comment but you’re not holding the merge for it.

This taxonomy saves hours of back-and-forth over what the author needs to fix and what’s optional context the reviewer added for completeness.

How to Phrase Feedback

---

Code review comments that produce defensive reactions share a pattern: they’re about the person, not the code. “Why did you do it this way?” sounds like accusation. “This approach doesn’t handle the nil case when the user isn’t authenticated” is about the code and is easy to respond to constructively.

Ask questions instead of declaring wrongness:

• “What happens when user is nil here?” instead of “this will crash if user is nil”
• “Could this be extracted into a method? The block is getting long” instead of “this block is too long”
• “Is there a test for the empty array case?” instead of “you forgot to test this”

Questions invite explanation. Declarations invite defense. An author who might have a good reason for their approach can share it in response to a question. An author told they did something wrong has to argue against a verdict.

Separate opinions from facts:

• Fact: “This query will load all records into memory — it needs a find_each for large datasets.”
• Opinion: “I’d use a service object here rather than putting this logic in the controller.”

Mark opinions as such. “I’d prefer” or “in my experience” signals that you have a view but aren’t insisting. It gives the author room to disagree and keep their approach, which is often fine.

What to Actually Look For

---

The things that matter most in a review, roughly in order:

1. Correctness and edge cases

Does the code handle nil, empty collections, concurrent access, invalid inputs, and race conditions? The most valuable thing a reviewer does is spot the case the author didn’t think of. Focus here before anywhere else.

2. Security

SQL injection, unescaped user input in views, insecure direct object references, credentials in code, over-permissive access controls. These are blocking issues. Don’t soft-pedal security findings.

3. Test coverage

Are the important paths tested? Is the test actually testing the behavior, or just the implementation? A test that always passes because it mocks everything isn’t providing coverage. A missing test for the error case is worth blocking a merge over.

4. Performance implications

N+1 queries, missing database indexes for new columns used in WHERE clauses, unbounded queries on large tables. These don’t always show up in tests but show up immediately in production.

5. Maintainability

Is the code readable by someone who didn’t write it? Are complex decisions explained? Is the naming accurate, or has something been renamed in a way that now misrepresents what it does?

When to Approve Despite Disagreement

---

A code review isn’t a negotiation where you withhold approval until every comment is addressed to your satisfaction. That model produces bottlenecks, builds resentment, and optimizes for code that looks like the reviewer would have written it.

Approve the PR if the code is correct, safe, tested, and maintainable — even if you’d have done it differently. Leave your suggestions as suggestions and trust the author’s judgment on their own code.

Reserve blocking on style or approach for cases where you have strong evidence that the chosen approach will cause problems, not just a preference for a different way. The question to ask yourself: “If this merges as-is, what actually breaks or gets harder?” If the honest answer is “nothing, I just prefer my approach,” that’s a suggestion, not a blocker.

Pro-Tip: End your reviews with a brief note on what you found valuable — a well-named abstraction, an edge case well-handled, a test that covers something non-obvious. This is not empty praise; it’s signal. It tells the author what kind of thinking you want to see more of, and it makes the review feel like a conversation between two professionals rather than an inspection. The developers whose code reviews people appreciate are the ones who improve the code and make the author feel heard — not the ones who find the most things wrong.

Conclusion

---

Effective code review is about finding the things that matter — correctness, security, test gaps, performance — and communicating them in ways that lead to better code rather than defensiveness. The taxonomy of comment types removes ambiguity. Asking questions instead of declaring problems keeps the tone collaborative. Approving code that’s correct but stylistically different from what you’d write models the trust that makes high-performing engineering teams possible. All of this is learnable, and all of it compounds — the better your reviews, the faster your whole team improves.

FAQs

---

Q1: How long should a code review take?
A PR under 200 lines with good tests and a clear description: 20–30 minutes. Larger PRs scale accordingly, but a PR over 500 lines of production code is usually worth asking to split. Reviews that take longer than 60 minutes typically indicate a missing design conversation that should have happened before implementation.

Q2: What if the author pushes back on all my feedback?
Distinguish between pushback on style (fine — let it go) and pushback on correctness or safety (worth holding firm with explanation). “I see your point, but this creates a race condition when two requests arrive simultaneously — here’s a concrete scenario” is a response to pushback that’s worth continuing. “I prefer map over each_with_object” is not.

Q3: Should I review code I don’t understand?
Partially. You can review the parts you understand and leave questions on the parts you don’t. “I’m not familiar enough with this part of the codebase to review it confidently — could you walk me through the approach?” is a legitimate and useful comment. Rubber-stamping code you don’t understand to clear the queue is not.

Q4: How do I review a PR from a more senior engineer without feeling awkward?
The same way you’d review anyone else’s code. Senior engineers make mistakes and appreciate reviewers who catch them. The goal is correct, safe code — not deference. Ask your questions; flag the thing that looks wrong. You’ll be right more often than you expect, and when you’re wrong, you’ll learn something.

Q5: Is it okay to approve with comments?
Yes, and it’s often the right call. “Approved — left two nits, feel free to address or ignore” is a legitimate review outcome that unblocks the merge without abandoning the feedback. Reserve requiring changes for blocking issues.