Nghệ thuật Code Review 🔍

Code Review là thời điểm quan trọng nhất trong vòng đời của một Pull Request. Làm tốt, nó là cỗ máy lan tỏa kiến thức, phát hiện bug, và xây dựng văn hóa kỹ thuật tốt. Làm sai, nó trở thành nguồn gốc của stress, conflict, và bottleneck khó chịu nhất trong team.

1. Mindset trước khi review

Trước khi mở PR ra, hãy xác định rõ vai trò của mình:

Bạn không phải là giám khảo — bạn là người đồng hành.

Code Review không phải để chứng minh bạn thông minh hơn author, mà để đảm bảo cả team cùng sở hữu và hiểu codebase. Mọi comment nên xuất phát từ câu hỏi: "Cái này có giúp codebase, team, và product tốt hơn không?"

The Prime Directive of Code Review

"Regardless of what we discover, we understand and truly believe that everyone did the best job they could, given what they knew at the time."

Đừng assume người kia viết tệ vì lười biếng. Họ có context hạn chế, time pressure, hoặc đơn giản là chưa biết pattern tốt hơn. Nhiệm vụ của bạn là chia sẻ kiến thức, không phải phán xét.

2. Review cái gì — Checklist thực chiến

Hãy tập trung vào những thứ mà Linter và Static Analyzer không làm được. Format code, cách đặt tên biến, khoảng trắng — những thứ đó phải được tự động hóa bằng pre-commit hook hoặc CI.

✅ Kiến trúc & Thiết kế

• Code mới có tuân thủ Design Patterns hiện tại không, hay nó tạo ra một "ngoại lệ đặc biệt" sẽ gây rắc rối về sau?
• Có class/function nào đảm nhận quá nhiều trách nhiệm không (vi phạm SRP)?
• Có circular dependency nào được tạo ra không?
• Abstraction có phù hợp không — không quá generic (over-engineering) cũng không quá cụ thể (hard-coded)?

✅ Logic & Correctness

• Happy path có đúng không — nhưng quan trọng hơn là edge cases:
  • Input rỗng, null, 0, -1, empty list
  • Integer overflow/underflow
  • Race conditions (concurrent access)
  • Off-by-one errors trong vòng lặp
• Error handling có đúng không? Lỗi có bị "nuốt" im lặng không?
• Return value có được handle đúng không?

✅ Security

• Có SQL Injection / NoSQL Injection risk không? (Dùng parameterized query chưa?)
• Input từ user có được validate và sanitize không?
• Log có vô tình ghi ra PII (Personal Information), token, password không?
• Có hardcode credential, secret, API key không?
• Authorization check có đúng không — "chỉ user sở hữu mới được xóa record của mình"?

✅ Performance

• Có N+1 query không (gọi DB bên trong vòng lặp)?
• Có query thiếu index trên field thường xuyên filter/sort không?
• Có tạo unnecessary object allocation trong hot path không?
• Có blocking I/O trong async context không?
• Có load cả table khi chỉ cần một vài records không (thiếu pagination hoặc LIMIT)?

✅ Observability

• Có log đủ không khi xảy ra lỗi quan trọng?
• Log level có đúng không? (ERROR vs WARN vs INFO)
• Có emit metrics/traces khi cần không?
• Log message có đủ context để debug sau này không? (Request ID, User ID, error details)

✅ Tests

• Test có cover đủ happy path và edge case không?
• Test có readable không, hay chỉ là "mock everything and assert nothing"?
• Có test nào đang test implementation detail thay vì behavior không?
• Nếu không có test, lý do là gì?

3. Cách viết comment xây dựng

Hệ thống nhãn comment (Comment Labels)

Đây là kỹ thuật cực kỳ hữu ích để tránh hiểu nhầm tầm quan trọng của comment:

Ví dụ thực tế:

[nit] Có thể đổi tên biến `data` sang `userProfile` để rõ hơn không?

[blocking] Chỗ này có SQL Injection risk vì dùng string interpolation.
Nên dùng parameterized query:
db.Query("SELECT * FROM users WHERE id = ?", userID)

[question] Tại sao chúng ta cần cache layer ở đây? 
Latency của DB call này có thực sự là bottleneck không?

[praise] Cách xử lý retry logic ở đây rất clean! 🎉

Từ ngữ — Hỏi thay vì ra lệnh

Cung cấp context và giải pháp

Đừng chỉ nói "cái này sai" mà không đưa ra gợi ý.

Xấu:

"Cái này có performance issue đấy."

Tốt:

"Mình thấy vòng lặp này gọi getUserById() mỗi iteration. Nếu list có 1000 items, đó là 1000 DB queries. Bạn có thể thử batch load users trước: batchGetUsersByIds(userIds) rồi tạo một map lookup không? Cái đó sẽ giảm từ O(N) queries xuống còn O(1)."

4. Perspective của Author — Cách chuẩn bị PR tốt

Code Review là two-way street. Author cũng có trách nhiệm tạo điều kiện tốt nhất cho reviewer.

Checklist trước khi tạo PR

• PR nhỏ: Tối đa 300-400 lines thay đổi thực sự. Nếu lớn hơn, chia thành nhiều PR có thứ tự (stacked PRs).
• PR description rõ ràng:
  • Context: Tại sao PR này tồn tại?
  • What changed: Đã thay đổi cái gì?
  • How to test: Reviewer nên test thế nào?
  • Screenshots/demo nếu có UI change.
• Self-review trước: Đọc lại diff của chính mình trước khi assign reviewer. Bạn sẽ bắt được nhiều vấn đề rõ ràng mà không cần tốn giờ của người khác.
• Ghi chú những chỗ uncertain: Dùng comment của tác giả (Author comment) để chú thích: "Tôi không chắc về approach này, hoan nghênh ý kiến" — điều này rất có giá trị.

Đừng defensive khi nhận comment

Comment về code không phải comment về bạn. Kỹ năng quan trọng nhất khi nhận feedback:

1. Đọc hiểu trước, phản hồi sau. Đừng reply ngay khi còn cảm thấy defensive.
2. Hỏi nếu không hiểu: "Bạn có thể giải thích thêm tại sao approach này tốt hơn không?"
3. Không nhất thiết phải đồng ý: Nêu rõ lý do nếu bạn nghĩ approach của mình đúng. Thảo luận, không phải tranh cãi.
4. Resolve comment sau khi fix: Đừng resolve comment mà không trả lời để reviewer biết bạn đã xử lý thế nào.

5. Quản lý thời gian và quy trình Review

SLA (Service Level Agreement) cho Code Review

Team nên có thỏa thuận rõ ràng. Ví dụ:

• PR kích thước nhỏ (<200 lines): Review trong 4 giờ làm việc
• PR kích thước trung bình (<500 lines): Review trong 1 ngày làm việc
• PR lớn (>500 lines): Schedued review — đặt lịch review cùng nhau

Không có SLA → Reviewer "sẽ review sau" → PR ngâm 3 ngày → Team lose velocity.

Khi nào nên Request Changes vs Comment vs Approve

• Request Changes: Có blocking issue cần sửa trước khi merge (security bug, logic error, missing critical test).
• Comment: Để lại nhận xét nhưng không block merge. Thường dùng cho nit, question, FYI.
• Approve with comments: Code đủ good để merge, có một vài optional suggestions.

Vấn đề "Review Anxiety" — Reviewer không chắc đủ kiến thức

Nếu bạn là reviewer mà không quen với một phần của codebase, hãy nói thẳng: "Tôi không quen với phần Auth này, nhưng tôi có thể review phần Business Logic. Bạn có thể assign thêm người quen với JWT flow không?"

6. Code Review ở quy mô lớn (Large-scale codebase)

CODEOWNERS (GitHub/GitLab)

Với codebase lớn, hãy dùng file CODEOWNERS để tự động assign reviewer đúng chuyên môn:

# Any change in /payments/** requires review from payments team
/payments/**    @payments-team @security-team

# Frontend changes require Design Lead review
/frontend/**    @design-lead

# Infrastructure changes require DevOps review
/infra/**       @devops-team

Automated PR Checks giảm tải cho reviewer

Tự động hóa tối đa để reviewer tập trung vào logic, không phải format:

• Linter/Formatter (trước khi PR được mở)
• Unit/Integration tests phải pass
• Coverage threshold (không drop dưới 80%)
• Security scan (Snyk, Dependabot, Semgrep)
• Bundle size check (với frontend)
• Benchmark regression check (với performance-sensitive code)

7. Xây dựng văn hóa Review tốt trong team

Pair Review thay vì Sequential Review

Thay vì Author → Reviewer A review → fix → Reviewer B review → fix → merge (mất 3-4 ngày), hãy thử:

• Author + Reviewer A ngồi review cùng nhau 30 phút → fix → Reviewer B async review → merge.

Learning-Oriented Review

Định kỳ (1 lần/tháng), team chọn 1-2 PR thú vị để review cùng nhau trong cuộc họp kỹ thuật. Mục đích không phải là để approve/reject mà là để cả team học cùng nhau, chia sẻ kinh nghiệm.

Blameless Postmortem cho PR gây bug

Khi một PR được merge mà sau đó gây bug production, đừng blame reviewer hay author. Thay vào đó:

• What process failed? Review không đủ kỹ? Thiếu test? Deploy quá vội?
• What can we improve? Thêm automated check? Improve Definition of Done?
• Document it: Ghi lại lesson learned để cả team học.