Giving and Receiving Feedback Questions

Situation: I'm reviewing a design doc that states "Algorithm X is O(n)" with no justification.

Request for clarification (tone: curious, constructive):

• "Can you expand the complexity reasoning for Algorithm X? I want to understand assumptions and worst-case behavior."
• Ask for the intended cost metric (time? space? average vs worst case? amortized?) and input model (random, adversarial, max n, distribution).

Evidence to request:

• Short pseudocode or annotated steps showing the exact loop/recursion structure.
• Per-step cost analysis: show how many times key operations run as a function of n (e.g., nested loops, recursive depth).
• Clarify hidden costs (hash table ops, allocations, I/O) and their assumed cost model.
• Formal argument or sketch: induction, loop invariants, or recurrence solved to O(n).
• Amortized analysis if using dynamic arrays/union-find/etc.
• Space complexity and constants if they matter.
• Representative benchmarks (microbench + realistic workloads), with input sizes, environment, and reproducible scripts.
• Edge cases (already-sorted, all-equal, pathological inputs) and worst-case examples.

Where I'd push back:

• If the claim ignores worst-case or adversarial inputs that the product may encounter.
• If the proof relies on unrealistic assumptions (e.g., O(1) hash ops without handling collisions).
• If the implementation uses hidden nested loops or recursion that can be Ω(n²) in practice.
• If performance matters at scale and only a hand-wavy claim is given—ask for measurable evidence or a formal bound.

Where I'd accept:

• Clear, concise pseudocode plus a correct recurrence/loop-count derivation showing O(n).
• A standard algorithm with a well-known proof (cite reference) and matching implementation.
• Reproducible benchmarks that confirm linear scaling across relevant n and adversarial cases.

Example review comment (concise):

• "Thanks — please add pseudocode and a short complexity derivation (worst/avg/amortized), note any assumptions (hash behavior, input distribution), and include one benchmark for n=[10³,10⁵,10⁷]. If you can show a loop invariant or recurrence that resolves to O(n), I'm happy to accept the claim."

This approach balances mentorship (teach how to justify) with pragmatic verification (proof + empirical).

Situation: In day-to-day reviews I aim to keep the codebase healthy and the team aligned.

Approach (brief): I prioritize clarity and impact—start with positive, state the issue with concrete evidence (file/line/test), explain why it matters (bug/risk/maintenance), propose one or two concrete fixes, and offer to pair if needed. I avoid judgmental language and focus on the code, not the author.

Example phrasing:
a) Small stylistic nit

• "Nit: In this function, the variable name x1 is a bit unclear. Could we rename it to userCount to improve readability? No functional change needed — happy to apply if you want."
  b) Design flaw with potential runtime impact
• "This loop can become O(n^2) when input grows (see lines 120–135). If we cache the lookup in a map or use a streaming aggregation, we can reduce it to O(n). Suggest: build a Map<id, value> outside the loop. I can sketch the change in a follow-up patch."
  c) Security or privacy concern
• "Potential security issue: user input is concatenated into a SQL string on line 78, which risks SQL injection. Could we use parameterized queries or prepared statements instead? Example: db.query('SELECT * FROM users WHERE id = ?', [userId]). Happy to help refactor."

How I keep feedback actionable and collaborative:

• Reference exact lines/tests and the specific impact (performance, correctness, security).
• Offer a concrete fix or code snippet rather than just pointing out problems.
• Use neutral language ("could", "suggest") and call out positives first.
• Offer to pair-program or open a follow-up PR to implement changes.
• Close the loop: after changes, acknowledge improvements to reinforce positive behavior.

Situation: At my last company, product feedback suggested our onboarding flow confused new users and a skeptical PM believed changes would risk conversion and retention.

Task: I needed to convince that a metric-based experiment could validate an improved flow while protecting core business metrics.

Action:

• I started by framing the hypothesis: "Changing step X to Y will reduce time-to-first-success and increase 7-day retention." That tied the experiment to business goals.
• I proposed an A/B experiment implemented behind a feature flag so rollout is instant and revertible. I described instrumentation: events for impressions, clicks, completion, errors, and user identifiers for cohort analysis.
• Success criteria (primary + guardrails):
  • Primary metric: increase in time-to-first-success (shorter) or completion rate by ≥3% with p<0.05 and minimum detectable effect calculated from baseline variance and required sample size.
  • Guardrail metrics: 7-day retention, conversion to paid, error rate, and load/latency — none should degrade beyond predefined thresholds (e.g., <1% relative decline).
• I showed the power analysis numbers, expected run time, and canary rollout plan (1% → 10% → 100%) with automated monitoring and alerts.
• To address the stakeholder’s skepticism, I incorporated their concerns as acceptance criteria (e.g., must not increase support tickets) and proposed ongoing checkpoints: daily dashboards, mid-experiment stop rules, and a post-run causal analysis including segmentation (new vs returning users).
• I offered a backout plan: disable flag, rollback DB changes, and hotfix scripts.

Result: The stakeholder agreed. After a 3-week run we observed a statistically significant 4% improvement in completion with no adverse guardrail signals. The change was rolled out fully and reduced support tickets by 12% in the following month.

This approach demonstrates respect for data, technical safety, and stakeholder risk concerns while giving a clear path to decide based on metrics.

Situation: On a quarterly feature, a teammate submitted a large PR (~900 LOC) that added a new payments adapter. The implementation worked but had minimal tests, mixed business logic with transport code, and later caused two regressions in staging.

Task: As the reviewer, I needed to give constructive feedback so the module was safer, testable, and had clear boundaries before merging.

Action:

• I started with positives: noted clear intent, good variable names, and helpful comments.
• Then I framed concerns as risks: “This class mixes domain rules with HTTP handling — that makes unit testing and future changes risky and likely to cause regressions.”
• I proposed concrete, small changes: extract a PaymentsService interface for business logic, move HTTP details into an Adapter, and add unit tests covering success, failure, and retry logic. I suggested splitting into two follow-up commits to keep the PR reviewable.
• I offered help: paired for 30 minutes to sketch the interface and wrote one example unit test to show pattern and mocking strategy.
• We agreed on acceptance criteria: 80%+ coverage for the new module and zero new regressions in the next two staging runs.

Result: The author was receptive — they thanked me and implemented the refactor and tests over two commits. Test coverage for the payments module rose from 22% to 86%. In the following two sprints we saw zero regressions attributed to payments (previously two per sprint). To make the lesson stick, I added a short code-review checklist item about “separate domain logic from transport” and a template for splitting large PRs; the team adopted both and regressions in similar modules dropped by ~60% over the next quarter.

This taught me that framing feedback as risk + concrete remediation, offering help, and embedding changes into team process yields both immediate fixes and lasting improvement.

Situation: A new hire submitted a pull request with several issues (style, a failing test, and an unclear function name) during their first week.

Task: My goal was to give feedback that fixed the problems, taught them our standards, and kept them motivated.

Action:

• I started with positive recognition: "Thanks for the thoughtful test coverage and clear commit messages — that helps a lot."
• I gave specific, actionable feedback with rationale and examples:
  • "Tests: One unit test is flaky because it depends on system time. Please use a fixed clock or mock time; for example, replace time.Now() with a Clock interface and inject a fake in tests."
  • "Naming: rename calculate to calculateInvoiceTotal — it clarifies intent and makes callers readable."
  • "Style: we prefer 2-space indentation in this repo; run our formatter (pre-commit hook) before pushing."
• I avoided absolute language and focused on the code: "This approach causes X" rather than "You did X wrong."
• I offered help and resources: links to our style guide, an example PR that implemented Clock, and an invitation: "If you want, we can pair for 20 minutes to walk through the test fix."

Result / Follow-up steps:

• Requested a small next step: "Can you update the test and rename the function? Ping me when ready and I’ll re-review."
• If they seem unsure, I schedule a short pairing session and follow up with a comment summarizing what we changed and why.
• I leave an encouraging closing: "Nice work overall — these fixes will make the change much more robust. Welcome aboard!"

This balances encouragement, clear instructions, and learning opportunities so the new hire feels supported and can iterate quickly.