Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl-Sensitive Sunglasses
 
PerlMonks  

How should I do (and document) effective semi-formal code review?

by radiantmatrix (Parson)
on Jun 18, 2008 at 20:12 UTC ( #692787=perlquestion: print w/ replies, xml ) Need Help??
radiantmatrix has asked for the wisdom of the Perl Monks concerning the following question:

I think of myself as a moderately good developer. I have a reasonable amount of experience, I know about common patterns, I reuse code when possible, I try to follow good development practice.

I use test-driven development, I religiously use a revision control systsem. I use Devel::Cover to check that my tests cover 100% of the code I write. Likewise, I ensure that I have good Pod::Coverage.

I've done informal code reviews before -- at many of my clients, my co-workers disdained code review, so I found ways to do it "on the sly": "Hey, I'm not sure about this module, would you look at it with me?"

Now, though, I have an opportunity to do more structured and formal code review. I don't want to bury the process in formality and paperwork, but it is important that I can point a manager at documentation and say "yep, I really did code reviews, here's the proof".

Can the members of the Monestary suggest:

  1. Ground rules for having useful code reviews
  2. Ideas for what kinds of documentation I should have to prove they've been done
  3. Any other information about conducting semi-formal code reviews in a way that's really useful for developing (esp. in Perl).

I'd certainly benefit from the experience of my esteemed brothers and sisters.

<radiant.matrix>
Ramblings and references
“A positive attitude may not solve all your problems, but it will annoy enough people to make it worth the effort.” Herm Albright
I haven't found a problem yet that can't be solved by a well-placed trebuchet

Comment on How should I do (and document) effective semi-formal code review?
Re: How should I do (and document) effective semi-formal code review?
by dragonchild (Archbishop) on Jun 18, 2008 at 20:55 UTC
    A code review is a meeting. All meetings have inputs, attendees, agendas, and outputs. In the case of a code review, it could look something like:
    • Inputs: Code to be reviewed; Specifications for said code
    • Attendees: Author, moderator, reviewers (no, the author cannot moderate the review of his code)
    • Agenda: Review the code
    • Outputs: Suggestions for improvements

    So, document that. A couple thoughts:

    • Some people talk about non-meeting code reviews, where people review the code at their leisure and give feedback via email. This system sucks because there's no impetus to do anything.
    • Unless you have management backing, you will not get people to give the amount of time necessary to review code properly. This breaks down as so (for roughly 100-200 lines of code spanning 2-3 functions):
      • 30min - 1h reading and understanding specifications (you have specs, right?)
      • 30min - 2h reading and understanding the context of the code (calling code, called code, environment, etc)
      • 1h - 3h reading and understanding the code in question
      • 1h for the actual meeting (Code reviews should never go past 1h. Anything more and the code needs rewritten or chunked better.)
      As you can see, this could be up to an entire workday per reviewer. And this doesn't count any followup reviews should the code in question need to be rewritten.

      Now, this will get better over time, but you should expect that in the worst case (and we all plan for the worst case, right?).

    • Expect a lot of egos, especially at first. It's difficult to hear someone you think is your inferior tell you what's wrong with your baby, particularly if they're right. This is why the author cannot be the moderator.
    • Don't even think you're going to teach the reviewers with your code. That's arrogance at its finest.

    My criteria for good software:
    1. Does it work?
    2. Can someone else come in, make a change, and be reasonably certain no bugs were introduced?

      (you have specs, right?)

      But of course. ;-)

      Thank you much for the input. I'd not thought of the review as a meeting.

      <radiant.matrix>
      Ramblings and references
      “A positive attitude may not solve all your problems, but it will annoy enough people to make it worth the effort.” Herm Albright
      I haven't found a problem yet that can't be solved by a well-placed trebuchet
Re: How should I do (and document) effective semi-formal code review?
by Tanktalus (Canon) on Jun 18, 2008 at 21:41 UTC

    We have a formal-informal code review tool that inspects our code tree, finds what has changed, creates diffs, etc., and emails them to the reviewer. It also appends a note in the bug-tracking tool to say that this has been done. Once the reviewer is done, s/he runs the tool again to mark it as complete, which sends a note back to the originator AND updates the bug-tracking tool with the comments. This may need to be done again if large enough changes are required to address the comments.

    The advantage here is really asynchronicity. You don't need to find time for everyone to get together at the same time. Theoretically, you should still be able to have multiple reviewers (due to a limitation of the tools, we can't right now, but we've requested fixes to the tools to allow it) all able to provide feedback. You definitely get documentation - it's in the defect remarks, as are the comments so generated. Now, whether that's a GOOD place for the documentation is debatable.

    In our case, we don't generally have a lot of experts in any given area of the code, so we can't really get a huge group together anyway. (Note that in our definition of "expert," if you even have a book on the subject in your office, you're considered an expert - we're not actually as picky as "expert" may sound).

    We also hold much more formal reviews once in a while. But I find those to be rare, for good or bad.

      What are the tools that you use?


      Perl is environmentally friendly - it saves trees

        We use IBM Rational Clearcase/Clearquest (not my choice). The tool for codereview was written on top of that by us. It just queries the current view to see what has been checked in, marks those files as being 'under review', and sends the review. Then the reviewer retrieves that into a temp directory, which marks those files as being retrieved for review, and gets to see all the diffs, as well as the full original and modified code. When done, the tool is invoked yet again to mark the files as passed review, and emails/logs in clearquest the comments, if any. The problem here for me is that these marks are singletons - you can't mark a file more than once. The purpose of the marks is so that a rejected review that results in further changes only needs to review those changes, not the whole thing again, because the tool will see that some files are already reviewed, but if you checkout/checkin again, the new versions of those files don't have the mark and get reviewed again.

        Of course, the tool is written in perl. ;-)

Re: How should I do (and document) effective semi-formal code review?
by eyepopslikeamosquito (Canon) on Jun 18, 2008 at 22:09 UTC

    I've been in charge of code reviews at work for about six months now. A few random points:

    • Before the code review, the code should be pushed through Perl::Tidy and Perl::Critic according to your internal coding standards. This avoids wasting time arguing about code layout and basic style issues. If you find things in a code review that were not detected by Perl::Critic, see if you can tweak your Perl::Critic policies to find them next time.
    • The code review must be in writing. Otherwise, there is no proof it has been performed.
    • Most of the code review work should be done before the code review meeting.
    • Have at least two code reviewers.
    • Take a look a Fagan Inspections. Though probably more formal than you want, you should get some good code review ideas from this well-respected method.

    According to Karl Wiegers, the Seven Deadly Sins of Software Reviews are:

    • Participants don't understand the review process.
    • Reviewers critique the producer, not the product.
    • Reviews are not planned.
    • Review meetings drift into problem-solving.
    • Reviewers are not prepared.
    • The wrong people participate.
    • Reviewers focus on style, not substance.

    Some useful code review links:

    Updated 23-June: Added "Seven Deadly Sins of Software Reviews" and associated links.

      It's a bit off-topic, but I'd like to know if there's a way to make perltidy alterations transparent to "annotate" in svn/svk. Both support a white-space ignoring mode, but there might be better solutions. Anyone know of one?

      I'm thinking of adding a property to perltidy commits, or maybe reformatting commits in general, then I'd need to find or build an annotate that could ignore it. (Although "ignore" is perhaps not correct, the annotations before this change need to make it over in some sensible way).

      See SVK::Command::Annotate, SVN::Web::Blame

Re: How should I do (and document) effective semi-formal code review?
by eyepopslikeamosquito (Canon) on Jun 19, 2008 at 00:32 UTC

    You might like to look at a web-based code review tool. Guido van Rossum's first project at Google was such a tool, called Mondrian. Though he was unable to open source that work, he has since released the open source Rietveld code review tool. I'm also aware of the similar-looking Crucible, sold by Atlassian. Though I've not used any of these tools yet, I may look into using one of these in the coming months. That said, I do enjoy the face-to-face communication of a code review meeting, and would personally not like to lose that.

Re: How should I do (and document) effective semi-formal code review?
by mctaylor (Novice) on Jun 19, 2008 at 18:27 UTC
    I think the most important thing to make a group embrace code reviews is building the Egoless programming culture first.

    Egoless programming is covered in the 1971 classic The Psychology of Computer Programming by Jerry Weinberg and a short "top ten" list at builder.com / TechRepublic.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://692787]
Approved by moritz
Front-paged by moritz
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others examining the Monastery: (6)
As of 2014-07-31 22:33 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (254 votes), past polls