Code Review Practices: Making Reviews Useful, Not Painful

1
2
3
4
5

# Bug: Returns wrong result for empty list
def average(numbers):
    return sum(numbers) / len(numbers)  # ZeroDivisionError!

# Comment: "What happens when numbers is empty?"

1
2
3
4

# What if user_id doesn't exist?
# What if the API times out?
# What if the file is empty?
# What if two requests come in simultaneously?

1
2
3
4
5
6
7
8
9

# SQL injection?
query = f"SELECT * FROM users WHERE id = {user_id}"

# Exposed secrets?
API_KEY = "sk_live_abc123"

# Missing authorization?
def delete_user(user_id):  # Who can call this?
    db.delete(user_id)

1
2
3
4
5
6
7
8

# N+1 query?
for user in users:
    orders = db.query(f"SELECT * FROM orders WHERE user_id = {user.id}")

# Unnecessary work?
def get_user(id):
    all_users = db.query("SELECT * FROM users")  # Fetching everything!
    return find_by_id(all_users, id)

1
2
3
4

# Will future developers understand this?
result = [x for x in data if x.a and not x.b or x.c and x.d > 5]

# Comment: "This condition is hard to follow. Could we extract to a named function?"

1
2
3
4
5
6
7
8

# Don't manually enforce these:
# - Line length
# - Import order  
# - Trailing whitespace
# - Quote style

# Instead, configure formatters and linters
# black, prettier, eslint run automatically

1
2
3
4
5
6

# Not worth commenting on:
for item in items:  # vs for i in items:
    
users_list = []  # vs user_list = []

if x is not None:  # vs if x != None:

1
2
3
4
5
6

PR Description:
- What: Add rate limiting to API endpoints
- Why: We're getting hammered by a misbehaving client
- How: Token bucket algorithm with Redis backend
- Testing: Added unit tests, tested manually with wrk
- Concerns: Not sure about the 100 req/min limit — feedback welcome

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14

# Code Review Guidelines

## Required
- At least one approval before merge
- All CI checks passing
- No unresolved blocking comments

## Timeframe  
- Reviews within 24 hours
- If blocked, ping in Slack

## Scope
- PRs should be < 400 lines when possible
- Split large changes into stacked PRs

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20

# .github/workflows/pr.yml
on: [pull_request]

jobs:
  checks:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      
      - name: Lint
        run: npm run lint
        
      - name: Type check
        run: npm run typecheck
        
      - name: Test
        run: npm test
        
      - name: Security scan
        run: npm audit