Code reviews – why and how?

I’m a big fan of code reviews. There are a lot of reasons for doing them, and my favourites include knowledge spreading, code consistency, quality and finding bugs early in the development process. I’ve benefited greatly from senior developers commenting my code and providing guidance and proposals in order to increase the quality of my code.

Having said that, code reviews can be time consuming and hard to do with little domain knowledge of the feature/system developed. Furthermore, some days your head is simply not in it. On those occasions, I keep a list of things that I look for in order to get me going. Following are five simple things to look for when conducting code reviews. Hopefully these tips will help you speed up the process and gets you thinking about the code.

Remember that the important thing here is to start a conversation about the code and reasoning about it. There is no silver bullet solution to any of these, however they can point to some issues that are worth discussing.

  1. Unit tests
    Are there any tests for the code? If not, why was none added? There might be valid reasons for not adding tests but make sure to understand the developer’s reason for not adding them. Can tests at least be added afterwards? This is an important question as the code should at least be testable at a later stage. If there are unit tests then make sure you understand what they are testing. Preferably you should be able to understand what the test is testing from its name.
  2. Logical statements
    By logical statements I mean all kinds of logical checks, like if and while statements etc. Long chains of logics are hard to follow, take the following code line for instance, which isn’t even that long:
    If (person.getAge() > 18 && person.getAge() < 21)
    It takes a while before you understand what this statement is supposed to check. If you instead think about the meaning behind the check you should be able to extract it to a private method. Why not add a new method on the object Person?
    If(person.isOldEnoughToDriveACarButNotAHeavyTruck())
  1. Getters & setters of private variables
    Keeping the state of an object private is good praxis, and I always try to limit the number of getters and setters. Are all the getters and setters added needed? Take the example above, we removed the need for a getAge method by introducing the isOldEnoughToDriveACarButNotAHeavyTruck method. So, if the rules for what ages you need to be in order to drive these different vehicles, there is only one place where you need to change it.
  2. Usage of null
    Returning null or using it as valid input variables should be kept to a minimum, preferably completely avoided. I like using Optionals in Java which is an alternative to null. Discuss if null (or lack of value) is reasonable for the object in question, if not there might be some design changes needed.
  3. Comments
    Now there are different opinions on using comments; some believe they improve the readability of the code, other consider them confusing and redundant. I belong to the latter of the two and believe that code should be self-explanatory. Comments should not be needed. If there are some complicated piece of code that needs explanation then I would at least opt for extracting it to another method with a self-explanatory name. I find that comments don’t actually help the reader, it might not even provide enough context. One comment might be meant for an undefined number of code lines for instance. They also have a tendency for sticking around. Never getting deleted even if the code it was commenting is not there anymore.

And that was all from me for now! Hope you got a little wiser on the topic and Happy Coding!
/ Jon