Code Review Best Practices Guide

# Code Review Checklist

## Correctness
- [ ] Does the code do what it's supposed to do?
- [ ] Are edge cases handled (empty states, errors, timeouts)?
- [ ] Are there sufficient tests for the change?

## Design
- [ ] Does the code follow the project's architecture?
- [ ] Are functions and classes appropriately sized?
- [ ] Is the API intuitive and consistent?

## Maintainability
- [ ] Is the code readable without comments?
- [ ] Are variable and function names descriptive?
- [ ] Is there unnecessary complexity?

## Security
- [ ] Are user inputs validated and sanitized?
- [ ] Are secrets and credentials never hardcoded?
- [ ] Are authentication/authorization checks in place?

## Performance
- [ ] Are there unnecessary database queries or API calls?
- [ ] Are large data sets handled efficiently?
- [ ] Are there obvious memory leaks?

# Bad: many unrelated changes in one commit
git log --oneline
# 7f3e2a1 "Fix bugs and update docs and refactor auth"

# Good: each commit has a single concern
git log --oneline
# a1b2c3d "Refactor auth middleware to use JWT"
# d4e5f6g "Update README with setup instructions"
# g7h8i9j "Fix: handle null callback in login handler"

# Good PR titles (clear what and why)
feat(auth): add OAuth2 login with Google and GitHub
fix(dashboard): correct chart rendering on Safari
refactor(scanner): extract file analysis into separate module
docs: update API reference for v3.2 endpoints

# Bad PR titles (vague)
"Fix stuff"
"Update code"
"Changes"
"WIP"

// ❌ Dismissive comment
// "This is wrong. Rewrite it."

// ✅ Constructive, specific comment
// "This function mutates the input array, which could cause
// unexpected side effects for callers. Consider returning
// a new array instead using .map() or spreading."



// ❌ Opinion without justification
// "Don't use forEach, use for...of instead."

// ✅ Opinion with reasoning
// "Using for...of here would let us `await` inside the loop
// and `break` early if we find the target. forEach can't
// do either. But if you're not awaiting or breaking,
// forEach is fine — this is optional."



// ❌ Nitpick without value
// "Rename `x` to `variableThatHoldsTheUserData`"

// ✅ Meaningful suggestion
// "Could we use a more specific name than `data`? Since this
// holds the parsed scan results, something like `scanResults`
// or `threatList` would make the intent clearer."

# .github/workflows/review-checks.yml
name: Pre-Review Checks
on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm ci
      - run: npm run lint
      - run: npm run typecheck

  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - run: npm ci
      - run: npm test -- --coverage

  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Secret scanning
        uses: trufflesecurity/trufflehog@v3
        with:
          extra_args: --results=verified,unknown

# GitHub CLI: set up branch protection
gh api repos/:owner/:repo/branches/main/protection \
  --method PUT \
  --field required_status_checks.contexts='["lint","test","security"]' \
  --field required_pull_request_reviews.required_approving_review_count=1 \
  --field enforce_admins=true

## Async Review (most common)
1. Developer creates PR
2. CI runs automated checks
3. Reviewer reviews asynchronously
4. Developer addresses feedback
5. Reviewer approves
6. PR is merged

Best for: Distributed teams, most changes

## Synchronous (pair review)
1. Developer and reviewer sit together (or screen share)
2. Walk through the code line by line
3. Discuss and fix issues immediately
4. PR approved and merged in one session

Best for: Complex changes, new team members, security-critical code

## Maintainer model
1. Contributors submit PRs
2. Maintainers review and merge
3. Contributors don't have merge access

Best for: Open source projects

// Review: are we logging sensitive data?
function handleLogin(req, res) {
  const { username, password } = req.body;

  // RED FLAG: logging passwords
  console.log('Login attempt:', { username, password }); // ❌

  // OK: logging only non-sensitive metadata
  console.log('Login attempt:', { username, ip: req.ip }); // ✅
}

// Review: is input properly sanitized?
function queryDatabase(userInput) {
  // RED FLAG: SQL injection possible
  const query = `SELECT * FROM users WHERE name = '${userInput}'`; // ❌

  // OK: parameterized query
  const query = 'SELECT * FROM users WHERE name = $1'; // ✅
  return db.query(query, [userInput]);
}

// Review: is there proper access control?
function deleteUser(req, res) {
  const { userId } = req.params;

  // RED FLAG: no authorization check
  db.query('DELETE FROM users WHERE id = $1', [userId]); // ❌

  // OK: verify the caller has permission
  if (req.user.role !== 'admin') { // ✅
    return res.status(403).json({ error: 'Unauthorized' });
  }
  db.query('DELETE FROM users WHERE id = $1', [userId]);
}