# Code Review Etiquette > How to give and receive code review feedback without damaging the relationship: tone, phrasing, picking your battles, handling criticism without getting defensive, and the unwritten norms nobody explains. --- # Code Review Etiquette Nobody teaches you this part. School teaches you to write code; it doesn't teach you what to say when a teammate's pull request has a bug, or how to not feel sick when a senior engineer leaves twelve comments on your first PR. Code review is where a lot of new developers get their first real taste of workplace friction - and most of them figure out the etiquette the hard way, by getting it wrong once. Writing the code is the straightforward part. The hard part is that every comment you write lands on a person, and every comment you receive feels like it's about you even when it isn't. This guide covers both sides: how to leave feedback that gets read instead of resented, and how to receive feedback without your stomach dropping every time a review comes back with changes requested. ## The phases 1. **[Reviewing Someone Else's Code Without Being a Jerk](01-reviewing-without-being-a-jerk.md)** - why text reads harsher than you meant it, phrasing feedback as questions instead of commands, and how to tell a style nitpick from a real bug from an architecture concern. 2. **[Receiving Feedback Without Getting Defensive](02-receiving-feedback-without-defensiveness.md)** - the code is not you, healthy pushback versus defensiveness, and what to do when you think the reviewer is wrong. 3. **[The Etiquette Nobody Tells You](03-the-etiquette-nobody-tells-you.md)** - reasonable response times, approving with comments versus blocking, and what LGTM culture gets wrong. Read them in order if you're new to this. If you just got a rough review and need to steady yourself before responding, start with Phase 2. --- # Reviewing Someone Else's Code Without Being a Jerk A comment that would sound fine said out loud can read as an attack in a PR thread. There's no face, no tone of voice, no shared context that this is Dave being Dave again. Text on a screen, at 2pm, sitting between the author and the "merge" button they were hoping to hit today. "This is wrong" reads as a verdict. "Why not use a map here instead of nested loops?" reads as a question you can actually answer. Same underlying concern, completely different experience for the person reading it. Assume every comment you write will be read in the worst possible mood, because eventually one will be. ## Commands versus questions Compare these two comments on the same line of code: > **Before:** "Change this to use `Promise.all`. This is inefficient." > > **After:** "Since these three fetches don't depend on each other, could we run them with `Promise.all` > instead of awaiting each one in sequence? Might shave a couple hundred ms off the load." The first is a command with a judgment attached ("inefficient"). It's technically correct and still lands badly - it tells the author what to do without explaining why, and it labels their work with a negative word. The second asks a question, states the reasoning, and gives a concrete reason to care. The author can say "good catch" or "actually I need them sequential because X" - either way, it's a conversation, not an order. You don't need this phrasing for every comment - "typo: `recieve` → `receive`" doesn't need a question mark. Save the soft framing for anything that questions a decision the author made on purpose. ## Observations work better than instructions "You should extract this into a helper" assumes you know their reasons for not doing that already. Try narrating what you're noticing instead: "I see this validation logic three times in this file - might be worth pulling into one function, might not be worth it for three call sites. Your call." That last phrase matters: it signals you're not blocking the PR over it. Real example, seen in an actual PR: a comment that said only "no" on a 40-line function. No explanation, no suggestion. The author had no idea what to fix and had to ask in Slack what the reviewer even meant. Compare to: "This function does validation, transformation, and the DB write all in one place - if the DB write fails partway through, we might've validated for nothing. Worth splitting?" Same objection, one version is usable. ## Picking your battles Not every PR comment carries the same weight, and treating them all the same way (either steamrolling everything or staying silent about everything) is how reviews go wrong. Rough hierarchy: - **Style nitpicks** (naming, formatting, "I'd write this differently but it works") - mention lightly, label them as opinion, and drop it after one round. "Nit: I'd call this `userCount` not `cnt`, but not blocking on it." - **Real bugs** (off-by-one, unhandled null, race condition, wrong logic) - these need to be raised clearly and are worth blocking the merge over. Don't soften a real bug into invisibility for the sake of politeness: "This will throw if `user` is null - looks reachable from the `/guest` route. Can we guard it?" - **Architecture concerns** (this whole approach might not scale, this couples two modules that shouldn't know about each other) - the biggest and rarest category. Raise it, but recognize it might mean redoing real work, so bring reasoning, not a preference, and be open to "let's ship this and revisit." A common early-career mistake is spending your credibility on nitpicks - fifteen comments about spacing and variable names, nothing about the null pointer three lines down. Reviewers who catch real bugs earn the right to be picky later. Reviewers who only nitpick get their comments skimmed. ## A quick gut check before you hit submit Before posting a review, skim your own comments and ask: if a stranger read only these, with no tone of voice, would they sound like feedback or like criticism of a person? If a comment only makes sense in your head as "I'm being efficient," rewrite it. Two extra sentences of context cost you nothing and save the author a defensive reread. Ready to check your understanding: ```quiz [ { "q": "Which comment is more likely to get a productive response?", "choices": [ "\"This is wrong, fix it.\"", "\"This throws if `user` is null - reachable from the /guest route. Can we guard it?\"", "\"Bad approach.\"" ], "answer": 1, "explain": "It names the specific problem, explains why it matters, and asks rather than orders." }, { "q": "A teammate used a slightly different variable naming style than you'd use, but the code works fine. What's the right move?", "choices": [ "Block the PR until it matches your preferred style", "Mention it lightly as a non-blocking nit, or let it go", "Say nothing in the PR, then complain about it later" ], "answer": 1, "explain": "Style preferences are the lowest-stakes category - flag them softly if at all, and don't block on them." }, { "q": "Why does 'This is inefficient, change it' tend to land badly even when it's technically correct?", "choices": [ "Because efficiency doesn't matter in code review", "Because it's a command with a judgment attached, and gives no reasoning to engage with", "Because reviewers should never mention performance" ], "answer": 1, "explain": "The problem isn't the content, it's the framing - no reasoning, no room for a response, only a verdict." } ] ``` ## Your turn: Priya's PR needs a decision Reading the hierarchy is the easy part. Deciding what to raise and what to let go, on an actual diff, with a teammate waiting, is the job. There is no single right answer below and nothing is scored right or wrong - but every round trip you ask for costs a day, and Priya is waiting to hear back. ```scenario { "title": "A 200-line PR, four issues, one deadline", "brief": "Priya's pull request lands in your queue at 4pm. It touches the checkout total and needs to ship today. Skimming it, you spot four things: a variable named cnt, a couple of spots with inconsistent indentation, a line that reads cart.discount.value with no null check, and a total() function that validates, transforms, and writes to the database all in one place. Nothing here is scored right or wrong, but every round trip you ask for costs a day, and Priya is waiting to hear back.", "prompt": "What's your first move?", "clock": { "unit": "days", "running": "until merge", "resolved": "to merge" }, "resolvedHeading": "Review submitted. Here's how it went.", "actions": [ { "id": "read-diff", "label": "Read the full diff before commenting", "cost": 0, "reveals": "+ let cnt = items.length;\n+ for (let i = 0; i < items.length; i++) {\n+ for (let j = 0; j < taxRates.length; j++) { ... }\n+ }\n+ const discounted = price - cart.discount.value;\n+ function total(cart) {\n+ // validates, transforms, and writes to the DB, all in one place\n+ }", "note": "Now you know what's actually here: one naming nit, a loop that could be a lookup, one line that assumes cart.discount is never null, and one function doing three jobs." }, { "id": "nit-lightly", "label": "Leave the naming and indentation as one light, non-blocking note", "cost": 0, "reveals": "you: \"Nit: cnt -> itemCount reads clearer, and a couple of spots have mixed indentation. Not blocking, just flagging.\"", "note": "Costs nothing. Priya can take it or leave it, and the PR isn't waiting on you either way." }, { "id": "flag-arch-lightly", "label": "Note the three-jobs-in-one function as a future concern, not a blocker", "cost": 0, "reveals": "you: \"total() validates, transforms, and writes to the DB together. If we ever need the transform on its own this'll be annoying to split out. Not blocking today, worth a follow-up ticket.\"", "note": "A real concern, raised without spending a round trip on it. Ships today, revisited later." }, { "id": "block-naming", "label": "Request changes over the cnt variable name alone", "cost": 1, "reveals": "you: Request changes - \"Please rename cnt to itemCount before merge.\"\npriya: renames it, pushes, re-requests review. 6:40pm.", "note": "A full day spent on a preference. The checkout fix now sits unmerged one more day, for a name." }, { "id": "block-arch", "label": "Request changes until total() is split into three functions", "cost": 1, "reveals": "you: Request changes - \"Can we split validate/transform/write into three functions before merge?\"\npriya: pushes a refactor the next morning.", "note": "The concern was real. Blocking today's fix on a refactor that could have been a follow-up ticket cost a day the team didn't have." }, { "id": "flag-bug", "label": "Request changes on the null-unsafe discount access; leave everything else as non-blocking notes in the same review", "cost": 1, "resolves": true, "reveals": "you: Request changes - \"cart.discount can be null for guest checkouts, this will throw. Can we guard it? Everything else here (naming, the loop, the function split) is a nit or a follow-up, not a blocker.\"\npriya: adds cart.discount?.value ?? 0, pushes. merged 5:15pm next day.", "note": "One round trip, spent on the one thing that could actually break checkout." } ], "debrief": { "ideal": 1, "text": "The round trip worth asking for is the one that stops a real bug from shipping. A name, a loop shape, a function that could be split later - those are notes, not blockers. A reviewer who spends a day on every nit gets the same round trip as one who spent it on the null check, and their next review carries less weight for it.", "notes": [ { "when": "if-taken", "action": "block-naming", "text": "Renaming cnt is a fair note. Blocking on it doesn't make Priya more likely to fix it, it just costs a day a non-blocking comment would have cost nothing." }, { "when": "if-taken", "action": "block-arch", "text": "The concern about total() doing three jobs was legitimate craftsmanship. Legitimate isn't the same as urgent, and it could have shipped as a follow-up ticket instead of a day." }, { "when": "if-not-taken", "action": "read-diff", "text": "You commented without reading the full diff first. Whatever you caught came from a skim, and a skim is exactly how a null-unsafe line like this one gets missed." }, { "when": "if-not-taken", "action": "flag-arch-lightly", "text": "You never raised the three-jobs-in-one function at all. That's a fine call to skip for a same-day fix, as long as it's a choice and not something you just didn't notice." } ] } } ``` --- # Receiving Feedback Without Getting Defensive Your PR comes back with eight comments and "changes requested" in red. Your stomach drops a little. That reaction is normal - you spent hours on this, and it feels like *you* got graded, not your code. It didn't. The two feel identical from the inside. They aren't the same thing, and learning to separate them is most of what makes code review survivable. ## The code is not you Nobody comments "this function is bad" and means "you are bad at your job." They mean the function, on this line, in this context, could be better. The reviewer usually didn't even think about you while writing the comment - they were looking at the diff, not psychoanalyzing your worth as an engineer. You're the one adding that layer. A trick that helps: read the comment as if it were left on code you wrote a year ago, not code you wrote this morning. Distance turns "you missed something" into "oh, that's a good catch" almost automatically - because there's no fresh ego attached yet. The goal is to get there without waiting a year. ## Healthy pushback versus defensiveness Disagreeing with a reviewer is not the problem. *How* you disagree is what separates a good engineer from one people dread reviewing. > **Defensive:** "It works fine, I tested it." (No reasoning. Treats "it works" as the only bar. Shuts the > conversation down.) > > **Healthy pushback:** "I went with a single query here because the join was getting expensive at our data > size - happy to split it if you're worried about readability, but wanted to flag the tradeoff first." The difference isn't tone, it's content. Defensiveness protects the code (and the ego) without engaging the actual concern. Healthy pushback engages the concern, states real reasoning, and leaves room for the other person to be right. Notice the healthy version doesn't apologize for disagreeing, either - "I disagree, and here's why" is not rude. Silence and instant compliance without explanation aren't better than pushback; they hide the disagreement instead of resolving it. Watch for defensiveness tells in your own replies: explaining your reasoning *after* getting annoyed instead of before, replying within ninety seconds, or leading with "well, actually" three comments in a row. None of these mean you're wrong. They mean you're reacting instead of responding - close the laptop for five minutes if you notice it happening. ## When you think the reviewer is actually wrong It happens. Reviewers make mistakes too - they misread the diff, don't have the context you do, or are applying a rule that doesn't fit this case. The move isn't to concede to avoid conflict, and it isn't to dig in silently and merge anyway. It's to make your case with specifics: > "I think this is actually safe - `items` can't be empty here because we validate it in the route handler > above (line 12). Want me to add a comment pointing that out, or are you thinking of a case I'm missing?" This does three things: states your position, gives the evidence, and invites them to correct you if you've missed something. Most disagreements resolve in one round of this. If it doesn't resolve after two rounds of back-and-forth, that's the signal to stop typing and talk - a five-minute call settles what a 20-comment thread won't, and text arguments have a way of hardening positions that a real conversation defuses in one sentence. If you're junior and the reviewer is senior, "they're probably right" is a reasonable prior - but a prior, not a rule. If you have a specific, checkable reason (a test that passes, a line of code that handles the case) it's worth saying so. Being wrong sometimes is the cost of ever being right out loud. ## What actually helps in the moment Before you reply to a review that stung: reread it once after the sting fades, usually the next time you look. Comments that felt harsh at 9am often read as completely reasonable at 9:15, once "they think I'm incompetent" has stopped being the story you're telling yourself. If a comment genuinely crossed a line - mocking, sarcasm, "how did this even pass CI" - that's a tone problem worth naming directly to the person, separate from the technical content. ```quiz [ { "q": "A reviewer comments that your function has a bug. What's the most accurate way to think about that comment?", "choices": [ "It's feedback about your competence as an engineer", "It's feedback about this specific code, not about you", "It means you should stop contributing to this codebase" ], "answer": 1, "explain": "Reviewers are looking at the diff, not judging your worth - the separation is a skill you practice, not a fact you accept once." }, { "q": "What makes a reply 'healthy pushback' rather than 'defensive'?", "choices": [ "Replying quickly so the reviewer knows you disagree", "Stating concrete reasoning and leaving room for the reviewer to be right", "Insisting the code works because it passed your own testing" ], "answer": 1, "explain": "Defensiveness protects the code without engaging the concern; healthy pushback engages it with real reasoning and stays open to being wrong." }, { "q": "You and a reviewer go back and forth twice on a comment thread with no resolution. What should you do next?", "choices": [ "Keep replying in the thread until one side gives up", "Merge anyway without addressing it", "Move the conversation to a quick call or chat" ], "answer": 2, "explain": "Text threads harden positions. A short live conversation resolves in minutes what a long comment thread often can't." } ] ``` --- # The Etiquette Nobody Tells You Some of code review's biggest friction points never get written down anywhere. Nobody tells you how fast you're supposed to respond, what "approve with comments" actually means versus blocking, or that "LGTM" can mean either "I read every line" or "I skimmed the diff on my phone between meetings." You're expected to absorb these norms by osmosis. Here they are directly. ## You don't owe an instant reply, but going dark blocks someone Code review isn't customer support - you don't need to drop what you're doing the second a review request lands. Reasonable norm: same business day for small PRs, within a day or two for bigger ones, with a heads up if it'll be longer ("in meetings all day, will look tomorrow AM"). Nobody expects you to review mid-focus block on your own work. What's not fine: a PR sitting unreviewed for four days with no comment, while the author refreshes the page wondering if they did something wrong. Every hour a PR waits is an hour someone can't merge, can't build on top of it, and often can't start their next task cleanly. Silence reads as "not a priority" even when it's really "I forgot" - so if you can't get to it, say so in one line. That costs ten seconds and saves someone a day of wondering. If you're the author and a review is stalled past a reasonable window, a polite nudge is not rude: "Hey, any chance you can take a look today? Kind of blocked on this." Reviewers generally want the reminder, not a reason to be annoyed. ## Approving with comments versus blocking Most PR tools give you three real options, and mixing them up causes real confusion: - **Approve** - "this is good to merge as-is." No unresolved concerns. - **Approve with comments** - "this is good to merge, and here are some thoughts that don't need to hold it up." Use this for nitpicks, "consider for next time," or a question you're curious about but don't need answered before merge. - **Request changes / block** - "don't merge this until we resolve X." Reserve it for actual bugs, security issues, or things you genuinely believe will cause a problem in production. The failure mode in both directions is common. Blocking on a naming preference makes the author (rightly) annoyed and trains people to see your reviews as friction to route around. Approving *without* comments on a PR that has a real bug, because you didn't want to seem difficult, is worse - it puts a bug in production to avoid an awkward conversation. If you found a real issue, block. If it's a preference, approve and mention it lightly. ## What LGTM culture gets wrong "LGTM" (looks good to me) is fine shorthand - the problem is what it's shorthand *for*. Two developers can both type "LGTM" on the same PR: one read every changed line, traced the logic, and thought about edge cases; the other opened the diff, saw it wasn't huge, and clicked approve between Slack messages. Both comments look identical. Only one of them is actually a review. Rubber-stamping happens for understandable reasons - review fatigue, trusting a teammate's track record too much, a backlog of five PRs waiting and a meeting in ten minutes. It's still a real cost: review is one of the few places that catches bugs before they ship, and an LGTM that means "I didn't really look" quietly turns that safety net into theater. If you don't have time to actually review something, "haven't had a chance to look closely, will circle back this afternoon" is more upfront than a fast, empty approval - and it's a completely normal thing to say. A useful personal rule: never approve a PR you haven't actually opened and scrolled through. If you trust the author enough to skip that, that's a legitimate call to make sometimes - make it on purpose, not by habit. ```quiz [ { "q": "A PR has a small naming inconsistency you'd personally do differently, but no functional issue. What's the right review outcome?", "choices": [ "Request changes and block the merge until it's renamed", "Approve, optionally mentioning it as a non-blocking note", "Say nothing and quietly rename it yourself later" ], "answer": 1, "explain": "Blocking is for real problems. A naming preference belongs in an 'approve with comments' note, not a block." }, { "q": "Why is going dark on a review request for several days a problem, even if you eventually review it thoroughly?", "choices": [ "It isn't a problem as long as the review itself is good", "It blocks the author from merging or moving on, and silence reads as low priority", "Reviews older than a day are automatically rejected by most tools" ], "answer": 1, "explain": "The cost isn't the eventual review quality, it's the time the author spends blocked and unsure whether the PR was even seen." }, { "q": "What's the actual problem with LGTM culture?", "choices": [ "The phrase 'LGTM' itself is unprofessional", "It can mean either a careful review or a fast rubber stamp, and you can't tell which from the outside", "Approvals should always include a paragraph of praise" ], "answer": 1, "explain": "The words look identical whether the reviewer read every line or skimmed the diff - that ambiguity is the real cost." } ] ``` ## Where to go next Code review etiquette is one piece of working well with other people's code. [Reading Legacy Code](/guides/reading-legacy-code) picks up right after this: how to make sense of code someone else wrote long before you got there, with no one around to ask.