Thursday, July 17, 2008

Code Reviews are your friend

I recently had a discussion revolving around the usefulness of mandatory code reviews. My coworkers were of the opinion that code reviews take too much time especially if they're mandatory for every bug fix. The point out that we already have a QA team who verifies that bugs are fixed. I tried to explain that they weren't looking at code reviews correctly, I'm not sure how many listened. During the course of the discussion I wrote down how I view code reviews and why I was excited that we were requiring them.

A code review is not QA validation, it is not for checking that the bug is really fixed. You should not even have to update your system with the code you are reviewing to test it out.

A code review is:

A second set of eyes. This is someone else who can see that you had a copy and paste error that will persist in the system, even if it doesn't cause bugs right now. This is someone else looking at the names of your variables and making sure they make sense. This is someone making sure you didn't get too close to the problem and miss something.

A search for alternative solutions. Your reviewer should be looking at different ways to solve the problem. He should see how you solved things and point out other methods that might have been simpler or cleaner or even more concise. Heck it's even possible your reviewer will question the need for the fix at all (this happens more often when your reviewer is more familiar with the system).He should bring these concerns to you even if there's no change to the code, so that you can have the knowledge for later, or so he can learn why you did things more complicated that necessary.

A look for unintended consequences. As the fixer of the bug you pored through the code making sure your change didn't break anything else, but having a second person who has different areas of expertise will help find more. Also your reviewer might look at the change and realize it has to be put in other parts of the system that suffer the same problem.

More edge case examination. The problem with all programming is what happens in the edge cases. What if that integer is negative? What if the array doesn't contain anything. As the fixer you try to imagine and test as many edge cases as you can. But again having a second set of eyes, one specifically looking for edge cases to break your fix helps alot.

Most importantly, it is a chance to learn new tricks. For both the reviewer and the fixer it is a chance to learn tricks from the other. When reviewing code look at how they solved the problem pick little gems of simplicity and elegance and put them in your own toolbox. As the fixer note when your reviewer talks about simplification or clarification learn how to be better together.

The last argument is why I also suggested that senior programmers should have their reviews performed by the newer employees. All I received were eye rolls at this point, because obviously having newer employees review would take longer (because more explanation is necessary) and defeats the point of the review which is to point out corrections. I think one is the solution for the other. When you take some time to explain your code you see your code as someone else sees it, and often you see its problems. Or the new employee questions will lead you down paths that you wouldn't have explored on your own. That plus the advantage it gives in sharing tricks makes this experience too valuable to pass up.

So take some time, use code reviews in this new light rather than as a verification process. Make sure your code changes are small enough that reviews are meaningful. Step up your new employees by expanding their minds with what you're working on. Keep learning, even from those new guys.

No comments: