Thursday, July 15, 2010

Code Reviews, the Lost Art

Nearly every software development shop worth its salt has some form of what is known as a code review and nearly every developer dislikes them. Most matured developers tend to like the idea of code reviews but given the choice, on there next commit they would likely opt to not send their code for code review. The reason why is simple, code reviews can delay the forward progress of the software and they take time. When you have other developers needing access to the library you just wrote it is hard to say we need to take a few hours to a week of our time to look over my code. I think we should be able to make code reviews better and into something everyone wants to do.

After a code review I often finding myself wondering was what was found worth my time and the reviewers time? Most of the code reviews that I have been apart of have had minor suggestions or more commonly code standards compliance problems. When you rummage through several hundred or a few thousand lines of code during a code review and all that is found is that you have a few extra blank lines or should change the name of a variable, it does seem like a bit of a wast.

I'm not saying that we should not care about those extra lines or any code standard for that matter. I'm a big fan of code standards, I think they help in the readability of code. I'm saying that there is a cost to code reviews, we have to weigh those costs against the rewards. When a reviewer only finds a few compliance issues, things that could be fixed by anyone that is reading through the code, it was not worth the time the reviewer spent reviewing.

So how do we make code reviews worth everyone's time? Simple, we change the intent of a code review back to what the actual intent was. Code reviews are put into place to find bugs. Bugs that would show up to an end user or other developers that are trying to use the code.

You may say, “well Cory that is what every code reviewer is doing, they are looking for bugs.” However that is not true, sure they are looking for obvious bugs like unassigned variables being referenced, but they are not looking for deep bugs. One of the most common bugs comes from input validation, and yet it is a bug that is often over looked in code reviews. This is because it is often difficult to tell exactly where input to a function is coming from and how much it should be trusted. Detecting multi-level bugs requires a reviewer to see how the multiple levels interact and the path of the code in correct and error states. This kind of review takes a lot of time and drastically increases the complexity of a code review. The sharp increase is due to the fact that we are moving a code review from a mostly passive practice to a very active process.

Obviously code reviews could not detect all bugs and there will be times when a code review will not find any bugs. From this active process of a code review you get a new found level of confidence. Bug counts will be decrease and actual development should increase. This confidence is how a code review pay for themselves.