Giving and Receiving Feedback Questions

Direct answer

I'll tell this as a story (illustrative skeleton). A peer on my team kept merging changes (adding code to the shared codebase so it goes live) late on Friday afternoons without telling the on-call engineer (the person on the rota who responds to alerts outside working hours), so the weekend pager (the automatic alert that wakes that person) took the hit. We were friendly, so raising it risked the relationship. I prepared the facts, asked permission to talk, said specific words, and agreed a small change we could both check.

Situation and stakes

In three of the last five weekends, a page was traced to a Friday-evening change from this peer. Nobody had complained to him. I was the person on call, so I had standing to speak. The risk: he was senior in tenure, proud of his speed, and likely to hear "you are careless."

Preparation (what I did)

• Listed the three incidents (one per affected weekend) with dates and what each page cost (hours of weekend time), separating what happened from my theory of why.
• Checked my own part: did the team have a "no Friday deploys" norm? No, so he had broken no rule. That changed my goal from "stop doing this" to "agree a norm together."
• Chose a private one-to-one meeting on a Tuesday, not a Friday or in a public channel, and sent a neutral invite: "Want to talk about how we handle late changes and on-call?"

What I actually said

"When changes land after 4pm Friday without a heads-up, on-call finds out from the pager. That happened on three of the last five weekends, including the cache change on the 14th. I'm not saying the changes were wrong. I'm saying I can't be ready for something I don't know about. How do you see it?"

He bristled for a minute ("they were small changes"), so I agreed they were small and asked what would make telling on-call easy. He suggested a one-line note in the on-call channel before merging.

Where the relationship stood afterwards

Slightly awkward for about a week, then normal. We wrote the "tell on-call before a late merge" line into the team's runbook (the shared how-to document). Two weeks later I told him the weekend pages from changes had stopped and thanked him. The follow-up check was simple: count pages tied to late changes each month.

What made it work

• It built psychological safety (people feel safe to speak and be corrected without punishment) by keeping it private, specific, and about the effect on on-call.
• I owned my uncertainty and invited his view.

Pitfalls

Do not deliver it by chat while angry, do not use "always," and do not skip the follow-up; feedback without a check is just a one-time complaint.

Direct answer

I lead with the risky bug, label it clearly as blocking, explain why it matters and suggest a fix, and then mark everything else as non-blocking (nits and optional ideas) so the author can see what must change before merge. I keep the minor style issues short, grouped, or handled by an automated formatter, so they do not bury the one comment that matters.

Sequence and structure

1. Top summary comment: "One blocking item: possible race condition in save_order (a bug where two requests running at the same time interfere with each other). Everything else is non-blocking: nits and optional suggestions that will not hold the merge."
2. The bug comment, first and prominent: say what could happen, when, and a possible fix. "Blocking: two requests can both pass the stock check and both decrement, leaving negative stock. Could we do the check and update in one transaction (a group of database steps that succeed or fail together)? Happy to pair."
3. Label the rest. Google's engineering practices recommend prefixing comments with a severity label: "Nit:" for a minor point (their wording: "Technically you should do it, but it won't hugely impact things"), "Optional" or "Consider" for an idea that is "not strictly required", and "FYI" for something to think about later. Note the difference: a Nit is small and worth fixing at some point (at the latest the next time the author touches that code) but never holds up the merge, while Optional is genuinely skippable. On my team I would say in the top summary which labels I will not hold the merge for, so the author is never guessing.
4. Reduce the volume. Instead of ten comments on the same naming habit, leave one: "Nit: several variable names use abbreviations; same throughout, optional to fix." Better still, ask for a linter or formatter (an automatic tool that enforces style) so humans stop discussing spacing.

Worked example

Out of 12 observations, 1 is the race, 8 are naming and spacing, 3 are small refactors. What the author sees: 1 comment labeled Blocking, 1 grouped Nit: comment covering the 8 style points, and 3 comments labeled Optional. In the review tool, "Request changes" is a status that blocks the merge until the author responds, and "Approve" lets it go ahead. The status is "Request changes" only because of the one blocking item; if the author fixes it, I approve even if the nits remain.

Trade-offs and pitfalls

• A pile of equal-weight comments hides the one important one.
• Marking everything blocking trains authors to argue about everything.
• If the nits are really about unclear code, they might be maintainability concerns; promote those to "Consider" with a reason rather than "Nit".

Direct answer

The review comment that worked best was one that named a specific risk, explained the cost in the author's own terms, and offered a small change they could make in the same pull request (PR, a proposed code change awaiting review). I then turned the repeated comment into a team checklist item, so the fix did not depend on me remembering to say it. The story below is a skeleton: swap in your own system and numbers.

Situation

Over about two months, several PRs touching a billing-calculation module came back with follow-up bug fixes within a week of merging. In my review of that module I kept leaving the same kind of comment: a function mixed rounding, currency conversion and database writes, so no test could reach the rounding rule alone. (A bug-fix follow-up within a week of merging is a cheap, countable sign that something slipped through review.)

How I framed it

I did not write "this is messy." I wrote a comment with three parts, in the style of the SBI model (Situation, Behavior, Impact, from the Center for Creative Leadership):

• Situation and behavior: "In apply_discount, rounding happens after the currency conversion and inside the same function that writes the invoice row."
• Impact: "That means we can only test rounding by creating a database row, and last month's rounding bug got through for that reason."
• A small ask: "Could we pull the rounding into a pure function round_amount(amount, currency) (a function whose output depends only on its inputs, with no database or network calls) so we can add table-driven tests (one list of input and expected-output rows, each run through the same test) for the edge cases? Happy to pair on it if useful."

How the author reacted

The author first replied that the extra function felt like over-engineering for a small change. I did not repeat the request. I asked which cases they were most worried about getting wrong, then showed that the two cases they named were exactly the ones the split would make testable. They made the change in that PR and added the tests themselves, which mattered more than my comment did.

Making the change stick

1. I added two lines to the PR template for that module: "Is any money-handling rule testable without a database?" and "Which edge cases did you test?"
2. I stopped leaving the comment by hand. The template question prompted authors, and my reviews could shift to other things.
3. In the team retro (the regular meeting where the team reviews what worked and what did not) I shared the before-and-after of that one PR and asked whether the template question was helping or just noise. One teammate asked to narrow it to money-handling code, and we did.

Result (illustrative numbers, for the shape of the story)

I counted PRs in the module that needed a bug-fix PR within a week: 5 of the 12 merged before the change (about 42%) and 1 of the 11 merged in the two months after (about 9%). That is a small sample, so I treated it as a signal, not proof, and kept counting. The team also now had a written review expectation, which is a process change that outlasted my involvement.

Feedback in a model review

The same pattern applies to a machine-learning model review. A teammate's evaluation script reported high accuracy, but the same users appeared in both the training rows and the test rows. The model can then score well by recognising users it has already seen, so the number overstates how it will do on new users. I commented with the evidence (the count of users present in both sets, from a quick query), asked for a split by user so each person lands wholly in training or wholly in test, and later the team added a "split key" line to the model-review template (a field stating what the data was split by, such as user or date). On the receiving side, I have had a comment of the same shape on my own work, and in both cases the useful parts were the concrete evidence and the template change that followed.

What I would do differently

Ask the author earlier what they were worried about, and agree on the measure (PRs needing a follow-up fix) before the change, not after.

Direct answer

I would not agree to a blanket rule. I would tell the lead, with an example, that the sandwich (praise, criticism, praise) works when the praise is specific and true and the criticism is small, and backfires when the praise is a ritual cushion that people learn to ignore or when the real issue gets buried. My rule is simpler: be specific, explain why, and make severity unmistakable; add praise where it is genuinely earned.

A performance suggestion on a colleague's pull request

The pagination logic is easy to follow, and the test for the empty page is a good catch.

Suggestion (non-blocking): load_all_users() is called inside the request handler and loads every user into memory (the program's working storage) before slicing out one page. That is fine at today's size but gets slower and heavier as the table grows. Could we ask the database for just one page? Two common ways are ORDER BY id LIMIT 20 OFFSET 40 (skip 40 rows, return the next 20, i.e. rows 41 to 60) or a keyset query (WHERE id > 40 ORDER BY id LIMIT 20: continue after the last id already seen; here 40, assuming ids 1 to 40 were on earlier pages, so it returns the same rows 41 to 60 as the OFFSET form). Both need the ORDER BY: without it the database may return any rows in any order. I can share a short example if useful.

"Non-blocking" is the severity label: the author may merge without fixing it, while "blocking" means it must be fixed before merge. The praise is specific and about this change; the suggestion has a reason and a severity; the closing offer is optional.

When the sandwich helps

• The person is new or visibly discouraged, and there is real good work to point at.
• The criticism is minor and the praise is related to it.
• It is a long relationship where warmth is already established.

When it backfires

• The praise is generic ("nice work!") and followed by "but": people learn to wait for the "but" and discount the first slice.
• The real message is buried, so the person misses that something is blocking.
• It is formulaic: if every comment follows the pattern, readers notice and distrust all of it.
• Urgent or serious issues (a security flaw): clarity matters more than cushioning.

Positive and negative balance

I don't aim for a fixed ratio. I aim to notice good work when it happens, say it when I mean it, and keep criticism separate and clear. A teammate who hears praise only as a cushion starts to hear all praise that way.

How I would raise this with the lead

Not as an objection to their intent (they want kind feedback). I would say, "I want review comments to be kind and clear. Can we agree on: specific praise when it's real, labeled severity, and the reason for every suggestion?" Then show the example above.

Trade-offs and pitfalls

• Dropping praise entirely makes reviews cold and one-directional.
• Over-polishing comments slows reviews; short and clear beats elaborate.
• Accept the lead's decision if the team wants the sandwich; make your own comments good within it.

Direct answer

I'd talk to the teammate privately, describe the specific pattern and its effect on the team, ask what is making reviews slow, agree a small concrete change, and track a number before and after. A pull request (PR) is a proposed code change that teammates review before it is merged, so a slow reviewer holds up everyone's work.

Prepare

• Pull the data: how long PRs wait for their first review. I'd call that the time to first review.
• Check causes: are they buried in their own work, getting huge PRs, or in a different time zone? The fix depends on the cause.

Deliver

"Over the last month, PRs assigned to you waited a median of about two days for a first look. Two teammates told me their work was blocked while waiting. I'd like us to get that down. What's getting in the way, and what would help?"

Possible agreements: a fixed 30-minute review slot each morning, a one-working-day target for a first response (even just "I'll look Thursday"), or asking authors to keep PRs small.

How I'd tell whether it helped

Measure the same thing before and after. Illustrative numbers (hours to first review, five PRs each):

• Before: 52, 70, 30, 66, 48. Median 52 hours.
• After: 20, 6, 26, 4, 10. Median 10 hours.

The median drops from 52 to 10 hours. I'd use the median since one very slow PR distorts an average. With only five PRs, it is evidence, not proof, so I'd look again over a couple of weeks.

I'd also check that quality did not drop: are reviews still catching problems, and are teammates reporting fewer blocked days?

Pitfalls

• Public shaming or a leaderboard of reviewers.
• Making speed the only goal, so reviews become rubber stamps.
• Skipping the conversation and quietly reassigning their reviews.