Code Review Best Practices: How to Give and Receive Feedback That Improves Code

Code review catches bugs that automated tests miss, spreads knowledge across the team, and creates a forcing function for clear code. Done poorly, it becomes a bottleneck, a source of friction, and a way for senior engineers to impose style preferences on junior ones. The difference is almost entirely in how both sides handle the conversation.

A pull request is a proposal. The reviewer's job is to evaluate whether that proposal should be merged, and the author's job is to make a clear case for it. Both sides have responsibilities that determine whether the review improves the code or just delays it.

What to look for as a reviewer

The single most common mistake reviewers make is spending time on style while missing correctness. The order of priority should be:

1. Correctness: Does the code do what the description says? Does it handle edge cases (empty input, null, overflow, concurrent access)? Are there off-by-one errors?
2. Security: Are inputs validated? Is user-supplied data ever passed to SQL, shell commands, or HTML without sanitization? Are secrets handled correctly?
3. Error handling: Are errors propagated or swallowed? Does the code handle network failures, missing files, and unexpected responses?
4. Design: Does this fit the existing architecture? Are abstractions at the right level? Will this be easy to change in six months?
5. Performance: Are there obvious inefficiencies that will matter at expected scale (N+1 queries, unnecessary copies of large data structures)?
6. Tests: Do the tests cover the new behavior? Do they test edge cases or only the happy path?
7. Style: Last and lowest priority. If the team has an agreed linter or formatter, style is handled by automation, not by comments.

Read the PR description before reading the diff. Understanding the intent helps you evaluate whether the implementation achieves it, rather than evaluating the code in isolation.

How to write review comments

The quality of a comment determines whether it improves the code or just creates work for the author to argue about. Four principles help:

Explain the why, not just the what. "Change this to X" gives the author nothing to reason about. "This function will return None for empty inputs because the loop never executes — consider adding an early return or raising a ValueError" gives them the context to decide how to fix it.

Distinguish blockers from suggestions. Use explicit markers so the author knows what is required and what is a take-it-or-leave-it observation:

# Comment prefixes used by many teams:

# REQUIRED / blocking:
# This will raise a KeyError when `user_id` is not in the cache.
# The caller has no way to handle that. Use .get() with a default or add a check.

# SUGGESTION / optional:
# (nit) You could use a list comprehension here for brevity,
# but the loop form is also readable. Up to you.

# QUESTION / seeking understanding:
# (q) Why is this setting cached separately from the user object?
# Is there a case where they can get out of sync?

# FYI / no action needed:
# (fyi) Python 3.11 added ExceptionGroup for this pattern,
# worth knowing for future reference.

Suggest, do not demand. "Why not use X?" is better than "Use X." The author has context you do not have. A question leaves room for the author to explain why X does not work here, which often surfaces important information.

Be specific about location and scope. "This function is too complex" is difficult to act on. "This function handles three distinct responsibilities: parsing, validation, and persistence. Splitting out the validation into a separate function would make it easier to unit test" is actionable.

Reviewing with a checklist

A mental checklist run through for each diff reduces the chance of missing categories:

# Reviewer mental checklist (not written into comments, just internal)

# Correctness
# - What happens with empty / null / zero inputs?
# - What happens if this external call fails or returns unexpected data?
# - Are any loops or recursions guaranteed to terminate?
# - Is concurrent modification possible here?

# Security
# - Any SQL built from user input? (Use parameterized queries)
# - Any shell calls with user data? (Use subprocess with list args, not shell=True)
# - Any file paths from user input? (Path traversal risk)
# - Are new API endpoints authenticated and authorized?

# Tests
# - Is there a test for the bug this fixes?
# - Are error paths tested, not just the happy path?
# - Could any test pass trivially (always returns True, asserts nothing)?

Keeping reviews small

The most effective structural improvement to code review is keeping pull requests small. A PR with 200–400 lines of diff receives meaningful feedback. A PR with 2000 lines of diff receives rubber stamps with occasional style nitpicks — the cognitive load is too high to catch correctness issues.

If you are the author of a large change, break it into a stack of smaller PRs where each one is independently reviewable. Feature flags let you merge code without exposing it to users. A "foundation PR" that adds infrastructure followed by "feature PRs" that use it is easier to review than one PR that does everything.

Receiving a review

The author's frame matters as much as the reviewer's. A few practices that keep reviews constructive:

• Separate your ego from your code. A comment on a function is not a comment on your value as a developer. The best authors treat reviewers as collaborators looking for problems together.
• Respond to every comment. If you made the change, say so. If you disagree, explain why. If you decided not to act on a suggestion, acknowledge it: "Good point — I am leaving it as-is because X, but I have opened a ticket to revisit this." Leaving comments unresolved creates ambiguity about whether they were read.
• Ask for clarification rather than guessing. If a comment is unclear, ask. A one-sentence question resolves in minutes; a misunderstood comment and a wrong change resolve in days.
• Do not merge with unresolved blockers. If you believe a blocker comment is wrong, resolve the disagreement in the PR thread or by talking to the reviewer. Do not merge and address it later — "later" rarely comes.

The goal of review

Code review is not a gatekeeping mechanism. It is a defect-detection and knowledge-sharing process. The best reviews produce code that is clearly correct, that the team (not just the author) understands, and that the reviewer would be comfortable maintaining. When review consistently achieves that, it earns the time it takes.