There’s an old joke that goes something like this:

A police officer is patrolling the neighborhood when he spots a drunk man crawling on his hands and knees beneath a streetlamp. The officer asks the man what he’s doing, and the man says he’s looking for his wallet.

After a few minutes of searching, the officer asks the man if he’s sure this is where he lost the wallet. The man replies, “No, I think I dropped it in the park.” “The park?!” the policeman asks, “Then why are you looking for it here?!”

“This is where the light is.”

Like the drunk man crawling on his hands and knees, we often make a critical mistake when it comes to reviewing code: doing what is easy rather than what is right.

What does this mean for your code reviews?

What is Easy

Any competent dev team has a whole suite of tools at their disposal for running automated checks for correctness. In general, these checks can be run manually with a single command and automatically by continuous integration tools.

This toolbox often includes unit tests, integration tests, acceptance tests, static analysis tools, security scanners, linters, and all sorts of home-brewed goodness. These tools make your team better. They make your team faster. They make your team lazy.

When someone opens a pull request and you begin reviewing it, there are a lot of things to check. So many, in fact, that you can’t feasibly check them all. So naturally you have to get the best bang for your buck. You know what’s cheap, fast, and easy? Automated tests!

Boom! Unit tests pass.

Boom! Thumbs up from the static analysis tool.

Boom! The linter says the code style is perfect.

This review is just about over, right?

Wrong. The review hasn’t even started yet.

What is Right

First and foremost, one must understand that a reviewer is not just a second, semi-automated continuous integration pipeline. As a reviewer, your job is to check the things which are too tricky or impossible to automate.

A reviewer’s mindset should be, what can I check that the computer can not? That is your strength, value, and purpose in this equation. It’s your side of the bargain.

While it may seem obvious, an outsized portion of talented engineers shirk their responsibility as a reviewer—not out of apathy or inability, but out of this ignorance.

Look for What’s Not There

Oftentimes, a reviewer finds their value in looking for what’s not there. There’s a skill to that. Like any skill, you can practice it and get better over time.

For instance, the new feature may be to allow the user to change their email address. You could run through testing, verify that the user can now change their email, and give the PR a thumbs-up.

When we look for what’s not there, though, we need to look at the implications of allowing a user to change their email address. Hidden in that requirement is another one: don’t let a user change anyone else’s email address. This requirement is so obvious that it tends not to ever be said explicitly, but it is obviously a crucial component.

After all, the difference between making this mistake and not making it could be more subtle than you think:

1
2
3
4
5
6
7
8
-- The correct query to update an email address
UPDATE user
SET email_address = 'hello@example.com'
WHERE user_id = 123;

-- An easy mistake to make which looks correct at first glance
UPDATE user
SET email_address = 'hello@example.com';

Accidentally updating all of the users’ email addresses technically fits the parameters of “allow the user to change their email address” but doesn’t quite fit the spirit.

A Reviewer’s Responsibilities

There is an endless list of these considerations, and most of them are judgement calls.

• Is this secure?
• Is this scalable enough?
• Is it sufficiently tested?
• Is this well-documented?
• Is this going to come back to bite us?
• Is this going to cause unanticipated side effects?
• Is there any requirement that was missed?
• Is this going to clash with another feature?
• Is there a better way?

Answering these questions is where an effective reviewer earns their keep while a lackluster reviewer sets the team up for failure. The failure could be introducing a bug, a vulnerability, or a bottleneck. If the PR introduces a structural problem, it’s possible the fallout won’t be felt for years.

The Drunkard

The phenomenon of tending to do what is easy rather than what is right is not a novel one, nor is it limited to the world of software development. It’s commonly known as the Streetlight Effect or, more amusingly, The Drunkard’s Search.

When papers are graded in school, the creativity of the work or its compelling nature are often secondary to its spelling and grammar. Why? Because spelling and grammar are easy to grade.

It takes a tremendous amount of effort to take in and digest a complex or nuanced argument, or to let the creativity of a story wash over you. There are no correct answers when it comes to these things. It’s subjective.

Spelling and grammar, on the other hand, have correct answers. You can be wrong and provably so. This makes for easy grading, so it is over-emphasized in classrooms today.

Does this mean spelling and grammar are unimportant? No, of course not. Neither is running automated tests. The distinction, though, is in recognizing where your time should be spent.

Conclusion

Code review is a vital, underrated step in the process of software development. You owe it to your team to give them thoughtful reviews and you should expect the same from them.

Do what is right, not what is easy.

Don’t be your team’s drunkard.