Code Review Best Practices: What to Look For, How to Give Feedback, and Automating Checks

Editorial Team · · 3 min read

What Makes a Code Review Actually Useful

Most code reviews are a waste of time. I don't say that to be provocative — it's just that the average PR review consists of someone skimming the diff, leaving a "looks good" comment, and hitting approve. That's not a review. That's a rubber stamp.

The reviews that catch real bugs share a few traits: the reviewer understands the system context, they're looking at specific categories of issues, and they don't try to review everything at once. Let's break down what actually works.

The Three-Pass Approach

I'd argue the most effective reviewers don't read code linearly. They do three passes:

Pass 1: Intent. Read the PR description and the test changes first. What's this supposed to do? If you can't answer that in 30 seconds, the PR description needs work — send it back before reading any implementation code.

Pass 2: Architecture. Look at the file list. Are changes in the right places? Is a new abstraction being introduced where an existing one would work? Are there changes in unexpected directories? This pass takes maybe two minutes and catches the biggest issues.

Pass 3: Line-by-line. Now read the actual code. But you're not reading all of it equally. Focus on:

  • Error handling paths — these are where bugs hide
  • Boundary conditions in loops and conditionals
  • Resource cleanup (database connections, file handles, locks)
  • Concurrency concerns if shared state is involved

Giving Feedback That Doesn't Start Arguments

The tone of review comments matters more than most engineers want to admit. There's a massive difference between "This is wrong" and "I think this might break when X happens — what do you think?" Both communicate the same technical concern, but one invites collaboration and the other invites defensiveness.

A few patterns that work well:

Prefix your comments with intent. Use labels like nit: for style preferences, question: for things you genuinely don't understand, and blocking: for issues that must be fixed. This saves everyone time because the author knows which comments they can address later and which ones block the merge.

Suggest concrete alternatives. Instead of "this could be simpler," show the simpler version. It takes you thirty extra seconds and saves a round-trip of back-and-forth.

# Instead of: "This logic is confusing"
# Try: "Would something like this be clearer?"
#
# users = [u for u in all_users if u.active and u.role in allowed_roles]
#
# Instead of the nested if/else chain on lines 45-62

Don't bikeshed. If it's a matter of personal preference and there's no project standard, let it go. Save your review capital for things that matter.

What to Actually Look For

Here's my checklist, roughly ordered by severity:

Correctness bugs. Off-by-one errors, null pointer risks, race conditions, missing error handling on I/O operations. These are the highest value catches because they'd ship to production otherwise.

Security issues. SQL injection, XSS, hardcoded secrets, overly permissive CORS, missing authentication checks on new endpoints. If you're not sure, flag it anyway — better to have a false positive than miss an actual vulnerability.

Performance landmines. N+1 queries, unbounded result sets, missing indexes for new query patterns, synchronous operations that should be async. You don't need to optimize everything, but catch the things that'll page someone at 3 AM when traffic spikes.

Maintainability. Duplicated logic that should be extracted, overly clever code that'll confuse the next person, missing or misleading comments on non-obvious behavior.

Automating the Boring Parts

Humans shouldn't be checking formatting, import order, or naming conventions. That's what linters are for.

A solid automated review pipeline looks like this:

# .github/workflows/pr-checks.yml
name: PR Checks
on: [pull_request]
jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm ci
      - run: npm run lint
      - run: npm run type-check

  tests:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm ci
      - run: npm test -- --coverage
      - uses: codecov/codecov-action@v4

  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm audit --audit-level=high
      - uses: github/codeql-action/analyze@v3

Tools like danger.js can also add automated comments — flagging large PRs, missing test files, or changes to critical paths like migration files. I've seen teams cut their review cycle time by 40% just by automating the mechanical checks.

Review Size and Turnaround

Google's internal research found that reviews over 400 lines get significantly less thorough attention. Keep PRs small. If a feature requires 2,000 lines of changes, break it into a stack of smaller PRs that each make sense independently.

On turnaround: aim for same-day reviews, ideally within a few hours. A PR sitting in review for three days is a context-switching tax on the author, who now has to re-load the mental model when feedback finally arrives. Some teams use a review rotation — one person each day is the designated first-reviewer, and their job is to respond to all open PRs within two hours.

When to Approve

Approve when the code is better than what it replaces and you don't see any correctness or security issues. It doesn't need to be perfect. It doesn't need to match how you'd write it. "Good enough and shipping" beats "perfect and stuck in review" every single time.