Friday, May 5, 2017

The importance of code reviews

In my discussions with development teams on how they can optimize their development processes and deliver high quality software, a topic that keeps coming up is the importance of doing code reviews. Many developers I meet with have a relaxed opinion on doing code reviews: they appreciate the quality assurance involved in someone else reviewing their work, but doing code reviews for others is often de-prioritized as the work load grows and the deadline is close.

There are three main reasons for why you should do code reviews:
  1. Code reviews reduce risk
  2. Code reviews minimize the amount of technical debt introduced into a code base
  3. Code reviews increase team collaboration
I'd like to go into detail on these three reasons to explain why you should increase your efforts on doing code reviews, for the sake of yourself and the rest of your team.



#1 Code reviews reduce risk

If you think about the amount of risk involved in creating a piece of software, it's a wonder we're brave enough to work as developers. One mistake and you could be tomorrows media headline (just ask the developers at Knight Capital).

Doing code reviews reduces the risk of a developer introducing security and privacy vulnerabilities to the code base, simply because having a second pair of eyes questioning what is committed will act as an extra verification. We've all met that one dev who tries to implement their own encryption method, or who outputs "This e-mail address is already in use" on a registration page, or who believes it's best to store all user data just in case we need it. With a code review, features that are implemented in poor judgement can be stopped or improved before making it to production. When it comes to matters of security, not all developers can be (or should be) security experts. How about identifying high-risk parts of your application and having one of your security experts do mandatory code reviews on those parts before the change is allowed to make it any further than a feature branch?

So far, I have only mentioned two types of risks: Security and privacy. The third type of risk is one of my favorites, called the "bus factor".

"The bus factor is a measurement of the risk resulting from information and capabilities not being shared among team members, from the phrase "in case they get hit by a bus". 

(Source: https://en.wikipedia.org/wiki/Bus_factor)

For those who have read "The Phoenix Project", you will remember Brent Geller who is called upon by everyone, at all times. He is the one guy who knows everything and who would leave the organization clueless if he were to be hit by a bus. A guy like Brent, together with the rest of his company, would benefit from a development process where other developers were forced to understand what he was doing and why. And remember how our hero Bill manages to reduce the bus factor in PartsUnlimited? He refuses to let Brent fix anything without another developer being part of the learning experience. He basically introduces pair programming and code reviews. Smart guy, huh?

(If you have no idea who or what I am talking about right now, stop reading this blog post and go read "The Phoenix Project" instead.)

#2 Code reviews minimize the amount of technical debt introduced into a code base

Technical debt in a code base is inevitable. If I didn't know better, I would believe that the second law of thermodynamics, which states that the total entropy of an isolated system can only increase over time, also applied to code. Sounds like something developers can relate to, right?

Humans are simple creatures. If we know someone is watching, we are more sustainable to doing the right thing instead of making shortcuts. If you know you are the only one looking at a piece of code, doing a quick and dirty fix, thinking "I'll clean this up later" will feel a lot easier than if you know that you have a colleague who will be looking at your code before it's allowed into your master branch. So the simple notion of knowing that someone will review your work will increase the overall quality of the code that you are writing.

There are several other reasons for technical debt than dirty quick fixes, of course. As your application grows over time, the architecture will become more complex, it becomes more difficult to avoid duplication of business logic and developers tend to want to rewrite someone elses code rather than refactor and improve it. Looking beyond the manual aspect of one developer reviewing another developers code, code reviews can include automated verifications to slow down the process of software rot. For example, you can set up an automated build that runs all unit tests and fails the build if any new tests are failing because of the changes you've made. That way, you are notified that you need to improve your efforts even before one of your colleagues sit down to review your code. Another example is running static code analysis, picking up on any credentials, keys or sensitive data that has been committed, automatically failing your pull requests.

So far, I've mentioned many concepts, such as feature branches, pull requests and the master branch, that are part of the GitHub workflow. If you feel a bit shaky on what these concepts are, I recommend this great visual tutorial on Understanding the GitHub Flow. If you're interested in seeing how Microsoft tools support this, keep on reading.

#3 Code reviews increase team collaboration

My favorite argument for why code reviews are important is a simple one: Ownership. Developers who feel like they own their code and who feel like they have a resposibility for their code base, will take better care of it. It works in the same way as renting vs buying a house. If you own it, you're more likely to invest in maintenance than if you rent the place.

Ask yourself this: Have you ever had a bug reported, where the developer receiving the bug report said: "Uh, but this is Steve's code, I don't know how this works"? This it what happens when developers don't view their software development as a team effort, simply because they don't work as a team. They have separated their development and their responsibilities into small bits and pieces of "This is mine and this is yours". There is no ours. In a developement team who actively does code reviews, it will be easier for one developer to continue the work of another, or to fix a bug introduced by someone else because they have seen the code before. Hey, they've even approved the code before, meaning they should understand it pretty well.

Last, but not least: Code reviews function as a way of upskilling. All development teams have members with different qualifications and seniority. Having a senior developer review the code of a junior developer can act as a verification of quality, while having a junior developer review the code of a senior developer will act as a learning experience. It will broaden their understanding of the archtectural descisions made and give them insight into patterns and practices they might not be familiar with themselves. Upskilling through code reviews will give you "T-shaped" developers, meaning your developers will have a broad skillset and the ability to collaborate across diciplines, while maintaining their targeted expertise in one area.

What now?

One of the common misconceptions of code reviews and the argument I hear most often for why developers don't do code reviews is that they don't have the time. Many tech leads are not able to see beyond the initial upskilling period that comes with any change introduced into your development process, and they are therefore blind for the true benefit of doing code reviews. Setting up a development process where developers are forced to review each others code will in the long run speed up the development, make you less prone to efficiency blockers such as vacations, unscheduled time off, developers changing jobs and (let's hope this one doesn't happen) someone being hit by a bus.

Code reviews in Visual Studio Team Services

In Visual Studio Team Services (VSTS), you can define branch policies for the branches in your Git repository:

 
In the example above, I have enabled branch policies for my master branch, requiring all changes to be submitted through pull requests. The creation of a pull request will queue a new build (based on my "dotnetcore CI HOL" build definition) and if the build fails, the pull request will be blocked. I am also checking that all pull requests have been associated with a work item and that all comments have to be resolved before the pull request is merged. At the bottom, I have declared that minimum 2 reviewers must approve my changes before I can merge, and I cannot approve my own changes.

Now, let's take a look at the code review experience that comes with a pull request in VSTS:


Here you can see the same policies (and some additional ones), the work items linked to this pull request and the reviewers.

As a reviewer, I am able to comment on the changes and approve them. I can also auto-complete the pull request, meaning that as soon as all the necessary reviewers have approved the pull request and all policies are green, the pull request will automatically be merged. This way you don't have to return to the pull request after a long running build has completed, it's all about improving the efficiency of the developers.

<sales-pitch>
If you would like to try this out, you can create a free VSTS account for up to 5 users. If your team is larger than that, anyone who has a Visual Studio license has access without additional costs.
</sales-pitch>

My hope is that more development teams will optimize their development efforts by taking advantage of doing code reviews. Feel free to share your thoughts!

No comments:

Post a Comment