Blog of Michal Kostic
Code review checklist

I spend significant amount of time at work doing code reviews - here are some lessons I have learnt.

Overview

I tend to accept code that correctly implements required functionality, code that is easy to read and easy to extend (in this order). I try not to reject commits on the topics that are up for debate or for which there is no “One True Way” of doing them.

What is the problem?

My first step is looking at linked issue. This check is just a click away since we require every commit message to contain a issue number. I highly recommend this policy. As result, all our commit messages look something like this:

   PROJ-123: I have change this and that.

Adopting issue number in the commit message is useful both ways. From the commit you can get to the original issue definition and from the ticket you can see how the problem was solved. You will be thankful for the links from the ticket to the code when you are trying to fix a similar issue in three months.

Is it solving the issue?

As a second step I try to understand what the code does and check it against to the required functionality.

Is it solving the issue? Great! Are there obvious errors, forgotten edge cases or decreased usability? Reject with a vengeance.

Do I understand the code?

In case I do not understand either the problem (issue) or the solutions (actual commit) I contact the commiter via phone or message. No exceptions here - I do not dare to review code I do not understand.

After I feel I understand enough I go for one of the following:

  • I had a weak moment and the code is obvious
  • I suggest adding appropriate comment to the code so next person does not have to figure it out
  • I update the system documentation on the project wiki
  • I update the issue description to make the problem definition clearer

Is the code of good quality?

During the review for correctness I keep an eye on overall code quality. Here are some of the issues I watch for.

Naming

Well named methods, classes and variables are the most important part of documentation. Holding this belief I am very strict when it comes to naming.

Sometimes there is no clear way how to name things. For that cases I tend to accept the name chosen by original developer, maybe suggesting to add a comment.

Simplicity

I have a strong urge to reject complex code. Problems should be solved in as simple and straight-forward manner as possible.

My rule is: Do not create new classes or abstractions unless you specifically need them. You never know what future will look like and chances are you won’t guess.

I like the “Rule of Three” as presented by Jeff Attwood (http://blog.codinghorror.com/the-delusion-of-reuse/). Roughly paraphrasing:

If you need it once, build it. If you need it twice, copy it. If you need it a third time, abstract it.

Flexibility/Extensibility

Keeping the previous point in mind, my next rule is - code should be easy to change.

Easy example of the rule is “no literals in the code” (use at least local variable if not a constant). Another example is ruthlessly extracting functionality to separate methods.

Single Responsibility Principle (Cohesion)

One class - one responsibility. Simple enough. I will probably reject a commit that has a class that breaks this rule.

If the class is in the gray area (kind of feels wrong but I can’t really put a finger on it) I do not reject. That uncertainity often comes from lack of information on how this class will be used. Wait with refactoring until the usage is clearer.

Idioms

Non idiomatic code is a big no-no. It confuses people and can introduce subtle bugs. Unless your unusual code is really well justified it is going to be rejected.

This might spun a question: “What is idiomatic java?”. To answer that I try to find examples on reputable sites (StackOverflow) or in the famous Java libraries. If all else fails I rely on my hunch coming from years of experience.

Some good justifications for non-idiomatic code are:

  • improved API of the class
  • overcoming known issues of the used library
  • improved readability on many places of the code, thanks to your clever hack.

Code smells

Code smells ask for an explanation. I tend to leave a comment requesting more details or discuss directly.

If you need a refresher on code smells see http://blog.codinghorror.com/code-smells/ (yes I like coding horror blog :)

Trade offs

The main goal is to make customers happy. Customers are happy when the application is reliable and works as expected. You get bonus points for accomodating their change requests.

Code improvements coming from code review should always keep this in mind.

Some questions to ask yourself:

  • Is the development effort worth the improvement?
  • Will we be able to introduce new features faster thanks to this?
  • Will we produce less bugs?
  • Is the improvemnt in code quality worth the time spent?

Positive communication

Important goal of code review process is to help people write better code. For me this means providing suggestions for improvement instead of pointing out what is wrong.

I believe all my teammates are smart and capable people. With this in mind I ask for explanations so I can understand better. It is too easy to push my way of doing things - let the commiter explain the code and leave some space for self expression.

Communication levels

  • too big or complex change, complete WTF, too serious issues - talk in person or on the phone with the commiter
  • major issues - leave comments in the code review tool
  • minor issues - possibly accept the commit and create a new issue for the improvements.

Conclusion

All hints mentioned above need to be considered on case by case basis. Some code that looks complex needs to stay so and some seemingly simple code is breaking the project in unknown ways. Defining where to put line for single responsibility is sometimes tough. Figuring idiomatic vs non-idiomatic is more art than science.

Keep the communication with the author positive and issues will sort out.

Most importantly - beautiful code is means to an end, not a goal itself.

Blog comments powered by Disqus