Pramatr Blog

Reviews are a widely used technique to analyse code for the presence of defects and potential improvements. Many successful teams continually review code to try to ensure a high level of quality and to constantly improve a developers ability to write good code. The arguments for and against code reviews have been made on many occasions, but one common unknown factor for many teams is the frequency at which they should take place.

Infrequent Reviews

Many teams operate on a feature complete code review. At the end of the cycle of work, several developers sit down together with the author critiquing the delivered work. What often transpires here is that due to the huge mass of code delivered, the chances of a thorough review are rendered utterly impossible. The plethora of code makes it difficult to find a starting point and thus potentially problematic code isn’t allocated the time it necessarily deserves. Any potential recommendations that come out of this kind of code may never see the light of day given the pressing work schedule (if the code was complete why are the changes necessary?). Most importantly for the team, reviewing code so infrequently can lead to demoralisation of its members.

Code reviews may pick apart the authors code; code that they have potentially sweat and cried over (ok maybe not) and worked hard to complete. Does it really make sense to save up all the potential criticism and deliver it in one skull crushing blow? Even if the criticism is perfectly valid, nobody likes to feel good about something only for it to be completely torn apart. Developers complain when customers only let them know what they actually want when they see what they don’t, is the same really acceptable for the code? If infrequent code reviews aren’t the answer, how often can code be successfully reviewed?

Email Reviews

Some teams never let the author of the code commit it to the repository. Instead, they email the code (typically in patch form) to the code owner or one of the principal reviewers. The idea behind this approach is the patches are always reviewed prior to them being committed, and the developers can work productively only focusing on the task in hand. This process has the ability to ensure that every change is scrutinised and verified to maintain the high quality standards that are set down. Having had to work with a system like this in the past, I can honestly say that it was fraught with problems and was one of the most frustrating I’ve ever worked with (I was one of the principal reviewers).

The person applying the patch quickly becomes a bottleneck in the process, how long can people really wait for the patch to be applied? What should the patch reviewer actually do with the patch, make the recommendations themselves or send an email back to the original author to apply the required changes. If multiple people are working on the code base, the chances of conflicts increase. If the patches are applied in the wrong order or peoples timing is just plain unlucky, the person applying the patch has a multitude of problems to deal with. The version control system is full of only one persons name, thus making it incredibly difficult to track down the original owner of the change. Lastly should the worst happen and the build fail………………….. guess who’s change it was that broke it?

Frequent Reviews

People like to know what and how they are doing with their work. If infrequent code reviews lead to a big bang delivery approach, then frequent code reviews take the inverse approach of small nudges in the right direction. By applying these frequent reviews, developers can deal with smaller suggestions and recommendations early in their development. Instead of developers feeling they have completed something only to be told it’s all wrong, they can be guided along the process to ensure they arrive at a right answer. The most difficult question here is; how frequent is frequent? This is a very developer specific metric.

Some developers require frequent attention and when I say frequent I mean every few hours (or more!). As developers become more experienced, this frequency typically reduces until the reviewed eventually becomes the reviewer. Depending on the type of project however, it’s still quite common for very experienced developers to like frequent code reviews. If frequent reviews become very frequent reviews, you might have unwittingly found yourself participating in quasi pair programming.

Pair Programming

Pair programming is not only a great way to develop but also to implicitly review code. A second developer sitting at the keyboard provides instantaneous reviewing of code. The second developer not only reviews the code, but also looks for potential problems and improvements to the evolving code. The second developer isn’t always necessarily the reviewer, and roles can switch between either developer during the exercise.

Having someone take over the keyboard encourages the development to be of higher quality and a second set of eyes prompts the coders to produce good code (obviously given a good pairing of developers). This instant review and feedback can actually reduce the number of bugs introduced into the system. As several developers produced the code, it also means that the team is never reliant on a single developer to address a given area of code. Pair programming is an excellent way of developing an reviewing code, but it’s not without it’s problems with some people finding the feedback is just too often.

Summary

By reducing the time between code reviews, teams can provide better guidance about the eventual quality of code and prevent storing up potential problems. The less frequent the code reviews are the more problems (especially with less experienced staff) that occur. Developers want to feel like that are doing a good job and as such then need small bits of constructive criticism often instead of lots of criticism delivered all at once. Reviewing code is best addressed frequently, providing quicker feedback, and reducing the amount of rework involved. As a developers ability increases, these issues typically subside and they become active in reviewing other peoples code.

If infrequent code reviews are problematic, are frequent code reviews the key to success?

Over the past few years, I’ve worked on numerous projects that have grown very quickly but have lacked a structured approach to development. The code is grown somewhat organically, being used in ways never imagined and bent into shapes it was never supposed to make. In the very worst cases, it ended up with Spaghetti Code that is self-obfuscating and unintelligible (even to the original author).

To help prevent this situation, teams need to embrace processes to help introduce structure and discipline to software development. This can be daunting to some teams at first, but it’s a far more productive and appealing than a team that is constantly under-pressure with various fire-fighting activities.

Kent Beck has said previously that he is not a great programmer but just a pretty good programmer with great habits. I’m a firm believer in that statement, and I think that any team that doesn’t try to develop those habits is starting from a weakened position. I’ve worked on both projects that embraced change and tried to improve their development process and also those that didn’t. I would sooner work on the former type of project every time.

With that in mind, there are very simple changes you can make to a project to help foster better habits. None of these techniques is new or groundbreaking (at least anymore), but I am still amazed at the number of projects that still don’t embrace them. There is plenty of information on these techniques out there, so there is no excuse if you want to do some more reading.
Read the rest of this entry »