Good code reviews are almost an art form. It takes a lot of discipline and, frankly, a lot of time to do a good review. Pull requests (in the style of GitHub or Stash) have made the mechanics of a code review significantly easier, but that doesn't mean you should slack on the review process itself. There are several good practices that you should follow, and several bad practices you should avoid at all costs!
Bad Code Review Practices
Let's start with some of the bad practices. Let's be fair, most of us want to know if we're doing something horrible, before learning to do something good. So, here's a few key things to avoid:
Reviewing Too Much at Once
This is probably the easiest trap to fall into. As a developer, you want to put a cohesive work product together for review. However, if the change is too large, most reviewers will just skim through it, looking for obvious defects. If the change is small, it's easier to spend just a couple minutes looking through it for issues.
Cisco, with a company called SmartBear, did a review of code reviews. They spent 10 months looking at the relationship between size, length, and defect rate of code reviews. What they found looks like this graph:
This shows that above ~400 lines of code, there were practically no meaningful defects filed. As the lines of code dropped, significantly better feedback was provided. Drop the size of your review!
Reviewing too Quickly
If you're trying to blaze through the pull requests, you're also likely to do a poor job. That same study that compared defect density to lines of code also looked at how much code you review per hour. This is what they found:
Notice how the defect rate drops the more LOC/hr you are trying to accomplish. This shows that ~500 LOC/hr is the most you can review and still be effective. Combining this with the previous graph, and ideal situation might be 2 reviews each around 250 LOC in one hour. And that's a full hour, not 10 minutes per review. Set a timer, because you're probably not spending enough time on them!
Reviewing Code Style
This one can be difficult. A lot of people don't want to give tough feedback to colleagues in the hope that the feedback they receive is similarly dampened. It helps nobody to sugar-coat the truth. Some developers need more feedback than others. It's easy for some people to get their feelings hurt, or feel like they're under a personal attack if their review goes poorly. Some developers fear giving feedback to more senior developers for fear of looking dumb or being laughed at. Some developers are not "people person" material, and the reviewer may want to avoid getting on their bad side. They may fear being labeled "not a team player." They may fear delaying the release because of a bug, and get pressure from somebody (usually project owner or scrum master) to "just sign off on it so we can make our release."
This really boils down to professionalism. It may take longer for some people, but you have to internalize that a code review is not a reflection on you as a person, nor your ability as a programmer. Every programmer makes mistakes. Separating your ego from the review process is a critical skill that developers must learn and develop over time. In addition, even senior developers need feedback. They're not perfect, either, and sometimes they will miss really silly issues. You can't be afraid to give or receive feedback, or your product, and therefore your business, will suffer.
Good Code Review Practices
Ok, now that you know all the stuff not to do, let's focus on what you should do to get the best bang for your buck. We already know an ideal timeframe and LOC target based on the above data, but there are some practices to make it even less painful:
Make it a Part of Your Process
Code reviews done after a commit to the mainline are unlikely to get feedback that will change the code. Code reviews done before a commit stall the developer from working on future progress. How do you balance these? Pull requests!
Interruptions are the bane of a developer who needs focus to complete their work at the velocity managers want. By making code reviews asynchronous, you allow developers to keep working. By developing in a story-specific branch, you can have both source control and before-commit semantics on your mainline.
Note that this being built-in to tools like GitHub and Stash are a compelling enough case to get your legacy projects off of SVN and migrate them to Git.
Given that you should be OK receiving and giving feedback, what happens when the author of the code doesn't agree with your feedback? As soon as a review is done, the negotiation begins. You will each make a case, and the worst thing to do is compromise. You're each agreeing to not get what you think is right, in order to move on to something else. That's a sure-fire way to drop your code quality. There are 3 ways to resolve this conflict:
- Reviewer retracts comments -- If the reviewer talks with the author and realizes his comments were in error or are not actually issues, he should retract them. Then there's no conflict.
- Reviewer stands their ground -- This may mean you want the author to go back to the drawing board. Or maybe, you are arguing about a known issue. For example, you noticed a potential SQL injection issue, and refuse to let it through the gates. If you both are standing your ground...
- Bubble up to manager/architect -- If your project has an architect, you can bring the issue to her and let her decide the correct approach. These situations are usually more agreeable to the reviewer than the reviewed, as the architect usually doesn't want to compromise quality.
Stick with these three paths of resolution, and your quality should stay high. In addition, the negotiation may actually be a learning opportunity for both parties. One of you may decide to retract an objection when you learn additional information. Code reviews are not an attacker/defender relationship, it is a collaborative peer/peer relationship. Treat it like that.
Hire Like You Mean it
This one should be relatively straightforward: don't hire people that bristle at code reviews. In any process, there are some people who simply don't want to follow it. In Agile, people complained about commitments and sizing. In waterfall, they complained about the overly-long design time. In XP, they complained about having another person sit so close. Even if you lock them all in individual offices, they will complain about the lack of communication.
This can be fixed in hiring. If you ask a candidate about code reviews and they scoff or talk down about them, and you're trying to promote them, that should be a no-hire. They will find a home somewhere else that doesn't do code reviews -- which is probably a majority of companies. If you care about your product, hire like you mean it. Walk the walk.
If you're programming for a company, that company owns the code you write. However, many developers treat the code they write as their property. They'll throw a fit if somebody else tries to mess around in "their" code. These people are likely to cause the same issues as described above. Lack of ownership means they're likely to take feedback objectively. If a developer doesn't want to do code reviews because "they wouldn't understand it" or "only I know what it does," that's a huge red flag for your team, and a huge red flag for your product. What happens when that guy leaves for greener pastures? Who's going to take over the hairball of code that he didn't let anyone poke at?
Prepare for Code Reviews
As the author of a piece of code, you should try to make sure you get the most meaningful feedback possible. That same Cisco study found another interesting correlation:
This shows that if you leave 0 comments, you're likely to get a significantly higher defect rate. Tools like Stash and GitHub allow you to add comments to a code review. This sort of prep might help alleviate some concerns right off the bat, or they might be used to justify a design decision that is unclear from just looking at the code. Much in the same way a negotiation is a learning opportunity for both parties, prep comments are a way to make sure you understand what your code is doing, and can explain it to other people.
Code Review Checklist
This one seems super simple, but it's remarkably effective. Most people will make the same few mistakes consistently, despite trying not to. It's helpful to keep a checklist so you don't forget to check your blind spots. Some common checklist items might be:
- Unit Tests -- Make sure unit tests exist for any new code. For bug fixes, see if there's a test that verifies the bug fix.
- Method Arguments -- Make sure the method arguments make sense. Do they have boolean arguments? Do they have 20-argument functions? Remember, you're trying to keep your quality high!
- Null Checks -- This probably doesn't need to be said, but you should inspect all method params and return values to see if null references are handled correctly. In Java, you can use the @NonNull annotation to get compiler help on this.
- Resource Cleanup -- If the program is modifying or creating files, make sure the files are closed gracefully. If they are opening a transaction, make sure it gets committed or rolled back. If they're opening a stream, make sure it gets closed or consumed before moving on.
- Security Concerns -- This is becoming a bigger issue in our industry. You should check to make sure your front-end code properly escapes user input, avoiding XSS attacks. Validate your inputs. Make sure you're encryping and salting passwords, Make sure you handle PII safely, and try to avoid collecting more than you need. If you're using a C-like language, make sure to check your buffers for potential overflows, or use a known safe library to consume your buffers.
Mix Up the Reviewers
You want to avoid having the same author's code consistently reviewed by one or two reviewers. Ideally, you would cycle through reviewers, making sure everybody gets a chance. This prevents any one reviewer to going code-blind to defects in another author's code, and makes sure that all developers have some basic knowledge about other parts of the system.
A good code review requires effort and time, but the benefits are real. There's a famous book called "Code Complete," that cites the following:
.. 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. Case studies of review results have been impressive:
- In a software-maintenance organization, 55 percent of one-line maintenance changes were in error before code reviews were introduced. After reviews were introduced, only 2 percent of the changes were in error. When all changes were considered, 95 percent were correct the first time after reviews were introduced. Before reviews were introduced, under 20 percent were correct the first time.
- In a group of 11 programs developed by the same group of people, the first 5 were developed without reviews. The remaining 6 were developed with reviews. After all the programs were released to production, the first 5 had an average of 4.5 errors per 100 lines of code. The 6 that had been inspected had an average of only 0.82 errors per 100. Reviews cut the errors by over 80 percent.
- The Aetna Insurance Company found 82 percent of the errors in a program by using inspections and was able to decrease its development resources by 20 percent.
- IBM's 500,000 line Orbit project used 11 levels of inspections. It was delivered early and had only about 1 percent of the errors that would normally be expected.
- A study of an organization at AT&T with more than 200 people reported a 14 percent increase in productivity and a 90 percent decrease in defects after the organization introduced reviews.
- Jet Propulsion Laboratories estimates that it saves about $25,000 per inspection by finding and fixing defects at an early stage.
Code reviews are another tool in the belt when you're focusing on quality. Throw in automated testing and automated deployments, and you're about ready to start continuously delivering your software.