Systematic Code Review Process

You’ve established that code gets reviewed before merge. Now you need a process that actually catches issues without slowing everything down. This means understanding what to look for, how to give feedback people will actually use, and when to automate parts of the process.

The Review Culture Problem

Most teams approach code review as a technical checklist: does the code work, are there tests, does it follow conventions. That’s necessary but not sufficient.

Code review is fundamentally a social practice. You’re asking someone to critique work you spent hours on. They’re trying to be helpful without being a jerk. Everyone’s busy. The author wants to ship. The reviewer has their own PRs to write.

The technical mechanics are straightforward. The human dynamics are where reviews fall apart.

A good review process makes it easy to do the right thing:

Small PRs are easier to create than large ones
Giving good feedback is clearer than giving bad feedback
Review happens quickly enough that it doesn’t feel like a blocker
Everyone knows what standards apply

Pull Request Best Practices

The quality of a code review is determined before anyone reviews it. Good PRs get good reviews. Bad PRs get rubber-stamped or ignored.

Size Guidelines

The research is pretty clear: review effectiveness drops sharply after 200-400 lines of code. Your brain can hold only so much context at once.

Aim for 200-400 lines of actual code changes. Not including:

Generated code (migrations, compiled assets, lockfiles)
Moved code that’s unchanged
Formatting changes from automated tools

If you’re over 400 lines, ask yourself:

Can this be split into multiple PRs that each stand alone?
Are there preparatory refactoring steps I could separate out?
Am I bundling multiple features together?

Real example of splitting a large PR:

Original PR (1,247 lines):

Add user authentication system

Split into:

PR #431 (147 lines): Add database schema for user accounts
PR #432 (203 lines): Implement password hashing service
PR #433 (178 lines): Add login/logout endpoints
PR #434 (234 lines): Add session management
PR #435 (189 lines): Add authentication middleware
PR #436 (142 lines): Add signup flow UI
PR #437 (154 lines): Add login flow UI

Each PR can be reviewed independently. Each does one thing. Each can be tested in isolation. Total review time is lower because context is clearer.

Writing Good PR Descriptions

Your PR description is the reviewer’s entry point. A good description answers three questions:

What changed?

## Changes
- Added email validation to signup form
- Updated User model to store email verification status
- Added email verification endpoint

Why did it change?

## Context
We're seeing spam accounts created with fake emails. Users requested
email verification to prevent this. Addresses issue #1234.

How should the reviewer test it?

## Testing
1. Start the app locally
2. Create a new account at /signup
3. Check your email for verification link
4. Click link and verify you're redirected to dashboard
5. Try creating another account with the same email (should fail)

Note: You'll need to set EMAIL_API_KEY in .env.local

This takes 2 minutes to write and saves the reviewer 20 minutes of figuring out what you did.

Compare to:

Added email verification

That tells the reviewer nothing. They’ll have to read every line of code to understand what’s happening.

Self-Review Before Requesting Review

Before you click “Create Pull Request,” review your own code:

Read the diff like you’re the reviewer
- GitHub/GitLab both let you view the PR before creating it
- You’ll find obvious issues you missed
Check for debugging code
- console.log() statements
- Commented-out code
- Temporary API keys
- Debug flags
Look for scope creep
- Did you fix a typo in an unrelated file?
- Did you refactor something that wasn’t part of this PR?
- Either remove it or call it out in the description
Add comments on tricky parts
- If something looks weird, explain why
- “This looks inefficient, but we need it to maintain sort order”
- “Skipping validation here because it’s already validated upstream”

Self-review catches about 30% of issues before anyone else sees them. That’s 30% fewer back-and-forth cycles.

When to Split PRs

Split when:

Multiple independent features: Adding a search bar and fixing the login bug? Two PRs.
Preparatory refactoring: Refactor the service layer first, add the new feature second.
Risk levels differ: Low-risk config change and high-risk database migration? Separate them.
Different reviewers needed: Frontend and backend changes can be reviewed by different people.

Don’t split when:

Changes are tightly coupled: The new API endpoint and the UI that calls it probably belong together.
Testing requires both parts: If you can’t meaningfully test one without the other, keep them together.

When in doubt, ask: “Could I safely deploy one without the other?” If yes, split them.

What to Look For

Here’s what you’re actually checking when you review code. Not every review needs to check everything—context matters—but this is your comprehensive checklist.

1. Correctness

Does the code do what it claims to do?

Look for:

Logic errors: Off-by-one errors, incorrect conditionals, wrong operators

// Bug: <= should be <
for (let i = 0; i <= array.length; i++) {
  // This will access array[array.length] which is undefined
}

Edge cases: What happens with empty input, null values, boundary conditions?

def calculate_average(numbers):
    return sum(numbers) / len(numbers)
    # Bug: crashes on empty list
    # Fix: Check if len(numbers) > 0 first

Error handling: Are errors caught and handled appropriately?

async function fetchUserData(userId) {
  const response = await fetch(`/api/users/${userId}`);
  return response.json();
  // Bug: No error handling if request fails
  // Fix: Check response.ok, wrap in try/catch
}

Race conditions: Can concurrent requests cause problems?

let counter = 0;

async function incrementCounter() {
  const current = counter;
  await someAsyncOperation();
  counter = current + 1;
  // Bug: Two concurrent calls can both read counter=0,
  // then both set it to 1 instead of 2
}

2. Design and Architecture

Does this fit with how the system should work?

Look for:

SOLID violations:

Single Responsibility: Does this class do too many things?
Open/Closed: Is this fragile when extended?
Dependency Inversion: Is this tightly coupled to implementations?

class UserService:
    def create_user(self, email, password):
        # Validates email
        # Hashes password
        # Saves to database
        # Sends welcome email
        # Updates analytics
        # Logs to audit system

    # This class has too many responsibilities
    # Split into: UserValidator, PasswordHasher, UserRepository,
    # EmailService, AnalyticsService, AuditLogger

Tight coupling: Does changing one part require changing many others?

// Tightly coupled to specific database
function getUser(id) {
  return PostgreSQL.query('SELECT * FROM users WHERE id = $1', [id]);
}

// Better: Inject dependency
function getUser(id, db) {
  return db.findUserById(id);
}

Duplicated logic: Is this reimplementing something that already exists?

// In file1.js
function formatDate(date) {
  const month = date.getMonth() + 1;
  const day = date.getDate();
  return `${month}/${day}/${date.getFullYear()}`;
}

// In file2.js
function formatDate(date) {
  // Exact same implementation
}

// Comment: "We already have formatDate in utils/date.js"

3. Complexity

Is this as simple as it could be?

Look for:

Nested conditionals: Can this be flattened?

// Hard to follow
if (user) {
  if (user.isActive) {
    if (user.hasPermission('edit')) {
      if (!document.isLocked) {
        return true;
      }
    }
  }
}
return false;

// Easier to follow
if (!user || !user.isActive) return false;
if (!user.hasPermission('edit')) return false;
if (document.isLocked) return false;
return true;

Long functions: Can this be broken into smaller pieces?

def process_order(order):
    # 200 lines of code doing validation, calculation,
    # database updates, email sending, analytics, etc.

# Better: Extract each concern into its own function
def process_order(order):
    validate_order(order)
    total = calculate_order_total(order)
    save_order_to_database(order, total)
    send_confirmation_email(order)
    track_order_analytics(order)

Clever code: Is this showing off or actually solving a problem?

// Clever but unclear
const result = arr.reduce((a,b)=>a+b,0)/arr.length||0;

// Clear
const sum = arr.reduce((total, num) => total + num, 0);
const average = arr.length > 0 ? sum / arr.length : 0;

4. Tests

Is the code actually verified to work?

Look for:

Test coverage: Are the important paths tested?
- Happy path (everything works)
- Error conditions
- Edge cases

Meaningful assertions: Do tests actually verify something?

// Weak test
test('creates user', async () => {
  await createUser('test@example.com', 'password123');
  // No assertions - this just checks it doesn't crash
});

// Strong test
test('creates user with hashed password', async () => {
  const user = await createUser('test@example.com', 'password123');
  expect(user.email).toBe('test@example.com');
  expect(user.password).not.toBe('password123'); // Verifies hashing
  expect(user.password.startsWith('$2b$')).toBe(true); // Verifies bcrypt
});

Test quality: Are tests testing the right thing?

# Testing implementation detail
def test_user_repository_uses_sqlalchemy():
    repo = UserRepository()
    assert isinstance(repo.db, SQLAlchemy)

# Testing behavior (better)
def test_user_repository_finds_user_by_email():
    repo = UserRepository()
    user = repo.find_by_email('test@example.com')
    assert user.email == 'test@example.com'

5. Security

Could this be exploited?

This is a specialized review skill. Start with common vulnerabilities:

SQL injection: Are database queries parameterized?

# Vulnerable
query = f"SELECT * FROM users WHERE email = '{email}'"
db.execute(query)

# Safe
query = "SELECT * FROM users WHERE email = %s"
db.execute(query, (email,))

XSS (Cross-Site Scripting): Is user input sanitized before display?

// Vulnerable
element.innerHTML = userInput;

// Safe
element.textContent = userInput;
// Or use a sanitization library if you need HTML

Authentication/Authorization: Are endpoints protected?

// Vulnerable
app.delete('/api/users/:id', async (req, res) => {
  await deleteUser(req.params.id);
  res.sendStatus(200);
});

// Protected
app.delete('/api/users/:id', requireAuth, requireAdmin, async (req, res) => {
  await deleteUser(req.params.id);
  res.sendStatus(200);
});

Secrets in code: Are there hardcoded credentials?

// Bad
const API_KEY = 'sk_live_abc123xyz789';

// Good
const API_KEY = process.env.API_KEY;

Sensitive data exposure: Is PII being logged or exposed?

// Bad
logger.info('User login:', { email, password, ssn });

// Good
logger.info('User login:', { userId: user.id });

See the OWASP Top 10 for a comprehensive security checklist.

6. Performance

Will this scale or cause problems under load?

Look for:

N+1 query problems:

# Bad: Makes N+1 database queries
users = User.all()
for user in users:
    print(user.posts.count())  # Queries database every iteration

# Good: Makes 1 query
users = User.all().prefetch_related('posts')
for user in users:
    print(user.posts.count())

Inefficient algorithms:

// O(n²) - slow for large arrays
function removeDuplicates(arr) {
  return arr.filter((item, index) => arr.indexOf(item) === index);
}

// O(n) - much faster
function removeDuplicates(arr) {
  return [...new Set(arr)];
}

Memory leaks:

// Memory leak: event listener never removed
function setupButton() {
  const button = document.querySelector('#myButton');
  button.addEventListener('click', handleClick);
}

// Fixed: cleanup when done
function setupButton() {
  const button = document.querySelector('#myButton');
  button.addEventListener('click', handleClick);
  return () => button.removeEventListener('click', handleClick);
}

Performance review requires judgment. Is this code on the critical path? Is it called once per request or a million times? Don’t optimize code that runs once a day.

7. Documentation

Will future developers understand this?

Look for:

Code comments where needed: Complex logic, non-obvious decisions, gotchas

// Good comment: explains WHY
// We use setTimeout instead of setInterval because the API rate limit
// is calculated from request start time, not completion time
setTimeout(pollAPI, 1000);

// Bad comment: explains WHAT (code already says that)
// Set timeout to 1000
setTimeout(pollAPI, 1000);

API documentation: Public functions, parameters, return values

def calculate_shipping_cost(weight, destination, speed='standard'):
    """
    Calculate shipping cost based on package weight and destination.

    Args:
        weight: Package weight in pounds (float)
        destination: ZIP code string
        speed: 'standard', 'express', or 'overnight' (default: 'standard')

    Returns:
        Cost in USD (float)

    Raises:
        ValueError: If weight is negative or destination is invalid
    """

README updates: If this changes how to run/deploy/configure the app

8. Style and Conventions

Does this match the codebase?

This is the least important category. Automate it if possible with linters and formatters.

Look for:

Naming conventions (camelCase vs snake_case, consistent with codebase)
File organization (does this go in the right directory?)
Import ordering
Existing patterns (if we handle errors one way everywhere else, keep it consistent)

If you have to choose between “correct” and “consistent with codebase,” choose consistent. You can refactor the whole codebase later.

Giving Effective Feedback

How you phrase feedback determines whether it gets acted on or ignored.

Be Specific and Constructive

Bad:

“This is confusing.”

Good:

“I’m having trouble following the flow in the processPayment function. Could we extract the validation logic into a separate function or add comments explaining the steps?”

Bad:

“Why did you use a for loop here?”

Good:

“This for loop works, but we could use map() here to make the transformation clearer. It also avoids the intermediate results array.”

Explain the Why

Don’t just tell people what to change—help them understand why.

Okay:

“Use const instead of let here.”

Better:

“Use const instead of let here—since total is never reassigned, const makes that intention clear and prevents accidental modification.”

Okay:

“Add error handling here.”

Better:

“Add error handling here—if this API call fails, the user will see a blank screen instead of a helpful error message. We should catch the error and show a message like ‘Unable to load data. Please try again.’”

Distinguish Must-Fix from Suggestions

Use clear prefixes or labels:

Blocking (must fix before merge):
- “Bug: This will crash if the array is empty”
- “Security: This is vulnerable to SQL injection”
- “Correctness: This returns the wrong result when X”
Non-blocking (suggestions):
- “Nit: Typo in comment”
- “Suggestion: Could simplify this with Array.reduce()”
- “Future: We should probably refactor this whole module eventually”

Some teams use GitHub review features:

“Request changes” for blocking issues
“Comment” for non-blocking feedback
“Approve” when ready to merge

Use Question Format

Questions feel less accusatory than commands.

Sounds harsh:

“You should use async/await instead of promises here.”

Sounds collaborative:

“Would async/await make this easier to read than promise chains? I find it clearer, but either works.”

Sounds harsh:

“This violates single responsibility principle.”

Sounds collaborative:

“This function seems to be doing validation, database access, and email sending. Would it be cleaner to split those concerns? What do you think?”

Praise Good Solutions

Don’t only comment on problems.

“Nice! I didn’t know you could use destructuring here. That’s much cleaner than the old approach.”

“This is a clever way to handle the edge case. I was worried about null values but you handled it well.”

“This refactoring makes the code much easier to test. Good call.”

Positive feedback:

Reinforces good practices
Makes critical feedback easier to accept
Teaches by highlighting good examples

Examples of Good Review Comments

**Security concern:** Lines 47-52

This endpoint doesn't check if the user is authenticated. An attacker could
call this directly and access any user's data by changing the ID in the URL.

Suggested fix:
- Add `requireAuth` middleware to this route
- Check that `req.user.id === req.params.userId` before returning data

Let me know if you need help with the middleware setup.

**Suggestion:** Line 89

This works, but I think we can simplify it:

Current:
```javascript
const validUsers = users.filter(u => u.isActive);
const userEmails = [];
for (let i = 0; i < validUsers.length; i++) {
  userEmails.push(validUsers[i].email);
}

Suggestion:

const userEmails = users
  .filter(u => u.isActive)
  .map(u => u.email);

Easier to read and less code. What do you think?


```markdown
**Question:** Lines 134-156

I'm trying to understand the sorting logic here. It looks like we're
sorting by date, then by priority, then by name—but the comments say
we should sort by priority first.

Is the comment out of date, or is there a bug in the sort order?

Receiving Feedback

You’re not just giving feedback—you’re receiving it too. How you respond determines whether you learn from review or just get frustrated.

Don’t Take It Personally

Your code is not you. Criticism of your code is not criticism of you as a person or developer.

When someone points out a bug, they’re doing their job. They’re preventing that bug from reaching production. Be grateful, not defensive.

If you feel defensive:

Step away for 10 minutes
Re-read the comment
Ask yourself: “Is this technically correct?”
Respond to the technical content, not the feeling

Ask Clarifying Questions

If you don’t understand feedback, ask.

“I’m not sure I follow—could you explain what you mean by ‘tight coupling’ here?”

“Can you point me to an example of the pattern you’re suggesting? I want to make sure I understand it correctly.”

“I see the issue, but I’m not sure how to fix it without breaking the existing tests. Any suggestions?”

Push Back When Appropriate

Not all review feedback is right. If you disagree, explain your reasoning.

Bad pushback:

“This is fine.”

Good pushback:

“I considered using a Set here, but we need to maintain insertion order and also allow duplicate IDs in some cases. The array approach handles both requirements. Should I add a comment explaining this?”

Bad pushback:

“The previous developer did it this way.”

Good pushback:

“I’m following the pattern established in user_service.py and order_service.py. If we want to change this pattern, we should probably refactor those files too. Want to open a separate ticket for that?”

If you and the reviewer can’t agree, escalate:

Get a third opinion from another team member
Discuss in team meeting
Defer to team lead or architect

Don’t merge unresolved disagreements without discussion.

Thank Reviewers

Review takes time and thought. Acknowledge it.

“Good catch! Fixed in latest commit.”

“I didn’t think about that edge case. Added a test for it.”

“Thanks for the thorough review. This is much cleaner now.”

It’s small, but it matters. People are more likely to give good reviews when they feel appreciated.

Automated Checks

Don’t waste human review time on things computers can check.

Linting

What it catches: Code style, common mistakes, unused variables

Tools:

JavaScript/TypeScript: ESLint, Prettier
Python: Pylint, Black, Flake8
Ruby: RuboCop
Go: golint, gofmt
Java: Checkstyle, SpotBugs

Configure these to run:

In your editor as you type
As a pre-commit hook
In CI before review

If your linter complains, fix it before requesting review.

Formatting

What it catches: Whitespace, indentation, line length

Tools:

Prettier (JavaScript/TypeScript)
Black (Python)
gofmt (Go)
rustfmt (Rust)

Configure these to run automatically on save or in a pre-commit hook. No human should ever comment on formatting in review.

Tests

What they catch: Broken functionality, regressions

Configure CI to:

Run all tests on every PR
Block merge if tests fail
Show coverage reports

If tests are failing, fix them before requesting review (unless you need help debugging).

Security Scans

What they catch: Known vulnerabilities, hardcoded secrets

Tools:

npm audit / yarn audit (JavaScript dependencies)
Snyk (multi-language dependency scanning)
git-secrets (hardcoded credentials)
Dependabot (automated dependency updates)

Run these in CI. Treat security failures as blocking.

Type Checking

What it catches: Type errors, null reference bugs

If your language has optional typing (TypeScript, Python type hints), enforce it:

TypeScript: tsc --noEmit in CI
Python: mypy in CI

What Humans Should Review

After automation handles the mechanical stuff, humans focus on:

Logic and correctness
Design and architecture
Security beyond what scanners catch
Whether the solution actually solves the problem
Clarity and maintainability

Common Pitfalls

Nitpicking

Problem: Reviewer leaves 20 comments about variable names and spacing, ignores the logic bug.

Solution:

Automate style checking
Focus comments on things that matter
If you must nitpick, clearly mark it: “Nit: typo in comment”

Slow Reviews

Problem: PRs sit for days or weeks without review.

Solution:

Set a team SLA (e.g., “all PRs reviewed within 24 hours”)
Rotate review responsibility
Limit PRs in progress per person
Make review part of daily routine (review first thing in morning)

Inconsistent Standards

Problem: Different reviewers enforce different rules.

Solution:

Document team coding standards
Codify standards in linters where possible
Discuss disagreements in team meetings, then document the decision
Senior developers should model consistency

Review Theater

Problem: Everyone knows reviews are cursory, but we pretend they’re thorough.

Solution:

Track metrics (see deep-water content)
Actually test code before approving
Call out rubber-stamp reviews when you see them
Make PRs small enough to actually review

Gatekeeping

Problem: Reviewer blocks reasonable changes over personal preferences.

Solution:

Distinguish between “wrong” and “different from how I’d do it”
Follow team standards, not personal standards
If you disagree with team standards, bring it up separately—don’t block PRs over it

Review Workflow Template

Here’s a practical workflow that works for most teams:

Author Workflow

Before creating PR:
- Self-review the diff
- Run linters and tests locally
- Write clear PR description
- Add any necessary comments to tricky code
Create PR:
- Keep it under 400 lines if possible
- Tag relevant reviewers
- Add labels (bug fix, feature, refactor, etc.)
- Link related issues
During review:
- Respond to comments within 24 hours
- Mark conversations as resolved when addressed
- Ask questions if feedback is unclear
- Push new commits to address feedback
Before merge:
- Verify all conversations are resolved
- Check that CI passes
- Squash/rebase if team policy requires it

Reviewer Workflow

When assigned:
- Review within 24 hours (or communicate delay)
- Read PR description first
- Understand what problem this solves
Review process:
- Start with big picture: Does the approach make sense?
- Then check specifics: Implementation details
- Actually run the code if possible
- Check tests cover important cases
- Look for security issues
Leaving feedback:
- Be specific and constructive
- Explain why, not just what
- Mark blocking vs. non-blocking issues
- Praise good solutions
Approving:
- Only approve if you’d be comfortable deploying this
- If you have minor suggestions, you can approve with comments
- If you have blocking concerns, request changes

Team Norms to Establish

Review SLA: How quickly should PRs be reviewed?
Minimum reviewers: How many approvals required? (Usually 1-2)
Who can merge: Author or reviewer?
Approval type: “LGTM” in comments or formal approval?
Squash vs. merge commits: What’s your Git history policy?
Review rotation: How are reviewers assigned?
Emergency process: What happens when production is down?

Document these in your team README.

What’s Next

This gives you a systematic approach to code review: what to check, how to give feedback, how to set up automation and process.

For building review culture at scale, handling metrics, specialized reviews (security, performance, accessibility), and advanced techniques, see the deep-water content.

Systematic Code Review Process

The Review Culture Problem

Pull Request Best Practices

Size Guidelines

Writing Good PR Descriptions

Self-Review Before Requesting Review

When to Split PRs

What to Look For

1. Correctness

2. Design and Architecture

3. Complexity

4. Tests

5. Security

6. Performance

7. Documentation

8. Style and Conventions

Giving Effective Feedback

Be Specific and Constructive

Explain the Why

Distinguish Must-Fix from Suggestions

Use Question Format

Praise Good Solutions

Examples of Good Review Comments

Receiving Feedback

Don’t Take It Personally

Ask Clarifying Questions

Push Back When Appropriate

Thank Reviewers

Automated Checks

Linting

Formatting

Tests

Security Scans

Type Checking

What Humans Should Review

Common Pitfalls

Nitpicking

Slow Reviews

Inconsistent Standards

Review Theater

Gatekeeping

Review Workflow Template

Author Workflow

Reviewer Workflow

Team Norms to Establish

What’s Next

Want to Go Deeper?

Related Topics