Developer Code Review Resources and Best Practices

// REVIEW COMMENT: This assumes the slice is non-empty. If len(users) == 0,
// this will panic. We should guard against the empty case.
if users[0].Role == "admin" {
    // ...
}

// SUGGESTED FIX:
if len(users) > 0 && users[0].Role == "admin" {
    // ...
}

# BAD REVIEW COMMENT: "this is wrong"
# GOOD REVIEW COMMENT: "Extracting the parsing logic into a separate
# function would make this testable in isolation and reusable if we
# add other input formats. What do you think?"

// VULNERABLE: Direct interpolation in SQL query
const query = `SELECT * FROM users WHERE id = '${userId}'`;

// SECURE: Use parameterized queries
const query = 'SELECT * FROM users WHERE id = $1';
db.query(query, [userId]);

# BAD: N+1 query pattern — one query for the list, one per item
users = User.objects.filter(active=True)
for user in users:
    print(user.profile.bio)  # Hits DB every iteration

# GOOD: Single query with select_related
users = User.objects.filter(active=True).select_related('profile')
for user in users:
    print(user.profile.bio)

# REVIEW COMMENT: "black and flake8 will flag this. Let's auto-format
# instead of discussing style in review. I've added a pre-commit hook
# config — see the repo's CONTRIBUTING.md."

<!-- Pull request template that enforces size limits -->
## Description
<!-- Why is this change needed? -->

## Type of Change
- [ ] Bug fix (< 100 lines)
- [ ] Feature (< 400 lines)
- [ ] Refactor (< 400 lines)
- [ ] Large change (split into stacked PRs please)

## Checklist
- [ ] Added tests for all new logic
- [ ] Updated documentation
- [ ] No new warnings from linter/formatter
- [ ] Each commit is a logical unit
- [ ] Commit messages follow conventional commits

<!-- Example of a structured review comment -->

**File:** src/services/payment.go:142  
**BLOCKER:** The `charge()` method doesn't handle the `409 Conflict`
response from Stripe. When a charge is retried, Stripe returns 409
with an existing charge ID. We need to catch this and return the
existing charge instead of propagating the error.

**Suggestion:** Wrap the HTTP call and check for `409` before
returning the error. See Stripe's idempotency docs for the expected
response shape.

**Severity:** High — this will cause duplicate charges in production
if the payment service restarts mid-request.

# Example of a well-structured PR description
## Summary
Replaces the in-memory session store with Redis-backed storage to
support horizontal scaling across multiple app instances.

## Motivation
The in-memory store causes session inconsistency when traffic routes
to different pods. This has been the root cause of 3+ Sev-2 incidents
this quarter.

## Design
- Uses go-redis v9 with connection pooling
- Sessions expire after 24h (TTL matched to existing JWT expiry)
- Falls back to in-memory if Redis is unavailable (graceful degradation)

## Testing
- Unit tests for store operations with a mock Redis client
- Integration test against a local Redis container
- Manually verified session persistence across two app instances

## Related Issues
Closes #342

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

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-node@v4
      - run: npm ci
      - run: npx eslint src/
      - run: npx prettier --check src/

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

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

# .gitlab-ci.yml snippet
code-review:
  stage: test
  script:
    - npm ci
    - npm run lint
    - npm run typecheck
    - npm test
  only:
    - merge_requests

# Pre-commit hook configuration (.pre-commit-config.yaml)
repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.6.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-added-large-files
        args: ['--maxkb=500']
  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.5.0
    hooks:
      - id: ruff
        args: [--fix]
      - id: ruff-format

# Example of a merge gate policy
# Block merge unless:
# 1. All CI checks pass
# 2. At least one approval from a senior engineer
# 3. No unresolved review threads
# 4. Branch is up to date with target

<!-- Psychological safe review language examples -->

INSTEAD OF: "This code is wrong."
USE: "I think there's an edge case here. If the token expires between
the validation check and the data fetch, we'll serve stale data."

INSTEAD OF: "Why did you do it this way?"
USE: "Can you walk me through the design choice here? I want to
understand the trade-offs you considered."

INSTEAD OF: "Fix this."
USE: "The error message leaks internal state. Let's use a generic
message and log the details server-side."

## Go Code Review Checklist

- [ ] `err` is always checked (no `_ =` for errors)
- [ ] Context is propagated through the call chain
- [ ] Goroutines are tracked (WaitGroup or errgroup)
- [ ] No global state mutation in tests
- [ ] Exported functions have doc comments
- [ ] No naked `return` in functions with named returns
- [ ] Interface definitions are minimal (1-3 methods)
- [ ] `defer` is used for resource cleanup
- [ ] No `init()` functions unless absolutely necessary
- [ ] Benchmarks exist for performance-critical paths

## Python Code Review Checklist

- [ ] Type hints are present on all new functions
- [ ] Context managers (`with` statements) used for resources
- [ ] No mutable default arguments (e.g., `def f(x=[])`)
- [ ] Exception handling is specific (no bare `except:`)
- [ ] Logging uses structured fields, not string formatting
- [ ] Database queries use parameterized inputs (no f-strings)
- [ ] Async functions use proper await patterns (no blocking calls)
- [ ] Test fixtures are scoped appropriately (function, class, module)
- [ ] No star imports (`from module import *`)
- [ ] Functions are < 50 lines (LSP rule)

## JavaScript/TypeScript Code Review Checklist

- [ ] Strict mode enabled (TypeScript: `strict: true`)
- [ ] No `any` type unless unavoidable (and justified in comment)
- [ ] Async/await used over raw Promise chains
- [ ] No `console.log` in production code
- [ ] React hooks follow rules of hooks (no conditional hooks)
- [ ] useEffect has proper dependency array (no lint-disable)
- [ ] Bundle size impact considered for new dependencies
- [ ] Event listeners are cleaned up in useEffect return
- [ ] API error responses are handled, not swallowed
- [ ] No direct DOM manipulation (use React refs)

## Security Code Review Checklist

- [ ] All user input is validated (type, length, format, range)
- [ ] SQL queries use parameterized statements or ORM
- [ ] No secrets or tokens in source code or config files
- [ ] Authentication checks exist on every protected endpoint
- [ ] Authorization checks verify ownership/role, not just auth
- [ ] File uploads validate type and size server-side
- [ ] Rate limiting is applied to mutation endpoints
- [ ] CORS is configured per-origin, not wildcard
- [ ] Output is encoded/escaped to prevent XSS
- [ ] Logging does not include PII or secrets

# Examples
feat(auth): add OAuth2 token refresh flow

fix(api): handle null pointer in user deserialization

docs(readme): update deployment instructions

refactor(payments): extract discount calculation to Strategy pattern

chore(deps): upgrade go-redis to v9.4.0