What is code review? Handling 12 comments without losing heart

What is code review? It's when someone else reads your changes before they go into the shared code, to catch bugs, ask about unclear parts and keep the code readable. Have you ever opened a pull request and found dozens of comments on it? Let's work through 12 of them together.

What do twelve comments actually tell you?

In the previous post, I put the fix for the "order 1044 adds up to 70 đồng" bug in the order system of Mộc on its own branch and opened a pull request. Mộc is a small Vietnamese coffee shop that sells online; the shop, Khoa and the comments in this post are made up. The code and its output are real.

A quick recap if you haven't read the earlier posts. The shop's old point-of-sale (POS) machine writes prices as the text "35.000", with a dot between thousands, and JavaScript reads the dot as a decimal point, so two salt coffees came to 70 đồng instead of 70,000 (about $2.80). The fix adds a parsePrice function that turns the text "35.000" into the number 35000, plus 11 tests with Vitest. The pull request has two commits.

The next morning, Khoa, the reviewer, had left 12 comments. Here are all twelve, numbered in the order they appear on the page:

1. orders.ts: "Why does parsePrice throw instead of returning null?"
2. orders.ts: "The regex ^\d{1,3}(\.\d{3})+$ is a bit hard to read. Could you add a few matching and non-matching examples in a comment?"
3. orders.ts: "You could shorten this to Number(String(raw).replace(/\D/g, "")) and skip the regex."
4. orders.ts: "The RawOrder type using Omit is a bit confusing for newcomers. Should it be its own type?"
5. server.ts: "The 'Normalize as soon as the file is read' comment is great, keep it."
6. server.ts: "Answering the question in your PR description: I think the server should stop on a bad price, not skip the order."
7. orders.ts: "parsePrice returns numeric prices unchanged. Negative prices, fractions or NaN all get through. And what if the old machine forgets the quotes?"
8. orders.test.ts: "The note on why '35,000' is rejected is very clear."
9. orders.test.ts: "There's no test for invalid numeric prices."
10. package.json: "Does anyone use the test:watch script?"
11. orders.ts: "customerLabel uppercases names, which is formatting, while the rest of orders.ts is data. Should the page do the formatting instead?"
12. First commit: "The commit body explains the cause really well."

On first reading, all I saw was the number 12. On second reading it looked different. Three comments are praise, three are questions, and only two, numbers 7 and 9, point at a real bug, and both are about the same bug.

The number of comments doesn't tell you how bad the code is. It only tells you how carefully the reviewer read it.

In what order should you read the comments?

What I do is read everything once without replying to anything. If you reply to the first comment right away, you might argue with something comment seven already explains, or fix something a later comment suggests removing.

After reading, I sort the 12 comments into four groups, each handled differently:

• Must fix: 7 and 9. This is a real bug, so it comes first.
• Worth doing: 2. Not a bug, but the next reader will need it.
• Needs discussion: 1, 3, 4, 6 and 10. Some are questions to answer, one is a suggestion I disagree with.
• Just acknowledge: 5, 8 and 12. A thank-you is enough.

And number 11? It's a fair point, but acting on it means changing what the orders page receives, which has nothing to do with the price bug. I put it in a fifth group: out of scope. I'll note it as a separate task and not do it in this pull request.

Google's code review guide, the eng-practices section for the person being reviewed, has advice that fits comment 2 well. When a reviewer doesn't understand a piece of code, the best response is usually to make the code clearer rather than to explain it in a reply. An explanation in a comment helps exactly one reader; a comment in the code helps every reader after.

Is comment 7 really a bug?

Before fixing anything, I checked whether Khoa was right. I wrote a small script that calls parsePrice with three numeric prices, then simulated the old machine forgetting the quotes. In JSON, "price": 35.000 without quotes is a number, and JSON.parse reads it as 35.

// Try the case Khoa raised in comment 7: the price is a NUMBER, but not a sensible one.
// Run from the repo folder: node try-numeric-prices.ts
import { normalizeOrder, orderTotal, parsePrice, type RawOrder } from "./src/orders.ts";

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

// The old machine forgot the quotes: "price": 35.000 in JSON is the number 35.
const order: RawOrder = JSON.parse('{"id":1050,"createdAt":"2026-11-04T08:00:00+07:00","source":"old-pos","items":[{"name":"Salt coffee","price":35.000,"quantity":2}]}');
console.log("price after JSON.parse:", order.items[0].price);
try {
  console.log("total of order 1050:", orderTotal(normalizeOrder(order)));
} catch (error) {
  console.log(`total of order 1050: ERROR ${(error as Error).message}`);
}

parsePrice(-25000) → -25000
parsePrice(35.5) → 35.5
parsePrice(NaN) → NaN
price after JSON.parse: 35
total of order 1050: 70

The last line made me wince. Order 1050 comes out at 70 đồng again, the exact bug this pull request exists to fix. The reason is the first line of parsePrice in the pull request:

// "35.000" → 35000. Anything else unusual throws right away instead of guessing.
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(`Invalid price: "${raw}"`);
}

I checked text prices very carefully and let numeric prices straight through, assuming orders from the website were always right. All 11 tests were green, because no test ever passed in a bad number. This is exactly what code review catches and tests don't: tests only check what their author thought of.

How do you fix things after a review?

I changed it so every price, whether it arrives as text or a number, goes through the same final check. A price must be a whole number of at least 1,000 đồng. That 1,000 đồng is a business rule, so I asked Khoa before writing it. Mộc's menu has nothing under 1,000 đồng, so a smaller price is almost certainly broken data.

// Mộc's menu has nothing under 1,000 đồng. A smaller price is almost certainly broken data,
// e.g. "35.000" losing its quotes in JSON and becoming the number 35.
export const MIN_PRICE = 1000;

// "35.000" → 35000. Unusual formats, or a price that isn't a whole number of at least MIN_PRICE, throw instead of guessing.
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(`Invalid price: ${display(raw)}`);
  return price;
}

function parsePriceText(raw: string): number {
  const text = raw.trim();
  // Dots between thousands. Matches: "35.000", "1.250.000". Does not match: "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(`Invalid price: ${display(raw)}`);
}

// Text in quotes, numbers as they are, so the error message tells "35" apart from 35.
const display = (raw: number | string) => (typeof raw === "string" ? `"${raw}"` : String(raw));

Number.isInteger returns false for both fractions and NaN, so one condition catches all three cases. The comment with matching and non-matching examples above the regex is my answer to comment 2.

Comment 9 asked for tests, so I added four lines to the list of rejected prices. Each line says why it's there:

    [""], // the price field was left empty
    [-25000], // a negative price: nothing on the menu costs less than zero
    [35.5], // a fraction: đồng prices have no decimals
    [Number.NaN], // the result of a broken calculation on the old machine
    [35], // "35.000" lost its quotes in JSON and became the number 35, below MIN_PRICE
  ])("rejects %j instead of guessing", (raw) => {

 Test Files  1 passed (1)
      Tests  15 passed (15)
parsePrice(-25000) → ERROR Invalid price: -25000
parsePrice(35.5) → ERROR Invalid price: 35.5
parsePrice(NaN) → ERROR Invalid price: NaN
price after JSON.parse: 35
total of order 1050: ERROR Invalid price: 35

There are now 15 tests, and order 1050 is rejected right away instead of quietly coming out at 70 đồng.

New commits or rewrite the old ones?

After the fix, I had two options. I could fold the changes into the old commits and force-push the branch, so the history looks as if the code was right from the start. Or I could add new commits.

For a pull request under review, I add new commits. Khoa has already read the first two. If I rewrite them, he has to reread everything to find what changed. With new commits, he only reads the new part. GitHub also shows which commits arrived after the latest review, so the reviewer can see straight away what I did with the feedback.

git add src/orders.ts src/orders.test.ts
git commit        # fix: reject invalid numeric prices
git add src/orders.ts
git commit -m "docs: add examples for the price regex"
git push

The first commit's body says which feedback it comes from:

fix: reject invalid numeric prices

From review: parsePrice returned numeric prices unchanged, so -25000, 35.5
and NaN went straight into the total. A "35.000" price that lost its quotes
in JSON became the number 35 and brought back the same 70 đồng bug.

Every price must now be a whole number of at least 1,000 đồng (MIN_PRICE).

ba10094 docs: add examples for the price regex
8d9d968 fix: reject invalid numeric prices
1241165 test: add Vitest and a test that locks in the order 1044 fix
18725d4 fix: read "35.000" prices from the old POS machine correctly

Your team may have a different convention, such as squashing everything into one commit on merge, and then you follow the team. What I want to avoid is force-pushing to a branch someone is reading without telling them.

If you want to redo this part yourself, start from the practice repo you built in the previous post. It ends exactly where this post begins: the fix/old-pos-prices branch with two commits, pushed to a "remote" repo on your own machine.

How do you disagree with a reviewer?

Comment 3 suggested replacing the whole text-parsing part with one line: strip every non-digit character, then convert to a number. It does look shorter. Instead of arguing about it, I tried exactly that suggestion and ran the tests:

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

     × rejects "35,000" instead of guessing
     × rejects "35.00" instead of guessing
     × rejects "35k" instead of guessing
     × rejects "" instead of guessing
 Test Files  1 failed (1)
      Tests  4 failed | 7 passed (11)

Four tests fail, and they're the four cases the pull request rejects on purpose. With the short version, "35,000" becomes 35000, "35.00" becomes 3500, "35k" becomes 35, and an empty field becomes 0. The function stops complaining about strange data and starts guessing, and guessing wrong in silence is what caused the 70 đồng bug in the first place. After trying it, I put the code back.

My reply to Khoa was based on that result, not on opinion:

I tried this: 4 rejection tests fail, "35,000" becomes 35000 and "35.00" becomes 3500 without any error. I'd like to keep the regex so unusual formats stop the process instead of being guessed. I've added matching and non-matching examples in a comment (comment 2) to make it easier to read. Does that work for you?

The reply has three parts. I tried what he suggested. I gave my reason with concrete data. And I ended with a question, because the reviewer may know something I don't. If Khoa still wants the change, talking for a few minutes is usually faster than ten rounds of comments.

The questions I answered briefly, and anything worth keeping long term went into the code. Comment 1 asked why throw instead of returning null. I answered that null forces every caller to remember to check, and forgetting once means null * 2 becomes 0, another silent bug. Throwing stops the server right when it reads the file, which is what he recommended in comment 6.

Comment 4 asked whether RawOrder should be its own type. I kept it and explained that Omit keeps the two types from drifting apart when Order gets a new field. This is more about taste than right or wrong, so I said clearly that I'm happy to change it if he thinks it's needed.

Comment 10 asked about test:watch. I use it while writing tests, so I kept it.

For comment 11, I replied that it's a fair point and I'd noted it as a separate task, because moving the formatting changes what the orders page receives.

For the three comments that were praise, I just wrote "thanks". It sounds unnecessary, but it tells the reviewer I read that far.

Is code review a grade on you?

I understand the jolt of seeing 12 comments, because at that moment it's easy to read them as a report card. But look back at the list at the top: most comments say nothing about the author's ability. They're questions, suggestions and praise. The one comment that pointed at a real bug saved order 1050 from coming out at 70 đồng before it reached the shop's server.

The Google guide I mentioned also advises not taking comments personally, because the goal of review is the quality of the code. In my view, the fastest way to not take it personally is to answer with code and output, as I did for comments 3 and 7, rather than with feelings.

There's one more thing I only understood after working for a while. The reviewer spent an evening reading your code. Twelve comments means they read it very carefully. A pull request with no comments at all sometimes just means nobody read it.

Approved, but Khoa has one more note

After two new commits and the replies, Khoa approved the pull request and merged it into main. The 70 đồng bug is fixed, there are tests to keep it from coming back, and four more cases the first version missed are covered.

But the approval came with a short note: "Next time you touch server.ts, let's clean it up. That file takes a while to read: names like rawOrders, orders, body don't say what they hold, and one function reads the file, normalizes and handles HTTP all at once." He's right. The code works but is hard to read, and every new person on the project will lose time there. Renaming and splitting functions safely, now that tests cover the code, is the subject of the next posts.

You can follow the whole story in the Taking over an old project series. If you're preparing for interviews, the Git and teamwork questions are in the interview question bank.