What a good code review actually checks for

Written By: BrainSoft In Engineering

Most code reviews I see in the wild are either a rubber stamp or a style argument. Someone opens a pull request, a colleague skims the diff, leaves a comment about spacing, approves, and everyone moves on. The bugs that hurt production rarely get caught this way, because nobody was actually looking for them.

A good review checks four things in order: does the change do what it claims, does it fail safely, is it testable and tested, and will the next person understand it. Everything else, including formatting, belongs in a linter. Here is how I work through a diff.

Correctness first, and that means reading the diff twice

The first pass is mechanical. I read the diff top to bottom and ask whether the code matches the description on the pull request. If the PR says "fix timezone handling in invoice dates" and the diff also renames a config key, that rename gets its own PR. Mixed changes hide bugs.

The second pass is where the real work happens. I trace the data. What goes in, what comes out, what happens at the boundaries. Off-by-one errors, inverted conditions, and wrong variable names all survive a skim and die under a trace. A pattern I look for constantly:

// looks fine, returns undefined for an empty list
return items.find(i => i.active).id;

// what the caller actually needs
const active = items.find(i => i.active);
return active ? active.id : null;

That second version is boring. Boring is the point. I do not ask for defensive code everywhere, but I do ask what the caller expects when the happy path does not happen.

Failure behaviour, error handling, and side effects

Once I believe the code works, I look at what happens when it does not. This is the part reviewers skip most often, and it is where incidents come from.

• Are errors swallowed? An empty catch block is a bug with a delay fuse.
• Do retries have a limit, and are they safe to repeat? A retried payment is worse than a failed one.
• Are database writes inside the same transaction as the reads they depend on?
• Does a new external call have a timeout, or will it hang a worker forever?
• Are logs free of tokens, passwords, and personal data?

I also check side effects. A function named buildReport that sends an email is lying to its reader. If a change touches shared state, caches, or global config, I want to see why in the description.

Tests, naming, and whether the next person can follow it

A change without a test is a change nobody can refactor later. I do not need full coverage, but I do want the test to fail if the fix is reverted. If I cannot tell from the test name what behaviour is protected, the test is documentation that failed.

Naming gets more attention than people expect from me. If I have to read a function body to learn what its name means, the name is wrong. Same for comments: explain why, not what. A comment saying // increment i is noise; a comment explaining that a retry exists because a vendor returns 500 on cold starts is worth keeping.

Finally, I check scope. Does this change belong here? Is there an existing helper it duplicates? That kind of review is where teams usually get the most value, and it is the sort of thing we build into engagements when clients ask us to review an inherited codebase through our services.

How to leave a review that people can act on

A review is a conversation, not a verdict. I separate comments into three buckets and say which is which: blocking issues, suggestions, and questions. "This will drop the last record on an empty page" is blocking. "Consider extracting this into a helper" is a suggestion. "Why did you choose a queue here?" is a question, and I actually want the answer.

Small PRs make all of this possible. If a diff is 900 lines, nobody is tracing data flow, and the review becomes theatre. I would rather review five focused PRs than one large one, even if the total work is identical. If you want a second set of eyes on how your team reviews code, get in touch.

Frequently asked questions

#Engineering