Abstract: Do you think code reviews aren’t important? Think again. Code reviews are often recognized as the best way to improve code quality. This article will discuss the importance of code reviews, different types of code reviews, and how to conduct a code review.
We all agree, bad code negatively affects applications. It introduces bugs. It is hard to maintain. It is difficult to enhance and extend.
In the January 2017 issue of DNC Magazine I discussed code standards as one way to improve the code you and your team produce. We’ve also heard for years that unit tests are a key step for improving code quality. I agree with this.
Unit tests are very important.
However, you can do better. It’s by using something that most teams either don’t do or don’t do effectively.
I’m talking about Code Reviews.
This article is published from the DNC Magazine for Developers and Architects. Download this magazine from here [PDF]
Code Reviews are Important
IBM engineer Michael Fagan first documented the idea of code reviews in 1976 when he wrote about them in the magazine article “Design and Code Inspections to Reduce Errors in Program Development.” (bit.ly/dncm29-fagan)
The importance of code reviews continued to be documented. In the 2011 Dr. Dobbs article “Do You Inspect?”, Capers Jones and Olivier Bonsignour wrote “Recent work by Tom Gilb, one of the more prominent authors dealing with software inspections, and his colleagues continues to support earlier findings that a human being inspecting code is the most effective way to find and eliminate complex problems that originate in requirements, design, and other non-code deliverables. Indeed, to identify deeper problems in source code, formal code inspection outranks testing in terms of defect-removal efficiency levels.” bit.ly/dncm29-drdobb
Let me repeat the most important part of this statement,
“a human being inspecting code is the most effective way to find and eliminate complex problems”.
Notice they didn’t say code standards. They didn’t say functional testing. They didn’t even say unit testing.
They said, “a human being.”
This is backed up elsewhere. In the book Code Complete(bit.ly/dncm29-codecomplete*), Steve McConnell writes, “software testing alone has limited effectiveness – the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent.”
* affiliate link
More recently, in 2016, SmartBear Software conducted a survey on improving code quality then proclaimed that code reviews are “The #1 Way to Improve Software Quality” (bit.ly/dncm29-smartb). When asked “What do you feel is the number one thing a company can do to improve code quality?”, 34% of respondents said, “Code Review”.
Figure 1: Code Quality Survey (source: SmartBear)
It’s pretty clear that code reviews are important.
Impact of Poor Quality
Let’s now look at how poor quality affects software.
Way back in 2002, the National Institute of Standards and Technology, a division inside the US Department of Commerce, produced a 203 page document called The Economic Impact of Inadequate Infrastructure for Software Testing (http://www.nist.gov).
Among other things, this study documented the number of hours it took to fix a bug that is found at different phases in the application lifecycle. They showed that a bug introduced in the coding phase but discovered in production, took an average of 14.8 hours to fix. But if found during the coding stage, it took 3.2 hours.
Figure 2: Time taken to fix a bug at different phases in the application lifecycle (Source:SmartBear)
The software company Parasoft analyzed the effect of publicly announced bugs from publicly held companies. Looking at only 2014, they found the average value of the companies dropped 3.75% the day the failure was announced. That may not sound like much when viewed as a percentage, but in dollars, it is a whopping $2.3 billion loss of value.
That’s just the day of the announcement. Cumulative effects were worse.
FullStory, a Silicon Valley startup created by former Google engineers called code reviews “a bionic cultural hammer” meaning that when you introduce code reviews to your company, it has a huge impact on the culture.
They outlined five ways this happens:
- Promotes Openness – Sets the tone for the entire company. Work is scrutinized by other team members and is welcome.
- Raises Team Standards – Makes sure all engineers share a similarly high level of standards.
- Propels Teamwork – Team members reconcile different viewpoints between reviewer and reviewee. In these cases, the manager doesn’t have to step in and “be the adult.” It also improves team communication.
- Keeps Security Top of Mind – Team members learn security hands-on and in context of their domain. Additionally, reviewers and reviewees constantly update their security knowledge.
- Shapes the Culture – People start to think in terms of quality and secure code.
Finally, 84% of participants in the SmartBear survey answered with Improved Quality when asked, “What do you think are the most important benefits of code review?”
Figure 3: Most important benefits of Code Review (Source: Smartbear)
When you attempt to introduce code reviews to your team, you may get some push back. First, some may say that it adds time to the schedule. Go back to the NIST analysis of when bugs are discovered and you’ll see that’s not true.
If a bug is found earlier, it takes less time and money to fix.
Second, it can become a battle of who is a better coder. A code review is not a contest. It is used to find areas the code and coder can improve. Even the best coders can write poor code. I know this from personal experience as both the reviewer and reviewee.
At this point, I have laid out a good case for conducting code reviews but have not defined what a code review is.
What exactly is a code review?
Wikipedia provides the following definition: “A code review is systematic examination (sometimes referred to as peer review) of computer source code. It is intended to find mistakes overlooked in the initial development phase, improving the overall quality of software.” That falls in line with what you’ve seen so far.
In his Pluralsight course, “Lessons from Real World .NET Code Reviews” (bit.ly/dncm29-ps-course), Shawn Wildermuth says that a code review determines what is being done well and what can be done better, it is not a witch hunt, sharing findings makes all developers involved better, and no one size fits all.
This last point is interesting as it implies there are different ways to do code reviews. And, in fact, there are.
At a high level there are four primary types of code reviews:
- Formal – A formal code review meeting is held. All participants gather around a conference room table to go through the code. Reviewers get the code ahead of the meeting and go through, noting things that are good and bad.
- Informal – Rather than a meeting, reviewers are generally emailed the code. They then reply with their comments.
- Automated – Software analyzes code and flags suspect code that doesn’t follow guidelines or has other potential issues. Human reviewers then enter additional comments that are sent back to the reviewee.
- Pull Request – Tools like GitHub allow reviewers to look at the Pull Request and comment on it before it’s merged. However, it can be difficult to see the changed code in context of the entire method or class.
You may find that one of a combination of these types of code reviews work best for your team. When I do my Code Reviews presentation at conferences I am often asked how often should you do a code review. The correct answer is before every merge or check-in.
Now let’s turn to the actual practice of a code review.
What to do when you’re the reviewer
So, you have in front of you code that a team member has written and you need to code review it. The things you do are the same no matter if you’re doing a formal review or a pull request. What do you do? Where do you start?
First step is to try to understand the problem being solved and then ask, “Does this code solve the problem?” A good second question is, “Where are the unit tests and do they pass?”
After this, it’s time to look how the problem is solved. By this, I don’t mean to analyze every little thing and compare it to your coding standards. For example, your coding guidelines may say that closing braces should be on their own line. If this is violated once in the code, it was probably overlooked. But if the reviewee consistently violates this standard, you should probably call it out.
Other things to look for include patterns and anti-patterns, what the reviewee is doing well and what they consistently do wrong, are there just enough comments, does the code belong where it is or should it be in its own method or class? This is big picture things that make the code easier to modify.
In his Pluralsight course, Shawn Wildermuth points out several ideas:
- Take your experience to bear. You may have more experience than the reviewee. The review can become mentoring time. The opposite can also be true. If you are less experienced than the reviewee, you can learn some new techniques.
- Find things to laud and things to fix
- No code is perfect. Maintainability is a feature.
- Don’t review too much code at a time.
MSDN.com (bit.ly/dncm29-msdn) also give some good advice. “Don’t review for code style; there are much better things on which to spend your time, than arguing over the placement of white spaces. Looking for bugs and performance issues is the primary goal and is where the majority of your time should be spent. Suggestions on maintenance best practices can be brought up after the bugs have been discussed.”
One final thing to keep in mind is what I call the “1 to M” rule which is, “Code is written once but read many times.”
This means that fancy or tricky coding should be avoided. If you can’t figure out what the code is doing by reading it, it wasn’t written well.
Before moving on, there are also things you should not do as the reviewer:
- Don’t get emotional – We all have team members that are more difficult to work with than others. If you’re reviewing code from that person, keep emotion out of the review.
- Don’t focus blame – Instead of saying things like “Why did you not follow the Single Responsibility Principle?” say “How does this code comply with the Single Responsibility Principle?”
- Don’t redesign the code – If you’re redesigning the code, you should probably write it in the first place. If the code is really, really bad, make this a mentoring moment.
- Don’t judge how you would have done it – Often a more senior developer reviews the code written by a junior developer. Because of having more experience, you tend to look at the problem with different eyes. Again, maybe a good candidate to mentor.
- Don’t include one-off problems – This comes back to the earlier comment about closing braces. Look for things the reviewee does frequently.
Now that we’ve addressed what the reviewer should and should not do, let’s flip the table and talk about the person who wrote the code.
What to do when you’re the reviewee
There are some things that should be done by the person who wrote the code before submitting the code for review.
- Have a checklist of review items – Keep a list of items that you frequently do wrong and correct them ahead of time. Also, have a list of items that reviewers often look for and check for these things too. In other words, do your own personal code review.
- Help maintain the coding guidelines – This gives you the opportunity to learn how to write better code. Are there things missing that should be there? Do things need to be removed or added?
- Remember the code isn’t you – We get attached to our code and tend to take criticism of it personally. We can learn from reviewers even if we have more experience than they do.
Code reviews are setup to make sure the code is in good condition. But we can also learn from them.
Wrap-up
Whether you are the reviewer or the reviewee, there are things you need to consider in the review process and not make it about the coder, but rather about the code.
Code reviews can sometimes become a mentoring experience.
Code reviews are an important part of the development process. If you’re not doing reviews, you should talk to your team about getting them started. If you are doing reviews, when was the last time you reviewed the code review process to determine if it’s effective or should be tweaked to improve it.
By doing reviews, you can help ensure your application is green, lush, and vibrant.
About Software Gardening
Comparing software development to constructing a building says that software is solid and difficult to change. Instead, we should compare software development to gardening as a garden changes all the time. Software Gardening embraces practices and tools that help you create the best possible garden for your software, allowing it to grow and change with less effort.
