Code Review Best Practices

Why Code Review?

Code review is the practice of having other developers examine your code changes before they merge into the main codebase. It's one of the highest-leverage activities in software development.

What code review provides:

1. Catch bugs early -- a second pair of eyes finds what you missed
2. Knowledge sharing -- reviewers learn the codebase, authors learn better patterns
3. Code quality -- consistent style, better architecture, fewer shortcuts
4. Team ownership -- everyone understands the codebase, no single points of failure
5. Security -- catch vulnerabilities before they reach production

Studies consistently show that code review catches 60-90% of defects before testing. It's cheaper to fix a bug in review than in production.

The Code Review Checklist

Every reviewer should mentally walk through these categories when examining a pull request.

1. Functionality

Does the code do what it's supposed to do?

[ ] Does the code match the ticket/requirement?
[ ] Are edge cases handled (empty inputs, nulls, boundaries)?
[ ] Does error handling cover failure scenarios?
[ ] Are there any off-by-one errors?
[ ] Does the code handle concurrent access if needed?

// Reviewer catches missing edge case

// Submitted code:
function getDiscount(items: CartItem[]): number {
  const total = items.reduce((sum, item) => sum + item.price, 0);
  return total > 100 ? total * 0.1 : 0;
}

// Review comment: "What if items is empty? The reduce works
// but should we return 0 discount for an empty cart explicitly?
// Also, what about negative prices from refund items?"

2. Readability

Can another developer understand this code in 6 months?

[ ] Are variable and function names descriptive?
[ ] Is the code self-documenting (clear intent)?
[ ] Are complex algorithms explained with comments?
[ ] Is the code unnecessarily clever?
[ ] Are magic numbers replaced with named constants?

// Before review: hard to understand
function calc(d: number[], f: number): number {
  return d.reduce((a, b) => a + b * f, 0);
}

// After review feedback: clear intent
const TAX_RATE = 0.08;

function calculateTotalWithTax(prices: number[], taxRate: number = TAX_RATE): number {
  return prices.reduce((total, price) => total + price * (1 + taxRate), 0);
}

3. Security

Is the code safe from common attack vectors?

[ ] Is user input validated and sanitized?
[ ] Are SQL queries parameterized (no injection)?
[ ] Are secrets hardcoded? (API keys, passwords)
[ ] Is authentication/authorization checked?
[ ] Is sensitive data exposed in logs or responses?
[ ] Are dependencies from trusted sources?

// Reviewer catches SQL injection vulnerability

// Dangerous:
const query = `SELECT * FROM users WHERE email = '${email}'`;

// Reviewer comment: "SQL injection risk. Use parameterized queries."

// Fixed:
const query = 'SELECT * FROM users WHERE email = $1';
const result = await db.query(query, [email]);

// Reviewer catches leaked sensitive data

// Dangerous:
app.get('/api/user/:id', async (req, res) => {
  const user = await db.findUser(req.params.id);
  res.json(user); // exposes password hash, internal IDs
});

// Reviewer comment: "You're returning the full user object including
// passwordHash. Return only the fields the client needs."

// Fixed:
app.get('/api/user/:id', async (req, res) => {
  const user = await db.findUser(req.params.id);
  res.json({
    id: user.id,
    name: user.name,
    email: user.email,
  });
});

4. Performance

Will this code perform well at scale?

[ ] Are there unnecessary database queries (N+1 problem)?
[ ] Is there any blocking operation on the main thread?
[ ] Are large datasets paginated?
[ ] Is caching used where appropriate?
[ ] Are there memory leaks (event listeners, intervals)?

// Reviewer catches N+1 query problem

// Submitted: 1 query for posts + N queries for authors
const posts = await db.query('SELECT * FROM posts');
for (const post of posts) {
  post.author = await db.query('SELECT * FROM users WHERE id = $1', [post.authorId]);
}

// Reviewer comment: "N+1 query. Use a JOIN or batch fetch."

// Fixed: single query with JOIN
const posts = await db.query(`
  SELECT posts.*, users.name as author_name
  FROM posts
  JOIN users ON posts.author_id = users.id
`);

5. Testing

Is the code adequately tested?

[ ] Are there unit tests for new logic?
[ ] Do tests cover happy path AND error cases?
[ ] Are edge cases tested?
[ ] Are tests readable and maintainable?
[ ] Is test data realistic?

6. Architecture

Does the code fit the system's design?

[ ] Does it follow existing patterns in the codebase?
[ ] Is the responsibility in the right layer?
[ ] Are there circular dependencies?
[ ] Is the change backwards compatible?
[ ] Are there any breaking changes to public APIs?

Providing Constructive Feedback

How you give feedback matters as much as what feedback you give. Poor delivery leads to defensive authors and toxic team dynamics.

The Principles

1. Criticize code, not people -- "This function is hard to follow" not "You wrote confusing code"
2. Ask questions instead of making demands -- "What do you think about extracting this into a helper?" not "Extract this"
3. Explain why, not just what -- "Consider using a Map here for O(1) lookups instead of array.find() which is O(n)" not just "Use a Map"
4. Offer alternatives -- show the better approach, don't just reject the current one
5. Acknowledge good work -- "Nice use of early returns here, really clean" goes a long way

Feedback Examples

Bad Feedback

"This is wrong."
"Why would you do it this way?"
"This is messy."
"No."

Good Feedback

"This works, but consider using a Set for the lookup instead of an
array — it gives O(1) membership checks vs O(n) for .includes().
With 10k items this will matter."

"Nit: this variable name 'data' is generic. Something like
'userProfiles' would make the code self-documenting."

"Question: is there a reason we're not using the existing
validateEmail() utility from src/utils? It handles edge cases
like unicode domains that this regex might miss."

"Optional suggestion: you could simplify this with a reduce()
but the for loop is perfectly fine too — just a style preference."

Categorizing Comments

Use prefixes to clarify the severity of your feedback:

[blocking]  "This SQL query is vulnerable to injection. Must fix before merge."

[suggestion] "Consider extracting this into a custom hook for reusability."

[nit]       "Minor: extra whitespace on line 42."

[question]  "Is this behavior intentional? The old version returned null here."

[praise]    "Great error handling here — love the user-friendly messages."

This helps authors prioritize. Not every comment is a blocker.

Receiving Feedback Gracefully

Being on the receiving end of code review requires emotional maturity. Your code is not you.

Principles for Authors

1. Assume good intent -- reviewers want to help, not attack
2. Don't take it personally -- feedback is about the code, not you
3. Explain your reasoning -- if you disagree, explain why calmly
4. Be open to learning -- reviewers might know patterns you don't
5. Say thank you -- reviewers spend time helping improve your code
6. Don't argue style -- if the team has a convention, follow it

Good Author Responses

"Good catch! Fixed in the latest commit."

"Thanks for the suggestion. I went with approach X because of
[reason], but I can see how Y would work too. Want me to switch?"

"I didn't know about that utility — using it now. Thanks!"

"You're right, I missed that edge case. Added a test for it too."

When You Disagree

"I see your point about using a Map, but this array will always
have <10 items and readability matters more here. The .find()
reads more naturally for this use case. Thoughts?"

Disagree respectfully, explain your reasoning, and be open to compromise.

Automated Checks

Automation handles the boring, objective parts of review so humans can focus on logic, architecture, and design.

Linting and Formatting

// .eslintrc.json
{
  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/recommended",
    "prettier"
  ],
  "rules": {
    "no-console": "warn",
    "no-unused-vars": "error",
    "@typescript-eslint/no-explicit-any": "error",
    "@typescript-eslint/consistent-type-imports": "error"
  }
}

// .prettierrc
{
  "semi": true,
  "singleQuote": true,
  "trailingComma": "all",
  "printWidth": 100,
  "tabWidth": 2
}

# .github/workflows/lint.yml
name: Lint
on: pull_request

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-node@v4
        with:
          node-version: 20
      - run: npm ci
      - run: npm run lint
      - run: npx prettier --check .
      - run: npx tsc --noEmit

Pre-Commit Hooks

Run checks before code even reaches the PR.

// package.json
{
  "scripts": {
    "prepare": "husky"
  },
  "lint-staged": {
    "*.{ts,tsx}": [
      "eslint --fix",
      "prettier --write"
    ],
    "*.{json,md,yml}": [
      "prettier --write"
    ]
  }
}

# .husky/pre-commit
npx lint-staged

Automated PR Checks Diagram

Developer pushes code
        |
        v
+-------------------+
| GitHub PR Created  |
+-------------------+
        |
        v
+-------+-------+-------+-------+
| Lint  | Types | Tests | Scan  |   <-- All automated
+-------+-------+-------+-------+
        |
        v
  All pass? ----No----> Block merge
        |
       Yes
        |
        v
  Ready for human review

Pull Request Templates

A good PR template ensures authors provide context. Without it, reviewers waste time figuring out what changed and why.

PR Template

Create this file at .github/pull_request_template.md:

## What

<!-- Brief description of the changes -->

## Why

<!-- Why is this change needed? Link to ticket/issue -->

## How

<!-- How does this change work? Any architectural decisions? -->

## Testing

<!-- How was this tested? -->
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Manual testing performed

## Screenshots

<!-- If UI changes, add before/after screenshots -->

## Checklist

- [ ] Code follows project style guidelines
- [ ] Self-review completed
- [ ] No console.log or debug statements
- [ ] No hardcoded secrets or credentials
- [ ] Documentation updated (if needed)
- [ ] Breaking changes noted (if any)

Good PR Example

## What

Add rate limiting to the /api/auth/login endpoint.

## Why

We've seen brute-force login attempts in production logs.
Rate limiting prevents automated attacks. Ticket: AUTH-456

## How

Using express-rate-limit middleware. Configured to allow
5 attempts per 15 minutes per IP. Returns 429 with
Retry-After header on limit exceeded.

## Testing

- [x] Unit tests for rate limit middleware
- [x] Integration test for 429 response
- [x] Manual testing with curl

## Checklist

- [x] Code follows project style guidelines
- [x] Self-review completed
- [x] No console.log or debug statements
- [x] No hardcoded secrets or credentials
- [x] Documentation updated (API docs)
- [ ] Breaking changes noted — N/A

Bad PR Example

## What

Fixed stuff

## Why

It was broken

## Testing

Works on my machine

This tells the reviewer nothing. They have to read every line of code without context.

Code Review Culture

Good code review isn't about tools or checklists alone -- it's about team culture.

Healthy Review Culture

1. Everyone reviews, everyone gets reviewed -- seniority doesn't exempt you
2. Review promptly -- don't let PRs sit for days; review within 4-24 hours
3. Keep PRs small -- aim for under 400 lines changed; large PRs get rubber-stamped
4. Two-way learning -- juniors review senior code too (they catch different things)
5. No gatekeeping -- reviews should unblock, not block

PR Size Guidelines

Size	Lines Changed	Review Quality	Recommended
XS	1-50	Excellent	Best
S	51-200	Good	Great
M	201-400	Adequate	Acceptable
L	401-800	Drops off	Split if possible
XL	800+	Rubber stamp	Must split

Research from Google shows that review quality drops dramatically after 200-400 lines. If your PR is over 400 lines, consider splitting it.

Review Turnaround Time

Goal: Review within 4 hours (same business day)

Morning PRs   -> reviewed by lunch
Afternoon PRs -> reviewed by end of day
Friday PRs    -> reviewed Monday morning (don't block weekends)

Pair Programming as an Alternative

For complex changes, pair programming can replace post-hoc code review:

Traditional:  Code alone -> PR -> Wait -> Review -> Fix -> Wait -> Merge
Pairing:      Code together -> PR (rubber stamp) -> Merge

Pairing is better when:
- The change is complex or risky
- The author is new to the codebase
- Two perspectives will produce a better design

Review Workflow

For Reviewers

1. Read the PR description and linked ticket
2. Understand the WHAT and WHY before reading code
3. Check out the branch and run it locally (for complex changes)
4. Review file by file, starting with tests
5. Leave comments with clear severity prefixes
6. Approve, request changes, or comment
7. Follow up on your comments after author responds

For Authors

1. Self-review before requesting review
2. Write a clear PR description (use the template)
3. Keep PRs small and focused (one concern per PR)
4. Add reviewers who know the affected area
5. Respond to all comments (even if just "done")
6. Don't push while review is in progress (unless asked)
7. Resolve conversations as you address feedback

Key Takeaways

• Code review catches 60-90% of defects before testing -- it's your best quality tool
• Use a checklist covering functionality, readability, security, performance, and testing
• Criticize code, not people -- ask questions, explain reasoning, offer alternatives
• Automate the boring stuff: linting, formatting, type checking, security scanning
• Use PR templates to ensure authors provide context
• Keep PRs under 400 lines -- quality drops dramatically after that
• Review within 4-24 hours -- stale PRs create merge conflicts and frustration
• Build a culture where everyone reviews and everyone gets reviewed
• Code review is a teaching opportunity, not a gatekeeping ceremony