Archive for August, 2010

Code Review == Code Smell

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, not addressed in a secondary process after the fact. In this light, code review represents a greater problem — a lack of 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. This lack of vision misses the point that decreased maintenance, avoiding re-work, or improved extensibility can all be cost saving outputs. These things are nearly impossible without software craftsmanship. These things 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 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 between code review, in that problems found are something that should have been handled during development, not after the fact. Also, the further along development goes, the more expensive it will be to fix the problems. 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, decreasing effectiveness. When reviewed by the original developers, they may have moved on mentally, so the team must spend time to shift focus. Just like bug fixing, there’s an incremental cost, depending on how late the problem is addressed.

A better approach is to have developers periodically review each other’s code, 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 that lack of quality will result in 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 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 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 addition to relying on the good habits and integrity of the developers, a team may choose to use of a monitoring tool, like PMD. Teams may also choose to practice pair programming. The benefits of pairing are widely known, and well beyond the scope of this writing. In most cases, it helps to document expectations, to provide references and examples for development. 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 also 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.