Code Reviews in Practice

Code Reviews in Practice

Code reviews are one of the best things you can do to improve code quality within an organisation, but the process isn't a silver bullet, and it isn't guaranteed to succeed. These are my observations on what works and what doesn't work when it comes to implementing code reviews into a software development environment.

I've worked at 3 companies now where code reviews have been introduced. At the first two it was a failure, but for different reasons each time. Only the third company has fully integrated code reviews and unlocked the full benefit of them.

Company A

There are different ways of implementing code reviews and the implementation plays a huge part in whether the process will be successful in the long term. At company A code reviews were implemented as a pre-commit script that checked for the phrase "Reviewed by " in the checkin comment. If the phrase wasn't present then the commit would fail. At first this worked fairly well, but it wasn't long before the process broke down.

The coders started to buddy up for the reviews and the reviewing became cursory at best. Everyone had their review buddy and eventually the "review" became a matter of just informing your buddy what you were checking in. Eventually people stopped bothering at all, even sometimes putting their own name as the reviewer.

The problem here wasn't really the process itself, it was more a matter of management and culture. It was never made clear to the team why this process was being imposed on them or what the benefits would be. This led to resentment and resistance towards the review process.

It is vital to get the team to understand and commit to the process on an individual basis.

For example, it was thought that the reviews were largely meant to catch coding errors. While reviews can catch coding errors they are also meant to improve code quality. That might seem like the same thing, but it really isn't, although the two are related. Code can be completely technically correct, while being still being low quality in terms of readability, maintenance and error handling.

Improving code quality is meant to make the code more robust and maintainable so that bugs aren't introduced later. We do this by applying a standard. This helps to keep things impersonal. Everyone has to stick to the same standard.

Over time the company culture might have been changed so that the team accepted the review process, but since it wasnt enforced the process withered and died.

Company B

At this company reviews were implemented using actual review software. I don't recall the name of it, but you would submit your changes to the server and then wait for people to give feedback. This kind of process is better suited to cold reviews as it can take a long time.  

The reviews often became rambling discussions of syntax as everyone gave their $0.02 on the code. These discussions are best kept out of the review process in order to keep the review as short as possible. This is also another reason why having a coding standard is important, because then the reviews can concentrate on more important matters as syntax/formatting issues are resolved quickly.

The culture here was more suited to code reviews, but ultimately the process died anyway. It was simply too time-consuming. 

Company C

Within this company the review process is widely accepted and recognised as playing a vital role in maintaining the integrity of the codebase. In terms of implementation the process is almost identical to that of Company A. The differences are purely in terms of company culture and management involvement.

There is a culture of commitment to quality that ensures the coders buy in to the process, understanding why the reviews are important. Other factors that help the process are an enforced coding standard, smaller commit sizes and enforcement from management.

Coding standards are a good idea whether you have code reviews or not, but the two complement each other nicely. Without code reviews, the standard might not be applied as rigorously as we'd like.

Without the standard, the code reviews could turn into free-ranging discussions of unimportant syntax details. The details of the standard (edit: in terms of formatting) don't really matter, it just matters that you have one. Every developer should have the maturity to adapt their coding style to the standard.

The smaller commit sizes help to keep the reviews short, as well as ensuring that all the coders are keeping in synch and keeping merge conflicts to a minimum.

Conclusions

The two most important factors to the success of a code review process are:

Keep the reviews short and sweet. This means conducting the reviews face-to-face if possible, keeping commit sizes small and doing most reviews one on one.

The process requires involvement from management, in terms of coding standards, but also in terms of getting the coders to buy-in to the process and understand why it is important.