Code Review Standards

Code Review Standards

Overview

Code Review Standards define how teams review code to catch bugs, share knowledge, maintain quality, and ensure consistency. Good code reviews make teams stronger.

Core Principle: "Code review is about learning, not judging. Review the code, not the person."

1. Code Review Checklist

# Code Review Checklist

## Functionality
- [ ] Code does what it's supposed to do
- [ ] Edge cases are handled
- [ ] Error handling is appropriate
- [ ] No obvious bugs

## Code Quality
- [ ] Code is readable and self-documenting
- [ ] Functions are small and focused
- [ ] No code duplication (DRY)
- [ ] Follows project conventions

## Testing
- [ ] Tests are included
- [ ] Tests cover edge cases
- [ ] Tests are readable
- [ ] All tests pass

## Security
- [ ] No sensitive data exposed
- [ ] Input is validated
- [ ] SQL injection prevented
- [ ] XSS vulnerabilities addressed

## Performance
- [ ] No obvious performance issues
- [ ] Database queries are optimized
- [ ] No N+1 queries
- [ ] Caching used appropriately

## Documentation
- [ ] Complex logic is commented
- [ ] API changes are documented
- [ ] README updated if needed
- [ ] Breaking changes noted

2. Review Process

graph TD
    A[Create PR] --> B[Self-Review]
    B --> C[Request Reviewers]
    C --> D[Reviewers Review]
    D --> E{Approved?}
    E -->|Changes Requested| F[Address Feedback]
    F --> D
    E -->|Approved| G[Merge]
    G --> H[Delete Branch]

3. Pull Request Template

# Pull Request Template

## Description
[Brief description of what this PR does]

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

## Related Issues
Closes #123

## Changes Made
- Added user authentication
- Updated database schema
- Added tests for login flow

## Screenshots (if applicable)
[Add screenshots for UI changes]

## Testing
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Tested locally
- [ ] Tested on staging

## Checklist
- [ ] Code follows style guidelines
- [ ] Self-reviewed my code
- [ ] Commented complex code
- [ ] Updated documentation
- [ ] No new warnings
- [ ] Added tests
- [ ] All tests pass
- [ ] No breaking changes (or documented)

## Deployment Notes
[Any special deployment considerations]

## Reviewer Notes
[Anything reviewers should pay special attention to]

4. Review Comments Guide

Good Comments

// ✅ Specific and actionable
"Consider using a Map instead of an object here for O(1) lookups"

// ✅ Asks questions
"What happens if userId is null here?"

// ✅ Suggests improvements
"This could be simplified using Array.filter()"

// ✅ Praises good code
"Nice! This is much more readable than the old version"

// ✅ Explains reasoning
"We should validate email format here to prevent invalid data in the database"

Bad Comments

// ❌ Vague
"This doesn't look right"

// ❌ Judgmental
"This is terrible code"

// ❌ Not actionable
"Needs improvement"

// ❌ Nitpicky without value
"Add a space here"

// ❌ Demanding without explanation
"Change this immediately"

5. Review Severity Levels

## Comment Severity

### 🔴 Blocking (Must Fix)
- Security vulnerabilities
- Data loss risks
- Breaking changes without migration
- Critical bugs

**Example**: "🔴 This SQL query is vulnerable to injection. Use parameterized queries."

### 🟡 Important (Should Fix)
- Performance issues
- Code quality concerns
- Missing tests
- Poor error handling

**Example**: "🟡 This N+1 query will be slow. Consider using eager loading."

### 🟢 Suggestion (Nice to Have)
- Code style improvements
- Refactoring opportunities
- Alternative approaches

**Example**: "🟢 Consider extracting this into a helper function for reusability."

### 💡 Question (Seeking Clarification)
- Understanding intent
- Asking about edge cases

**Example**: "💡 What happens if the user is already logged in?"

### 🎉 Praise (Positive Feedback)
- Good solutions
- Clever implementations
- Learning moments

**Example**: "🎉 Great use of TypeScript generics here!"

6. Review Size Guidelines

## Optimal PR Size

### Small (< 200 lines) ✅
- Easy to review thoroughly
- Quick feedback cycle
- Lower risk

### Medium (200-500 lines) ⚠️
- Acceptable
- May need multiple review sessions
- Consider breaking up if possible

### Large (> 500 lines) ❌
- Hard to review thoroughly
- Slow feedback
- High risk of missing issues

**Solution**: Break into smaller PRs

Breaking Up Large PRs

## Example: Large Feature → Multiple PRs

**Instead of**: One 2000-line PR with entire feature

**Do**:
1. PR #1: Database schema changes (100 lines)
2. PR #2: Backend API endpoints (300 lines)
3. PR #3: Frontend components (400 lines)
4. PR #4: Integration and tests (200 lines)

Each PR is reviewable and can be merged independently.

7. Review Turnaround Time

## Response Time SLAs

| PR Size | First Response | Final Approval |
|---------|----------------|----------------|
| Small (< 200 lines) | 2 hours | 4 hours |
| Medium (200-500) | 4 hours | 8 hours |
| Large (> 500) | 8 hours | 24 hours |

### Tips for Reviewers
- Set aside dedicated review time daily
- Review small PRs immediately
- Use GitHub notifications
- Block calendar time for reviews

8. Self-Review Checklist

# Before Requesting Review

## Code Quality
- [ ] Removed console.logs and debugger statements
- [ ] Removed commented-out code
- [ ] No TODO comments (or created issues for them)
- [ ] Formatted code (Prettier)
- [ ] Linted code (ESLint)

## Testing
- [ ] Ran all tests locally
- [ ] Added new tests
- [ ] Tested edge cases manually

## Documentation
- [ ] Updated README if needed
- [ ] Added JSDoc comments for public APIs
- [ ] Updated API documentation

## Git
- [ ] Meaningful commit messages
- [ ] Squashed WIP commits
- [ ] No merge commits (rebased)
- [ ] Branch is up to date with main

## PR Description
- [ ] Clear description of changes
- [ ] Screenshots for UI changes
- [ ] Linked related issues
- [ ] Added deployment notes

9. Handling Feedback

For Authors

## Responding to Feedback

### ✅ Good Responses
- "Good catch! Fixed in abc123"
- "I considered that, but chose X because Y. What do you think?"
- "Great suggestion! Implemented in def456"
- "I don't understand. Could you clarify?"

### ❌ Bad Responses
- "This is fine as is"
- "That's not important"
- "We can fix that later" (without creating issue)
- Ignoring comments

### When You Disagree
1. Explain your reasoning
2. Ask for clarification
3. Suggest alternatives
4. Escalate if needed (tech lead)
5. Default to reviewer if no strong opinion

For Reviewers

## Giving Feedback

### Do
- ✅ Be specific and actionable
- ✅ Explain the "why"
- ✅ Suggest alternatives
- ✅ Praise good code
- ✅ Ask questions

### Don't
- ❌ Be vague or judgmental
- ❌ Nitpick without value
- ❌ Demand changes without explanation
- ❌ Ignore the PR
- ❌ Approve without reading

10. Code Review Automation

GitHub Actions

# .github/workflows/pr-checks.yml
name: PR Checks

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
      - run: npm ci
      - run: npm run lint
      
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
      - run: npm ci
      - run: npm test
      
  type-check:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
      - run: npm ci
      - run: npm run type-check
      
  size-check:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Check PR size
        run: |
          FILES_CHANGED=$(git diff --name-only origin/main | wc -l)
          LINES_CHANGED=$(git diff --stat origin/main | tail -1 | awk '{print $4}')
          if [ $LINES_CHANGED -gt 500 ]; then
            echo "⚠️ PR is large ($LINES_CHANGED lines). Consider breaking it up."
          fi

Danger.js (Automated Review Comments)

// dangerfile.js
import { danger, warn, fail, message } from 'danger';

// Warn on large PRs
const bigPRThreshold = 500;
if (danger.github.pr.additions + danger.github.pr.deletions > bigPRThreshold) {
  warn('⚠️ This PR is quite large. Consider breaking it into smaller PRs.');
}

// Require tests
const hasTests = danger.git.modified_files.some(f => f.includes('.test.'));
if (!hasTests) {
  warn('⚠️ No test files were modified. Did you add tests?');
}

// Require description
if (danger.github.pr.body.length < 10) {
  fail('❌ Please add a description to your PR.');
}

// Check for console.log
const jsFiles = danger.git.modified_files.filter(f => f.endsWith('.ts') || f.endsWith('.js'));
for (const file of jsFiles) {
  const content = await danger.github.utils.fileContents(file);
  if (content.includes('console.log')) {
    warn(`⚠️ Found console.log in ${file}. Remove before merging.`);
  }
}

// Praise for good PR
if (danger.github.pr.additions < 200) {
  message('🎉 Nice small PR! Easy to review.');
}

11. Review Metrics

interface ReviewMetrics {
  avgReviewTime: number;  // hours
  avgPRSize: number;  // lines
  approvalRate: number;  // percentage
  commentsPerPR: number;
  bugsFoundInReview: number;
}

// Track effectiveness
function calculateReviewQuality() {
  const bugsFoundInReview = 15;
  const bugsFoundInProduction = 2;
  
  return {
    catchRate: (bugsFoundInReview / (bugsFoundInReview + bugsFoundInProduction)) * 100,
    // Target: > 85%
  };
}

12. Code Review Standards Checklist

• Process Defined: Review process documented?
• PR Template: Template with checklist?
• Review Checklist: What to look for documented?
• Comment Guidelines: How to give feedback?
• Severity Levels: Blocking vs suggestions clear?
• Size Guidelines: PR size limits defined?
• Turnaround SLAs: Response time expectations?
• Automation: Automated checks in place?
• Metrics: Tracking review effectiveness?
• Training: Team trained on review standards?

Related Skills

• 45-developer-experience/lint-format-typecheck
• 45-developer-experience/commit-conventions
• 45-developer-experience/onboarding-docs