Mục đích chính của code review là tìm ra phần mã khó bảo trì

• Code review không hẳn là một quy trình để bắt lỗi hay bảo đảm tính toàn vẹn, mà gần với quá trình sớm làm lộ ra những đoạn mã khó bảo trì về sau
• Nếu reviewer đọc mã mà thấy khó hiểu, rất có khả năng người bảo trì trong tương lai cũng sẽ gặp cùng vấn đề
• Tốt hơn nên sửa ngay lúc tác giả ban đầu vẫn còn nhớ bối cảnh; kỳ vọng rằng chỉ kiểm tra mã là có thể tìm lỗi một cách ổn định là không thực tế
• Với reviewer, “hãy thử hiểu và đánh dấu những chỗ không hiểu” là nhiệm vụ khả thi hơn “hãy tìm lỗi”
• Một review tốt không bắt đầu từ việc chứng minh mọi thứ hoàn hảo, mà từ việc để lại ghi chú và yêu cầu cải thiện ở những điểm chưa hiểu được

---

Thay đổi trọng tâm của code review

• Mục đích cốt lõi của code review không phải là để reviewer tìm lỗi, cũng không phải để bảo chứng rằng mã không có bug
• Kỳ vọng rằng chỉ lướt qua mã là có thể tìm ra bug nói chung là yếu trong thực tế
• Vì vậy, trọng tâm của review được đặt vào “người khác sau này có thể đọc và sửa được không” hơn là “đây có phải là mã đúng không”

Mã khó hiểu là tín hiệu rủi ro về bảo trì

• Reviewer đọc mã để cố hiểu mã đang làm gì và hoạt động như thế nào
• Những phần không hiểu được trở thành tín hiệu rủi ro cho thấy người bảo trì trong tương lai có thể bị mắc kẹt
• Với loại mã này, tốt hơn nên sửa ngay khi tác giả ban đầu vẫn còn nhớ bối cảnh

Nhiệm vụ review khả thi hơn việc tìm bug

• Yêu cầu “hãy tìm bug trong đoạn mã này” là một công việc khó đánh giá thành công hay thất bại
• Dù tìm được vài bug, vẫn có thể đã bỏ sót các bug khác còn ẩn, nên với reviewer, điều dễ trở nên rõ ràng chỉ là thất bại
• Ngược lại, yêu cầu “hãy thử hiểu đoạn mã này, và nếu không hiểu được thì chỉ ra” có tiêu chí rõ ràng hơn
  • Không cần hiểu hoàn hảo mọi thứ
  • Chỉ cần ghi lại những phần không hiểu
  • Nếu đã cố hiểu toàn bộ và để lại ghi chú tại những điểm bị mắc, tức là đã thực hiện vai trò của review

Tiêu chí review trong thực tế

• Mã mà reviewer không hiểu tự thân nó có thể trở thành đối tượng cần sửa
• Comment review không chỉ là báo cáo bug, mà còn có vai trò làm lộ ra phần thiếu giải thích, vấn đề cấu trúc và luồng xử lý khó đọc
• Theo tiêu chí này, điều quan trọng hơn việc chứng minh tính đúng đắn của mã là đưa mã về trạng thái mà các thành viên trong nhóm sau này có thể đọc và xử lý được

Phát hiện bug là tác dụng phụ

• Điều này không có nghĩa là code review hoàn toàn không tìm được bug
• Bug có thể được phát hiện trong quá trình review, nhưng khó kỳ vọng code review là phương pháp để tìm tất cả hoặc phần lớn bug
• Điều kiện thành công thực tế hơn là kiểm tra khả năng hiểu được của mã, và cùng tác giả ban đầu cải thiện ngay những phần khó bảo trì

[GN⁺]

Các ý kiến trên Hacker News

• Review code không chỉ có một mục đích duy nhất. Tìm ra code khó bảo trì là quan trọng, nhưng đó không phải mục đích duy nhất, và có lẽ cũng không phải mục đích quan trọng nhất
  Nó đóng vai trò như một cơ chế an toàn khiến việc merge mã độc trở nên khó hơn, ngay cả khi lập trình viên hay AI đi theo hướng kỳ lạ; đồng thời người không ở quá sát vấn đề có thể nhìn thấy cách làm tốt hơn hoặc những vấn đề bị bỏ sót
  Người hiểu rõ hơn các phần khác của hệ thống cũng có thể phát hiện vấn đề tương tác, việc review khiến ít nhất một người nữa trở nên quen thuộc với đoạn code đó, và là cơ hội học hỏi cho cả tác giả lẫn reviewer
  Điều này đặc biệt quan trọng khi có chênh lệch kinh nghiệm: khi mentoring nhân viên mới, tôi thêm họ làm reviewer cho mọi PR của mình để họ thấy cách tôi làm việc, và cũng review PR của họ để định hướng. Thỉnh thoảng tôi cũng học được từ họ
  Bắt bug cũng đúng, nhưng không nên là cơ chế chính; nó đặc biệt quan trọng với bug bảo mật và hiệu năng, vốn khó bắt bằng kiểm thử tự động

  • Thêm một lý do cũ nữa: mọi người viết code khác đi khi họ biết code của mình sẽ được review và từ code đó sẽ hình thành ấn tượng dài hạn về năng lực và mức độ phù hợp với tổ chức của người gửi
  • Tương tự điểm số 3, người quen thuộc hơn với subdomain đó có thể phát hiện những chỗ không khớp với code hiện có hoặc có khả năng gây vấn đề
  • Tôi hiểu “mục đích duy nhất” theo nghĩa là hãy hiểu code và nêu vấn đề với những phần không hiểu được. Nếu ý là sau khi hiểu rồi thì có thể chỉ ra những chỗ sai, ngớ ngẩn hoặc không an toàn, thì tôi thấy hợp lý
    Đặc biệt là với module hóa và phân rã, khi đã hiểu toàn bộ một PR khổng lồ, bạn sẽ có một mô hình trong đầu và bắt đầu thấy liệu nó có thể bảo trì được không, một ngày nào đó có trở thành cơn ác mộng hoàn toàn không, hay nằm đâu đó ở giữa

• Tôi cho rằng có lẽ phần quan trọng nhất của review code là chuyển giao kiến thức
  Ở team nhỏ của chúng tôi, trừ trường hợp gấp, cả team đều đánh dấu approve PR trước khi merge, nhờ vậy mọi thành viên đều nắm đại khái trạng thái hiện tại của codebase. Điều này giúp tránh những cú bất ngờ kiểu “toàn bộ hệ thống mà tôi phụ thuộc vào đã biến mất” như tôi từng gặp ở các công ty bị silo hóa hơn trước đây
  Nó cũng là nơi để đặt câu hỏi về cách hoạt động và tăng mức độ hiểu biết. Trong một team vận hành tốt, mọi developer nên hiểu ở mức nào đó toàn bộ hệ thống, kể cả những phần họ không trực tiếp đụng vào
  Một chức năng quan trọng khác là kiểm tra kiến thức tổ chức. Gần đây tôi thay đổi một bảng một chút, và đồng nghiệp cho biết một microservice mà tôi chưa tính đến đang ghi vào bảng đó nên sẽ bị hỏng. Tôi thậm chí không biết microservice đó tồn tại, cũng không biết nó truy cập bảng đó, nhưng nhờ lần kiểm tra đó mà tránh được một vấn đề lớn hơn và tình huống phải dọn dữ liệu

  • Việc “cả team nhỏ đánh dấu approve PR trước khi merge” có vẻ tốt với một codebase nhỏ di chuyển chậm, nhưng nếu áp đặt cho team lớn hoặc codebase thay đổi nhanh, nó dễ trở thành một nghi thức hình thức, nơi mọi người lướt qua code qua loa rồi bấm nút approve
    Cuối cùng ai cũng bận việc của mình và không muốn trở thành người chặn một PR quan trọng, nên chỉ bấm approve; thực tế thì không ai thật sự review code
  • Tôi tò mò team đó có quy mô bao nhiêu. Có lẽ nếu vượt quá năm kỹ sư thì cách đó sẽ không mở rộng được
    Với các vấn đề như “toàn bộ hệ thống mà tôi phụ thuộc vào đã biến mất”, tôi nghĩ kiểm thử tự động có thể bắt được ngay cả khi người phụ thuộc vào hệ thống đó không có mặt là cực kỳ quan trọng
    Tôi cũng rất ủng hộ quyền sở hữu chung đối với mọi thứ. Việc mọi người trên thực tế sở hữu một phần cụ thể của codebase, đặc biệt là component do chính họ tạo ra, là điều tự nhiên, nhưng nó dẫn đến silo và bus factor thấp. Không nên có cấu trúc trong đó một người sở hữu một hệ thống, rồi hệ thống đó lại phụ thuộc vào một component do một người khác sở hữu
  • Tôi tò mò nếu đồng nghiệp đó không có trong review thì điều gì sẽ ngăn thay đổi gây hỏng đó được đưa lên production. Nếu review code quan trọng, thì test nằm ở đâu
  • Đôi khi tôi vẫn tạo PR và tag developer khác, dù code đó kiểu gì cũng sẽ được merge ngay. Mục đích là cung cấp một đường dẫn dễ xem để biết cái gì đã được merge và vì sao, giúp mọi người không bỏ lỡ nội dung bên trong codebase
  • Với các thay đổi hoặc công việc dọn dẹp không tầm thường, luôn nên có thêm một cặp mắt thứ hai. Nhưng cách “mọi người đọc mọi thứ” không thể mở rộng với N lớn
    Khi có quá nhiều thứ phải đọc, không ai theo kịp được, nên ta phải ủy quyền, viết tài liệu và tổ chức phiên tổng quan

• Tôi luôn nghĩ tốt nhất nên xem review code là cánh cổng nơi code từng thuộc sở hữu của tác giả chuyển sang sở hữu của team hoặc dự án
  Code tôi review không còn là code của bạn, mà sắp trở thành code của chúng ta. Đương nhiên khả năng bảo trì là một yếu tố lớn trong đó

  • Thật sự là một môi trường đáng ghen tị và xa xỉ
    Team chúng tôi bắt đầu dùng AI, nên tôi đã đổi sang cách đơn giản hơn. Tôi không để lại comment, chỉ đánh giá approve theo kiểu nhị phân: “thứ này có điên rồ quá mức không, hay có thể cho qua”
    Tôi đang tiết kiệm thời gian và sức khỏe tinh thần của mình

• Làm vậy chỉ khiến cả reviewer lẫn tác giả trở nên lười hơn
  Mục đích của code review là đa diện. Nó bao gồm việc xem code có khó bảo trì không, có thể có bug không, có thể làm đơn giản và gọn gàng hơn không, có phù hợp với style code của dự án không, có giúp người khác hiểu code không, có onboard thành viên junior không, có sanity check các quyết định thiết kế không
  Những bài viết nhẹ kiểu này nhìn chung gần với việc reviewer code lười biếng tự biện minh cho mình hơn

  • Có một checklist riêng cần xem trong review
    Cần xem liệu về mặt chức năng nó có đạt mục tiêu như mô tả trong issue hay PR không, có code thừa như output debug còn sót hoặc API key riêng tư không, có lỗi hiển nhiên như rò rỉ bộ nhớ, edge case chưa xử lý, lỗ hổng bảo mật, lời gọi API lỗi thời không
    Cũng cần xem có thể làm cho dễ hiểu hơn không, nên thêm hay bỏ abstraction, tên biến/phương thức có tốt hơn không, dùng nhiều hơn hay ít hơn phong cách functional thì có tốt hơn không
    Cần kiểm tra liệu có nhất quán với codebase hoặc style guide không, có cải thiện hiệu năng rõ ràng như dùng hash set thay vì list hoặc áp dụng lazy evaluation không, test đã đủ chưa
    Tôi cũng không hoàn toàn đồng ý với quan điểm rằng code không hiểu được thì không nên cho qua. Có những đoạn code đơn giản là thật sự rất khó hiểu, và mục tiêu là làm cho nó đúng về chức năng đồng thời dễ hiểu nhất có thể
  • Ngành này đã nỗ lực rất nhiều để chuyển từ “đổ lỗi cho tác giả” sang “đổ lỗi cho quy trình và đội ngũ”
    Nhưng bài này gần như chỉ là mồi câu, giống như nói “mọi người nghĩ bữa tối là để ăn thức ăn, nhưng thực ra không phải là ăn mà là kết nối với gia đình và bạn bè”. Đây là một kiểu lập luận quy giản lỏng lẻo rất ăn khách trên HN
  • Giờ đây khi AI làm tốc độ viết và triển khai code nhanh hơn, trọng tâm nên chuyển sang review. Cần xác nhận code có thực sự chạy đúng không, mọi giả định của chúng ta có đúng không, có tác dụng phụ ngoài ý muốn nào không
    Tôi cảm thấy review và debugging tốn thời gian hơn rất nhiều so với việc viết/sản xuất code, và chỉ “cầu cho nó chạy” thì chưa bao giờ kết thúc tốt đẹp

• Câu “nhìn vào code thì nói chung không thể tìm được bug” hoàn toàn không đúng. Ở mỗi mức abstraction đều có thể tìm được rất nhiều, và những thứ đó được gọi là code smell
  File descriptor không được đóng, coroutine không được await, các khối try/catch khổng lồ trả về một giá trị nào đó mà không ghi log lỗi, ép kiểu sai, v.v. đều thuộc loại này
  Như một nguyên tắc chung, không nên xem type checker, compiler, runtime chỉ là các bước phải vượt qua. Cần làm việc cùng các bước này, thừa nhận chúng là công cụ có giá trị, và không nên đi ngược lại chúng

  • Tôi không hiểu đang nói gì. Tôi từng bắt được bug chỉ bằng code review mà không chạy code, và ngược lại người khác cũng từng làm vậy với code của tôi, tôi cũng đã thấy điều đó trong review của người khác
    Có thể định nghĩa “nói chung” theo cách nào đó để nó đúng về mặt kỹ thuật, nhưng như vậy thì gần như chẳng còn ý nghĩa gì

• Nếu muốn đưa ra một tuyên bố rộng về code review, điều quan trọng là phải định nghĩa đang nói đến loại code review nào
  Hiện nay review PR bất đồng bộ kiểu GitHub và comment inline là chuẩn, nhưng review không chỉ có vậy. Trước đây từng có những quy trình review trực tiếp gần giống phản biện luận văn hoặc thuyết trình hội nghị
  Các tài liệu nói rằng code review là một thực hành chất lượng hữu ích, thực ra là một trong số ít thực hành chất lượng hữu ích, phần lớn xuất phát từ các quy trình review có cấu trúc chặt chẽ hơn nhiều so với hiện nay
  Cá nhân tôi thấy review PR kiểu GitHub trước thời LLM chủ yếu là để khiến người ta cảm thấy quy trình ổn hoặc để tick vào checkbox quản trị, còn trong thời đại LLM thì hiệu quả so với chi phí trở nên tệ hơn nhiều và có lẽ sẽ biến mất

  • Review đồng bộ hiện vẫn khả thi. Một trong những quản lý thời đầu của tôi đã dạy rằng nếu code review “tiêu chuẩn” qua lại hơn một lần, thì hầu như lúc nào cũng tốt hơn nếu gặp trực tiếp, hoặc nếu có ít nhất một người làm từ xa thì họp Zoom để chốt, rồi để lại nội dung đã thống nhất trong comment
    Nếu dùng một ẩn dụ kỹ thuật hơi gượng ép, giao tiếp văn bản bất đồng bộ bị mất mát nhiều hơn lời nói về lượng thông tin có thể mã hóa thành công, và throughput cũng thấp hơn. Vì vậy khi cần trao đổi nhiều thông tin, đáng để trả chi phí đồng bộ hóa
  • Ở một trong những công việc đầu tiên của tôi, chúng tôi phải in gói thay đổi ra để review và ký tên. Còn có người phụ trách lưu bản cuối trong tủ hồ sơ
    Nó gần với kỹ thuật truyền thống hơn, và mọi người đều phải nghĩ về phần mềm như một thứ lâu bền hơn

• Đảm bảo khả năng bảo trì đúng là một trong những lợi ích của code review, nhưng nói đó là mục đích duy nhất thì là một tuyên bố khá táo bạo
  Code review cũng là công cụ giúp đội ngũ nắm được các thay đổi code và chia sẻ trách nhiệm chung đối với toàn bộ codebase

• Code review không phải là một thứ đơn nhất. Có nhiều lý do như chia sẻ kiến thức, rửa trách nhiệm, chất lượng code, tuân thủ quy định, và như mọi khi, mục đích của nó phụ thuộc vào ngữ cảnh sử dụng

  • Nói rằng hầu hết mọi người không hiểu lý do của peer review nghe khá thiếu hiểu biết. Bản thân việc tin rằng chỉ có một mục đích duy nhất có vẻ là dấu hiệu cho thấy thiếu kinh nghiệm làm việc với các đội ngũ hoặc con người khác

• Nếu tác giả là nhà toán học, câu “nhìn vào code thì nói chung không thể tìm được bug” có lẽ không có nghĩa là việc tìm bug hoàn toàn bất khả thi
  Nó gần với ý rằng không thể tìm được mọi bug hoặc một bug cụ thể hơn

  • Dựa trên kinh nghiệm học các lớp toán ở đại học, các nhà toán học đôi khi giao tiếp với người khác rất kém. Điều đó giải thích vì sao họ nghĩ điều mình nói và cách gần như mọi người đọc nó lại khác nhau
  • Nếu là nghĩa đó thì đúng
    Nối với luận điểm về khả năng bảo trì, có thể nói thêm rằng một mục tiêu của review cũng là làm code đơn giản nhất có thể để tăng xác suất debug được bằng review. Nó không ngăn bug theo nghĩa tuyệt đối, nhưng làm tăng xác suất
  • Có vẻ tác giả là nhà toán học nhưng lại không hiểu ý nghĩa của lượng từ trong ngôn ngữ tự nhiên của mình. “Nhìn vào code thì nói chung không thể tìm được bug” nghĩa là “nhìn vào code thì nói chung không thể tìm được bất kỳ bug nào”, chứ không phải “không thể tìm được mọi bug”
    Cách hiểu thứ nhất có liên quan nhưng sai, cách hiểu thứ hai đúng nhưng không liên quan
    Có lẽ tác giả muốn nói “với một bug đã cho, nói chung không thể tìm được nó bằng cách nhìn vào code”, tức “không phải với mọi bug B đều có thể tìm được B”, nhưng điều này cũng đúng mà lại xa rời trọng tâm

• Tôi từng làm việc cùng cả những người thường xuyên từ chối đề xuất trong PR, lẫn những người chấp nhận đề xuất
  Với người chấp nhận đề xuất, tôi bắt đầu nghĩ rằng họ đang sẵn sàng chia sẻ với tôi một phần quyền sở hữu ở mức nào đó. Có cảm giác cả hai cùng bảo trì và hiểu đoạn mã, cùng nhìn về một hướng
  Ngược lại, với PR của người hay từ chối đề xuất trong PR, ý muốn tham gia của tôi giảm đi. Nếu đằng nào cũng bị từ chối thì tại sao phải bỏ thời gian review thật kỹ?

  • Nhóm chúng tôi thường thêm tiền tố trước comment như thought, change, nit, fix, chat
    Ví dụ thought là kiểu “sau này foo có thể trở nên phổ biến hơn, khi đó hãy refactor”, change là “đây là một abstraction bị rò rỉ nên tôi muốn mô hình hóa như bar”, nit là “tên này chưa trực quan lắm, hãy cân nhắc Baz hoặc Boo”, fix là “unit test này đang kiểm tra nhầm field”, chat là “đây là quyết định lớn sẽ định hình cách giải cho nhóm vấn đề này về sau, nên hãy thảo luận với team trước”
    Một số tiền tố có nghĩa là PR sẽ bị dừng cho đến khi sửa, còn một số chỉ là comment có thể chấp nhận hoặc không. Nhờ vậy tác giả thấy rõ đâu là thứ “cần đạt được cùng một cách hiểu”, đâu là “cách diễn đạt theo sở thích” hoặc “quan sát”
    Tuy nhiên, nếu đã để lại nit mà đối phương không đồng ý và bỏ qua thì cũng không nên phật ý. Nếu bạn cảm thấy rất mạnh mẽ về nó, lẽ ra đó không nên là nit
  • Nếu cho rằng điều đó quan trọng, hãy để lại đề xuất mang tính chặn và buộc phải có trao đổi.