Code reviews catch bugs, spread knowledge, and maintain code quality. Done poorly, they're a bottleneck and source of frustration. Here's how to do them well.
Why Code Review Matters#
Benefits:
✓ Catch bugs before production
✓ Share knowledge across team
✓ Maintain consistent code style
✓ Mentor junior developers
✓ Document design decisions
✓ Reduce technical debt
Costs (if done poorly):
✗ Slow down development
✗ Create team friction
✗ Become rubber-stamp exercises
✗ Block urgent fixes
For Authors#
Before Requesting Review#
1## Self-Review Checklist
2
3- [ ] Code compiles and tests pass
4- [ ] I've reviewed my own diff
5- [ ] No debug code or TODOs left
6- [ ] Added tests for new functionality
7- [ ] Updated documentation if needed
8- [ ] PR is small and focused (<400 lines ideal)
9- [ ] Descriptive title and descriptionWriting Good PR Descriptions#
1## What
2
3Add user authentication with JWT tokens.
4
5## Why
6
7Users need to log in to access their dashboards.
8Implements requirement from JIRA-123.
9
10## How
11
12- Added JWT token generation on login
13- Middleware checks tokens on protected routes
14- Tokens expire after 24 hours
15- Refresh tokens handled separately
16
17## Testing
18
19- Unit tests for token generation/validation
20- Integration tests for login flow
21- Manual testing in staging
22
23## Screenshots
24
25[If UI changes, include before/after]
26
27## Notes for Reviewers
28
29- Authentication logic is in `src/auth/`
30- Would appreciate extra scrutiny on the refresh token flowKeep PRs Small#
Size guidelines:
< 100 lines → Quick review (< 30 min)
100-400 lines → Standard review (30-60 min)
400-1000 lines → Large review (needs multiple sessions)
> 1000 lines → Split into smaller PRs
Large PRs get:
- Superficial reviews
- Longer wait times
- More merge conflicts
- Higher defect rates
For Reviewers#
Review Process#
1## Review Steps
2
31. **Understand context**
4 - Read PR description
5 - Check linked issues/tickets
6 - Understand the why
7
82. **High-level pass**
9 - Does the approach make sense?
10 - Are there architectural concerns?
11 - Is this the right place for this code?
12
133. **Detailed review**
14 - Logic correctness
15 - Edge cases
16 - Error handling
17 - Security implications
18
194. **Code quality**
20 - Readability
21 - Naming
22 - Tests
23 - Documentation
24
255. **Run it**
26 - Pull branch locally if complex
27 - Test manually if neededGiving Feedback#
1## Comment Types
2
3**Blocking (must fix)**
4> [BLOCKING] This SQL query is vulnerable to injection.
5> Use parameterized queries instead.
6
7**Suggestion (should consider)**
8> [SUGGESTION] Consider using a Map here instead
9> of repeated array lookups—O(1) vs O(n) per access.
10
11**Nit (optional)**
12> [NIT] This could be more readable as a ternary.
13
14**Question (seeking understanding)**
15> [QUESTION] Why did you choose to put this here
16> instead of in the UserService?
17
18**Praise (positive reinforcement)**
19> Nice solution! I hadn't thought of using
20> WeakMap for this use case.Constructive Criticism#
1## ❌ Unhelpful Comments
2
3"This is wrong."
4"Why did you do it this way?"
5"This is confusing."
6"This code is bad."
7
8## ✅ Helpful Comments
9
10"This will throw if `user` is null. Consider adding
11a null check: `if (!user) return null;`"
12
13"I find this hard to follow. Would it help to extract
14the validation logic into a `validateUser()` function?"
15
16"This approach works, but it makes N+1 database calls.
17Here's an alternative that batches the queries: [code]"
18
19"I'm not sure I understand the reasoning here.
20Could you explain why we need this check?"Automation#
Pre-Review Automation#
1# GitHub Actions - Run before review
2name: PR Checks
3on: [pull_request]
4
5jobs:
6 checks:
7 runs-on: ubuntu-latest
8 steps:
9 - uses: actions/checkout@v4
10
11 - name: Install dependencies
12 run: npm ci
13
14 - name: Lint
15 run: npm run lint
16
17 - name: Type check
18 run: npm run typecheck
19
20 - name: Test
21 run: npm test
22
23 - name: Check bundle size
24 run: npm run analyzeAutomated Code Review#
1# .github/danger.yml
2danger:
3 rules:
4 - name: PR size
5 condition: additions > 500
6 message: "This PR is quite large. Consider splitting it."
7
8 - name: Test coverage
9 condition: test_files_changed < source_files_changed
10 message: "Please add tests for the new code."
11
12 - name: No console.log
13 condition: diff.includes("console.log")
14 message: "Remove console.log statements before merging."Review Culture#
Healthy Norms#
1## Team Agreements
2
31. **Timely reviews**
4 - First response within 4 hours (business hours)
5 - Complete review within 24 hours
6
72. **Respect**
8 - Review the code, not the person
9 - Assume good intentions
10 - Be constructive
11
123. **Learning**
13 - It's okay to not know things
14 - Questions are welcome
15 - Explain the "why" not just the "what"
16
174. **Ownership**
18 - Authors own the final decision
19 - Reviewers share responsibility
20 - Both celebrate successes
21
225. **Efficiency**
23 - Automate style/format checking
24 - Don't block on preferences
25 - Approve with comments when appropriateResolving Disagreements#
1## When You Disagree
2
31. **Discuss in comments**
4 - Exchange reasoning
5 - Link to documentation/research
6 - Stay focused on technical merits
7
82. **Move to synchronous**
9 - If comments go back-and-forth 3+ times
10 - Schedule a quick call
11 - Screen share if helpful
12
133. **Escalate if needed**
14 - Bring in a third opinion
15 - Tech lead makes final call
16 - Document the decision
17
184. **Disagree and commit**
19 - Once decided, support the decision
20 - Don't relitigate in future reviewsMetrics#
1## Healthy Review Metrics
2
3| Metric | Target |
4|--------|--------|
5| Time to first review | < 4 hours |
6| Time to approval | < 24 hours |
7| Review iterations | < 3 |
8| PR size | < 400 lines |
9| Comments per PR | 2-10 |
10| Approval rate | > 80% first time |
11
12## Warning Signs
13
14- Reviews taking multiple days
15- PRs sitting with no comments
16- Only certain people reviewing
17- Large PRs becoming normal
18- Comments mostly about styleConclusion#
Code review is a skill that improves with practice. Focus on finding real issues, giving actionable feedback, and building a culture of learning.
The goal is better code and better engineers—not perfect code or point-scoring.