Những lượt review mình làm dở đều giống nhau ở một điểm: mở diff ra khi chưa quyết định mình đang đọc nó để làm gì, rồi gõ bất cứ thứ gì nảy ra trong đầu, dòng này qua dòng khác. Sáu người cùng merge vào một nhánh chung biến cái thói quen đó thành khoản thuế cả team trả. Đây là thứ mình chốt rằng một lượt review là để làm, và — quan trọng hơn — không phải để làm.
Ba thứ mình đang kiểm
Tính đúng, không phải "code này có đẹp không" mà "cái này có làm đúng thứ nó tự nhận, trong những điều kiện mà lúc viết tác giả không ngồi trong đó không". Tác giả đã chạy đường đẹp rồi; phần còn lại là của người review.
Ranh giới — thay đổi này có làm xê dịch những gì phần còn lại của hệ thống được phép giả định hay không. Cái engine duyệt mình sở hữu chỉ có đúng một class gateway public để đi vào; module khác gọi vào đó và không gọi gì khác. Một comment kiểu gọi qua gateway, đừng gọi thẳng service bên dưới trông như góp ý style nhưng không phải: đó là khác biệt giữa một cửa vào và bốn cửa vào. Mình viết nó vài lần; sau đó ranh giới thành một rule của lint và không ai phải viết lại nữa.
Người đọc sáu tháng nữa, mà thường chính là tác giả và thường đang trực. Ở lượt review, mình là người ngoài đầu tiên đọc đoạn code này. Nếu mình phải dựng lại từ diff xem nhánh này sinh ra để làm gì, thì người cầm máy trực cũng sẽ phải dựng lại y hệt, lúc 3 giờ sáng, với một đối tác đang chờ.
Nó không phải để làm gì
Không phải để viết lại code của tác giả trong đầu mình rồi chấm điểm khoảng cách. Lực kéo này rất mạnh, nhất là khi có cái chức danh: mình sẽ tổ chức khác đi, nên đây là mười một comment dắt tác giả về phía cấu trúc của mình. Thứ chui ra là đoạn code hình dạng của một sự thoả hiệp giữa hai mô hình trong đầu và không khớp với cái nào — và người phải debug nó lúc 3 giờ sáng thường là người có mô hình vừa thua. Code mình sẽ không viết như vậy vẫn có quyền tồn tại, nếu nó đúng, nằm trong ranh giới của nó, và đọc được.
Nó cũng không phải chỗ để lôi thiết kế ra cãi lại. Nếu hình dạng đã sai thì không lượng comment theo dòng nào cứu được, và bốn mươi cái note kéo một nhánh đã viết xong về một hình dạng khác là cách đắt nhất để nói một câu chuyện mười phút. Nên đôi khi nước đi mạnh nhất của một lượt review là dừng sớm: đây là câu hỏi thiết kế, không phải câu hỏi diff, mang nó ra khỏi diff đi. Còn khi mình thấy mình viết đúng một comment chặn trên hai nhánh khác nhau, đó không phải vấn đề của review — đó là cuộc nói chuyện thiết kế mình còn nợ team từ trước khi hai nhánh đó tồn tại.
Mỗi comment tự nói nó có chặn hay không
Mỗi comment mở đầu bằng một từ và một dấu hai chấm. Chặn: có thứ sai, mình sẽ không approve tới khi nó đổi. Ý thích: mình sẽ làm cách khác, nghe hay không tuỳ bạn, kiểu gì mình cũng approve. Hỏi: mình chưa hiểu chỗ này, và câu trả lời có thể là mình đang sai.
Cái tiền tố đó gánh nhiều việc hơn toàn bộ phần đứng sau nó. Hồi chưa có nó, mọi người coi mọi thứ mình viết là bắt buộc — mình là lead, mà một comment không nhãn từ lead thì đọc ra thành một mệnh lệnh — nên có người sửa theo thứ họ không đồng ý, và vài chỗ sửa xong tệ hơn chỗ cũ. Ba comment chặn cộng mười một ý thích là một thông điệp hoàn toàn khác với mười bốn cái comment, mà tác giả không thể biết mình nhận cái nào nếu mình không viết ra.
Luật mình tự áp cho mình: nếu không gọi tên được cái gì vỡ thì nó không phải comment chặn. "Nhìn mong manh lắm" chỉ là ý thích, cho tới khi mình viết ra được cái input làm nó gãy.
Review theo repo, không theo thói quen của mình
Repo nào cũng có quan điểm riêng, và trên một nền tảng khoảng 150 supplier code trên chừng 90 tích hợp thì mấy quan điểm đó chịu lực. Một module supplier viết theo một contract cố định và đăng ký bằng code của nó trong một registry duy nhất; có đúng hai pattern offer được ghi thành tài liệu và không có cái thứ ba. Nên câu hỏi review đầu tiên cho một tích hợp mới là cái này là pattern nào, và code có nói ra điều đó không — chứ không phải mình có thích cách đặt tên biến hay không. Một quy ước mình hơi không ưa nhưng áp nhất quán vẫn hơn style mình thích áp trong một file: chính sự nhất quán làm cho một người mở một module chưa từng đọc vẫn đoán đúng.
Khi mình thật sự muốn đổi quy ước, nó đi thành một merge request riêng, kèm lập luận viết ra. Nhét style của mình vào một lượt review tính năng là cách để repo có ba style và không còn dấu vết ai chọn cái nào.
Bốn câu hỏi
Hỏi với mọi thay đổi đụng tới tiền, tới state được lưu, hoặc tới supplier:
Retry thì sao? Không phải tick ô "có idempotent không" — mà cụ thể chuyện gì xảy ra nếu cái này chạy hai lần. Đồng hồ SLA từng cấp trong engine duyệt là hai delayed job dùng thuần như đồng hồ: job nổ ra thì đọc lại database rồi mới quyết, nên một lần nổ muộn là no-op và không có gì phải đi huỷ. Tính chất đó có được vì câu hỏi này được hỏi trước khi code được viết.
Trả về rỗng thì sao? Kết quả rỗng không ném exception, không bắn alert, và trông y hệt một câu trả lời — cả một region từng ra 0 hotel trong khi mọi service trong chuỗi đều báo 200.
Còn ai đọc cái state này? Một chỗ ghi ở đây là một chỗ đọc ở nơi mình không nhìn tới. Các dòng bảo hiểm phải khớp với chặng bay và với tổng tiền của đơn qua vài service, và một thay đổi đúng cục bộ ở một trong số đó là cách kinh điển làm vỡ sự khớp ấy.
Nhìn từ ngoài thì lỗi trông như thế nào? Có supplier trả HTTP 200 với lỗi nằm trong body — một lời nhắc thường trực rằng ý kiến của tầng transport về cú gọi đó vô giá trị. Nếu đường lỗi kết thúc bằng một mã lỗi có tài liệu mà bên gọi xử lý được thì lượt review gần xong.
Thứ mình đếm được
Mình không có con số thời gian review, cũng không có tỉ lệ lỗi. Không ai đo review trước hay sau bất kỳ thay đổi nào trong này, nên không có cặp trước–sau nào để đưa ra.
Thứ đếm được là cái các lượt review để lại. Engine duyệt mang 11 mã lỗi có tài liệu, 7 sơ đồ luồng, và 18 user story viết ngược ra từ code đã có. Ranh giới giữa các module của nó do lint canh chứ không do mình canh. Mỗi thứ trong số đó khởi đầu là một comment mình phải viết tới lần thứ hai — tín hiệu đáng tin duy nhất mình tìm được cho việc một comment review nên thôi làm comment và trở thành một cái rule.