How We Teach Code Review: Find the Defect, Not the Nit - Vets Who Code

New Cohort Starts: Apr 05, 2027
Most new engineers review code the way they were reviewed: they comment on what they can see. A variable name. A missing semicolon. A function that could be a one-liner. Those comments are easy to write and easy to resolve, and a review full of them can approve a change that loses money in production.
This is the first post in a series on how Vets Who Code teaches the practices that separate a good engineer from a new one. It's written for the people who will one day review our troops' code: senior engineers, tech leads and hiring managers. We'd rather show you the bar than tell you about it.
Every topic in the Hashflag Graph, our curriculum, ends in an evidence criterion: a thing the troop produces that someone else can check. Code review lives on the code review domain page, and the first topic there is Review code for correctness and risk. Its criterion:
Finds a real defect in someone else's diff
Three words in that sentence matter. Real means a defect that changes behavior, not a style preference. Someone else's means the troop didn't write it and can't lean on what they meant to do. Diff means the review happens on the change as submitted, the way it does at work, not on a whole codebase with unlimited time.
The graph marks this topic's exit depth as scaffolded, one step short of unassisted. We take help away slowly here because finding defects in other people's code is a skill senior engineers are still sharpening.
Here's a change of the kind a troop gets asked to review. A teammate adds store credit to checkout:
It has a test, the test passes, and the code is short and readable. A review that stops at the surface writes something like this:
nit: balance → creditBalance?
nit: could inline applied.
Both comments are fine. Neither one is a review. Here's what this diff actually does.
getBalance and setBalance are two separate round trips to the store. Nothing stops another request for the same customer from running between them. Walk it through with a $50 balance and two tabs open on checkout:
The customer spent $50 of credit twice. The final balance looks correct, so nothing downstream notices. The unit test can't catch it either, because it runs one request at a time.
What does applyStoreCredit do when totalCents is negative, as it would be on a refund path that reuses this function? Math.min(5000, -2000) is -2000, so the code writes a balance of 7000 and returns 0. Maybe that's intended. The diff doesn't say, and that silence is worth a question.
A defect comment has to survive being read by someone who is busy and slightly defensive. We teach troops a three-part shape: what you observed, what it causes, and what you're asking for.
blocking: getBalance and setBalance are separate calls, so two concurrent checkouts for the same customer can both read the old balance and both apply it. With a $50 balance, two tabs can each take $50 off their orders.
Could this be a single conditional update, or a transaction that locks the row? A test that fires two applyStoreCredit calls with Promise.all and asserts the total applied is at most the starting balance would pin it down.
question: what should happen when totalCents is negative? Right now it adds credit back. If refunds aren't supposed to come through here, a guard would make that explicit.
Label severity. blocking, question and nit tell the author what has to change before merge and what's optional. Nits are allowed. They're just never the whole review.
The fix the author lands might look like this. Your database client will differ; the point is that the read and the write happen under one lock:
Troops don't find defects like that by staring harder. They find them by asking the same questions of every diff, in the same order:
The same questions apply when the diff came from an AI assistant. A model writes the read-then-write version of applyStoreCredit as readily as a person does, and it looks just as confident. Our AI engineering domains hold troops to the same standard. On the evals domain page, Recognize silent LLM failure is judged by whether the troop "identifies a confident wrong answer in review." Building with AI makes review more important, not less.
The question our mentors bring to troop work is the one a model can't answer: would this survive code review at a real company? Troops prove each criterion alone, against a published rubric, and the work is graded by someone who is not them. Code review is where that standard gets applied. A troop who can only spot nits hasn't met the criterion. A troop who can explain a race to its author, kindly and with a test that proves it, has.
That's also the reviewer you want on your team.
The next post flips the seat: The Reviewable Pull Request covers how troops write a change that a reviewer can actually check.
Vets Who Code is free for every troop, and it stays that way because people like you keep it running. If this post was useful, Donate or sponsor us on GitHub.
Vets Who Code is a free, remote software engineering accelerator for U.S. veterans, service members and spouses. If you know one who's ready to become a software engineer, send them this post and tell them to Apply. We'll take it from there.
31860 Sandy Plains Road, Ste 204 PMB 206
© 2026 Vets Who Code — All Rights Reserved
Vets Who Code is a registered 501(c)(3) nonprofit — EIN 86-2122804. Donations are tax-deductible to the fullest extent allowable under the law.