Code reviews seem to be the bane of many developers. Very few developers that I know like to participate in code reviews. Once they do participate, the criticisms about the code are superficial. Some examples are criticizing the lack of comments, violations to the naming conventions in the guidelines, and even the formatting of the code.
To top it all off, if you work in a shop that first presents an online code review to become familiar with the code, then a formal meeting to discuss the code, little to no prep time is spent by the reviewers. This is an enormous waste of time. How can a code review be valuable. More importantly, what can you do to change your companies culture, to not think of these as meetings of despair?
Eliminate the Superficial Aspects
Get rid of all of the things that sit there on the surface, that make code reviews appear to be a waste of time. Think of code reviews as M&Ms. If you don't know what M&Ms are, that may make this analogy even more poignant. They are small chocolate candies with a thin candy shell, and they have an 'm' printed on them. The thin candy shell is just there to keep the chocolate from melting in your hand people. The superficial aspects of a code review that developers tend to focus on is like that thin candy shell. There really is chocolate inside of a code review, metaphorically speaking.
Tools
I mean software tools, not your engineers. Many code analysis tools exist, both commercial and open-source. These tools can inspect the code both statically and dynamically. That there should be no reason for a developer to have to point out violations of the coding guidelines, variable names, formatting, use of forbidden language constructs. These tools can be run automatically, and they are highly configurable. I agree, if this is what your code reviews have consisted of, it has been a waste of your time.
Excuses
We all like to believe that excuses simply make everything better, therefore, we don't have to feel guilt or can't be blamed for something. An excuse is usually a misappropriation of logic to rationalize something. A person can only create a finite amount of logic. Don't believe me, look up the definition of death. What if you applied all of that logic wasted from excuses to efforts that could improve your software?
The Meeting
Many times, the meeting simply becomes a formality. If you have a good collaborative code review tool where developers can review the code, make comments and have discussions over the course of a few days, this will definitely be time better spent than having a meeting, and you could eliminate this formality altogether. The code review tool will record the entire discussion and even allow you to generate the action items that must be completed before the code is accepted. The details of the review can then be tracked and referenced later if needed.
This form can be especially beneficial for introverts. Introverts generally prefer to think about their answers before they speak, or may not arrive at an answer before the conversation has moved onto the next topic. This can give them more time to arrive at the answer, or question they are looking for. I personally believe there are many benefits to gathering the reviewers in a meeting. However, I will address that in a different section.
Effective Reviews
Make things as simple as possible, but no simpler.
Albert Einstein
Effective reviews are both constructive and concise. The purpose is to increase the overall value of the project, the code. You increase value by ensuring quality. If the code reviews your team has tends to focus on the superficial elements of the previous section, your reviews are too simple.
Note: To authors of the review:
Hopefully your team can hold constructive and useful reviews. Here are a few quick tips to keep in mind:
- Do your best to avoid becoming defensive. Sometimes, it may feel like a personal attack. In most cases, it's not. Even if it is, take charge of the discussion and keep it focused on the code. Proactively ask for feedback. Is there anything that you could have done better? This is especially helpful when your team has not broken passed the thin-candy shell of code reviews.
- There's no need to make excuses for your code when someone else points out a defect. If it was a mistake, all that's required is for you to correct it. If it's something that you don't quite understand, ask the person who pointed it out to elaborate. This is a learning opportunity.
- I don't see people do this often. Point out code that you are proud of, or is the result of solving a difficult problem and how you arrived at that solution. The others many not realize the work involved for you to simplify a nasty problem to such a simple and elegant solution.
Constructive
To be constructive, you must build. This isn't the time to tear apart the author's code and destroy their every sense of self-worth. Besides, you don't need an excuse like a code review to do that. You can do that anytime you want to.
Leave out "You"
It's very easy for a person to attach their identity with the work they produce. This is especially true if they are craftsman like a software developer. In your explanations and reasoning, try to focus on the issue itself. Discuss the issue, what effects it may cause, and potential ways to resolve the issue. Yes, it is a defect, that just happens to be in this code. However, it's not a personal attack, and the issue itself can be rectified. This is about improving code quality, not fixing the social issues you have with a co-worker.
Make Suggestions
I have adopted this approach from Scott Meyer's Effective Series of books. I try to avoid telling others how to do things, with the exception of when I am the team lead or architect and the implementation is unacceptable. Simply adding the work "consider" at the beginning of your sentence is all that it takes in most cases. The choice is the author's. Most of the time, they will respond by following your suggestion. If they don't use your suggestion, remember, this is their work not yours. There's no need to take it personally.
Add Compliments
Just like in book and movie reviews, the critic will usually try to point out any redeeming qualities even for worst pieces of work that they review. The same should occur with code reviews. For the most part, code is just code. Every now and then, there's a defect. Hopefully, there are some highlights to be able to point out as well. This piece of advice is especially important for the leaders and architects of the team. There are two reasons that this piece of advice is valuable:
- Affirm to the author that you recognize value in their work, and you are not simply looking for flaws.
- Highlight examples of good practices and ideas that you think others should follow. These diamonds in the rough may otherwise go unnoticed.
Concise
Assuming that you want to spend as little time as possible on a code review, this section provides some suggestions that may help your efficiency.
Independently Reviewed
Have each member of the review, inspect the code independently to optimize the amount of time spent reviewing code. You provide the opportunity for people to tune out if your code review is a meeting where one person navigates the display, One or two may participate, the others may be checking their email, or posting selfies on Facebook. Reviewing independently is not only more efficient, it also often produces a different collection of issues spotted by each reviewer. So a more thorough review occurs.
Divide and Conquer
In general, I think that developers tend to write code in feature sets that are too large. This causes much more code to be inspected, and boredom to set-in as each file is reviewed. Soon it all looks the same. If this review were like counting to 100, it would sound like this: "One, two, skip a few, ninety-nine, one-hundred".
Solve this problem by dividing up the responsibilities among the reviewers. One could focus on resource management, another on correct integration into the existing system, while a third focuses on overall style. These roles could be assigned to each developer for a subset of the files, and their role changes for a different set of files. There are many ways this can be done. However, assigning specific tasks will help insure that each reviewer doesn't focus on the same element.
One thing I would definitely not recommend, is splitting up the files and only have one person review that subset of files. You miss out on the potential benefit of the diverse background and experience levels of many people reviewing the code. We all have learned and arrived where we are at by different paths, just as we have all been bitten by different "bugs" that have changed how we develop and what we value in our code.
Where's the Value?
I have often heard "The value of something is the price you are willing to pay for it." Time is a precious resource, in fact, it is the resource that I value the most in life. When I consider my work day, it is also the most valuable resource. There are only so many things that I can accomplish in a fixed amount of time. It is important to remember that you are a part of a team developing this software project. This is bigger than anything you can create by yourself in a reasonable amount of time. Investing time in an effective code review, is investing in both your team and the quality of the code that you work in.
Improved Quality
Hopefully it goes without saying, there is an opportunity for the quality of the code to be improved. If your code is not improving because of the code reviews that you hold, let's work under the assumption that your organization is doing it wrong. With the suggestions in this article, is there anything that could help improve that? If not, there is more likely a more fundamental issue in your organization that needs to be resolved before code reviews can contribute.
Addressing Issues While In Development
It is often easier to fix a defect, while you are still developing the code that contains the defect. That is one of the purposes of the code review. Why can't it be fixed later? It may be possible to fix it at a later time; when it is discovered. Hopefully, that is not when you are on site with a customer trying to provide answers.
What if it's not a defect, but a fundamental implementation issue. One that relies heavily on global values and made most of the object hierarchy friends with each other. Some things become almost impossible to resolve unless you have dedicated resources to resolve it. At this point, you do. So take advantage of them.
Transfer of Knowledge
This is one that I do not think many people think of at all. So much focus is placed on the author as being the center of attention, the focus of criticism in fact, that many developers do not think of this time as an opportunity to learn something themselves. You may learn a new technique. You may learn about a set of utility functions that were not housed in the most convenient location, and now that you know of them, your tasks will become much simpler. Programming techniques and system architecture just scratch the surface of potential for what you can learn.
It's a Waste of My Time
That's funny. This seems an awful lot like an excuse... Let's put it in perspective. Think about all of the hours you have spent watching reality TV over the last decade. Compare all of those hours devoted towards Honey Boo-Boo and Pregnant at 16 with time spent on code reviews. Which do you think is a bigger waste of time?
Attitude
The first thing that needs to change is the attitude and perception of the review. Rather than stating "It's a waste of my time.", ask this question instead "How can I find value in this?". Positive outlooks tend to beget positive outcomes. If you do not expect to get anything out of reading code written by someone else, you'll probably spend that time looking at pictures of kittens on The Internet.
I realize this advice may seem like something you teach your child, but as adults, we're just as prone to becoming jaded and stuck in a rut with our opinion and attitudes. There's also the chance that any form of meeting or social interaction is waste of your time; that can change too. Before you even start the code review, you're criticizing the processes of the code review.
Safety Precaution
First, appreciate that bit of irony.
Next, if you ever go work in the aeronautics industry, please send me an email and let me know what company you work for. Because every line of code is scrutinized multiple times. And once code has been blessed, it is very difficult to go back in and make changes. I prefer to not fly on planes where the developers believe that code reviews are a waste of time. Similar processes are also in place for DoD contractors, and where the potential for bodily harm or the loss of life exists like with medical devices and nuclear production facilities.
Summary
Code reviews can provide value if you apply your time spent towards constructive activities. There are many valuable aspects to a code review, beyond verifying the code. This is an opportunity for all participating members of the team to learn new and better ways to solve problems. The knowledge about how the system works can be spread amongst the participants. It can also be an opportunity to discuss the non-tangible aspects of the development that does not appear in the final code.
There is value in performing code reviews, and you do not have to dig to deep to find it. Mostly, it only takes a redirection of your energies, and for some a minor attitude adjustment. Formal code reviews are not always appropriate. Sometimes a buddy check will suffice. Either way, good judgment is required.
