Code review là gì? Nhận 12 bình luận mà không nản

Code review là gì? Đó là khi một người khác đọc thay đổi của bạn trước khi nó vào code chung, để bắt lỗi, hỏi chỗ chưa rõ và giữ code dễ đọc. Bạn đã bao giờ mở pull request ra và thấy hàng chục bình luận chưa? Cùng mình xử lý 12 bình luận nhé!

Mười hai bình luận nói lên điều gì?

Ở bài trước, mình đưa bản sửa lỗi "đơn 1044 tổng 70 đồng" của hệ thống đơn hàng quán Mộc lên một nhánh riêng và mở pull request. Mộc là một quán cà phê nhỏ bán online, còn quán, anh Khoa và các bình luận trong bài đều là minh họa. Code và kết quả chạy thì là thật.

Nhắc lại ngắn gọn cho bạn chưa đọc các bài trước. Máy bán hàng cũ của quán ghi giá dạng chuỗi "35.000", và JavaScript hiểu dấu chấm là dấu thập phân, nên 2 ly cà phê muối thành 70 đồng. Bản sửa thêm hàm parsePrice đổi chuỗi "35.000" thành số 35000, kèm 11 test bằng Vitest. Pull request có hai commit.

Sáng hôm sau, anh Khoa, người review, để lại 12 bình luận. Mình chép lại đủ cả 12, đánh số theo thứ tự xuất hiện trên trang:

1. orders.ts: "Vì sao parsePrice ném lỗi mà không trả về null?"
2. orders.ts: "Regex ^\d{1,3}(\.\d{3})+$ hơi khó đọc. Thêm vài ví dụ khớp và không khớp vào comment được không?"
3. orders.ts: "Có thể viết gọn thành Number(String(raw).replace(/\D/g, "")), khỏi cần regex."
4. orders.ts: "Kiểu RawOrder dùng Omit hơi khó hiểu với người mới. Có cần tách kiểu riêng không?"
5. server.ts: "Comment 'Chuẩn hóa ngay khi đọc file' rất hay, giữ nhé."
6. server.ts: "Trả lời câu em hỏi trong mô tả PR: anh nghĩ máy chủ nên dừng khi gặp giá sai, đừng bỏ qua đơn."
7. orders.ts: "parsePrice trả nguyên giá dạng số. Giá âm, số lẻ hay NaN đều đi qua. Còn nếu máy cũ quên dấu ngoặc kép thì sao?"
8. orders.test.ts: "Ghi chú vì sao từ chối '35,000' rất rõ."
9. orders.test.ts: "Thiếu test cho giá dạng số không hợp lệ."
10. package.json: "Script test:watch có ai dùng không?"
11. orders.ts: "listOrders trả về key tiếng Việt khach, tong trong khi phần còn lại đặt tên tiếng Anh. Có nên thống nhất không?"
12. Commit đầu: "Phần thân commit giải thích nguyên nhân rất rõ."

Đọc lần đầu, mình chỉ thấy con số 12. Đọc lần hai thì khác. Ba bình luận là khen, ba bình luận là câu hỏi, và chỉ hai bình luận, số 7 và số 9, chỉ ra một lỗi thật, mà cả hai nói về cùng một lỗi.

Số lượng bình luận không nói lên code tệ tới đâu. Nó chỉ cho biết người review đọc kỹ tới đâu.

Đọc bình luận theo thứ tự nào?

Cách mình làm là đọc hết một lượt mà chưa trả lời cái nào. Trả lời ngay bình luận đầu tiên thì dễ cãi những điều mà bình luận thứ bảy đã giải thích, hoặc sửa một chỗ mà bình luận sau đề nghị bỏ đi.

Đọc xong, mình chia 12 bình luận vào bốn nhóm, và mỗi nhóm có một cách xử lý riêng:

• Phải sửa, gồm số 7 và 9. Đây là lỗi thật, sửa trước tiên.
• Nên làm theo, gồm số 2. Không phải lỗi, nhưng người đọc sau sẽ cần.
• Cần trao đổi, gồm số 1, 3, 4, 6 và 10. Có câu hỏi cần trả lời, có đề nghị mình không đồng ý.
• Chỉ cần ghi nhận, gồm số 5, 8 và 12. Cảm ơn là đủ.

Còn số 11 thì sao? Nó đúng, nhưng sửa nó nghĩa là đổi định dạng dữ liệu mà trang đơn hàng đang trả về, một việc không liên quan tới lỗi giá. Mình xếp nó vào nhóm thứ năm là ngoài phạm vi. Mình sẽ ghi lại thành một việc riêng, không làm trong pull request này.

Hướng dẫn về code review của Google, bộ tài liệu eng-practices phần dành cho người được review, có một lời khuyên mà mình thấy đúng với số 2. Khi người review không hiểu một đoạn code, cách tốt nhất thường là sửa code cho dễ hiểu hơn, chứ không phải giải thích trong bình luận. Lời giải thích trong bình luận chỉ giúp đúng một người đọc, còn comment trong code giúp mọi người đọc về sau.

Bình luận số 7 có đúng là lỗi không?

Trước khi sửa, mình kiểm tra xem anh Khoa có đúng không. Mình viết một file thử nhỏ gọi parsePrice với ba giá dạng số, rồi giả lập trường hợp máy cũ quên dấu ngoặc kép. Trong JSON, "price": 35.000 không có ngoặc kép là một con số, và JSON.parse đọc nó thành 35.

// Thử trường hợp anh Khoa nêu trong bình luận số 7: giá là SỐ nhưng không hợp lý.
// Chạy trong thư mục kho: node thu-gia-so.ts
import { normalizeOrder, orderTotal, parsePrice, type RawOrder } from "./src/orders.ts";

for (const gia of [-25000, 35.5, Number.NaN]) {
  try {
    console.log(`parsePrice(${gia}) →`, parsePrice(gia));
  } catch (error) {
    console.log(`parsePrice(${gia}) → LỖI ${(error as Error).message}`);
  }
}

// Máy cũ quên dấu ngoặc kép: "price": 35.000 trong JSON là số 35.
const don: RawOrder = JSON.parse('{"id":1050,"createdAt":"2026-11-04T08:00:00+07:00","source":"may-cu","items":[{"name":"Cà phê muối","price":35.000,"quantity":2}]}');
console.log("giá sau JSON.parse:", don.items[0].price);
try {
  console.log("tổng đơn 1050:", orderTotal(normalizeOrder(don)));
} catch (error) {
  console.log(`tổng đơn 1050: LỖI ${(error as Error).message}`);
}

parsePrice(-25000) → -25000
parsePrice(35.5) → 35.5
parsePrice(NaN) → NaN
giá sau JSON.parse: 35
tổng đơn 1050: 70

Dòng cuối là dòng làm mình giật mình. Đơn 1050 lại ra tổng 70 đồng, đúng con số của lỗi mà pull request này sinh ra để sửa. Lý do nằm ở dòng đầu của parsePrice trong pull request:

// "35.000" → 35000. Định dạng lạ thì báo lỗi ngay, không đoán.
export function parsePrice(raw: number | string): number {
  if (typeof raw === "number") return raw;
  const text = raw.trim();
  if (/^\d{1,3}(\.\d{3})+$/.test(text)) return Number(text.replaceAll(".", ""));
  if (/^\d+$/.test(text)) return Number(text);
  throw new Error(`Giá không hợp lệ: "${raw}"`);
}

Mình kiểm tra rất kỹ giá dạng chuỗi, còn giá dạng số thì cho qua ngay vì nghĩ rằng đơn từ web luôn đúng. 11 test đều xanh, vì không test nào đưa vào một con số sai. Đây chính là thứ code review bắt được mà test không bắt được, vì test chỉ kiểm tra những gì người viết nghĩ tới.

Sửa theo review thế nào?

Mình sửa để mọi giá, dù đến dưới dạng chuỗi hay số, đều qua cùng một bước kiểm tra cuối. Giá phải là số nguyên, và phải từ 1.000đ trở lên. Con số 1.000đ là một quy tắc nghiệp vụ, nên mình hỏi lại anh Khoa trước khi viết. Menu của Mộc không có món nào dưới 1.000đ, nên một giá nhỏ hơn gần như chắc chắn là dữ liệu hỏng.

// Menu của Mộc không có món nào dưới 1.000đ. Giá nhỏ hơn gần như chắc chắn là dữ liệu hỏng,
// ví dụ "35.000" bị mất dấu ngoặc kép trong JSON nên thành số 35.
export const MIN_PRICE = 1000;

// "35.000" → 35000. Định dạng lạ, hoặc giá không phải số nguyên từ MIN_PRICE trở lên, thì báo lỗi, không đoán.
export function parsePrice(raw: number | string): number {
  const price = typeof raw === "number" ? raw : parsePriceText(raw);
  if (!Number.isInteger(price) || price < MIN_PRICE) throw new Error(`Giá không hợp lệ: ${hienThi(raw)}`);
  return price;
}

function parsePriceText(raw: string): number {
  const text = raw.trim();
  // Nhóm nghìn bằng dấu chấm. Khớp: "35.000", "1.250.000". Không khớp: "35,000", "35.00", "3500.000".
  if (/^\d{1,3}(\.\d{3})+$/.test(text)) return Number(text.replaceAll(".", ""));
  if (/^\d+$/.test(text)) return Number(text);
  throw new Error(`Giá không hợp lệ: ${hienThi(raw)}`);
}

// Chuỗi trong ngoặc kép, số thì giữ nguyên, để thông báo lỗi phân biệt được "35" với 35.
const hienThi = (raw: number | string) => (typeof raw === "string" ? `"${raw}"` : String(raw));

Number.isInteger trả về false cho cả số lẻ lẫn NaN, nên một điều kiện bắt được ba trường hợp. Dòng comment có ví dụ khớp và không khớp phía trên regex là phần trả lời cho bình luận số 2.

Bình luận số 9 đòi test, và mình thêm bốn dòng vào danh sách các giá bị từ chối. Mỗi dòng có một câu nói vì sao nó ở đó:

    [""], // ô giá bị bỏ trống
    [-25000], // giá âm: không món nào có giá âm
    [35.5], // số lẻ: tiền đồng không có phần thập phân
    [Number.NaN], // kết quả của một phép tính hỏng ở máy cũ
    [35], // "35.000" mất dấu ngoặc kép trong JSON thành số 35, nhỏ hơn MIN_PRICE
  ])("từ chối %j thay vì đoán", (raw) => {

 Test Files  1 passed (1)
      Tests  15 passed (15)
parsePrice(-25000) → LỖI Giá không hợp lệ: -25000
parsePrice(35.5) → LỖI Giá không hợp lệ: 35.5
parsePrice(NaN) → LỖI Giá không hợp lệ: NaN
giá sau JSON.parse: 35
tổng đơn 1050: LỖI Giá không hợp lệ: 35

Giờ có 15 test, và đơn 1050 bị từ chối ngay thay vì âm thầm ra 70 đồng.

Thêm commit mới hay sửa lại commit cũ?

Sau khi sửa, mình có hai lựa chọn. Mình có thể gộp thay đổi vào commit cũ rồi đẩy đè lên nhánh, để lịch sử trông như thể code đúng ngay từ đầu. Hoặc mình thêm commit mới.

Với một pull request đang được review, mình chọn thêm commit mới. Anh Khoa đã đọc hai commit đầu. Nếu mình viết lại chúng, anh ấy phải đọc lại toàn bộ để tìm xem cái gì đã đổi. Commit mới thì anh ấy chỉ cần đọc đúng phần mới. GitHub cũng hiển thị các commit thêm vào sau lần review gần nhất, nên người review thấy ngay mình đã làm gì với góp ý.

git add src/orders.ts src/orders.test.ts
git commit        # fix: từ chối giá dạng số không hợp lệ
git add src/orders.ts
git commit -m "docs: thêm ví dụ cho regex đọc giá"
git push

Commit đầu có phần thân nói rõ nó đến từ góp ý nào:

fix: từ chối giá dạng số không hợp lệ

Theo góp ý review: parsePrice trả nguyên giá dạng số, nên -25000, 35.5
và NaN đi thẳng vào tổng tiền. Giá "35.000" mất dấu ngoặc kép trong JSON
thành số 35 và cho ra lại đúng lỗi 70 đồng.

Giờ mọi giá phải là số nguyên từ 1.000đ (MIN_PRICE) trở lên.

6196596 docs: thêm ví dụ cho regex đọc giá
bfccfa8 fix: từ chối giá dạng số không hợp lệ
f55e9b1 test: thêm Vitest và test khóa lỗi đơn 1044
2444423 fix: đọc đúng giá dạng "35.000" của máy bán hàng cũ

Nhóm của bạn có thể có quy ước khác, ví dụ gộp hết thành một commit khi merge, và khi đó bạn theo quy ước của nhóm. Điều mình muốn tránh là đẩy đè lên một nhánh mà người khác đang đọc, khi chưa nói với họ.

Nếu bạn muốn làm lại phần này trên máy, mình có một script dựng sẵn kho Git ở đúng trạng thái cuối bài trước, với hai commit trên nhánh fix/gia-don-hang-may-cu và một kho "từ xa" nằm ngay trên máy thay cho GitHub. Script chép code từ các thư mục của bài 1 và bài 3, nên nó chỉ chạy được khi bạn có đủ code của các bài đó:

git switch -q -c fix/gia-don-hang-may-cu
cp "$GOC/03-test-vitest/src/orders.ts" "$GOC/03-test-vitest/src/server.ts" src/
git add src/orders.ts src/server.ts
toi commit -q -m 'fix: đọc đúng giá dạng "35.000" của máy bán hàng cũ'
cp "$GOC/03-test-vitest/src/orders.test.ts" src/
cp "$GOC/03-test-vitest/package.json" "$GOC/03-test-vitest/package-lock.json" .
git add package.json package-lock.json src/orders.test.ts
toi commit -q -m "test: thêm Vitest và test khóa lỗi đơn 1044"
git push -q -u origin fix/gia-don-hang-may-cu

Không đồng ý với người review thì nói thế nào?

Bình luận số 3 đề nghị thay toàn bộ phần đọc chuỗi bằng một dòng: bỏ hết ký tự không phải chữ số, rồi đổi thành số. Nghe gọn thật. Thay vì tranh luận bằng lời, mình thử đúng đề nghị đó trên máy rồi chạy test:

export function parsePrice(raw: number | string): number {
  return Number(String(raw).replace(/\D/g, ""));
}

     × từ chối "35,000" thay vì đoán
     × từ chối "35.00" thay vì đoán
     × từ chối "35k" thay vì đoán
     × từ chối "" thay vì đoán
 Test Files  1 failed (1)
      Tests  4 failed | 7 passed (11)

Bốn test đỏ, và chúng là bốn trường hợp mà pull request cố ý từ chối. Với cách viết gọn, "35,000" thành 35000, "35.00" thành 3500, "35k" thành 35, và ô trống thành 0. Hàm không còn báo lỗi khi gặp dữ liệu lạ nữa, nó đoán, và đoán sai trong im lặng chính là thứ đã sinh ra lỗi 70 đồng. Thử xong, mình trả code về như cũ.

Câu trả lời mình gửi anh Khoa dựa trên kết quả đó, không dựa trên ý kiến:

Em thử cách này rồi anh ạ: 4 test từ chối bị đỏ, "35,000" thành 35000 và "35.00" thành 3500 mà không báo lỗi. Em muốn giữ regex để gặp định dạng lạ thì dừng lại, thay vì đoán. Em đã thêm ví dụ khớp và không khớp vào comment (bình luận số 2) cho dễ đọc hơn. Anh thấy vậy ổn không?

Câu trả lời có ba phần. Mình đã thử điều anh ấy đề nghị. Mình đưa ra lý do bằng dữ liệu cụ thể. Và mình kết bằng một câu hỏi, vì người review có thể biết điều mình chưa biết. Nếu anh Khoa vẫn muốn đổi, hai người nói chuyện trực tiếp vài phút thường nhanh hơn mười lượt bình luận qua lại.

Các câu hỏi thì mình trả lời ngắn, và câu trả lời nào đáng giữ lâu dài thì đưa vào code. Bình luận số 1 hỏi vì sao ném lỗi mà không trả về null. Mình trả lời rằng null buộc mọi nơi gọi hàm phải nhớ kiểm tra, và quên một chỗ là null * 2 ra 0, một kiểu lỗi âm thầm khác. Ném lỗi thì máy chủ dừng ngay lúc đọc file, đúng như anh ấy đề nghị ở bình luận số 6.

Bình luận số 4 hỏi có cần tách kiểu RawOrder không. Mình giữ nguyên và giải thích rằng Omit giúp hai kiểu không lệch nhau khi Order thêm trường mới. Đây là chuyện sở thích nhiều hơn đúng sai, nên mình nói rõ mình sẵn sàng đổi nếu anh ấy thấy cần.

Bình luận số 10 hỏi về script test:watch. Mình dùng nó khi viết test, nên mình giữ.

Bình luận số 11 mình trả lời rằng nó đúng và mình đã ghi thành một việc riêng, vì đổi tên key trong dữ liệu trả về có thể làm hỏng trang đang đọc nó.

Còn với ba bình luận khen, mình chỉ viết "cảm ơn anh". Nghe thì thừa, nhưng nó cho người review biết mình đã đọc tới đó.

Code review có phải là chấm điểm mình không?

Mình hiểu cảm giác chột dạ khi thấy 12 bình luận, vì lúc đó ai cũng dễ đọc chúng như một bảng điểm. Nhưng nhìn lại danh sách ở đầu bài, phần lớn bình luận không nói gì về năng lực của người viết. Chúng là câu hỏi, đề nghị, và lời khen. Bình luận duy nhất chỉ ra lỗi thật thì đã cứu đơn 1050 khỏi con số 70 đồng trước khi nó tới máy chủ của quán.

Tài liệu của Google mà mình nhắc ở trên cũng khuyên đừng coi bình luận là chuyện cá nhân, vì mục tiêu của review là chất lượng của code. Theo mình, cách nhanh nhất để không coi nó là chuyện cá nhân là trả lời bằng code và kết quả chạy, như với bình luận số 3 và số 7, thay vì bằng cảm xúc.

Có một điều nữa mà mình chỉ hiểu khi đã đi làm một thời gian. Người review dành cả buổi tối để đọc code của bạn. Mười hai bình luận nghĩa là họ đã đọc rất kỹ. Một pull request không có bình luận nào đôi khi chỉ có nghĩa là không ai đọc.

Đã được duyệt, nhưng anh Khoa còn một ghi chú

Sau hai commit mới và các câu trả lời, anh Khoa duyệt pull request và merge vào main. Lỗi 70 đồng đã được sửa, có test giữ nó không quay lại, và có thêm bốn trường hợp mà bản đầu bỏ sót.

Nhưng kèm lời duyệt là một ghi chú ngắn: "Lần sau sửa server.ts mình nên dọn luôn. File này đọc mất thời gian: tên biến kiểu rawOrders, orders, body không nói nó chứa gì, và một hàm vừa đọc file, vừa chuẩn hóa, vừa trả HTTP." Anh ấy nói đúng. Code chạy đúng nhưng khó đọc, và mỗi người mới vào dự án sẽ mất thêm thời gian ở đó. Đặt tên và tách hàm sao cho an toàn, khi đã có test che chắn, là việc của các bài viết sau.

Bạn theo dõi cả câu chuyện ở series Nhận dự án cũ. Nếu đang ôn phỏng vấn, các câu hỏi về Git và làm việc nhóm có trong ngân hàng câu hỏi phỏng vấn.