Code Review Process

Difficulty: Intermediate

Question

How do you approach code review? What do you look for, and how do you give constructive feedback?

Answer

Code review serves multiple purposes: catching bugs, ensuring code quality, sharing knowledge, and maintaining consistency. A good reviewer balances thoroughness with speed.

What to look for: 1. Correctness: Does the code do what it claims? Are edge cases handled? 2. Design: Is the approach sound? Are there simpler alternatives? 3. Readability: Can someone new to the codebase understand this? 4. Tests: Are there adequate tests? Do they test the right things? 5. Security: SQL injection, XSS, secrets in code, authentication bypasses? 6. Performance: N+1 queries, unnecessary re-renders, missing indexes?

Feedback etiquette: - Ask questions instead of making demands: 'What do you think about...' vs 'Change this.' - Distinguish between blockers and nitpicks (prefix with 'nit:'). - Praise good code, not just criticize problems. - Suggest alternatives, do not just point out issues. - Be timely: review within 24 hours to avoid blocking teammates.

Code review is a conversation, not a gate. The goal is to make the code better together while building team knowledge.

Code examples

Code Review Checklist

## Code Review Checklist

### Correctness
# - [ ] Logic handles edge cases (null, empty, boundary values)
# - [ ] Error handling is present and appropriate
# - [ ] No off-by-one errors in loops or array access
# - [ ] Async operations handle failures and timeouts

### Security
# - [ ] No secrets hardcoded (API keys, passwords)
# - [ ] User input is validated and sanitized
# - [ ] SQL queries use parameterized statements
# - [ ] Authentication/authorization checked on all endpoints
# - [ ] No sensitive data in logs or error messages

### Performance
# - [ ] No N+1 query problems
# - [ ] Database queries use appropriate indexes
# - [ ] No unnecessary API calls or re-renders
# - [ ] Large lists are paginated

### Maintainability
# - [ ] Functions are focused (single responsibility)
# - [ ] Variable/function names are descriptive
# - [ ] No duplicated logic (DRY)
# - [ ] Complex logic has comments explaining WHY

### Tests
# - [ ] New code has test coverage
# - [ ] Edge cases are tested
# - [ ] Tests are readable and maintainable

Use a checklist to ensure consistent reviews. Not every item applies to every PR, but having the list prevents overlooking important aspects.

Review Feedback Examples

# BAD feedback (vague, demanding, unhelpful):
# "This is wrong."
# "Rewrite this."
# "Why would you do it this way?"

# GOOD feedback (specific, constructive, educational):

# Blocker:
# "Bug: This will throw if `user` is null (line 45).
#  We should add a null check:
#  `if (!user) return res.status(404).json({ error: 'Not found' });`"

# Suggestion:
# "nit: Consider using `Array.find()` instead of `filter()[0]`
#  for better readability and early exit:
#  `const admin = users.find(u => u.role === 'admin');`"

# Question (learning opportunity):
# "I'm curious about the choice to use `any` here.
#  Would a generic type work? Something like:
#  `function process<T extends User>(data: T): T`
#  This would give us type safety while staying flexible."

# Praise:
# "Nice use of the builder pattern here! This makes
#  the query construction much more readable."

# Security concern:
# "Security: `req.body.id` is used directly in the SQL query.
#  This is vulnerable to SQL injection. Please use
#  parameterized queries: `WHERE id = $1, [req.body.id]`"

Good review feedback is specific, explains why, and offers alternatives. Prefix nitpicks with 'nit:' so the author knows it is not a blocker.

Review Workflow with gh CLI

# View PR details
gh pr view 42
gh pr diff 42

# Start a review (batch comments)
gh pr review 42 --comment --body "Overall looks good, a few suggestions:"

# Add line-specific comments via GitHub UI:
# 1. Click on the changed file
# 2. Click the + icon on the line number
# 3. Type your comment
# 4. Choose: 'Add single comment' or 'Start a review'
#    (Start a review batches comments into one notification)

# Approve the PR
gh pr review 42 --approve --body "LGTM! Great test coverage."

# Request changes
gh pr review 42 --request-changes --body \
  "Please address the security concern on line 45."

# Check CI status before approving
gh pr checks 42
# ci/test     pass   2m30s
# ci/lint     pass   45s
# ci/build    pass   1m15s

# Resolve a conversation thread
# (Done via GitHub UI after the author addresses feedback)

Batch comments into a single review to reduce notification noise. Always check CI status. Resolve conversation threads to track what has been addressed.

Key points

Concepts covered

Code Review, Review Checklist, Feedback Etiquette, Review Tools, Pair Programming