Posted on Oct 31, 2018
Code reviews are a little like eating your vegetables: You know your supposed to eat them and that they’re good for you, but unless there’s a parental unit watching over you, it ain’t gonna happen. The same is true for code reviews. We know they’ll help us improve the quality of our code and the overall system, but unless they’re part of the project’s culture we’re probably not going to ask others to reviews our code.
Why another article about code reviews? Because code reviews work, and I want to see more organizations adopt the practice. The complexity of our applications and the demands placed on our software engineers is increasing every day, and unless we incorporate tested and proven practices we will only meet with failure.
Just to make sure we’re on the same page, let’s start with a formal definition of what a code review is and then work from there.
The activity of systematically examining computer source code with the intent of finding mistakes created or overlooked in the development process, and thus improving the overall quality of the software.
The idea is that a developers working on a project submit the work they’ve performed to be reviewed by other developers on the project or within the organization. The reviewers then look over the code for any flaws (logical, syntax, complexity, etc.) or design issues and return their findings to the submitting developer. The purpose is fourfold:
In times gone by, these submissions (i.e. patches) would be often be provided through email, but with the advent of modern source code management software such as Git, code reviews have never been easier to perform.
While the four reasons above provide the technical purpose of code reviews, they’re only the beginning of the benefits to be had from incorporating a formal code review process into your organization.
It should come as no surprise from what we’ve discussed so far that code reviews reduce the number of defects which are introduced into a software system, but what kind of numbers are we talking about?
According to Steve McConnell, author of Code Complete 2, “the average effectiveness of design and code inspections are 55 and 60 percent” effective at detecting defects. That’s between 10-35% more effective than automated tests (which you should also be doing).
Furthermore, several case studies cited in the book showed that code reviews cut errors by 82% at Aetna Insurance, 99% in IBM’s Orbit project, 90% from an AT&T project with more than 200 people. Finally, NASA’s Jet Propulsion Laboratories estimated it “saves about $25,000 per inspection by finding and fixing defects at an early stage”.
At first glance, the idea that code reviews speed up the development process seems like nonsense. After all, with a recommended speed of no more than 500 lines of code an hour, reviewing code can take a long time. That’s time the reviewer isn’t working on adding features or fixing bugs. That’s time the submitter may be blocked from moving forward. Plus, if there are questions about the patch or discussions which arise through the patch, that is still more time not being put toward completing features.
But the point of software development isn’t produce code or even features. It’s to create a functioning product.
As we’ve already seen, code reviews can reduce defects by an average of 60%. That’s 60% fewer bugs making it to your customers, but it also means 60% fewer bugs that developers have to deal with in the future. It’s been found that developers spend an average of “80% of development costs on identifying and correcting defects”. How much of that 80% could be reduced by creating 60% fewer defects to begin with? What could your team do with that extra time?
If reduced defects where the only thing code reviews offered to increasing development their value would still be justified. But code reviews do more than reduce defects: they improve the overall quality of the codebase.
To begin, knowing your code will be reviewed motivates developers to take more care towards producing the best code possible. This isn’t limited to the raw logic of a codebase, but also to the organization and architecture as well. No one wants to get called out for writing shoddy code. Furthermore, when other team members review code they bring their own perspective, knowledge, and experience to bear on the changes, providing more opportunities to perfect the logic, reduce complexity, and improve the overall quality of the code.
So what? What does code quality have to do with development time?
It’s self-evident that logic takes more effort and time to think through and reason about as complexity increases. The same is true in programming. When a code base is saddled with high amounts of technical debt, isn’t well organized, or has portions the team is afraid to change, it’s necessarily going to take longer to make changes. On the other hand, when the code is well maintained and organized it’s much easier to reason about and make changes. Even the largest codebases can keep complexity to a minimum.
If you can get today’s work done today, but you do it in such a way that you can’t possibly get tomorrow’s work done tomorrow, then you lose.
– Martin Fowler, Refactoring: Improving the Design of Existing Code
Before we go into how code reviews encourage a healthy engineering culture, it’s important to understand why a healthy engineerine culture is important. There are two primary reasons.
First, a healthy engineering culture is one which is concerned about following best practices, building well crafted products, and is much more productive. All of this results in lower costs for the organization. Developers in this kind of environment are more engaged with the projects and organization, care more about the quality of their work, and consistently look for ways of improving their craft.
Second, this type of culture attracts higher caliber engineers. It’s difficult to Attract Good Developers, but a strong culture eases this pain. The best developers want to be where they will be pushed and have opportunities to excel.
…one of the most important factors for success of a company is the quality of its engineers. The best way to lower development costs is to hire great engineers: they don’t cust much more than mediocre engineers but have tremendously higher productivity…If you code base is a wreck, word will get out, and this will make it hard for you to recruit. As a result, you are likely to end up with mediocre engineers.
– John Ousterhout, A Philosophy of Software Design
Part of having a healthy culture is ensuring it can adapt to change. By incorporating code reviews into your development process, your team has the opportunity to become familiar with areas they might not otherwise have encounter; be it the same project or others. This increased breadth of knowledge creates a flexibility in the team, allowing members to stand in for others in the case of sickness or other reasons. This shared knowledge also allows multiple people to understand and solve problems.
As Martin Fowler notes:
Code reviews help spread knowledge through a development team. Reviews help more experienced developers pass knowledge to less experienced people. They help more people understand more aspects of a large software system. They are also very important in writing clear code. My code may look clear to me, but not to my team. That’s inevitable–it’s very hard for people to put themselves in the shoes of someone unfamiliar with the things they are working on.
– Martin Fowler, Refactoring: Improving the Design of Existing Code
Not only do code reviews widen the team’s knowledge of the projects they support, but being exposed to the code other people write also deepens their knowledge of what a technology has to offer. Today’s languages and frameworks can be immense and no one gets to know it all, but by reading code we get to to see how other developers solve problems, what API components they use, and what patterns they lean on. There’s always something to learn from code.
There is one final point I want to make before we move on to the process of code reviews: a team that has a healthy practice of reviewing code is a team that has a strong sense of ownership over a product. When you feel a sense of ownership over something, you want to see it flourish and succeed. This sense of ownership translates into engagement and purpose which then translates into increased productivity.
There are two reasons for this. On one hand, knowing that others will be looking over your work tends to motivate you to work a little harder. On the other, code reviews require trust: trust from the submitter that the reviewer won’t be overly harsh in the response, and trust from the reviewer that his or her comments will be listened to. Trust builds relationships and is at the core of the best teams.
Now that we’ve looked at the “what” and the “why”, let’s look at “how” to go about performing code reviews.
It is my opinion that above all else, four rules must be followed without exception to successfully integrate code reviews into a team:
All code reviews start with a pull request (PR). Every tool handles them a little differently, so you’ll want to check the documentation for the specifics for your tool. When submitting a PR, there are three guidelines you need to follow:
Logger.info "got here"); and all the tests pass.
Once a pull request has been submitted it’s time to review it. To do that, you’ll want to first be mindful off these four things:
When your receive a request for review and you begin looking through the code, what should you look for? Rather than providing you with a list of a hundred code smells, potential problems, or gotchas you might see, I think it makes sense to provide a high-level view of what to look for:
If you’re performing a code review, you’re going to have to give feedback. It may be as simple as “Looks good!”, “ship it!”, or even just a “👍”, but more often than not you’ll need to critique the code, point out issues, and ask questions. As developers, we have a tendency to rush through the communication related tasks to get back to what we’re most comfortable with. In our haste, we become more terse and direct resulting in unnecessary problems. To avoid any potential problems, try following these guidelines:
Code reviews require too much effort and are too important to allow reviewers to waste their time commenting about coding style. Instead, reviewers should spend their time thinking through the logic and design of the pull request, and automated tools should be used to either point out style issues or better yet, automatically format them for you.
Just because someone’s asked you to review their PR doesn’t mean your initial response must be either “Approve” or “Reject”. Instead, think of it as the beginning of a conversation. Like any conversation, there will be points you will need clarification on in order to keep moving forward. The same is true for PRs. If there is logic or design choices you don’t understand, ask the author to clarify those choices. It may be that he or she doesn’t fully understand it either. If you don’t understand it now, what’s the likelihood you’ll understand it the next time you encounter it?
Text is a horrible medium for communication. It doesn’t convey facial expressions, tone of voice, or any sort of body language. It’s just text. The meaning of a sentence can vary just by where we as the reader put emphasis. For example, read the sentence “Why did you write it this way?” in the following ways:
Do you hear the difference? It’s the same text, just read with different emphasis. As much as we hate to admit it, we’re emotional creatures, and as such we bring our emotions with us into whatever activities we do, including reading. If we’ve had a bad day or didn’t sleep well, it’s easy to read feedback in a more negative light than what was intended. It’s therefore incumbent upon the reviewer to take extra care when writing comments.
By taking more time to clearly communicate what’s good or bad about the code, ask clear questions, and provide potential solutions, it reduces potential back-and-forths between you and the submitter. This leads to faster development times. Furthermore, adding clear explanations gives the submitter context and insight into what you were thinking at the time of the review, reducing the likelihood of them taking things the wrong way.
I’ve never seen this played out within an organization, but I’ve heard stories and I’ve seen it in open source projects. I hate that I even have to mention it, but reviewing code is not a time for the reviewer to play a game of “Gotcha”. Nor should it be used as an opportunity to belittle the submitter.
We all make mistakes and there isn’t a single individual that gets to know the right way to do everything. When a developer submits their code for review, they’re trusting the reviewer to be fair and provide honest and helpful feedback. Don’t betray that trust.
The last thing to remember to do as a reviewer is compliment good work when you see it. Simple comments like “That’s cool!”, “I didn’t know you could do that!”, or even just, “Good job” can be great encouragements to the submitter. It doesn’t have to be done in every code review, but when you see something good, call it out.
As my mom likes to say, “A candle loses nothing by lighting another candle.”
Choose not to be harmed — and you won’t feel harmed. Don’t feel harmed — and you haven’t been.
– Marcus Aurelius
The piece of the code review process is the hardest: it’s how to receive feedback. It’s the hardest because it involves our emotions and our ego. Unlike other creative endeavors which are subjective in their nature, programming has a strong objective element to it. When an art critic gives a negative review of a piece of work, the artist can always respond with, “The critic failed to see my vision.” With programming, on the other hand, a code reviewer can say a piece of code is wrong and then provide evidence to support the argument. There isn’t a comeback for facts.
On a personal level, receiving code reviews is the most important part of the code review process. It’s at this point that you have the opportunity to grow. Will you listen to the suggestions offered or disregard them? Listening will require humility, you might have to admit that you’re wrong about some piece of your work. You might have to change how you’ve “always done it”. But it’s the only way to grow.
In most cases, the feedback we receive is done with the best intent. Most of the time we review code, we’re just looking for potential problems – it’s rare that someone has a personal grudge against the submitter – so try to see it in that light. Furthermore, both submitter and reviewer want the same thing: high quality functioning software.
Code reviews are the most effective activity development teams can adopt to drive down defects. Perhaps more importantly, they’re the best activity your team can engage in to transfer knowledge, increase overall development speed, promote a healthy engineering culture, and build a sense of ownership within the team. None of this should be underestimated, healthy engineering cultures are far more effective at attracting and keeping the best developers.
When incorporating code reviews into your organization, remember these four rules:
Whether your submitted code to be reviewed or have been asked to do the review, remember foremost that your teammates all have the same goal: doing good work and creating a great product. You’re on the same team. Encourage one another, hold each other accountable, and regardless of whether you’re the submitter or reviewer you have the opportunity to learn and grow from each PR. You just have to be humble enough to do so.