Code Review Best Practices for Engineering Teams

You submit a pull request. Three days later: crickets. Or worse—you get back a comment saying “looks good” with zero engagement. Or the opposite extreme: 47 nitpicky comments about formatting while missing a critical security flaw.

Code reviews are broken on most teams.

After conducting thousands of code reviews across multiple organizations and building systems like Cortex that handle code at scale, I’ve learned that great code reviews aren’t about catching bugs—they’re about building a culture of excellence.

Let me show you how to do code reviews that actually matter.

Why Code Reviews Matter

The best code reviews serve three critical purposes:

1. Knowledge Transfer

Code reviews are the fastest way to level up a team. Every review is a teaching opportunity:

• Junior developers learn patterns from seniors
• Senior developers learn domain context from specialists
• Everyone learns from seeing different approaches

2. Quality Assurance

Fresh eyes catch what you missed:

• Logic errors
• Edge cases
• Security vulnerabilities
• Performance issues
• Accessibility problems

3. Code Ownership

Reviews distribute knowledge across the team:

• No single points of failure
• Multiple people understand each system
• Easier maintenance and debugging

The goal isn’t perfection—it’s shared understanding and continuous improvement.

The Reviewer’s Checklist

High-Impact Issues (Must Fix)

These are non-negotiable:

Security Vulnerabilities

// ❌ Security risk: SQL injection
const query = `SELECT * FROM users WHERE email = '${userEmail}'`;

// ✅ Use parameterized queries
const query = 'SELECT * FROM users WHERE email = $1';
const result = await db.query(query, [userEmail]);

// ❌ Security risk: Exposing sensitive data
res.json({
  user: {
    id: user.id,
    email: user.email,
    password: user.password, // Never expose hashed passwords!
    ssn: user.ssn
  }
});

// ✅ Expose only necessary data
res.json({
  user: {
    id: user.id,
    email: user.email
  }
});

Correctness Issues

// ❌ Off-by-one error
for (let i = 0; i <= items.length; i++) {
  process(items[i]); // Will crash on last iteration
}

// ✅ Correct bounds
for (let i = 0; i < items.length; i++) {
  process(items[i]);
}

// ❌ Race condition
async function updateCounter() {
  const current = await getCounter();
  await setCounter(current + 1); // Another process might update between get/set
}

// ✅ Atomic operation
async function updateCounter() {
  await db.query('UPDATE counters SET value = value + 1 WHERE id = $1', [counterId]);
}

Performance Problems

// ❌ N+1 query problem
async function getPostsWithAuthors(postIds: string[]) {
  const posts = await db.getPosts(postIds);

  for (const post of posts) {
    post.author = await db.getUser(post.authorId); // Database call in loop!
  }

  return posts;
}

// ✅ Batch load authors
async function getPostsWithAuthors(postIds: string[]) {
  const posts = await db.getPosts(postIds);
  const authorIds = posts.map(p => p.authorId);
  const authors = await db.getUsers(authorIds);
  const authorMap = new Map(authors.map(a => [a.id, a]));

  return posts.map(post => ({
    ...post,
    author: authorMap.get(post.authorId)
  }));
}

Medium-Impact Issues (Should Fix)

These improve maintainability:

Unclear Naming

// ❌ Cryptic names
function proc(d: any): any {
  const r = d.map(x => x * 2);
  return r.filter(y => y > 10);
}

// ✅ Clear, descriptive names
function processMetrics(rawMetrics: number[]): number[] {
  const doubledMetrics = rawMetrics.map(metric => metric * 2);
  return doubledMetrics.filter(metric => metric > 10);
}

Missing Error Handling

// ❌ Unhandled promise rejection
async function fetchUserData(userId: string) {
  const response = await fetch(`/api/users/${userId}`);
  return response.json();
}

// ✅ Proper error handling
async function fetchUserData(userId: string): Promise<User> {
  try {
    const response = await fetch(`/api/users/${userId}`);

    if (!response.ok) {
      throw new Error(`Failed to fetch user: ${response.statusText}`);
    }

    return await response.json();
  } catch (error) {
    logger.error('Error fetching user data', { userId, error });
    throw new UserFetchError(`Could not load user ${userId}`, { cause: error });
  }
}

Missing Tests

// A new feature without tests is incomplete
// ❌ No tests for critical business logic
function calculateDiscount(price: number, userType: string): number {
  if (userType === 'premium') return price * 0.8;
  if (userType === 'regular') return price * 0.95;
  return price;
}

// ✅ Include tests with the implementation
describe('calculateDiscount', () => {
  it('applies 20% discount for premium users', () => {
    expect(calculateDiscount(100, 'premium')).toBe(80);
  });

  it('applies 5% discount for regular users', () => {
    expect(calculateDiscount(100, 'regular')).toBe(95);
  });

  it('applies no discount for other users', () => {
    expect(calculateDiscount(100, 'guest')).toBe(100);
  });
});

Low-Impact Issues (Nice to Have)

These are stylistic preferences:

Code Style

Use automated tools (Prettier, ESLint) for this instead of manual reviews:

# Let automation handle formatting
npm run lint:fix
npm run format

Don’t waste review time on:

• Semicolons
• Single vs double quotes
• Tab vs spaces
• Line length

Configure these once in your linter and forget about them.

How to Give Feedback

The Feedback Framework

1. Start with Appreciation

Begin every review by acknowledging good work:

Nice work on the caching implementation! The cache invalidation strategy
is solid and the tests are thorough.

People are more receptive to criticism when they feel their effort is valued.

2. Be Specific and Actionable

❌ "This code is confusing"
✅ "The nested conditionals make this hard to follow. Consider extracting
   each condition into a well-named function."

❌ "This might have performance issues"
✅ "This O(n²) loop will be slow with large datasets. Consider using a
   Map for O(1) lookups: [link to example]"

❌ "This doesn't follow best practices"
✅ "This couples the service to a specific database. Consider injecting
   a Database interface to make testing easier."

3. Explain the “Why”

Don’t just point out problems—teach:

Instead of:
> Use const instead of let

Write:
> Use const instead of let here since userId is never reassigned.
  This prevents accidental mutations and signals intent to future readers.

Instead of:
> Add error handling

Write:
> Add error handling here because network requests can fail. Without it,
  the promise rejection will crash the app. Consider wrapping in try/catch
  and returning a user-friendly error message.

4. Offer Solutions, Not Just Problems

❌ "This approach won't scale"

✅ "This approach will be slow with 10k+ users because it loads all records
   into memory. Consider implementing pagination:

   ```typescript
   async function getUsers(page: number, limit: number) {
     const offset = (page - 1) * limit;
     return db.query(
       'SELECT * FROM users LIMIT $1 OFFSET $2',
       [limit, offset]
     );
   }
   ```"

5. Use Questions to Spark Discussion

Instead of:
> This is wrong

Try:
> What happens if the API returns null here? Should we handle that case?

Instead of:
> Change this to use async/await

Try:
> Would async/await make this easier to read? It would avoid the callback
  nesting we have here.

Tone and Language

Do This ✅

- "What do you think about extracting this into a helper function?"
- "I'm not sure I understand why we need this check. Can you explain?"
- "Nice solution! One edge case: what happens if the array is empty?"
- "This works, but have you considered using Array.reduce here? It might be more concise."

Avoid This ❌

- "This is wrong"
- "You should know better"
- "Why didn't you just use X?"
- "This is terrible code"
- "Obviously this won't work"

Remember: You’re reviewing code, not judging the person.

How to Receive Feedback

The Growth Mindset

Every piece of feedback is a learning opportunity. Great developers:

1. Assume Good Intent

The reviewer is trying to help, not attack you.

2. Ask Questions

If feedback is unclear:

Thanks for the feedback! Can you help me understand what you mean by
"this approach won't scale"? Are you concerned about database performance
or something else?

3. Acknowledge and Respond

✅ "Good catch! Fixed in the latest commit."
✅ "Great point. I've added error handling and tests."
✅ "I considered that approach but went with this one because [reason].
   What do you think?"

4. Don’t Take It Personally

❌ "This code is fine. You're being too picky."
✅ "I see your point. Let me refactor this to be clearer."

5. Know When to Push Back

Sometimes you’ll disagree. That’s okay:

"I understand your concern about performance, but I profiled this with
realistic data and it's consistently under 100ms. Given our current scale,
I think the simpler implementation is worth it. Happy to add optimization
later if we see it become a bottleneck. What do you think?"

Structuring the Review Process

Before You Start Reviewing

Check the PR Description

A good PR should have:

## What changed
Added user authentication with OAuth2

## Why
Users requested social login to reduce signup friction

## How
- Integrated passport.js for OAuth
- Added Google and GitHub providers
- Updated user schema to support multiple auth methods

## Testing
- Added unit tests for auth service
- Tested login flow manually with both providers
- Verified existing users can link social accounts

## Screenshots
[Include relevant screenshots or recordings]

If the description is missing or unclear, ask the author to clarify before reviewing.

Understand the Context

@author Before I review in detail, can you help me understand:
- What user problem does this solve?
- Why this approach over [alternative]?
- Are there any parts you're uncertain about?

The Review Process

1. High-Level Review (5 minutes)

Scan the changes for:

• Overall architecture
• Breaking changes
• Missing tests
• Security concerns

2. Detailed Review (15-30 minutes)

Go file by file:

• Read the code carefully
• Run it locally if complex
• Check tests are comprehensive
• Look for edge cases

3. Leave Feedback (10 minutes)

Group related comments:

## Architecture
[Comments about overall design]

## Security
[Security-related feedback]

## Testing
[Test coverage feedback]

## Nitpicks
[Optional style suggestions]

Approval Criteria

Approve when:

• ✅ Code solves the stated problem
• ✅ No security vulnerabilities
• ✅ No correctness bugs
• ✅ Has adequate test coverage
• ✅ Documentation is updated if needed

Don’t block on:

• Minor style preferences
• Bike-shedding (arguing about trivial details)
• Perfect optimization
• Theoretical future problems

Review Anti-Patterns

The Rubber Stamp

❌ "LGTM" [without actually reviewing]

This defeats the entire purpose of code review. If you don’t have time for a proper review, say so:

✅ "I won't have time to review this properly until tomorrow.
   Is that timeline okay, or should we find another reviewer?"

The Novel

❌ 47 comments about spacing, formatting, and minor style preferences

Focus on what matters. Let automated tools handle formatting.

The Blocker

❌ "I don't like this architecture. Rewrite the entire thing."
[After the PR is complete]

Architecture discussions should happen before implementation, not during review.

If you catch a major issue:

✅ "I think we need to discuss the architecture here. The tight coupling
   to Redis will make testing difficult. Can we hop on a call to explore
   alternatives before you continue?"

The Ghost

Requesting changes, then disappearing for days.

Timely reviews are critical. If you request changes:

• Check back within 24 hours
• Approve once addressed
• Don’t leave PRs in limbo

Team Practices That Work

1. Set Review SLAs

Establish expectations:

• Small PRs (<100 lines): Review within 4 hours
• Medium PRs (100-500 lines): Review within 24 hours
• Large PRs (500+ lines): Break them up if possible

2. Use PR Templates

Create a template to guide authors:

## Description
[What changed and why]

## Type of Change
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update

## Testing
- [ ] Added unit tests
- [ ] Tested manually
- [ ] Updated integration tests

## Checklist
- [ ] Code follows team style guide
- [ ] Self-reviewed code
- [ ] Commented hard-to-understand areas
- [ ] Updated documentation
- [ ] No new warnings

3. Rotate Reviewers

Don’t always review the same person’s code. Rotate to:

• Spread knowledge across the team
• Prevent knowledge silos
• Expose everyone to different coding styles

4. Review Your Own PRs First

Before requesting review:

1. Read through your own changes
2. Add comments explaining complex logic
3. Check for debugging code or TODOs
4. Verify tests pass

5. Celebrate Good Code

When you see excellent code, call it out:

This is a really clean implementation! The use of dependency injection
makes testing so much easier. I'm going to use this pattern in my next PR.

Positive reinforcement shapes team culture.

AI-Assisted Code Reviews

Tools like GitHub Copilot and Claude can help with reviews:

What AI Is Good At

• Catching common bugs
• Suggesting performance improvements
• Identifying security vulnerabilities
• Generating test cases

What AI Misses

• Business logic correctness
• System-wide architectural concerns
• Team-specific conventions
• User experience implications

Use AI as a first pass, but don’t skip human review.

Example workflow:

1. AI reviews code → Catches 10 issues
2. Human reviews code → Catches 5 architectural issues + validates AI suggestions
3. Author addresses all feedback
4. Human approves

Real-World Example: Good vs Bad Review

Bad Review ❌

Reviewer: "This doesn't follow our coding standards"

[3 days later]

Author: "What standards? Can you be specific?"

[2 days later]

Reviewer: "Use const instead of let on line 47"

[1 day later]

Author: "Fixed"

[3 days later]

Reviewer: "Also fix the naming on line 52"

Problems:

• Vague feedback
• Slow responses
• Drip-feeding comments
• 9 days to merge a simple fix

Good Review ✅

Reviewer (same day):

"Nice work on the caching implementation! A few suggestions:

## Must Fix

1. Line 47: Potential race condition in cache update. Consider using
   atomic operations or a lock. Example: [code snippet]

2. Missing error handling around the Redis call on line 89. If Redis is
   down, this will crash the app.

## Should Fix

3. The function name `doThing` isn't very descriptive. Maybe
   `updateCacheWithLock` to clarify what it does?

4. Could you add a test for the cache miss scenario?

## Nice to Have

5. Consider extracting the retry logic into a helper since we use it in
   3 places.

Overall looks good! Let me know if you have questions on any of these."

[2 hours later]

Author: "Great feedback! I've addressed items 1-4. For #5, I'll create a
separate ticket since it touches other parts of the codebase. Sound good?"

[1 hour later]

Reviewer: "Perfect! Approved. 🚀"

Benefits:

• All feedback at once
• Specific and actionable
• Categorized by priority
• Merged same day

Metrics That Matter

Track these to improve your review process:

Time to Review

Goal: <24 hours for most PRs

Long review times kill momentum and create bottlenecks.

Review Iteration Count

Goal: 1-2 rounds

If you’re consistently going 3+ rounds:

• PRs might be too large
• Standards might be unclear
• Communication might need work

PR Size

Goal: <200 lines for most PRs

Large PRs are harder to review thoroughly. Encourage small, focused changes.

Bugs Found in Review

Goal: Catch critical bugs before production

Track what percentage of production bugs could have been caught in review.

Action Plan

This Week

1. Review your last 5 PRs:

   • Were reviews timely?
   • Was feedback specific and actionable?
   • Did you explain the “why”?

2. Pick one anti-pattern to eliminate:

   • Stop rubber-stamping
   • Reduce response time
   • Give more constructive feedback

This Month

1. Create a PR template for your team
2. Establish review SLAs
3. Set up automated linting to reduce style comments
4. Track time-to-review metrics

This Quarter

1. Run a team retrospective on code reviews
2. Create a team coding standards doc
3. Rotate review assignments systematically
4. Celebrate examples of great reviews

Conclusion: Reviews as Culture

Code reviews are more than quality checks—they’re how teams learn, grow, and build shared standards.

Great code reviews:

• Transfer knowledge
• Prevent bugs
• Improve code quality
• Build team cohesion
• Create learning opportunities

Poor code reviews:

• Slow down development
• Frustrate developers
• Miss critical issues
• Create knowledge silos
• Erode team morale

The difference between the two isn’t technical skill—it’s intentionality, empathy, and clear communication.

Invest in your review culture, and everything else improves.

Resources

• Google’s Code Review Guidelines
• Best Practices for Code Review by SmartBear
• How to Do Code Reviews Like a Human by Michael Lynch

---

Part of the Developer Skills series. Building teams that ship great code.

What’s your biggest code review challenge? How does your team handle reviews? I’d love to hear what’s working (and what’s not) for you.