Session 01 - Code Review Culture

"Code review không phải để tìm lỗi - đó là việc của automated tests. Code review là để chia sẻ kiến thức và nâng cao chất lượng thiết kế." - Trisha Gee

1. Why Code Reviews Matter

Ba giá trị cốt lõi

ROI của Code Review

• Bug tìm trong code review: fix 1-2 giờ
• Bug tìm trong QA: fix 4-8 giờ (reproduce + fix + retest)
• Bug tìm trong production: fix 16-40 giờ (incident + investigation + fix + deploy + postmortem)

Nghiên cứu từ SmartBear: Code review bắt được 60-90% defects trước khi đến QA, giảm cost-of-quality lên tới 4x.

2. Review Guidelines & PR Template

PR Template chuẩn

markdown
## What does this PR do?
[Mô tả ngắn gọn thay đổi gì và tại sao]

## Type of change
- [ ] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature causing existing functionality to change)
- [ ] Refactoring (no functional changes)
- [ ] Documentation update

## How to test
1. [Step-by-step để reviewer test]
2. [Expected behavior]

## Checklist
- [ ] Code follows project coding standards
- [ ] Self-review completed
- [ ] Unit tests added/updated
- [ ] No new warnings introduced
- [ ] API documentation updated (if applicable)
- [ ] Database migration included (if applicable)

## Screenshots (if UI change)
| Before | After |
|--------|-------|
| | |

## Related Issues
Closes #[issue number]

PR Size Guidelines

Nghiên cứu: Review effectiveness giảm 50% sau 400 lines. Sau 800 lines, reviewer bắt đầu "skim and approve".

3. What to Look For in a Review

Review Pyramid (ưu tiên từ trên xuống)

         /\
        /  \        DESIGN
       / Is the approach right? \
      /    Does it fit the architecture?  \
     /──────────────────────────────────\
    /                                    \       LOGIC
   /    Bugs? Edge cases? Error handling? \
  /    Concurrency issues? Null checks?    \
 /──────────────────────────────────────────\
/                                            \   READABILITY
| Naming? Comments? Complexity? DRY?         |
| Can a new team member understand this?     |
\────────────────────────────────────────────/
\                                            /   TESTS
 \ Test coverage? Right assertions?         /
  \ Edge cases tested? Mocks appropriate?  /
   \──────────────────────────────────────/
    \                                    /   STYLE
     \ Formatting? Linting? Conventions /
      \ (Should be automated!)         /
       \──────────────────────────────/

Checklist chi tiet

Design & Architecture:

• Approach phù hợp với architecture hiện tại?
• Có violate SOLID principles không?
• Có tạo unnecessary coupling giữa modules?

Logic & Correctness:

• Edge cases: null, empty, boundary values?
• Error handling: exceptions caught đúng level?
• Concurrency: thread-safe? Race conditions?
• Security: input validation? SQL injection? XSS?

Readability & Maintainability:

• Naming rõ ràng, self-documenting?
• Method length < 30 lines (guideline)?
• Cyclomatic complexity acceptable?
• Comments giải thích "why" chứ không phải "what"?

Tests:

• Unit tests cover new logic?
• Edge cases tested?
• Test names mô tả behavior (Given_When_Then)?

4. Constructive Feedback - Good vs Bad Comments

Bad Comments (avoid)

// Quá chung chung
"This is wrong."

// Mang tính cá nhân
"Why would you do it this way?"

// Commanding without explanation
"Change this to use a factory pattern."

// Nitpicking style (should be automated)
"Add a space before the bracket."

Good Comments (aim for)

// Specific + explanation + suggestion
"Consider using IMemoryCache here instead of a static Dictionary.
Static Dictionary won't be cleared when memory pressure is high,
and it's harder to unit test. Example:
services.AddMemoryCache();
// Then inject IMemoryCache"

// Question format (non-threatening)
"What happens if `order.Items` is null here? Should we add
a null check or is there a guarantee upstream?"

// Praise when deserved
"Nice use of the Strategy pattern here - makes it very easy
to add new payment providers later."

// Acknowledge trade-offs
"I see you chose eager loading here. For this query it makes sense
since we always need the related entities. Just flagging it
for awareness since the table might grow."

Feedback Framework: "Observation + Impact + Suggestion"

5. Review Speed & SLA

Recommended SLA

Review Speed Best Practices

1. Review nhỏ, review thường xuyên - 2-3 sessions/ngày, 30 phút mỗi session
2. Block time cho review - đặt calendar slot cố định
3. Author responsibility: PR description rõ ràng giúp reviewer nhanh hơn
4. Automate the boring stuff - formatting, linting, security scan chạy trước review
5. Two approvals rule - 1 team member + 1 senior/lead

Anti-pattern: PR mở > 3 ngày không ai review → merge conflicts tăng, context switching đau đớn. Track "PR aging" metric.

6. Auto-Review Tools

Tool Stack khuyến nghị cho .NET

Cau hinh SonarQube Quality Gate

yaml
# Minimum quality gate cho PR
quality_gate:
  new_code:
    coverage: ">= 80%"
    duplicated_lines: "<= 3%"
    maintainability_rating: "A"
    reliability_rating: "A"
    security_rating: "A"
    security_hotspots_reviewed: ">= 100%"

Nguyên tắc: Tools bắt style + obvious bugs. Con người focus vào design + logic + knowledge sharing.

7. Pair Programming as Alternative

Khi nào Pair thay cho Review?

Pair Programming Formats

• Driver-Navigator: 1 người code, 1 người review real-time. Swap mỗi 25 phút (Pomodoro)
• Ping-Pong (TDD): A viết test fail → B viết code pass → B viết test fail → A viết code pass
• Strong-Style: "Muốn idea vào máy tính, phải đi qua tay người khác" - Navigator chỉ đạo, Driver code

Pair Programming ROI

• Giảm 15% defects so với solo dev (nghiên cứu Williams & Kessler)
• Tăng 20% development time nhưng giảm 40% debugging time
• Knowledge transfer tức thì - không chờ code review async

Thực chiến: Không cần pair 100% thời gian. Dùng pair cho complex/risky parts, solo cho routine work, code review cho everything else.

Session tiếp theo: Session 02 - Tech Debt Strategy sẽ đi sâu vào cách quản lý và trả nợ kỹ thuật.