Smells Like Code Review

On 2010/08/09, in signal, webdev, by a b

I’ve been engaged in various forms of code review over the last few months, and have come to the conclusion that, while it’s better than nothing, teams should wonder why there is the need for code review. Craftsmanship should be integrated into the act of writing code, and teams should be collaborating in real time. Neither of these things should be addressed in a secondary process, after the fact. In this light, code review represents a greater problem — a lack of team unity and a commitment to craftsmanship in the coding process. If you find code review is time consuming, or difficult to integrate into your process, it might help to address issues that are closer to the root of the problem.

Most teams will claim some level of attention to quality, but not many make it the foundation of their work. All teams should be delivery focused, but take this too far, and craftsmanship can quickly suffer. Worse, management has a bad habit of failing to see the value of craftsmanship. ‘Faster’ is always the watch word. Development time should deliver as many features as possible. Similarly, remote resources are common. Even when a team is entirely local, they may be working in an environment that is not conducive to collaboration. These shortcomings are common ‘cost saving’ tactics. Ironically, the goals of decreased maintenance, avoiding re-work, or improved extensibility can also be part of a cost saving strategy. These things are nearly impossible without collaboration and software craftsmanship., and cannot, cannot be rushed. While there does need to be a balance, if quality is compromised, code review is often used to keep craftsmanship from being ignored all together.

This may be easier to swallow because software development has a long tradition of testing for defects and applying fixes defects well after the code has been delivered. There is a strong parallel post-delivery QE and code review, in that problems that should have been handled during development, are instead handled after the fact. It’s commonly accepted that, the further along development goes, the more expensive it becomes to fix problem. The same principles apply to code review.

In this light, the most costly and least effective approach is to review code after delivery. When reviewers are not the original developers, the code will be harder to understand, increasing time spent and decreasing effectiveness. Even reviewed by the original developers, if time has passed they may have moved on mentally, so the team must waste resources shifting focus. Just like bug fixing, churn has an incremental cost, depending on how late the problem is addressed.

A better approach is to have developers periodically review each other’s code before delivery, preferably at some specified interval. While still not ideal, this at least takes place closer to when the code is written. Review might be done on completion of a feature, or at the end of a sprint. There are tools which can watch SCM, provide structure for the review process, highlight changes, track comments, and manage the workflow of the review. Again, time will be lost to to the review process, along with the cost of switching from code writing to reviewing and back.

Notice a trend of time being spent? Once we accept a disconnection between developers, or decrease in quality, the result is time being spent on either code review, or defects. It seems obvious what the best choice is. Rather than spend the time in a reactionary way, time could be spent making an investment in the quality of the code. The need for review is the ‘code smell’ that indicates not enough investment is being made during development.

How do we make ‘review’ part of the actual development process? First, teams need to be able to collaborate. Pair programming is a huge part of this. There’s research to suggest that this practice does not add a significant increase to overall development time. To enable this, teams would ideally be local, and supplied with a working environment that encourages collaboration.

We also need to decide what craftsmanship means. There are entire books on the subject, but it’s not hard to figure out some basics. A first step could be agreeing on consistent formatting. Beyond that are more standards, like package naming, the organization of the code, and other development practices. At the most comprehensive, a plan around craftsmanship should also include architectural strategies and patterns. Ideally, there is an ongoing dialog within the team, where discussion of the code’s design is synonymous with deciding how the principles of craftsmanship will be applied.

Once these standards are set, there are various tactics for ensuring they are implemented in real time. In most cases, it helps to document expectations, to provide references and examples for development. In addition to relying on the good habits and integrity of the developers, a team may choose to use of a monitoring tool, like PMD. Individuals should feel they have the time to apply these practices at every step of development. If development is rushed, this is usually the first thing to go, so management needs to provide the necessary breathing room.

Striving for craftsmanship in real-time will bring up the quality of delivery, lowering the amount of defects, which will further the return on the investment. This process may need to be backed up with an occasional code review, preferably a causal peer review, prior to delivery. The need for review should decrease as the commitment within the team grows. In addition, as the team improves, the findings of review will become less significant, meaning less time will be needed to make corrections. Essentially, your reviews will smell better.


Comments are closed.