Code Review Best Practices: How to Give and Receive Feedback Like a Senior

Why Code Review Matters Beyond Bug-Finding

Most developers think code review is about catching bugs. It is — but that's the smallest part.

Code review is how:

• Knowledge transfers across the team (everyone sees every change)
• Architecture decisions get challenged before they're entrenched
• New developers learn the codebase at scale
• Standards stay consistent without a style-police role
• Authors are forced to explain their reasoning (which often reveals flaws)

In interviews, how you talk about code review reveals your engineering maturity. Senior engineers lead productive reviews. Junior engineers either rubber-stamp PRs or leave demoralising comments.

As the Reviewer

What to Actually Look For

Correctness (high priority)

• Does the code do what the PR description claims?
• Are edge cases handled? (null, empty, concurrent, negative numbers)
• Is error handling correct — or swallowed silently?
• Are there off-by-one errors, race conditions, or data loss scenarios?

Design (high priority)

• Does this belong here? (right layer, right class, right file)
• Is there a simpler way to achieve the same result?
• Does this introduce coupling that will hurt future changes?
• Is the interface/method contract clean and unsurprising?

Security (high priority)

• Any SQL injection, XSS, or SSRF risk?
• Are secrets hardcoded? Input validated at the boundary?
• Does the auth check happen before the data access?

Readability (medium priority)

• Can you understand what this does without reading the tests?
• Are names accurate (not data, result, temp, thing)?
• Is there dead code, commented-out code, or TODO that should be a ticket?

Performance (context-dependent)

• Is this on a hot path? If so, does it have obvious inefficiency?
• N+1 queries in a loop?
• Large allocations in frequently-called code?

Tests (always)

• Do the tests cover the meaningful cases, not just the happy path?
• Are tests coupled to implementation details (testing internals, not behaviour)?
• Do tests actually fail when the code is broken?

What NOT to Do in a Review

Don't nitpick style without a linter. "I prefer var here" is not a code review comment — it's a style opinion. If the team cares about it, add a linter rule. If there's no rule, let it go.

Don't leave vague criticism. "This doesn't feel right" is useless. Say what the actual concern is.

Don't approve code you don't understand. If you can't follow the logic after reading it twice, that's a signal — either it's overly complex, or you need an explanation. Ask.

Don't use reviews to rewrite the author's approach. If an alternative design would be significantly better, raise it as a suggestion — not a demand. Offer to pair or discuss.

Comment Formatting

Prefix your comments to signal intent:

nit: Could use a clearer variable name here — e.g., `pendingOrderIds` vs `items`

suggestion: Consider extracting this into a private method — it's called twice

question: Is this intentionally O(n²)? Should be fine for small datasets but
          worth noting if the list could grow.

blocker: This could throw NullReferenceException if `customer` is null —
         the caller doesn't guarantee a non-null result here.

praise: Nice use of the Outbox pattern here — this is exactly the right approach.

Separate blockers (must fix) from suggestions (worth considering) from nits (optional). If everything feels equally important, nothing gets fixed.

The "Two-Pass" Technique

First pass (macro): Read the PR description and diff at a high level. Understand the intent. Ask: does this approach make sense before I look at the details?

Second pass (micro): Read the code carefully. Look for correctness issues, edge cases, naming, tests.

This prevents getting lost in naming debates while missing a data race.

As the Author

Before You Open the PR

Self-review your own diff. Open the PR, read the diff as if you're a stranger. You'll catch ~30% of your own issues this way.

Write a useful PR description:

## What
Added Redis caching for the product catalogue endpoint.

## Why
Products rarely change but are fetched on every order creation.
SQL Server was handling 800 reads/sec for mostly identical data.

## How
- Cache-aside pattern with a 10-minute TTL
- Cache key: `product:{id}`
- Invalidated on product update via domain event

## Testing
- Unit tests: cache hit/miss paths
- Manual: verified in staging with Redis insight showing hit rate >95%

## Notes
- No cache warming — first request per product hits DB. OK for now.
- TODO: investigate tag-based invalidation if product updates become frequent

A good description answers: what, why, how, what you tested, and what trade-offs you made.

How to Receive Criticism

Assume good intent. A comment pointing out a bug is not an attack on your intelligence. It's the system working correctly.

Separate ego from code. "Your code is wrong" is not "you are wrong." Code can be improved; that's the point.

Ask for clarification before defending. "I'm not sure I follow — are you concerned about X or Y?" often reveals the comment was about something different from what you assumed.

Don't argue about style. If someone prefers a different naming convention and there's no enforced rule, ask the team to decide once and move on. Style arguments in reviews are a tax on everyone.

When you disagree on design:

"I see your concern about X, but I chose this approach because of Y.
 Would it help if I added a comment explaining the trade-off?
 Or would you prefer to discuss this in a call?"

Offer a path to resolution. Don't just leave competing opinions in the comment thread.

Review Patterns for Teams

The 24-Hour Rule

PRs should get initial feedback within 24 hours (business hours). Stale PRs block the author, create merge conflicts, and signal that reviews aren't valued.

Review Rotation

Don't always send reviews to the same senior engineer. Rotate reviewers so knowledge spreads and no one becomes a bottleneck.

Size Limits

Small PRs get reviewed faster and more thoroughly. Split large features into:

• Setup PR (interfaces, migrations, scaffolding)
• Implementation PRs (one logical unit per PR)
• Tests PR (if the feature is large enough to warrant it)

A PR that touches 50 files gets rubber-stamped. A PR that touches 5 files gets read.

Async-First

Don't comment "can we talk about this?" and wait for a meeting. Write the concern down. Async reviews scale; synchronous review meetings don't.

Code Review in Interviews

Questions you might be asked:

"How do you approach code reviews?"

Show you know the levels: correctness first, then design, then readability. Mention separating blockers from suggestions. Show you care about the author's experience, not just the code.

"Tell me about a time you gave difficult feedback in a review."

Use STAR. Show that you separated the concern (the code) from the person. Show that it resolved constructively.

"How do you handle disagreements in code review?"

Acknowledge that disagreements happen. Show you escalate to a discussion (call, team decision) rather than leaving unresolved comment threads.

Quick Reference

Reviewer Checklist

• [ ] Read the PR description before the diff
• [ ] Understand the intent before judging the implementation
• [ ] Check correctness, edge cases, error handling first
• [ ] Check tests cover meaningful cases
• [ ] Separate blockers from suggestions from nits
• [ ] Don't nitpick style if there's no linter rule
• [ ] Approve code you would defend if asked about it later

Author Checklist

• [ ] Self-review the diff before opening
• [ ] Write a PR description explaining why, not just what
• [ ] Keep PRs small — aim for < 300 lines of production code
• [ ] Link related tickets, issues, or design docs
• [ ] Respond to all comments before merging
• [ ] Mark conversations as resolved when addressed