Code Review Best Practices

Introduction

Code review is one of the highest-return activities in software development. It catches bugs early, shares knowledge across the team, and maintains consistent code quality. This guide covers practices for both authors (who submit code) and reviewers (who evaluate it).

For Authors: Making Your Code Reviewable

1. Keep PRs Small

Aim for 200-400 lines of changed code per PR. Large PRs overwhelm reviewers and increase the chance of bugs slipping through.

# Bad: 2000 lines across 15 files
# Good: 150 lines in 3 related files

Strategies for small PRs:

Split large features into stacked PRs
Extract refactoring into separate PRs
Use feature flags to merge incrementally

2. Write a Clear Description

A good PR description answers:

What changed and why
How to test it
Links to tickets, designs, or related PRs
Screenshots for UI changes

## What
Add email validation to the user registration form.

## Why
Currently invalid emails pass through and cause downstream
processing errors in the marketing automation pipeline.

## How to test
1. Go to /register
2. Enter "not-an-email" — should see validation error
3. Enter "user@example.com" — should pass

## Related
Fixes #142

3. Self-Review Before Submitting

Review your own PR first. You will catch:

Leftover debug code (console.log, print)
Unintended changes
Missing tests or documentation
Typo-level bugs

4. Respond to Feedback Constructively

Assume positive intent from reviewers
Ask clarifying questions instead of defending
Separate “must fix” from “nice to have” suggestions
Update the PR promptly after feedback

For Reviewers: Giving Effective Feedback

1. Review Within 24 Hours

Fast turnaround keeps the author in context and prevents blocked work. If you cannot review in 24 hours, delegate or let the team know.

2. Use a Review Checklist

Systematic reviews are more thorough:

Category	Questions
Functionality	Does it do what the PR claims? Are edge cases handled?
Tests	Are there tests for new logic? Do existing tests still pass?
Readability	Are names clear? Is complexity justified?
Security	Are inputs validated? Are secrets exposed?
Performance	Are there N+1 queries? Unnecessary allocations?
Maintainability	Is there duplicated code? Will this be hard to change?

3. Categorize Feedback

Use severity levels to help authors prioritize:

Level	Meaning	Example
Blocking	Must fix before merge	”This query lacks an index; it will scan the entire table”
Suggestion	Consider it	”You could simplify this with `map` instead of `for`”
Nitpick	Personal preference	”I prefer single quotes for strings”

4. Ask Questions, Don’t Dictate

Instead of: “Change this to use a dictionary.”

Ask: “Would a dictionary make the lookup faster here?”

Questions encourage discussion and help the author learn, whereas orders create resistance.

5. Approve with Comments

If the code is acceptable but you have minor suggestions, approve the PR and let the author decide whether to address them. Don’t block merges for nits.

Review Patterns That Work

Pair Review

Two reviewers alternate: one reads logic, the other reads tests. Catches different categories of issues.

Tool-Assisted Review

Let automation handle the boring stuff:

Linters (ESLint, Black, Prettier) for style
Static analysis (SonarQube, CodeClimate) for complexity
Security scanners (Snyk, CodeQL) for vulnerabilities
CI tests for regressions

Reserve human review for architecture, logic, and intent.

Review Roulette

Rotate reviewers so knowledge spreads evenly. Avoid having only senior engineers review everything.

Metrics to Track

Metric	Target	Why
PR size	<400 lines	Review quality drops sharply above this
Review turnaround	<24 hours	Prevents context loss and blocked work
Defect escape rate	Decreasing	Bugs found in production vs. review
Review participation	>80% of team	Knowledge sharing

Common Mistakes

Bike-shedding: Spending review time on trivial style issues while missing real bugs
Rubber-stamping: Approving without reading because “they never make mistakes”
Gatekeeping: Using review power to enforce personal preferences
Delayed feedback: Waiting days to review, forcing the author to re-learn context
No follow-up: Suggesting changes but never checking if they were made

Frequently Asked Questions

Q: How do I review code in an unfamiliar language or domain? A: Focus on what you can evaluate: test coverage, variable naming, obvious logic errors, and documentation clarity. Ask domain experts for the technical nuances.

Q: What if the author disagrees with my feedback? A: Discuss it. If it’s a blocking issue and you cannot agree, escalate to the team lead. For suggestions, let the author decide and move on.

Q: Should I block a PR for missing tests? A: Yes, if the PR adds logic that can be tested. No, if it’s a pure refactor with existing coverage, or UI changes that require E2E tests owned by another team.