Những Tiêu Chí Và Vấn Đề Khi Review Code

Ronin Engineer

6 Tháng 3 2023

<h2 id="the-goal-of-code-review-m%E1%BB%A5c-%C4%91%C3%ADch-c%E1%BB%A7a-code-review">The Goal of Code Review (Mục đích của Code Review)</h2><p>Code Review là quá trình đánh giá code để đảm bảo code đạt được những tiêu chuẩn về chất lượng và thiết kế. Mục đích của việc code review:</p><ul><li>Đảm bảo sản phẩm đạt chất lượng cao nhất.</li><li>Đảm bảo chất lượng của codebase được cải thiện theo thời gian.</li><li>Phát triển bản thân<ul><li>Soft Skills:<ul><li>Reviewer và reviewee được luyện khả năng trình bày, giải thích ý tưởng làm sao cho người khác dễ hiểu</li><li>Học cách lắng nghe</li></ul></li><li>Hard Skills:<ul><li>Học hỏi, chia sẻ kỹ thuật, kinh nghiệm chuyên môn</li></ul></li></ul></li></ul><figure class="kg-card kg-image-card"><img src="https://images.viblo.asia/b5256155-4379-4dad-911b-52450377f407.jpg" class="kg-image" alt="driving.jpg" loading="lazy" width="825" height="648"></figure><h2 id="code-review-points-nh%E1%BB%AFng-ti%C3%AAu-ch%C3%AD-c%E1%BA%A7n-review">Code Review Points (Những Tiêu Chí Cần Review)</h2><h3 id="functionality">Functionality</h3><ul><li>Kiểm tra logic xử lý đã đúng yêu cầu chưa?<ul><li>Nếu là một tiến trình nhiều bước, đảm bảo số lượng các bước và thứ tự các bước là tối ưu nhất</li><li>Cập nhật đúng trạng thái</li></ul></li><li>Suy nghĩ như người dùng để tìm đủ corner cases, edge cases</li><li>Để ý những vấn đề về concurrency (deadlock, race conditions, ...)</li></ul><h3 id="design">Design</h3><ul><li>Code changes có làm thay đổi kiến trúc của code base không?</li><li>Code changes có tích hợp được phần còn lại của hệ thống không?</li><li>Đây có phải thời điểm để thêm tính năng này không?</li><li>Code changes có khả năng mở rộng về sau không?</li></ul><h3 id="readability">Readability</h3><ul><li>Naming: một tên biến dễ hiểu là một tên biến đủ dài để thể hiện nó là gì và làm gì nhưng cũng không quá dài để đọc</li><li>Style: nên thống nhất một coding style</li><li>Comments: cần cô đọng, rõ ràng và chủ yếu giải thích vì sao (why) thay vì nó là gì (what)</li><li>Complexity: Khi developer viết đoạn code tổng quát hơn mức cần thiết, hoặc thêm 1 tính năng mà hiện tại hệ thống chưa cần tới nó. Như vậy nó làm tăng độ phức tạp cho hệ thống.</li><li>Duplication: logic của đoạn code thêm mới có trùng với function nào trước đó không?</li></ul><h3 id="consistency">Consistency</h3><p>Sự thống nhất rất quan trọng, nhất là công việc về kỹ thuật. Sự thống nhất trong codebase giúp code dễ đọc hơn, tránh được bugs không đáng có, chuyển đổi dễ dàng hơn và tạo điều kiện cho làm việc nhóm thuận tiện hơn.</p><p>Nếu team có thống nhất về style guide, convention và codebase rõ ràng thì developer nên theo để tạo sự thống nhất.</p><h3 id="testing">Testing</h3><p>Test rất quan trọng. Test như việc mình đi khám sức khỏe định kỳ. Nếu sức khoẻ của mình có vấn đề, bác sỹ sẽ có những khuyến cáo, điều chỉnh để mình cải thiện sức khỏe. Test giúp hệ thống phần mềm chạy đúng và phát triển bền vững.</p><p>Ta cần cung cấp đầy đủ unit test, integration test, end-to-end test.</p><h3 id="documentation">Documentation</h3><p>Những thay đổi về cách build, test, tương tác, logic xử lý quan trọng hay release code đều cần được cập tài liệu tương ứng.</p><h3 id="good-things">Good Things</h3><p>Code review không chỉ tập trung tìm lỗi mà ta cần tuyên dương và phổ biến những cách xử lý hay, hiệu quả (good practices) cho developer khác.<br>Đôi khi việc tuyên dương developer làm gì tốt còn đem lại nhiều giá trị hơn là chỉ cho họ làm gì chưa tốt.</p><h2 id="code-review-process-quy-tr%C3%ACnh-review-code">Code Review Process (Quy Trình Review Code)</h2><ol><li>Reviewee (developer) thực hiện xong feature hoặc fix bugs</li><li>Reviewee tạo merge request có tên nhánh theo format feature/create-user, feature/FC-4201, fix/update-user</li><li>Reviewee đảm bảo code compile, unit tests, automation tests thành công</li><li>Reviewee thông báo cho reviewer lập lịch code review</li><li>Reviewer lên checklist những đầu việc cần review theo những tiêu chí trên và những tiêu chí cần thiết cho features, bugs tại thời điểm đấy.</li><li>Reviewee đánh giá và thực hiện theo ý kiến thống của 2 bên</li><li>Reviewer review lại và duyệt merge request</li></ol><h2 id="attitude-while-reviewing-th%C3%A1i-%C4%91%E1%BB%99-khi-review">Attitude While Reviewing (Thái Độ Khi Review)</h2><ul><li><strong>Lịch sự</strong></li><li><strong>Đóng góp</strong></li><li>Học hỏi và giáo dục <strong>không chỉ trích, chê bai</strong></li></ul><h2 id="how-to-write-code-review-comments-vi%E1%BA%BFt-b%C3%ACnh-lu%E1%BA%ADn-nh%C6%B0-n%C3%A0o">How to Write Code Review Comments? (Viết Bình Luận Như Nào?)</h2><ul><li>Hãy lịch sự, tử tế. Tránh những bình luận mang tính chê bai khiến reviewee bực tức dẫn đến bất đồng quan điểm, khó tiếp thu, lắng nghe từ 2 phía, thậm chí gây mâu thuẫn nội bộ.<ul><li>Không nên: “Tại sao bạn dùng threads ở đây trong khi rõ ràng nó không đem lại lợi ích gì về hiệu năng”</li><li>Nên: “Xử lý đồng thời (concurrency) ở đây sẽ tăng độ phức tạp của hệ thống. Vì nó không giúp tăng hiệu năng của hệ thống. Thay vì dùng nhiều threads, bạn có thể dùng single thread cho đơn giản”</li></ul></li><li>Giải thích tại sao bạn đưa ra ý kiến đấy. Nó sẽ giúp reviewee hiểu rõ hơn và học được thêm kiến thức, kinh nghiệm.</li><li>Đưa ra hướng giải quyết. Không cần đưa ra một giải pháp cụ thể, chi tiết vì reviewee là người thực hiện feature hoặc fix bug. Họ có cơ hội để luyện tập và có thể đưa ra hướng giải quyết khác hay hơn</li><li>Gán nhãn mức độ nghiêm trọng của comment:<ul><li><strong>TODO</strong> (Critical): cần được xử lý ngay vì code changes ảnh hưởng tới codebase, security, … của hệ thống</li><li><strong>NIT</strong> (Nitpick): vấn đề thứ yếu. Bạn nên xử lý vấn đề đó nhưng nó không có ảnh hưởng lớn.</li><li><strong>OPTIONAL</strong>: “Tôi nghĩ cách này hay hơn nhưng nó không phải bắt buộc”</li><li><strong>FYI</strong> (For Your Information): “Tôi muốn bạn cần xử lý vấn đề này ở thời điểm này. Bạn có thể suy nghĩ cách xử lý sau“</li></ul></li></ul><h2 id="resolving-conflicts-gi%E1%BA%A3i-quy%E1%BA%BFt-tranh-lu%E1%BA%ADn">Resolving Conflicts (Giải Quyết Tranh Luận)</h2><ol><li>Lắng nghe. Cả 2 bên cần lắng nghe, hiểu rõ ý kiến của 2 bên</li><li>Phân tích ưu nhược điểm của giải pháp của 2 bên và những giải pháp khác</li><li>Phân tích ngữ cảnh, yêu cầu, tìm ra thứ tự ưu tiên tính chất của hệ thống (đặc biệt quan tâm tới tính mở rộng và bảo mật của hệ thống)</li><li>Chọn ra giải pháp phù hợp với yêu cầu trên</li><li>Nếu vẫn chưa chọn được giải pháp thì cần tham khảo ý kiến của:<ul><li>Cả team</li><li>Technical Lead</li><li>Head of Engineer</li></ul></li></ol><h2 id="best-practices">Best Practices</h2><ul><li>Sử dụng thông tin, kiến thức kỹ thuật và dữ liệu để loại <strong>bỏ quan điểm, ý kiến chủ quan chưa đúng đắn</strong>. Giải thích bằng cách đưa những <strong>nguyên tắc liên quan sẽ thuyết phục hơn</strong>. Ví dụ: SOLID, KISS, …</li><li><strong>Less is more</strong>. Khi ta tập trung vào 1 merge request quá lâu, độ hiệu quả sẽ giảm dần theo thời gian và có thể dẫn tới nhiều bugs hơn. <strong>Small Commit → Short Review → Better Code</strong>. Vậy bao nhiêu là small commit? Theo dữ liệu của dev Google: ~250 lines of code / CR</li><li><strong>Checklist</strong>. Để tránh thiếu sót khi review, ta cần có checklist. Ở thời điểm đầu, checklist có thể ít đầu việc, nhưng nó sẽ được bổ sung theo thời gian. Với mỗi feature, có thể có những đầu việc cần kiểm tra khác nhau.</li><li><strong>Leverage Tools</strong>. “A good developer is a lazy developer”. Sử dụng các Tool Static Code Analysis trước khi review, ví dụ như SonarQube, ...</li><li><strong>Tăng tốc độ review code</strong> giúp hạn chế không khí nặng nề của buổi review, hạn chế việc phàn nàn từ phía developers. Ngoài ra, tiết kiệm được thời gian của cả reviewer + reviewee</li></ul><hr><p>🏢 System Design VN: <a href="https://fb.com/groups/systemdesign.vn?ref=roninhub.com">https://fb.com/groups/systemdesign.vn</a></p><hr><h2 id="references">References</h2><ul><li><a href="https://www.evoketechnologies.com/blog/code-review-checklist-perform-effective-code-reviews/?ref=roninhub.com">https://www.evoketechnologies.com/blog/code-review-checklist-perform-effective-code-reviews/</a></li><li><a href="https://google.github.io/eng-practices/review/reviewer/?ref=roninhub.com">https://google.github.io/eng-practices/review/reviewer/</a></li><li><a href="https://medium.com/transparent-data-eng/good-practices-of-code-review-guide-for-beginners-8c084cd70be3?ref=roninhub.com">https://medium.com/transparent-data-eng/good-practices-of-code-review-guide-for-beginners-8c084cd70be3</a></li></ul>

The Goal of Code Review (Mục đích của Code Review)

Code Review là quá trình đánh giá code để đảm bảo code đạt được những tiêu chuẩn về chất lượng và thiết kế. Mục đích của việc code review:

Đảm bảo sản phẩm đạt chất lượng cao nhất.
Đảm bảo chất lượng của codebase được cải thiện theo thời gian.
Phát triển bản thân
- Soft Skills:
  - Reviewer và reviewee được luyện khả năng trình bày, giải thích ý tưởng làm sao cho người khác dễ hiểu
  - Học cách lắng nghe
- Hard Skills:
  - Học hỏi, chia sẻ kỹ thuật, kinh nghiệm chuyên môn

Code Review Points (Những Tiêu Chí Cần Review)

Functionality

Kiểm tra logic xử lý đã đúng yêu cầu chưa?
- Nếu là một tiến trình nhiều bước, đảm bảo số lượng các bước và thứ tự các bước là tối ưu nhất
- Cập nhật đúng trạng thái
Suy nghĩ như người dùng để tìm đủ corner cases, edge cases
Để ý những vấn đề về concurrency (deadlock, race conditions, ...)

Design

Code changes có làm thay đổi kiến trúc của code base không?
Code changes có tích hợp được phần còn lại của hệ thống không?
Đây có phải thời điểm để thêm tính năng này không?
Code changes có khả năng mở rộng về sau không?

Readability

Naming: một tên biến dễ hiểu là một tên biến đủ dài để thể hiện nó là gì và làm gì nhưng cũng không quá dài để đọc
Style: nên thống nhất một coding style
Comments: cần cô đọng, rõ ràng và chủ yếu giải thích vì sao (why) thay vì nó là gì (what)
Complexity: Khi developer viết đoạn code tổng quát hơn mức cần thiết, hoặc thêm 1 tính năng mà hiện tại hệ thống chưa cần tới nó. Như vậy nó làm tăng độ phức tạp cho hệ thống.
Duplication: logic của đoạn code thêm mới có trùng với function nào trước đó không?

Consistency

Sự thống nhất rất quan trọng, nhất là công việc về kỹ thuật. Sự thống nhất trong codebase giúp code dễ đọc hơn, tránh được bugs không đáng có, chuyển đổi dễ dàng hơn và tạo điều kiện cho làm việc nhóm thuận tiện hơn.

Nếu team có thống nhất về style guide, convention và codebase rõ ràng thì developer nên theo để tạo sự thống nhất.

Testing

Test rất quan trọng. Test như việc mình đi khám sức khỏe định kỳ. Nếu sức khoẻ của mình có vấn đề, bác sỹ sẽ có những khuyến cáo, điều chỉnh để mình cải thiện sức khỏe. Test giúp hệ thống phần mềm chạy đúng và phát triển bền vững.

Ta cần cung cấp đầy đủ unit test, integration test, end-to-end test.

Documentation

Những thay đổi về cách build, test, tương tác, logic xử lý quan trọng hay release code đều cần được cập tài liệu tương ứng.

Good Things

Code review không chỉ tập trung tìm lỗi mà ta cần tuyên dương và phổ biến những cách xử lý hay, hiệu quả (good practices) cho developer khác.
Đôi khi việc tuyên dương developer làm gì tốt còn đem lại nhiều giá trị hơn là chỉ cho họ làm gì chưa tốt.

Code Review Process (Quy Trình Review Code)

Reviewee (developer) thực hiện xong feature hoặc fix bugs
Reviewee tạo merge request có tên nhánh theo format feature/create-user, feature/FC-4201, fix/update-user
Reviewee đảm bảo code compile, unit tests, automation tests thành công
Reviewee thông báo cho reviewer lập lịch code review
Reviewer lên checklist những đầu việc cần review theo những tiêu chí trên và những tiêu chí cần thiết cho features, bugs tại thời điểm đấy.
Reviewee đánh giá và thực hiện theo ý kiến thống của 2 bên
Reviewer review lại và duyệt merge request

Attitude While Reviewing (Thái Độ Khi Review)

Lịch sự
Đóng góp
Học hỏi và giáo dục không chỉ trích, chê bai

How to Write Code Review Comments? (Viết Bình Luận Như Nào?)

Hãy lịch sự, tử tế. Tránh những bình luận mang tính chê bai khiến reviewee bực tức dẫn đến bất đồng quan điểm, khó tiếp thu, lắng nghe từ 2 phía, thậm chí gây mâu thuẫn nội bộ.
- Không nên: “Tại sao bạn dùng threads ở đây trong khi rõ ràng nó không đem lại lợi ích gì về hiệu năng”
- Nên: “Xử lý đồng thời (concurrency) ở đây sẽ tăng độ phức tạp của hệ thống. Vì nó không giúp tăng hiệu năng của hệ thống. Thay vì dùng nhiều threads, bạn có thể dùng single thread cho đơn giản”
Giải thích tại sao bạn đưa ra ý kiến đấy. Nó sẽ giúp reviewee hiểu rõ hơn và học được thêm kiến thức, kinh nghiệm.
Đưa ra hướng giải quyết. Không cần đưa ra một giải pháp cụ thể, chi tiết vì reviewee là người thực hiện feature hoặc fix bug. Họ có cơ hội để luyện tập và có thể đưa ra hướng giải quyết khác hay hơn
Gán nhãn mức độ nghiêm trọng của comment:
- TODO (Critical): cần được xử lý ngay vì code changes ảnh hưởng tới codebase, security, … của hệ thống
- NIT (Nitpick): vấn đề thứ yếu. Bạn nên xử lý vấn đề đó nhưng nó không có ảnh hưởng lớn.
- OPTIONAL: “Tôi nghĩ cách này hay hơn nhưng nó không phải bắt buộc”
- FYI (For Your Information): “Tôi muốn bạn cần xử lý vấn đề này ở thời điểm này. Bạn có thể suy nghĩ cách xử lý sau“

Resolving Conflicts (Giải Quyết Tranh Luận)

Lắng nghe. Cả 2 bên cần lắng nghe, hiểu rõ ý kiến của 2 bên
Phân tích ưu nhược điểm của giải pháp của 2 bên và những giải pháp khác
Phân tích ngữ cảnh, yêu cầu, tìm ra thứ tự ưu tiên tính chất của hệ thống (đặc biệt quan tâm tới tính mở rộng và bảo mật của hệ thống)
Chọn ra giải pháp phù hợp với yêu cầu trên
Nếu vẫn chưa chọn được giải pháp thì cần tham khảo ý kiến của:
- Cả team
- Technical Lead
- Head of Engineer

Best Practices

Sử dụng thông tin, kiến thức kỹ thuật và dữ liệu để loại bỏ quan điểm, ý kiến chủ quan chưa đúng đắn. Giải thích bằng cách đưa những nguyên tắc liên quan sẽ thuyết phục hơn. Ví dụ: SOLID, KISS, …
Less is more. Khi ta tập trung vào 1 merge request quá lâu, độ hiệu quả sẽ giảm dần theo thời gian và có thể dẫn tới nhiều bugs hơn. Small Commit → Short Review → Better Code. Vậy bao nhiêu là small commit? Theo dữ liệu của dev Google: ~250 lines of code / CR
Checklist. Để tránh thiếu sót khi review, ta cần có checklist. Ở thời điểm đầu, checklist có thể ít đầu việc, nhưng nó sẽ được bổ sung theo thời gian. Với mỗi feature, có thể có những đầu việc cần kiểm tra khác nhau.
Leverage Tools. “A good developer is a lazy developer”. Sử dụng các Tool Static Code Analysis trước khi review, ví dụ như SonarQube, ...
Tăng tốc độ review code giúp hạn chế không khí nặng nề của buổi review, hạn chế việc phàn nàn từ phía developers. Ngoài ra, tiết kiệm được thời gian của cả reviewer + reviewee

🏢 System Design VN: https://fb.com/groups/systemdesign.vn

References

java

beginner