| « Do your objects talk to strangers? | The Object-Oriented PHP Masterclass Is Back! » |
It’s a well-accepted concept that code review improves the quality of the code produced by your team. Many teams use code reviews, most famously at Mozilla, where every change bigger than a grammatical typo is reviewed by a peer.
Code reviews are effective because they put a second set of eyes on a particular bit of code, and force one developer to explain to another in clear language what that code does. It’s difficult to get an overly-complicated bit of code reviewed, simply because the reviewer may not be able to understand it, and the chances of getting bad code into the system are greatly reduced by using the code review system.
But code reviews aren’t perfect, and can often become problematic if they aren’t totally understood. There are a bunch of common problems that companies face when doing code reviews. Here are some of the most common, along with ways you can solve them in your organization.
Code reviews are almost always conducted via text communications between reviewer and developer. Rarely do the two sit together and review code. This can result in communications challenges between the two parties.
In particular, developers are protective of their work (it is, after all, creative work that took energy and effort). A comment perceived as harsh by the developer can spell disaster and tension between the two parties.
Solving this can be challenging, but it’s not impossible. First, developers be encouraged to seek out in-person code reviews for discussions, which can improve the body language comprehension of both parties. If in-person isn’t available, do it via video conference or telephone call. You may lose something in these formats, but they’re still not as bad as text-only communication. And developers should pair program often with other developers, learning their coding habits and styles. This provides context to comments received on proposed changes, because they can recall the style of the reviewer when reading their written comment.
It’s easy to descend into a situation where the code review process becomes a nitpicking process, focused on finding every little thing wrong with the code instead of improving the quality of the code and keeping out bad stuff. This is dangerous, for several reasons. It undercuts the legitimacy of the code review process. It also frustrates developers who spend hours writing code, and then rewriting it, and rewriting it again. Plus, it takes much more time!
Reviewers need to understand that code reviews are not an opportunity to substitute their logic for that of the developer. Nor is it an opportunity to show how smart they are, or how much better they are at programming. Code reviews are opportunities to spot things that are dangerous, bad, mistaken or wrong in the code, and to offer quality improvement suggestions.
I use a three-tier system for code reviews. Comments that begin with “You might…” indicate that I am making a suggestion, based on my experience. The developer can choose to ignore my suggestion or not, based on their experience. “You should…” comments indicate something I feel strongly about. The developer is strongly encouraged to consider the suggestion, but again, I leave this to their discretion.
Comments beginning with “You must…” are not optional. But these kinds of comments are reserved for security vulnerabilities, coding standard violations, obvious bugs, or other serious code problems. These are the only blocking code review comments, meaning that all other code review comments can be ignored, but these must be dealt with to obtain my approval for the patch. Everything else gets a thumbs-up.
Why do I do it this way? Simply put, any developer on a team with me was hired for a reason, and is presumed to be competent. If the quality of their code is not up to par, code review isn’t a place to fix that; they should be retrained or dismissed. Otherwise, it’s assumed that their code is already of acceptable quality, and the code review process is to catch things they may have missed, not considered, or outright screwed up (we all make mistakes).
Because the difference between a developer and a reviewer is usually only their relationship to a particular change proposal, possible reviewers often get busy and miss open patches. And so, review requests can go unanswered for weeks, or even months. This slows down development and can even result in bitrot (where the patch is no longer applicable to the project or the project has changed to the point where the patch can’t be applied). This wastes everybody’s time.
Almost every team I’ve ever worked on has a status meeting, daily or weekly, to address the status of the project. Include a time for reviewing outstanding requests, and making sure they’re all under active review. This was part of our process at Mozilla, and it worked effectively. In many cases, pull requests still sat around, but not because there was no active reviewer; they were works in progress, or changes that needed more time. No pull request went unreviewed for more than a week, because we always checked their status at our weekly meetings.
Often, developers review code differently from other developers. They have unique nits, different styles, and their own opinions. The same code can be held up by five different people for different reasons, and approved unchanged by a 6th. This is clearly sub-optimal.
How do you fix the subjective nature of code reviews? The answer is complicated, because you can’t fix the subjective nature of individual developers, but there are tools and techniques that can help.
First, consider a checklist. Many different fields use checklists to ensure that different people perform the same operations in the same way. Pilots, doctors, judges, all use checklists for this reason.
In addition, you can consider using static analysis tools like Code Climate to evaluate pull requests and highlight problems. Developers should use the tool, and then defend anything the tool flags as problematic. These become discussion points between the developer and the reviewer.
Finally, develop standards for what is absolutely unacceptable in a pull request. Some basics include security vulnerabilities, code that breaks tests (or has no tests of its own), obvious bugs, code that doesn’t meet the specification (if you have one), and code that doesn’t compile. You can add standards as you want. These standards should be items that can’t be automated, but can be seen by skilled developers.
Code reviews are great for improving quality and keeping silly errors out of your code. And yet, they require care to ensure that they continue to be productive and effective tools. Like any tool, they can become more of a drain than an aid, and it’s up to us to ensure that we fix the problems and keep their usefulness.
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 1/2/2015 at 11:25 am
Eloi (@eloipoch) wrote at 1/5/2015 5:27 am:
I just want to add that I began, as you explain, with “You …” but later I changed to “We …” because we are a team and maybe who improves the code is another dev or myself as reviewer or the feature had been developed in pair
| « Do your objects talk to strangers? | The Object-Oriented PHP Masterclass Is Back! » |