A Good Code Review

What is code review?

The short explanation of what "code review" means is this: Someone other than the author looks at code changes and makes suggestions before the changes go into the product.

Why is code review?

Having experienced code review at many different companies and while interacting with open source projects with many different reviewers, I have whittled down my idea of what a code review should be to a few simple rules.

Every single comment on a code review should address a failure of one of the following guidelines:

1. Is this code effective in accomplishing the stated goal of the changes?
2. Is this code obviously correct in execution (e.g. apparently defect-free)?
3. Is this code free of major security or performance issues?

Every code review comment should clearly address some problem with one of the three items above. If a comment does not, it should be explicitly self-identified as both non-blocking and an "opinion," "thought," or "concern." Sometimes an "opinion" can manifest as a "suggestion," but these are explicitly non-blocking, and should be self-identified as such.

If a review does not address any of these issues, but also fails to self-identify as non-blocking opinions, consider that what the review does - in fact - is arbitrarily gatekeep software changes.

The only meaningful review is one that catches implementation issues ("You said this does X, but in reality it does Y, which is similar, but not quite right."), or security flaws ("This would allow a malicious user to expose our database structure."). Reviews that contain anything else are meaningless FUD that are used - most often - to enforce arbitrary adherance to actively harmful cult practices.

How is code review?

A convenient way to make sure your own code reviews are on target is to use a small table of "review metadata" to categorize them. You could include two items to start: "Type" and "Blocking".

Type could be one of six options (but your values could vary):

• Operation: This doesn't seem to accomplish the stated goal.
• Defect: There seems to be a logic problem.
• Security: This causes a security problem.
• Performance: This causes a noticeable performance problem.
• Thought: This made me think of something.
• Suggestion: Here's another way to do this.

The blocking value of your metadata would depend heavily on which type you use, but it's safe to say "Thought" or "Suggestion" types would be non-blocking, while the rest would almost certainly be blocking issues.

Here's an example review metadata table:

That table might follow a comment like: "When I ran this in my browser, all of my fans spun up to their max speed. I checked the memory profile and this change consumes 800MB of additional memory when the page loads."

Suffice to say: If a review comment can't fall into any of these types, it probably doesn't have much value to the code author. Likewise: if none of a review's comments are blocking issues as defined above (Operation, Defect, Security, Performance), then the review should automatically be approved!