Combining Code Review with Paired Programming:
An approach by Kevin Klinemeier and Charlie Sheppard
The first rule of the Code Review: You don't leave the code review with more work to do.
The second rule of the Code Review: You don't leave the code review with more work to do.
The third rule of the Code Review: If this is your first time, you have to ... uh... listen. We're not that hardcore.
Code reviews can be stressful, frustrating, and/or a waste of time. This approach attempts to avoid at lot of that by being more like a pair programming session (with keyboard and projector) than a review of a bunch of printed-out code. Take a laptop with your favorite IDE and the latest version of the codebase. Sit down with a group of 3-5developers and pick a few classes to review.
As you review a piece of code, rather than noting suggestions as tasks for later (see rule #1), actually make those changes to the codebase. If they're too large to be made in a single code review, pare the change down to what you *can* do, and do that. At the end, check in your changes and be done with it.
This approach tends to be much more interesting for the participants because there's more interaction, and is more productive because you can see your changes made as you suggest them.
It is important to be serious about the no-extra work rule. This keeps your developers from thinking, "Ugh, we have to go to an hour-long meeting from which I'll gain two more tasks to do this week. Screw that, I don't have time." Sacrifice your changes before you sacrifice the no-extra work clause. Adding extra work in the review causes the review to be skipped in 'crunch time', and ultimately lost along the wayside.
In general, when you start or amend your code review process, let your developers know that they should expect that they *will* find changes. Nobody is capable of keeping all the possible considerations in mind while they're writing software. That's why we do code reviews, and why paired programming is so powerful. Also, even if your software is very, very well written, you don't put several developers in a room and ask, "Hey, what do you think of this piece of code?" and get "No changes" as a result. It just doesn't happen.
The last thing, and this is obvious, but be polite. There may very well be a reason the code is the way it is, or even if there isn't (it really is that bad), it's likely the author and the rest of the team already knows better. Laugh at yourselves from time to time, and overall stay positive. Talk more about what can be fixed, rather than problems left behind.
Tuesday, May 24, 2005
So, despite my February posts to the contrary, last week I finally understood what Mock Objects are. Or more directly, I finally understood what they aren't. Martin Fowler's article entitled Mocks Aren't Stubs really laid it out well. Stubs are used for state-based testing. Mocks are used for interaction-based testing. Using a Mock when what you want is a Stub generally results in the interaction-testing features getting "in your way". This is the case for the www.mockobjects.com mock implementation of the various HTTPxxx interfaces. They add a lot of useful methods for testing interaction, but when all I want is to set attributes, they're "in the way".
Martin Fowler's Article:
Martin Fowler's Article: