Lesson 12

Code Review

Catch bugs, share knowledge, and enforce standards through structured review

Code Review Pull Requests Severity Scoring SOLID Compliance Definition of Done Review Loop

Why Code Review?

Code review is one of the most effective quality practices in software engineering. It is not about finding fault — it is about building better software together.

🐛 Catch bugs early — before they reach staging or production
📚 Share knowledge — reviewers learn the codebase, authors learn better patterns
📏 Enforce standards — SOLID, Clean Code, naming conventions applied consistently
🔒 Security gate — a second pair of eyes catches vulnerabilities
🤝 Collective ownership — no one person "owns" a module; the team owns the code

The Goal of Code Review

The goal is not to prove the code is wrong. The goal is to ensure the code is correct, maintainable, secure, and aligned with team standards.

What to Review

A thorough code review examines multiple dimensions of quality:

Dimension	What to Check	Key Questions
SOLID Compliance	Single Responsibility, dependency direction, interface segregation	Does this class do one thing? Are dependencies injected?
Clean Code	Naming, function size, DRY, readability	Can I understand this in 30 seconds? Are names intention-revealing?
Test Quality	Coverage, edge cases, test readability	Are tests testing behavior, not implementation? Are edge cases covered?
Security	Input validation, auth checks, data exposure	Is user input sanitized? Are secrets hardcoded?
Performance	N+1 queries, unnecessary loops, memory leaks	Will this scale? Are there obvious bottlenecks?
Architecture	Layer boundaries, coupling, dependency direction	Does this follow our architecture? Are boundaries respected?

Severity Scoring System

Every finding in a code review is assigned a severity level. This removes ambiguity — everyone knows which issues block a merge and which are suggestions.

Severity	Label	Action Required	Examples
🔴 S0	Critical	Blocks merge. Must fix immediately.	Security vulnerability, data loss risk, broken functionality
🟠 S1	High	Blocks merge. Must fix before approval.	SOLID violation, missing tests for new logic, race condition
🟡 S2	Medium	Should fix. Won't block merge if acknowledged.	Naming improvement, minor code smell, missing edge case test
🔵 S3	Low	Optional. Nice to have.	Style preference, minor refactor suggestion, documentation tweak

Merge Blockers

Any 🔴 S0 (Critical) or 🟠 S1 (High) finding blocks the PR from being merged. These must be resolved and re-reviewed before approval.

The Review Loop

Code review is not a one-shot event. It is an iterative process that continues until the code meets the team's quality bar.

📤

Submit PR Author

🔍

Review Code Reviewer

🛠️

Address Feedback Author

🔄

Re-review Reviewer

✅

Approve & Merge Both

Iteration Loop

If S0/S1 findings are discovered during review, the author must fix them and the reviewer re-reviews. This loop repeats until all blockers are resolved.

Best Practices: As a Reviewer

The Reviewer's Mindset

🧠 Understand first — Read the PR description and linked ticket before looking at code
❓ Ask why, not what — "Why did you choose this approach?" beats "This is wrong"
💡 Suggest, don't command — "Consider using X because..." not "Change this to X"
⏰ Review within 24 hours — Stale PRs kill velocity. Treat reviews as high priority
👍 Call out good code too — Positive feedback reinforces good practices
🎯 Use severity labels — Every comment should have S0/S1/S2/S3 so the author knows what to prioritize

Example Review Comment

🟠 S1: This database query runs inside a loop, creating an N+1 problem.
Each iteration hits the DB, so 100 items = 100 queries.

Consider batch-loading all items upfront:

  const items = await repo.findByIds(allIds);  // 1 query
  const lookup = new Map(items.map(i => [i.id, i]));

This drops N+1 queries down to 1.

Best Practices: As an Author

The Author's Responsibilities

🔍 Self-review first — Review your own diff before requesting review. Catch the obvious issues yourself
📏 Keep PRs small — Under 400 lines of change. Large PRs get superficial reviews
📝 Write a descriptive PR description — What changed, why, how to test, screenshots if UI
🧪 Include tests — New code without tests will be flagged S1
🙏 Don't take feedback personally — Review is about the code, not about you
💬 Respond to every comment — Either fix it or explain why you disagree. Never ignore feedback

The 400-Line Rule

Studies show that review effectiveness drops dramatically after 400 lines. If your PR is larger, split it into smaller, logical chunks. Each PR should represent one coherent change.

What Blocks a Merge?

A PR cannot be merged if any of the following are true:

🔴 Any S0 (Critical) finding is unresolved
🟠 Any S1 (High) finding is unresolved
🚫 CI pipeline is not green
🚫 No reviewer approval
🚫 Definition of Done checklist is incomplete

Definition of Done Checklist Integration

Code review is one component of the Definition of Done. Before marking a story as "Done," verify the full checklist:

## Definition of Done Checklist
- [ ] Code implements all acceptance criteria
- [ ] Unit tests written and passing
- [ ] Integration tests written (if applicable)
- [ ] Code reviewed — zero S0/S1 findings
- [ ] CI pipeline green
- [ ] Documentation updated (if API changed)
- [ ] No hardcoded secrets or credentials
- [ ] Backward compatible (or migration provided)
- [ ] Approved by at least one reviewer

Key Takeaway

Code review is a skill. Great reviewers are specific, respectful, and severity-aware. Great authors make reviewing easy by keeping PRs small, descriptive, and well-tested. Together, they build a codebase the whole team can trust.

Knowledge Check

Which severity levels block a PR merge?

← Lesson 11: CI/CD Pipeline Lesson 13: AI & LLM Engineering →