A quick note before this starts: I didn't actually run ten interviews for this post. What follows is a synthesis — patterns I've seen show up over and over in my own review comments, in other people's, and in the general "why did you block this" conversations that happen on pretty much every team that takes code review seriously. If you've been reviewing code for more than a year, I'd bet most of this is going to feel familiar rather than new. That's kind of the point. These aren't ten hot takes. They're ten things that keep independently showing up across very different teams, which probably means they're just true.
I've grouped them into ten reasons, each with a small example and, more importantly, a note on how to raise the issue without it turning into a fight or a demoralized author. Because the "what" of a rejection is usually easy. The "how you say it" is where reviews go well or badly.
1. Broken Behavior
This is the obvious one, but it's worth stating plainly: if the code doesn't do what it claims to do, nothing else matters yet. Style, naming, architecture — none of it is relevant if the function returns the wrong value.
function isEligibleForDiscount(user) {
return user.age > 65 || user.age < 18;
}
If the actual business rule is "65 and older," this excludes someone who just turned 65. Small, easy to miss, and exactly the kind of thing that survives a glance because the code reads fine — it's fluent, grammatical, wrong.
How to raise it constructively: State the discrepancy, not the failure. "I think this excludes someone exactly at 65 — should the comparison be >=?" is a question, not an accusation, and it gives the author room to say "actually that's intentional" if there's context you're missing.
2. Security Problems
Security issues get treated differently from ordinary bugs because the cost of being wrong isn't "annoying," it's "someone's data leaked." That difference in stakes is why these tend to be non-negotiable even when they're inconvenient to fix right before a release.
query = f"SELECT * FROM users WHERE email = '{email}'"
String-interpolated SQL is the classic example, but the pattern shows up everywhere — unescaped output going into HTML, secrets logged in plaintext, auth checks that verify identity but not permission.
How to raise it constructively: Don't soften this one into vague language. "Nit: might want to look at this" undersells a real risk. Be direct about why it matters without being alarmist: "This is vulnerable to SQL injection since email comes straight from the request — can we use a parameterized query here?" Directness isn't the same as harshness.
3. No Tests for Risky Changes
Not every change needs a wall of new tests. But changes that touch money, permissions, data deletion, or anything hard to undo are a different category, and "it worked when I tried it" isn't the same as "it's covered."
def refund_order(order_id, amount):
order = db.get_order(order_id)
order.refunded_amount += amount
db.save(order)
No check that amount doesn't exceed the order total, no test verifying the accumulation logic, no test for calling this twice. This is exactly the kind of function where a bug doesn't show up as a crash — it shows up as a quiet accounting discrepancy three weeks later that someone in finance has to chase down.
How to raise it constructively: Explain the category of risk rather than just asking for "more tests," which can feel like busywork. "Since this touches refund amounts directly, could we add a test for double-refunding and for a refund larger than the order total? Mostly want to make sure double-submission doesn't double-refund." That gives the author a concrete, reasoned target instead of an open-ended homework assignment.
4. Unclear Naming
This one is easy to dismiss as bikeshedding, and sometimes it is. But naming is usually a proxy for something else — either the author isn't totally sure what the variable represents, or the code is doing something slightly different from what its name implies.
data = get_data(id)
if data:
process(data)
What is data? An order? A user? A raw API response? A parsed object? The name tells you nothing, and six months from now, "what does data actually contain" becomes an archaeology project.
How to raise it constructively: Ask instead of dictating. "What does data represent here — maybe pending_order or similar would make the next line clearer?" Sometimes the act of answering that question makes the author realize the variable is doing two different jobs, which is a more valuable outcome than just picking a better name.
5. Excessive Complexity
Code that's technically correct but takes five reads to understand is still a liability — it's just a slower one. Complexity tends to accumulate gradually, one small conditional at a time, until a function that started simple is six levels of nested logic deep and nobody remembers why.
function getStatus(order) {
if (order.cancelled) {
if (order.refunded) {
return "cancelled_refunded";
} else {
if (order.refundPending) {
return "cancelled_refund_pending";
}
return "cancelled";
}
} else {
if (order.shipped) {
return order.delivered ? "delivered" : "shipped";
}
return order.paid ? "processing" : "pending";
}
}
Every branch here is individually reasonable. Together, they're hard to hold in your head at once, and that's the actual cost — not that any single line is wrong, but that verifying correctness requires simulating every path manually.
How to raise it constructively: Point at the shape, not the person's competence. "This has a lot of nested branches — would a lookup table or early returns make the cases easier to scan?" Offering a concrete alternative (rather than just "this is too complex") turns the comment into something actionable instead of a vague judgment.
6. Duplicated Logic
A little duplication is often fine, even healthy — premature abstraction can be worse than a few repeated lines. But when the same rule is implemented in more than one place, you've created a situation where fixing a bug requires remembering every location it was copied to, and someone always forgets one.
# in billing.py
if not re.match(r"^[^@]+@[^@]+\.[^@]+$", email):
raise ValueError("invalid email")
# in signup.py, written by someone else, months later
if "@" not in email or "." not in email:
raise ValueError("bad email")
Two different validation rules for the same concept, drifting further apart every time either one gets touched.
How to raise it constructively: Name the pattern rather than just the instance. "I think this is the third place we validate emails, and they're not all doing the same check — might be worth pulling into a shared function at some point, doesn't need to block this PR." Flagging it without demanding an immediate refactor respects the scope of what the author was actually trying to ship.
7. Poor Error Handling
The failure path is where a lot of real bugs live, precisely because it's the path least exercised during normal development. Code that handles success gracefully and failure carelessly tends to look complete right up until it isn't.
try {
await processPayment(order);
} catch (e) {
console.log("payment failed");
}
The payment failed, the user probably still thinks their order went through, and the only record of what happened is a console log nobody's watching.
How to raise it constructively: Ask what the user or caller experiences on failure, not just what the code does. "If this catch fires, what does the customer see? Right now I think they'd get a success response — should we surface the failure back to them?" Grounding the comment in the actual downstream experience makes the stakes concrete instead of abstract.
8. Unexpected Side Effects
A function whose name promises one thing but does another is a specific, sneaky kind of bug, because it's invisible at every call site — the code reads correct everywhere it's used, and the mismatch only becomes apparent when something downstream breaks for reasons nobody can trace back to this function.
def get_user_display_name(user):
user.last_seen = datetime.now()
db.save(user)
return f"{user.first_name} {user.last_name}"
A function that looks like a pure getter is quietly writing to the database on every call. Call it in a loop to render a list of a hundred names, and you've just issued a hundred writes nobody intended.
How to raise it constructively: Point out the mismatch between name and behavior specifically. "This reads like a pure getter but it's also updating last_seen — is that intentional? If so, might be worth splitting the write out or renaming so it's obvious from the call site." The goal is making the side effect visible, not implying the author was being sneaky — usually it just accreted there over time.
9. Architectural Problems
Sometimes a change is locally correct and still a problem, because of where it lives rather than what it does. A new responsibility bolted onto an existing module, a new dependency pointing the wrong direction, a boundary quietly crossed — these are the hardest comments to make well, because they're rarely about the diff itself.
# inside the OrderService
def send_slack_notification(order):
requests.post(SLACK_WEBHOOK_URL, json={"text": f"New order {order.id}"})
Nothing here is "wrong" in isolation. But now OrderService — which should own order logic — also owns notification formatting and an external HTTP dependency, and every future change to how notifications work has to route through a class that was never meant to care about them.
How to raise it constructively: These comments land best when they're explicitly framed as bigger-than-this-PR, so the author doesn't feel blocked on solving something out of scope. "This works, but I think notification logic living inside OrderService is going to get tangled as we add more notification types — worth a separate NotificationService down the line? Not blocking this PR on it." Naming the concern without demanding it be resolved right now respects both the code and the person's afternoon.
10. Code That Future Developers Cannot Understand
This is the hardest one to pin down because it's not really a rule — it's a judgment call about whether someone with less context than the author, arriving in a year, can safely modify this code without breaking something they can't see.
def calc(x, y, z):
return x * 0.85 + y * (1 if z > 3 else 0.5) - 12
Every one of these numbers means something. None of that meaning is written down anywhere. Whoever touches this function next either has to reverse-engineer the business rule from the arithmetic, or — more likely — just leaves it alone forever because nobody wants to be the one who breaks a formula they don't understand.
How to raise it constructively: Ask the author to explain it in the comment thread, and then ask if that explanation could live in the code instead of in the PR discussion. "Could you walk me through what 0.85 and the z > 3 threshold represent? If it's a business rule, I think it'd help future-us to have that as a comment or named constant." Often the author does know exactly what it means — the review is really just prompting them to write it down somewhere more permanent than their own memory.
Rejecting Code Without Discouraging the Developer
All ten of the reasons above can be delivered in a way that either helps someone grow or makes them dread opening their next PR. The difference usually comes down to a few habits.
Separate the code from the person. "This has a bug" and "you wrote a bug" describe the same fact but land completely differently. The first is about the artifact. The second, even unintentionally, is about the author's competence.
Distinguish blocking issues from everything else. Not every comment needs to hold up a merge, and treating a naming nit with the same tone as a security hole trains people to either panic at every comment or ignore all of them. Say plainly when something isn't blocking.
Ask before asserting, when there's room to be wrong. A reviewer often doesn't have the full context the author does. "Why did this end up this way?" invites an explanation you might be missing. "This should be done differently" doesn't leave room for one, even when you're right.
Explain the why, not just the what. "Move this to a separate function" is an instruction. "This function is doing three things, which makes it hard to test them independently — could we split it up?" is a reason, and reasons generalize to the next PR in a way that instructions don't.
Say something positive when it's actually true. Not as a cushioning tactic, but because reviews that are 100% critique start to feel like a performance review nobody asked for, and people stop putting effort into approaches worth praising because the effort never gets acknowledged.
None of this means lowering the bar. A PR with a real security hole should still get blocked, clearly and without hedging. The point isn't to be gentler about what gets rejected — it's to be more deliberate about how, so that the person on the other end of the review comes away having learned something, instead of just having been told no.
The reasons a PR gets rejected are, in the end, pretty consistent across teams and codebases. What varies enormously is whether the developer who wrote it feels like the review made their code better, or just made their afternoon worse. That difference is almost entirely in the delivery, not the content.
Top comments (0)