Code Review Culture

Code Review Culture

Why Code Review Matters

Code Review Objectives

Correctness

// Does it work?
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

// Review: Check edge cases (empty array, null items)
function calculateTotal(items) {
    if (!items || items.length === 0) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Quality

// Is it well-written?
// Bad: Complex, hard to understand
function d(a){return a.reduce((b,c)=>b+c.p,0)}

// Good: Clear, readable
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

Readability

// Can others understand?
// Bad: Unclear variable names
function x(y){return y.map(z=>z*2)}

// Good: Descriptive names
function doubleNumbers(numbers) {
    return numbers.map(number => number * 2);
}

Design

// Is the approach sound?
// Bad: Tight coupling
function processOrder(order) {
    saveToDatabase(order);
    sendEmail(order);
    updateInventory(order);
    chargeCreditCard(order);
}

// Good: Separation of concerns
function processOrder(order) {
    const orderService = new OrderService();
    orderService.process(order);
}

Tests

// Are there adequate tests?
// Bad: No tests
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

// Good: With tests
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

// Tests
describe('calculateTotal', () => {
    it('calculates total correctly', () => {
        const items = [{ price: 10 }, { price: 20 }];
        expect(calculateTotal(items)).toBe(30);
    });

    it('handles empty array', () => {
        expect(calculateTotal([])).toBe(0);
    });
});

Security

// Are there security vulnerabilities?
// Bad: SQL injection vulnerability
function getUser(id) {
    const query = `SELECT * FROM users WHERE id = ${id}`;
    return db.query(query);
}

// Good: Parameterized query
function getUser(id) {
    const query = 'SELECT * FROM users WHERE id = $1';
    return db.query(query, [id]);
}

Code Review Process

Step-by-Step Process

┌─────────────────────────────────────────────────────────────────┐
│  Code Review Process                                         │
├─────────────────────────────────────────────────────────────────┤
│                                                                  │
│  1. Create PR ──▶ 2. Automated Checks ──▶ 3. Reviewers Assigned ──▶ 4. Reviewers Provide Feedback ──▶ 5. Developer Addresses Feedback ──▶ 6. Approval and Merge │
│                                                                  │
│  └───────────────────────────────────────────────────────────────┘                      │
└─────────────────────────────────────────────────────────────────┘

Step 1: Create PR

# Create pull request
git checkout -b feature/new-feature
git add .
git commit -m "Add new feature"
git push origin feature/new-feature

# Create PR on GitHub

Step 2: Automated Checks

# GitHub Actions workflow
name: CI
on: [pull_request]

jobs:
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Run tests
        run: npm test
      - name: Run linter
        run: npm run lint
      - name: Run type check
        run: npm run type-check

Step 3: Reviewers Assigned

// Assign reviewers automatically
// GitHub: CODEOWNERS file
# Frontend code
*.js @frontend-team
*.jsx @frontend-team

# Backend code
*.py @backend-team
*.go @backend-team

# Database migrations
migrations/ @db-team

Step 4: Reviewers Provide Feedback

// Good review comment
// "I noticed that this function doesn't handle the case where `items` is null.
// Consider adding a null check or using optional chaining."
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

Step 5: Developer Addresses Feedback

// Address feedback
function calculateTotal(items) {
    if (!items) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

// Respond to reviewer
// "Thanks for catching that! I've added a null check."

Step 6: Approval and Merge

# Merge PR after approval
# GitHub: Click "Merge pull request"
# Or use GitHub CLI
gh pr merge --merge

Review Size

Small PRs (< 400 lines)

// Good: Small, focused PR
function calculateTotal(items) {
    if (!items) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Large PRs (> 1000 lines)

// Bad: Large, hard to review
// Too many changes in one PR
// Break into smaller PRs

Break Up Large PRs

# Break large PR into smaller ones
# PR 1: Add calculateTotal function
# PR 2: Add tests for calculateTotal
# PR 3: Update UI to use calculateTotal

Review Turnaround Time

Goal: Review Within 24 Hours

// Good: Fast feedback
// Review within 24 hours
// Keeps momentum

Blocked PRs Slow Down Team

// Bad: Long delays
// PR sits for days
// Blocks other work

Giving Feedback

Be Kind and Constructive

// Good: Constructive feedback
// "I think we could simplify this by using array methods.
// What do you think about using reduce() instead of a for loop?"

// Bad: Harsh feedback
// "This is terrible code. Rewrite it."

Explain the "Why"

// Good: Explains why
// "I noticed you're using `var` instead of `const`.
// Using `const` is better because it prevents reassignment
// and makes the code more predictable."

// Bad: No explanation
// "Use const instead of var."

Suggest Alternatives

// Good: Suggests alternatives
// "Instead of using a for loop, we could use map()
// which is more functional and easier to read."

// Example:
// Before:
const doubled = [];
for (let i = 0; i < numbers.length; i++) {
    doubled.push(numbers[i] * 2);
}

// After:
const doubled = numbers.map(n => n * 2);

Ask Questions Instead of Demanding

// Good: Asks questions
// "Have you considered using TypeScript here?
// It might help catch type errors at compile time."

// Bad: Demands
// "You must use TypeScript here."

Praise Good Work

// Good: Praises good work
// "Great job on the tests! They're comprehensive
// and easy to understand."

Review Comment Types

Blocking Comments

// Must fix before merge
// "This function has a security vulnerability.
// Please use parameterized queries instead of string interpolation."
function getUser(id) {
    const query = `SELECT * FROM users WHERE id = ${id}`;
    return db.query(query);
}

Non-Blocking Comments

// Nice-to-have, not required
// "Consider adding a comment explaining why we're using
// this particular algorithm."
function calculateTotal(items) {
    return items.reduce((sum, item) => sum + item.price, 0);
}

Question Comments

// Asking for clarification
// "I'm not sure why we're using this approach.
// Could you explain the reasoning?"
function processOrder(order) {
    // Complex logic
}

Praise Comments

// Recognizing good work
// "Great job on the error handling!
// It's comprehensive and user-friendly."
function processOrder(order) {
    try {
        // Process order
    } catch (error) {
        // Handle error gracefully
    }
}

Receiving Feedback

Don't Take It Personally

// It's about the code, not you
// Feedback helps improve code quality

Ask for Clarification

// If unclear, ask for clarification
// "Could you explain why you think we should use
// this approach? I'm not sure I understand."

Discuss Disagreements Respectfully

// Good: Respectful discussion
// "I see your point, but I think this approach is
// better because... What do you think?"

// Bad: Argumentative
// "You're wrong. This is the right way."

Learn from Feedback

// Use feedback to improve
// "Thanks for the feedback! I'll keep that in mind
// for future PRs."

Review Checklist

Code Works as Intended

// [ ] Code works as intended
function calculateTotal(items) {
    if (!items) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Tests Added/Updated

// [ ] Tests added/updated
describe('calculateTotal', () => {
    it('calculates total correctly', () => {
        const items = [{ price: 10 }, { price: 20 }];
        expect(calculateTotal(items)).toBe(30);
    });
});

Edge Cases Handled

// [ ] Edge cases handled
function calculateTotal(items) {
    if (!items || items.length === 0) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Error Handling Present

// [ ] Error handling present
function processOrder(order) {
    try {
        // Process order
    } catch (error) {
        // Handle error
        console.error('Error processing order:', error);
        throw new Error('Failed to process order');
    }
}

No Obvious Bugs

// [ ] No obvious bugs
function calculateTotal(items) {
    if (!items || items.length === 0) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Readable and Maintainable

// [ ] Readable and maintainable
function calculateTotal(items) {
    if (!items || items.length === 0) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Follows Style Guide

// [ ] Follows style guide
// ESLint rules
// Prettier formatting

No Security Issues

// [ ] No security issues
function getUser(id) {
    const query = 'SELECT * FROM users WHERE id = $1';
    return db.query(query, [id]);
}

Documentation Updated

// [ ] Documentation updated
/**
 * Calculate the total price of items.
 * @param {Array} items - Array of items with price property
 * @returns {number} Total price
 */
function calculateTotal(items) {
    if (!items || items.length === 0) return 0;
    return items.reduce((sum, item) => sum + item.price, 0);
}

Automated Checks

Linting

// package.json
{
  "scripts": {
    "lint": "eslint src/",
    "lint:fix": "eslint src/ --fix"
  }
}

// .eslintrc.js
module.exports = {
    extends: ['eslint:recommended'],
    rules: {
        'no-console': 'warn',
        'no-unused-vars': 'error'
    }
};

Formatting

// package.json
{
  "scripts": {
    "format": "prettier --write src/"
  }
}

// .prettierrc
{
  "semi": true,
  "singleQuote": true,
  "tabWidth": 2
}

Tests

// package.json
{
  "scripts": {
    "test": "jest",
    "test:watch": "jest --watch",
    "test:coverage": "jest --coverage"
  }
}

Type Checking

// package.json
{
  "scripts": {
    "type-check": "tsc --noEmit"
  }
}

Security Scanning

// package.json
{
  "scripts": {
    "security": "npm audit",
    "security:fix": "npm audit fix"
  }
}

Review Tools

GitHub Pull Requests

# Create PR
gh pr create --title "Add new feature" --body "Description"

# View PR
gh pr view

# Merge PR
gh pr merge --merge

GitLab Merge Requests

# Create MR
git push -o merge_request.create

# View MR
gitlab mr show

# Merge MR
gitlab mr merge

Bitbucket Pull Requests

# Create PR
bitbucket pr create --title "Add new feature" --description "Description"

# View PR
bitbucket pr view

# Merge PR
bitbucket pr merge

Review Best Practices

Review Promptly

// Don't block teammates
// Review within 24 hours

Focus on Important Issues

// Don't nitpick
// Focus on important issues

Automate What Can Be Automated

// Use linters, formatters, tests
// Don't manually check style

Set Time Limits

// Limit review time
// 30 minutes per review

Synchronous Discussion for Complex Topics

// Discuss complex topics synchronously
// Video call, pair programming

Common Review Mistakes

Rubber-Stamping

// Bad: Approving without reading
// Don't just click "Approve"
// Actually review the code

Nitpicking

// Bad: Focusing on trivial issues
// Don't nitpick style issues
// Let linters handle those

Not Explaining Feedback

// Bad: Not explaining feedback
// Always explain why
// Help the author understand

Long Delays

// Bad: Long delays
// Review promptly
// Don't block teammates

Self-Review

Review Your Own Code

// Before submitting PR
// Review your own code
// Catch obvious issues

Self-Review Checklist

// Self-review checklist
// [ ] Code works as intended
// [ ] Tests added
// [ ] Edge cases handled
// [ ] Error handling present
// [ ] No obvious bugs
// [ ] Readable and maintainable
// [ ] Follows style guide
// [ ] No security issues
// [ ] Documentation updated

Pair Programming as Review

Real-Time Review

// Pair programming
// Real-time review during coding
// Faster feedback

Great for Complex or Risky Changes

// Use pair programming for
// Complex features
// Risky changes
// Critical bugs

Review Metrics

Review Turnaround Time

// Measure review turnaround time
// Goal: < 24 hours

Review Thoroughness

// Measure review thoroughness
// Comments per PR
// Blocking comments
// Non-blocking comments

Defect Escape Rate

// Measure defects in production
// Bugs found after merge
// Goal: < 5% of PRs

Building Review Culture

Everyone Reviews

// Not just seniors
// Everyone reviews
// Juniors, seniors, leads

Code Review Guidelines Document

# Code Review Guidelines

## Review Process
1. Create PR
2. Automated checks pass
3. Assign reviewers
4. Reviewers provide feedback
5. Developer addresses feedback
6. Approval and merge

## Review Checklist
- [ ] Code works as intended
- [ ] Tests added/updated
- [ ] Edge cases handled
- [ ] Error handling present
- [ ] No obvious bugs
- [ ] Readable and maintainable
- [ ] Follows style guide
- [ ] No security issues
- [ ] Documentation updated

## Feedback Guidelines
- Be kind and constructive
- Explain the "why"
- Suggest alternatives
- Ask questions instead of demanding
- Praise good work

Regular Feedback on Review Quality

// Give feedback on reviews
// Help reviewers improve

Celebrate Good Reviews

// Celebrate good reviews
// Recognize thorough reviews

Remote Team Reviews

Async-First

// Written feedback
// Not real-time

Video Call for Complex Discussions

// Use video call for
// Complex topics
// Disagreements

Timezone-Aware

// Don't block overnight
// Be considerate of timezones

Real Examples

Good Review Comments

// Good: Constructive feedback
// "I noticed that this function doesn't handle the case where `items` is null.
// Consider adding a null check or using optional chaining."

// Good: Explains why
// "I noticed you're using `var` instead of `const`.
// Using `const` is better because it prevents reassignment
// and makes the code more predictable."

// Good: Suggests alternatives
// "Instead of using a for loop, we could use map()
// which is more functional and easier to read."

// Good: Praises good work
// "Great job on the tests! They're comprehensive
// and easy to understand."

Bad Review Comments

// Bad: Harsh feedback
// "This is terrible code. Rewrite it."

// Bad: No explanation
// "Use const instead of var."

// Bad: Demands
// "You must use TypeScript here."

// Bad: Argumentative
// "You're wrong. This is the right way."

Summary Checklist

Before Reviewing

• Understand the context
• Read the PR description
• Review the code
• Run the code locally
• Check the tests

During Review

• Focus on important issues
• Be kind and constructive
• Explain the "why"
• Suggest alternatives
• Ask questions instead of demanding

After Review

• Respond to feedback
• Make requested changes
• Update documentation
• Re-request review
• Merge when approved