Tôi tìm gì khi review code

Năm đầu tiên đi review pull request, tôi để lại những bình luận về cách đặt tên và định dạng, vì đó là những thứ tôi nhìn thấy. Chúng cũng là những thứ mà một bộ định dạng đáng lẽ phải lo, và những lượt review ấy chẳng bắt được con bug nào.

Đây là thứ tự tôi đi bây giờ.

1. Nó có làm đúng thứ phần mô tả nói không

Trước khi đọc bất kỳ dòng code nào: hãy đọc phần mô tả, rồi kiểm tra xem cái diff có làm đúng điều đó không.

Những sai sót việc này bắt được thì chẳng tinh vi gì. Một PR mang tên “sửa cú sập ở trạng thái rỗng” mà cũng tiện tay tái cấu trúc luôn tầng mạng. Một bản sửa lỗi làm đổi hợp đồng API. Một tính năng lặng lẽ xóa đi một hành vi mà người khác đang phụ thuộc vào.

Nếu phần mô tả không nói thay đổi này để làm gì thì đó là bình luận đầu tiên, và mọi thứ khác phải chờ. Tôi không review được một thay đổi mà tôi đang phải đoán mục đích, và người đọc nó sau một năm cũng vậy.

2. Chuyện gì xảy ra khi nó hỏng

Đường đi thuận lợi thường đã đúng — đó là đường mà tác giả đã chạy thử. Những câu hỏi thú vị đều nằm ở các đường còn lại:

• Nếu mất mạng thì sao? Không phải mạng chậm — mà là mất hẳn. Bật chế độ máy bay giữa lúc đang gửi request.
• Nếu mảng này rỗng thì sao? Phần tử đầu, phần tử cuối, chỉ số không, phép chia cho số lượng.
• Nếu cái này chạy hai lần thì sao? Một cú bấm đúp, một lần thử lại, một thông báo đến giữa lúc đang thử lại.
• Nếu người dùng rời màn hình giữa chừng thao tác thì sao? Công việc có bị hủy không, và cái completion handler có còn ghi vào một thứ đã bị hủy không?
• Nếu cái optional này là nil trong môi trường thật nhưng không bao giờ nil trong dữ liệu test của bạn thì sao?

Phần lớn các con bug thật tôi bắt được khi review đều là một trong năm cái đó, và tất cả đều hỏi được mà không cần hiểu sâu về tính năng.

3. Mô hình trạng thái có đúng không

Đây là thứ có đòn bẩy cao nhất khi làm đúng và khó đổi nhất về sau, nên nó đáng được dành thời gian review.

Cụ thể thì tôi tìm những trạng thái biểu diễn được nhưng lẽ ra không nên tồn tại:

struct ViewState {
    var isLoading: Bool
    var items: [Item]?
    var error: Error?
}

Tám tổ hợp, trong đó ba cái có nghĩa. isLoading == true kèm một error khác nil là biểu diễn được, nên ở đâu đó có một nhánh xử lý nó, hoặc là không có và rồi nó xảy ra trong môi trường thật.

enum ViewState {
    case loading
    case loaded([Item])
    case failed(Error)
}

Ba trạng thái, tất cả đều có nghĩa, và trình biên dịch bảo đảm mọi switch xử lý hết. Đề xuất điều này khi review là một thay đổi thật để yêu cầu, và nó đáng được yêu cầu, vì cái thay thế là một lớp lỗi tồn tại suốt vòng đời của màn hình đó.

4. Một năm nữa cái này có đọc được không

Không phải “cái này có khôn ngoan không” — khôn ngoan thường mới là vấn đề. Các câu hỏi:

• Một người chưa đọc cái ticket có hiểu vì sao thứ này ở đây không? Nếu lý do không hiển nhiên thì nó cần một dòng chú thích, và chú thích nên nói vì sao chứ đừng nói cái gì.
• Các cái tên có lương thiện không? Một hàm tên validate mà cũng lưu dữ liệu luôn là một con bug đang chờ ai đó gọi nó với kỳ vọng nó chỉ kiểm tra.
• Có dòng chú thích nào giải thích code làm gì không? Hãy xóa nó đi và làm cho code tự nói điều đó.
• Có code bị comment lại không? Xóa đi. Git nhớ hộ rồi.

Thứ tôi cố ý không bình luận

Bất cứ thứ gì một bộ định dạng hay một cái linter nên bắt được. Nếu tôi đang bình luận về khoảng trắng thì dự án cần một bộ định dạng, và đó là một cuộc trò chuyện riêng với cả đội chứ không phải một dòng ghi chú trên PR của ai đó.

Sở thích phong cách. guard hay if let, cái else đặt ở đâu, có dùng trailing closure hay không. Nếu cả hai dạng đều đọc được thì lựa chọn của tác giả thắng. Mỗi bình luận kiểu này tốn thiện chí và chẳng mua được gì.

Những thứ tôi sẽ làm khác đi nhưng không phải là tệ hơn. Đây là kỷ luật tôi mất lâu nhất để học được. “Chỗ này tôi sẽ dùng dictionary” không phải một bình luận review, trừ khi cái mảng đó thật sự là vấn đề.

Tôi viết bình luận thế nào

Hỏi thay vì ra lệnh, ở chỗ tôi không chắc. “Nếu items rỗng ở đây thì chuyện gì xảy ra?” cho kết quả tốt hơn “chỗ này sẽ sập khi rỗng”, một phần vì tôi sai chừng một phần ba số lần, và câu hỏi thì không bắt ai phải mất mặt.

Nói rõ bình luận nào là chặn merge. Tôi thêm tiền tố nit: hay non-blocking: cho những cái tùy chọn. Không có nó, tác giả phải đoán xem một ghi chú về phong cách có phải điều kiện để merge không, và họ thường đoán là có.

Duyệt kèm bình luận khi chẳng có gì chặn cả. Giữ một PR lại vì ba chuyện vặt thì phí một ngày và dạy người ta rằng review là một chướng ngại.

Điều tôi cố nhớ

Một lượt review là về đoạn code, và nó được đọc bởi một con người đã bỏ ra một ngày để viết nó. Duyệt một thứ có một khiếm khuyết nhỏ gần như luôn rẻ hơn một vòng review qua lại khiến ai đó cảm thấy công sức của mình bị soi mói — và cái khiếm khuyết nhỏ ấy có thể được sửa ở PR tiếp theo, bởi một trong hai chúng tôi, trong hai phút.