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.

In reply to Re: Formal code review specifications and reporting format by pdcawley
in thread Formal code review specifications and reporting format by Nuke

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.