Code Review Best Practices

Status: Complete
Category: Delivery
Default enforcement: Soft
Author: PushBackLog team

Tags

• Topic: delivery, quality, collaboration
• Skillset: engineering
• Technology: generic
• Stage: review, delivery

Summary

Code review is a structured process in which one or more engineers examine changes to a codebase before they are merged, with the goals of improving code quality, sharing knowledge, catching defects early, and maintaining consistency. Done well, code review is one of the highest-leverage practices in software engineering. Done poorly — as a bureaucratic gate, a vehicle for style wars, or a rubber stamp — it adds delay without adding value.

Rationale

Reviews catch defects cheaply

Finding a defect in code review costs a fraction of finding it in production. The closer to the origin a defect is caught — in review rather than in QA, in QA rather than in production — the cheaper it is to fix. Code review is the last structured opportunity to catch logic errors, security issues, and design problems before they become customer-facing failures.

Reviews distribute knowledge across the team

Knowledge concentrated in one person is a risk. When one engineer understands a module, a service, or a deployment process and no one else does, the team has a single point of failure. Code review forces at least one other engineer to read and understand every change. Over time this creates shared understanding, reduces key-person dependencies, and improves the team’s collective ability to debug, extend, and maintain the codebase.

Reviews shape culture and norms

The tone and substance of code reviews shape the team’s culture. A team where reviewers provide constructive, specific, kind feedback creates psychological safety and continuous learning. A team where reviews are nitpicky, inconsistent, or dismissive creates fear and withdrawal. Code review practices are worth discussing explicitly as a team.

Guidance

What reviewers should check

Correctness:

• Does the code do what the PR description says it does?
• Are there edge cases the author didn’t handle?
• Are error cases and unhappy paths covered?

Security:

• Does user input get validated and sanitised before use?
• Are there SQL injection, path traversal, or SSRF opportunities?
• Are secrets handled correctly (not hardcoded, not logged)?

Design:

• Is the change appropriately sized — one logical change per PR?
• Does it introduce unnecessary coupling or complexity?
• Will it be easy to maintain, test, and extend?

Tests:

• Do the tests actually verify the described behaviour?
• Is the test coverage adequate for the change?
• Are the tests brittle (testing implementation rather than behaviour)?

Readability:

• Are names clear and consistent with the surrounding code?
• Is the code intelligible to a future engineer unfamiliar with the change?

What reviewers should not block on

• Personal preferences not captured in the team’s style guide
• Refactorings unrelated to the change
• Minor issues that could be addressed in a follow-up PR (raise as a non-blocking comment)

Use signals to distinguish blocking from non-blocking comments:

# Non-blocking: suggest, but don't require
nit: This variable name could be clearer — `userRecord` instead of `u`

# Blocking: must be addressed before merge
[blocking] This SQL is vulnerable to injection — use parameterised queries

Author responsibilities

• Write a clear PR description: what changed, why, how to test it, any known trade-offs
• Keep PRs small: reviewable in under 30 minutes; aim for < 400 lines changed
• Respond to all comments: acknowledge, address, or reason why not
• Don’t merge under conflict: rebase or merge main into your branch first
• Don’t merge under failing CI: all checks must pass before review begins

PR size guidelines

For large features, break the work into smaller PRs. If a large PR is unavoidable, structure it so the reviewer can understand the context with a clear description and any supporting diagrams.

Review turnaround time

Slow code reviews block delivery. Teams should agree on and hold to SLAs:

• Initial review: within one business day of PR creation
• Response to author changes: within one business day
• If a PR is blocked waiting for review for more than a day, escalate via direct message rather than waiting

Self-review before requesting review

Before clicking “Request Review”, do a self-review in the GitHub/GitLab diff view:

• Have you re-read the full diff?
• Are there debug statements or console.log() calls left in?
• Does every TODO added in this PR have a tracking issue linked?
• Does the PR description accurately reflect the change?

Reviewer etiquette

• Assume good intent — the author made the best decision they could with available context
• Ask questions before making assertions — “What was the reasoning here?” beats “This is wrong”
• Praise good decisions explicitly — code review is a teaching opportunity both ways
• Distinguish facts from opinions — “The spec says X” vs “I prefer X”
• Approve with comments when comments are non-blocking — don’t hold up the PR for nits