Viết comment review và xử lý bất đồng Trung cấp

Trang này trả lời: comment thế nào để tác giả biết ngay vấn đề là gì và có bắt buộc sửa không, và làm gì khi hai bên không đồng ý.

Hỏi thay vì ra lệnh, và nói rõ mức độ

Mỗi comment phải cho tác giả biết hai thứ: vấn đề là gì và có bắt buộc sửa không. Google dùng tiền tố Nit:, Optional:/Consider:, FYI:. Conventional Comments chuẩn hoá thành cú pháp:

<label> [decorations]: <subject>

[discussion]

Label	Dùng khi	Mặc định chặn?
praise:	Khen thật lòng một cách làm tốt	Không
nitpick:	Sở thích cá nhân, chuyện nhỏ	Không (bản chất non-blocking)
suggestion:	Đề xuất cải thiện, nói rõ vì sao tốt hơn	Tuỳ decoration
issue:	Vấn đề cụ thể; nên kèm suggestion	Thường có
todo:	Việc nhỏ nhưng cần làm	Có
question:	Nghi ngờ nhưng chưa chắc; nhờ tác giả làm rõ	Tuỳ decoration
thought:	Ý tưởng nảy ra khi đọc, không đòi hỏi gì	Không
chore:	Việc theo quy trình trước khi merge (chạy job, cập nhật doc)	Có
note:	Thông tin người đọc nên biết	Không

Cột “Mặc định chặn?” là cách hiểu của cụm này. Spec chỉ nói rõ nitpick, thought non-blocking “by nature” và note luôn non-blocking; label khác nên ghi rõ bằng decoration. Spec còn ba label tuỳ chọn typo, polish, quibble.

Decoration: (blocking), (non-blocking), (if-minor) — chỉ sửa nếu thay đổi nhỏ. Ví dụ: suggestion (non-blocking): ..., issue (security, blocking): ....

Team không cần đủ 9 label. Mặc định tối thiểu: issue, question, suggestion, nit, praise, kèm (blocking) khi chặn.

Comment trước và sau

#	Trước	Sau	Vì sao tốt hơn
1	“Sai rồi.”	issue (blocking): Hai request hoàn tiền song song cùng đọc refunded = 0 và cùng ghi — có thể hoàn gấp đôi. Dùng UPDATE ... SET refunded = refunded + $1 WHERE refunded + $1 <= amount và kiểm tra rowCount?	Kịch bản lỗi cụ thể và hướng sửa
2	“Sao lại làm thế này?”	question: Mình thấy findAll() rồi lọc trong app. Bảng orders ~8 triệu dòng — có lý do gì không đẩy WHERE xuống query không?	Hỏi để hiểu, đưa số liệu
3	“Đổi tên biến đi.”	nitpick (non-blocking): timeout → timeoutMs để call site không phải đoán đơn vị.	Có lý do, có nhãn non-blocking
4	“Bạn lại quên validate.”	issue (security, blocking): sortBy từ query string đi thẳng vào ORDER BY. Đi qua map allowlist như mục Identifier trong Secure coding nhé.	Nói về code, không về người; link quy ước
5	“Viết lại cái này theo Strategy pattern.”	thought: Nếu sau này thêm phương thức hoàn tiền thứ ba, switch này có thể đáng tách ra Strategy. Hiện 2 nhánh thì giữ nguyên là ổn.	Không đẩy over-engineering
6	“Thiếu test.”	todo: Thêm test cho case hoàn đúng 100% và hoàn vượt số đã thu (phải trả 422). Đây là biên của CHECK constraint.	Nói rõ case nào và vì sao
7	(Không comment gì phần tốt)	praise: Tách calculateRefund() thành hàm thuần làm test 6 case làm tròn không cần mock — rất gọn.	Conventional Comments khuyên ít nhất một praise thật lòng
8	15 comment “thiếu dấu chấm phẩy”, “dùng nháy đơn”	Một comment: chore: Repo chưa chạy Prettier trong CI, mình mở PR riêng để thêm. Không chặn PR này.	Sửa hệ thống thay vì từng dòng
9	Reviewer chấp nhận “OK, để PR sau dọn.”	issue (blocking): Hàm mới thêm flag thứ ba vào processOrder. Cleanup “để sau” thường không bao giờ xảy ra — tách thành hai hàm trong PR này nhé.	Độ phức tạp mới phải dọn trước khi merge

Hai quy tắc ngắn:

• Comment vào code, không vào người. “Hàm này…”, không phải “Bạn…”.
• Tác giả giải thích trong thread thì xin họ đưa vào code. Google: “Explanations written only in the code review tool are not helpful to future code readers.” Code rõ hơn là tốt nhất; comment trong code là phương án hai.

Khi bạn là reviewer bị phản đối

Theo Google:

1. Xem lại xem họ có đúng không. Tác giả thường hiểu phần code đó hơn bạn.
2. Vẫn tin yêu cầu của mình cải thiện code health thì giải thích lại, kèm dữ liệu hoặc nguyên tắc thiết kế. “Technical facts and data overrule opinions and personal preferences.”
3. “Để PR sau dọn”: Google gọi đó là “a common way for codebases to degenerate”. Độ phức tạp mới do PR thêm vào thì dọn trước khi merge, trừ khi khẩn cấp. Vấn đề có sẵn xung quanh: tạo ticket gán cho tác giả, có thể kèm TODO trỏ tới ticket.
4. Lịch sự thì người ta hiếm khi giận vì bị yêu cầu sửa; họ giận vì cách viết comment.

Khi bạn là tác giả nhận comment

• Không trả lời lúc đang bực. Hỏi: “Reviewer đang cố nói điều gì có ích?”
• Không hiểu thì hỏi lại thay vì đoán và sửa sai hướng.
• Không đồng ý thì nói trade-off, không viết “Không, mình không làm đâu”.
• Reviewer bối rối ở chỗ nào → sửa code cho rõ ở chỗ đó, đừng chỉ giải thích trong thread.

Khi nào chuyển sang call

Đang tải diagram…

Google khuyến nghị gặp mặt hoặc video call khi khó đồng thuận, ghi kết quả thành comment trên PR, rồi mới escalate (thảo luận nhóm, tech lead, maintainer, engineering manager).

• “Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.”
• Mốc “2 vòng” là quy ước của cụm này, không phải của Google; nó chặn những thread 40 comment.

Thread 31 comment về một cái enum

Team 6 người, service đơn hàng NestJS. Reviewer muốn trạng thái đơn là bảng tra order_statuses; tác giả muốn enum PostgreSQL. Thread kéo 31 comment trong 4 ngày, PR (sẵn sàng từ ngày 1) bị rebase hai lần vì conflict.

Call 15 phút đi tới kết luận: trạng thái không do người vận hành thêm, cần CHECK ở DB, nên dùng text + CHECK constraint — không phải phương án ban đầu của ai. Kết luận được ghi vào PR và tài liệu quy ước của team.

Quy tắc team đặt ra sau đó: comment thứ ba trong cùng một thread bất đồng là tín hiệu để hẹn call.

Tiếp theo: Anti-pattern và metric.