Improving code quality with code reviews

Today I will talk about how we are currently using code reviews to build better solutions and how it gives me more confidence in the quality that we are shipping.

Working in teams brings several challenges, one of them is a mixture of coding styles causing code inconsistency across your project making it hard to read or follow what it does.

The bigger the team, the more important it is to transfer your knowledge about what you've worked on so you have a limited Bus-factor in your team.

Or have you ever been working on a new feature fully confident that it's ready to ship only to notice that you've forgotten to take into account about the caching? Or that you've forgotten to update the documentation (if any)?

Sounds familiar to you?

Note - While some of these "pain points" can be tackled by using tools like Roslyn Analyzers or FxCop I prefer a more humane approach and discuss the why instead of the how.

Code reviews to the rescue

By using code reviews we can avoid these problems by collaborating before we ship the code - Let's first take a look at an example:

public void Process(Order o)
{
	DateTime timeStamp = DateTime.Now;

	// ...
	
	MyMethod<Order>(o, timeStamp, true);
}

While this code could perfectly process your order, there are some issues:

  • Variable o represents a certain amount of state but what does it represent?
    Looking at the signature it is clearly an Order how do I know that in a 50-line method at the bottom?
  • Time zones, they are evil! What happens if this code runs in the US?
  • Calling MyMethod<T> takes in a boolean but what does it really do and how does the boolean come in to play?
  • How does the caller know what the Process method does? Hopefully bill for the order? Also, it couldn't hurt to add additional documentation throughout the implementation in certain cases.

While performing a code review, the reviewer can indicate these concerns with the reviewee and have a polite & constructive discussion backed by a set of coding guidelines. By doing this both parties get to know how the other person thinks about it and they learn from each other but also learn to express how they did something or what they have forgotten about.

Having a second pair of eyes on a certain topic can help a lot because everybody has a different perspective and this can make sure that you forget about a certain topic and leads to interesting discussions. This gives you a certain "Don't worry, I've got your back" feeling and forces you to think deeper about what you've written.

At the end of the review, the reviewee can have some feedback to process where after the code gets the seal of approval and is ready to ship.

Next to the code quality you also perform small knowledge transfers with each other. You will not remember everything but when needed you will remember certain pieces that can help you guide to the potential bug or cause.

Last but not least is automated testing. It's a good thing to add the unit/behavior/scenario testing to your reviews as well because then the reviewer gets an indication of what you are and are not testing. Do the tests make sense or should the reviewee cover additional scenarios?

Challenges

Using code reviews is of course not a free lunch and it comes with its own difficulties.

The biggest challenge is that your team members need to be open for feedback and willing to incorporate the feedback. If they are not up for it you will just spend your valuable time to only notice that they are ignoring it. You as a team will need to decide whether or not you want to commit to the code-review-system.

Every review takes a decent amount of time so incorporate that into your planning. The reviewer needs to go through it, discuss it with the reviewee and then the reviewee need to process the feedback. However, one might argue that it is better to take your time during development instead of having to spend twice that amount while fixing bugs or trying to understand what's going on.

New to code reviews? Here are some tips!

After using code reviews for a while, I've learned a couple of things on how to not do it or what can be challenging. Here are some tips that help you avoid some pitfalls.

Review early & frequently - The earlier you review, the better. This avoids reviewing something that is considered ready while you've misunderstood some aspects of it or have re-invented the wheel.

Define code guidelines - Agree upon a list of coding guidelines with your team to back your reviews. By doing this you have a clear list of styles, paradigms and do's & don't do's that the team should follow to unify your coding styles & practices. This makes reviewing a lot easier and a clear guidance on how it should look.

An example of this could be that each parameter should be checked for null and that it should throw an ArgumentNullException when appropriate.

Add code reviews to your definition-of-done - By adding code reviews to your definition of done you are certain that each new feature or bug fix has passed at least two pair of eyes and that multiple people agree on it. By doing that you also remove the burden of one person being responsible for one aspect since it's the whole team that is responsible for it.

Don't review to bash, review to teach & improve - Finding a balance between being strict or agreeing with everything is hard. If you just bash it will have a negative impact on the team collaboration and frustrations will arise. Be constructive & open.

Review in-person but have a look at the changes in advance - This allows you to have a personal opinion instead of simply following reviewee. This avoid you having to make decisions on the spot but instead digesting it first so you can think of obvious aspects

Challenge and be challenged - Ask questions about the topic to see if the reviewee has covered all the possible scenarios and learn about how they envision it. Discussions are a good thing, not a bad thing.

Learn from each other - Don't be afraid to say what you like and don't like or if you don't know about something. Learn from others and how he/she did it this way and not like how you thought about it.

Conclusion

While the internet has a wide variety of blogs talking about this "concept" I wanted to share my vision on this since I'm a big fan of this practice and believe that this really improves the quality. However, your success will depend on the cooperation of your colleagues and project management if they want to commit in doing so.

One thing is certain - I've used this on my current project but will keep on doing so in the future.

Thanks for reading,

Tom.