At last month's DCPHP meeting, we did our first Code Review. I wasn't sure how it would go or even if anyone would participate but when I put out the call for code samples, one of our long-time and respected members stepped up. He provided us with a nice little ~100 line PHP class that was relatively self-contained but obviously part of a larger application. All in all, a great example.
Obviously, one of the things that gets people worked up about Code Reviews – or showing their work in general – is that other people will look at their work.
To be blunt: Well, duh. That's the point of a Code Review.
But let's stop and think about that for a second. How often do you share code with the rest of the world? Even if you're an Open Source developer, you probably don't share your first version with many people until you've been doing it for quite a while. You probably float it around a very small circle of people whom you respect and/or work with – I hope those groups aren't mutually exclusive. After they've picked at it a bit and given you feedback, you tweak, re-write where necessary and commit to whatever repository you have. After you've done this for a while, you've probably learned the big ugly things to look for and you fix them yourself and at some point, you feel comfortable sharing your code without a great deal of “kicking around” internally because you've already done it mentally.
As a result of all this, I had concerns about how the first one would go. In an attempt to set the right tone and get things rolling in a positive way, I laid out some direction:
All criticism had to be constructive – criticizing someone else is easy but rarely useful.
And then there were four types of things we looked for:
- Logical/Fundamental issues – Obviously this includes syntax errors – there were none – but also things related to the proper flow and functionality of the class.
- Security/Performance issues – Just because code works as designed doesn't mean it doesn't do other things you don't want it to. Or to put it more coherently: the code should accomplish the desired task and no more while using the required resources efficiently.
- Best Practices/Standards – This is somewhat of a squishy subject. There are numerous competing coding standards out there and they're competing for valid reasons. Luckily, Best Practices are relatively clear and becoming more established.
- Style/Preference Choices – This is the squishiest point of all. This starts to touch on the design of the code versus the rest of the system, whether the code should support multiple [choose one: operating systems, databases, http functionality] which really only matters if the code is supposed to be re-distributable.
And finally, as I moderated the actual session, I didn't let assertions stand. If they made a recommendation for doing something one way or another, they had to say why. A simple “well, it's better” doesn't cut it when you're trying to learn.
The first Code Review was such a success and there was such interest that we're doing it again this week – September 9th 2009 – at DCPHP.
* If you're interested in the specific and discussion around the Code Review, check out Brandon Savage's post Peer Review: Taking Code And Making It Better or his entire series on Peer Reviews.