Re: Formal code review specifications and reporting format

[pdcawley]

I'm faintly wary of 'rigid' criteria for code reviews, especially having spent some time dealing with different 'legacy' code bases, which all seem to go to hell in their own different, yet depressingly similar ways.

My general approach when asked to do a review is to follow a checklist along these lines:

1. If you can, find out the level of experience of the person whose code you're reviewing. Ask them what they think is good or bad about the code. Make notes.
2. Do a quick read through the code, looking for stuff that jumps out as being particularly bad or particularly good. Make a few notes.
3. Compare your notes about what you liked/disliked with what the stuff the programmer liked/disliked.
4. Quickly outline good and bad things about the code. Try and find something to praise.
5. Arrange the problems in order of importance. If you have things which you think are bad and the programmer thinks are good, or vice versa, treat them as the most important part.
6. Start with the most important problem, explain what the problem is and suggest fixes, bearing in mind the experience of the person you're reviewing for. Repeat until time runs out.

Try, if you possibly can, to do the last three steps face to face rather than writing 'em down. Best of all is when you have access to a whiteboard, CRC cards and an editor window or two, but make do with what you've got.

If you can't do that, write the stuff down. Start with the good things, give an overview of issues to address. Address them, suggesting solutions (or requesting further discussion for the hard bits), and finish by reiterating the good bits and inviting further discussion.

Yes, these suggestions are touchy feely, but I'm a touchy feely kind of guy. Checklists are all well and good, but try and go beyond the 'ticking boxes' Kwalitee Assurance approach.

Try and treat code reviews (either giving or receiving them) as a learning opportunity. Be aware of your reviewee's feelings and don't trample on them unduly. But don't ignore problems either, that's doing nobody any favours.