Reading code, and reviewing it without being unbearable

A review is not a spelling test. Prettier already handles the spacing, ESLint already catches the unused variable, and a human who spends their review pointing at those has spent it badly.

Review is the only step that catches the things no tool can see: that the code solves the wrong problem, that it will be unreadable in six months, that there is a case nobody thought about. Set the tools up so the review can be about those.

Let the tools go first

npm install --save-dev eslint prettier

Prettier settles formatting so nobody argues about it. ESLint catches real mistakes — an unused variable, a case that falls through, an await in a loop, a promise nobody handled. Run both before you push:

{
  "scripts": {
    "lint": "eslint .",
    "format": "prettier --write .",
    "check": "prettier --check . && eslint . && npm test"
  }
}

The value is not tidiness. It is that a reviewer who never has to mention formatting will mention something that matters.

Two ESLint rules worth turning on early, because each maps to a bug from the previous lesson:

"no-floating-promises"      // a promise nobody awaited or caught
"eqeqeq"                    // == instead of ===

"" == 0 is true. null == undefined is true. [] == false is true. Use === and the entire table stops mattering.

What to open a pull request with

The description is not paperwork. It is the difference between a review that takes ten minutes and one that takes an hour.

Fix the delivery date being stored a day early

`setHours(0,0,0,0)` works in local time, so an order placed at 00:30 IST was
stored against the previous day. The API echoed the date the client sent, which
is why this looked correct in testing.

Switched to UTC-midnight helpers and a date-only column.

Decisions worth a look:
- `parseIsoDate` returns UTC midnight, so "today" is the same instant everywhere.
  A user in a different timezone now sees the shop's day, not their own — which
  is what we want for a shop, and wrong for a calendar app.

Verified: 14 tests, and an order placed at 00:15 IST now stores the right date.

What changed, why, the decisions worth arguing about, and what was actually run. A reviewer who reads that knows where to spend their attention.

Keep it small. A 200-line diff gets read. A 2,000-line diff gets approved without being read, which is worse than no review, because now two names are on it. If a change is genuinely large, split it: a mechanical rename in one PR, the behaviour change in another.

Reviewing: the order to look in

Not top to bottom. In this order:

Does it solve the right problem? The most expensive bug to find late, and no test catches it. Read the description against the diff.
Are there cases it misses? Empty array. Zero. One item. The same request twice. A slow network. A second tab.
Will this be readable in six months? Where did you have to re-read? Say so — that is the most useful comment in most reviews.
Is it tested where it matters? Not coverage percentage: is the rule in this diff pinned down by a test?
Details. Names, structure, duplication.

Mostly people do this in reverse, and run out of attention before step 1.

Things worth looking for specifically in JavaScript

|| where ?? was meant — the zero and empty-string bug.
A promise that is never awaited, or forEach with an async callback.
await inside a loop where Promise.all was meant, or vice versa.
Mutation of an argument, or a shared array pushed to from two places.
An empty catch.
this inside a callback — a method passed as onClick={this.save} loses it.
A listener added and never removed, in anything long-lived.

Comments that land

This is wrong.

If `pieces` is 0 here, `||` will make it 1 and the customer is billed for a
tiffin they did not get. `??` instead?

Say the consequence, not the verdict. And mark what is optional:

nit: `orderCount` would read better than `n`. Not blocking.
question: is it possible for two tabs to submit this at once?
blocking: this returns 0 for a failed lookup, so a bad ID looks like an empty cart.

Three words that remove most review friction. A reviewer who does not label severity leaves the author guessing which of eight comments must be fixed.

Ask rather than assert when you are not sure. "Why UTC here?" is a better comment than "this should be local time" when you have not read the rest — and half the time the answer teaches you something.

Say what is good, specifically. Not "LGTM". "Pulling the date parsing into one helper makes the rest of this obvious" tells the author which instinct to repeat.

Receiving a review

Every comment is about the code. The reviewer cannot see how long you spent, and would not review differently if they could.

Answer the ones you disagree with — "I chose this because…" is a legitimate response, and sometimes the reviewer changes their mind. But if a reviewer misread your code, that is usually a fact about the code, not about the reviewer. The fix is a clearer name, not a reply explaining it.

Push fixes as new commits so the reviewer can see what changed, rather than force-pushing a rewritten branch mid-review.

Reviewing your own code first

The cheapest review there is. Open your own diff on GitHub before you request anybody — not in your editor, where you have been looking at it for three hours.

You will find, reliably: a console.log, a commented-out block, a file you did not mean to include, a name you meant to change, and something you would be embarrassed to have someone else point out. Ten minutes, and it makes the human review about the design.

Read the diff, not the files. git diff main is what the reviewer sees.

Check your work

What review is for: what no tool can check — wrong problem, unreadable code, missed cases.

Why set up ESLint and Prettier: so nobody spends a review on formatting.

Why ===: "" == 0, null == undefined and [] == false are all true.

What a PR description needs: what, why, the debatable decisions, what was verified.

Why small PRs: a big diff gets approved unread.

The review order: right problem, missing cases, readability, tests, then details.

Three labels: nit, question, blocking.

Why say the consequence: "this is wrong" cannot be acted on.

What it means when a reviewer misreads your code: the code needs to be clearer.

Why review your own diff: you will find the console.log yourself.

Practice

Add ESLint and Prettier to your capstone and run them. Count the findings.
Turn on eqeqeq and fix what it reports.
Evaluate "" == 0, null == undefined, [] == false and NaN == NaN.
Open a PR against your own repo and read the diff on GitHub, not locally.
List what you found. Compare the list to the one in this lesson.
Write a PR description with the four parts.
Find a || default in any open-source JavaScript repo and decide whether it should be ??.
Review a friend's code — or an open PR in a public repo — following the five steps in order. Notice where you were tempted to start.
Rewrite a blunt review comment as a consequence plus a question.
Take the harshest review comment you have received and rewrite the code so the comment would not have been needed.

That is the module, and the course's last idea before the capstone: the code is read far more often than it is written, and almost always by someone who does not have you to ask.