Code Review Best Practices: Giving and Receiving Feedback

Code reviews improve quality, share knowledge, and catch bugs before production.

Reviewer Checklist

Correctness

• Does the code do what it claims?
• Are edge cases handled (null, empty, overflow)?
• Are error paths correct?
• Is the logic correct for all inputs?

Design

• Is the change in the right place?
• Does it follow existing patterns?
• Is it too complex? Can it be simplified?
• Does it violate SOLID principles?

Testing

• Are there adequate tests?
• Do tests actually verify the behavior?
• Are edge cases tested?

Security

• Any SQL injection, XSS, or injection vulnerabilities?
• Is sensitive data logged or exposed?
• Are inputs validated and sanitized?

Performance

• Any N+1 query issues?
• Are database queries indexed appropriately?
• Is caching used where appropriate?

Giving Good Feedback

# Bad feedback
"This code is wrong"
"Why would you do it this way?"
"I would have done it completely differently"

# Good feedback
"This might cause issues with concurrent access—consider using a transaction here"
"Nit: Could we rename `d` to `daysSinceLastLogin` for clarity?"
"What do you think about extracting this into a shared utility? We use similar logic in UserService"

# Question, don't command
"Could this cause an issue if user is null?"
vs
"This will throw if user is null"

Receiving Feedback

• Don't take feedback personally; it's about the code
• Respond to all comments (LGTM, done, or explain why you disagree)
• Ask for clarification on ambiguous feedback
• It's OK to disagree—explain your reasoning

Pull Request Best Practices

## Summary
Brief description of what changed and why.

## Changes
- Added `PaymentService` to handle Stripe integration
- Refactored `OrderController` to use the new service
- Added unit tests for payment failure scenarios

## Testing
- [ ] Unit tests pass
- [ ] Integration tests with Stripe test mode
- [ ] Manual testing of checkout flow

## Screenshots (if UI changes)

Automation to Reduce Review Load

# .github/workflows/pr-checks.yml
- name: Lint
  run: npm run lint

- name: Type check
  run: npm run typecheck

- name: Tests
  run: npm test

- name: Bundle size check
  run: npm run build && npx bundlesize

Review Turnaround

• Reviewer: Aim to review within 1 business day
• Author: Respond to feedback within 1 business day
• Small PRs: Easier to review (aim for <400 lines)
• Stale PRs: Re-request review after addressing all comments

Good code reviews are collaborative conversations, not gatekeeping.