Code Review Goodness

I can honestly say that prior to Yahoo!, I never had good code reviews. I think there were several reasons for this.

First, my code reviews weren't emphasized as being important. Sure, when a new project starts, there's the initial enthusiasm to discuss code reviews, but once it gets going and milestones are missed, reviews become secondary and it's far more important to just crank out and release something. The code works so it must be good!

The second is that some people just don't like to have their code reviewed. Maybe it's because they're oversensitive or maybe it's because they're considered "too smart" and having constructive criticism is a huge needle to their ego. These people never have their stuff reviewed or when it's reviewed, it's given a cursory look and a quick nod. I think these people don't go through the review process because they don't feel that it's worthwhile -- it's a waste of time and "cuts" into their productivity.

The third is the whole ambiguous nature of "code review." Really, what's a code review? Well, you're reviewing the parts that makeup your application and evaluating whether those parts are "good enough" to be a part of the whole. We're still ambiguous because what is "good enough"? "Good enough" allows you to set the bar as high or as low as you want and that's the problem. It's arbitrary, but in fact, good code reviews are well defined in scope and purpose.

Code reviews are full of goodness.

First, define what "good" means and stick to it. Focus on best practices or use recognized standards as a guideline for meaning "good". So, if you're a front-end developer, and you're coding for HTML 4.01 strict mode, you'd only want to follow that standard. You don't want to write well-formed XHTML! Likewise, if you're writing JavaScript, you'd want to avoid using eval for security or the with statement for performance reasons. You'll want to cache your variables and reduce function scope. There's of course a whole lot more, but you get the picture.

"Good" also means adhering to standards dictated by your system. For example, I'm working on a project that loads pages in a particular way. So, I have to write JavaScript in a way that optimizes page load. While other ways will work, those aren't the best ways to do it in this system.

Second, it's really important that developers learn from code reviews. Besides creating good software, the purpose of any code review is to teach and inspire. It's not to criticize. It's more of a round table discussion between passionate people wanting to write good code and producing good software. People should come into a review knowing that they'll leave with something useful.

In all code reviews, there should be "experts." I don't mean having a Douglas Crockford or a Nicholas Zakas at your side although they wouldn't hurt. In your organization, find your best HTML, CSS or JavaScript people and have them attend the sessions. They need to be humble, active and leading participants. Whenever a point is brought up, it must be explained why it should be done that way and why that's the best way.

Code reviews are egalitarian. Everyone should have their code reviewed and have a voice in the review. You may be really good at JavaScript, but are you really that good at semantic markup? In my experience as a front-end developer, I've found that most people focus on certain areas of web development while not focusing on others. So, it's always great to get everyone involved.

The other day a colleague was looking over some markup that I had written and commented on how I was using the alt attribute in the image tag and how in this case, it probably wasn't necessary. The text was already in the link next to the image and using alt would probably be more confusing than anything else. I was focusing on the markup and he was thinking accessibility! People will look at your code and think about it differently. It's those extra "eyes" that get you to good quality code.

I've rambled long enough about code review goodness! Now get to it!

Besides, it's also a long weekend!