Code Review #2

We held our second code review this Friday. Two months between holding code reviews is a little long, but we’re still working out the process. Some things to remember for next time:

  • Prepare for the code review ahead of time. This code review was being held towards the end of the Sprint, so the afternoon before we got it arranged. Some review is better than none, but the code review ended up being more of a code walk-through.
  • Adding a static analyzer like Checkstyle really helps with keeping the code clean. Since the developers had spent a good bit of time working down 150+ Checkstyle warnings to 9 there were very few basic defects, like one character variable names, or really long methods.
  • Figure out a better way to do followup. After 90 minutes we had a number of items to fix, but since they were handwritten notes by all the participants it will be a bit difficult to check up on. We may get a chance to use a tool like Crucible the next time around.
  • Next time don’t even hold the review until all the classes covered have tests. Two of the reviewed classes had zero test coverage.

On the positive side we found quite a few subtle defects in 90 minutes from cut and paste errors to iterating over collections that only had one item. I think the three of us were all in agreement that having new pairs of eyes on the code was really helpful in catching some things.

3 responses to “Code Review #2”

  1. Did you review the tests (where they existed) as well as the code?

    Rapid Development gave me the idea that code reviews were as much about showing off the best ideas as they are about catching mistakes. Are you finding that to be so? Do you think these reviews help to even out the skills across the teams?

  2. Ed Gibbs is My New Hero

    He’s actually practicing two of the best practices I know of: regular one-on-ones and code reviews. In my own management career I found it very hard to stick w/the weekly one-on-one schedule even though I believe it is hugely important….

  3. Ed Gibbs says:

    On reviewing test code:

    Given the 90 minutes we had, we didn’t review the related tests. There were two main reasons for this. First, I let the tech lead select the classes to be reviewed and he didn’t select test classes, though as I write this that seems an obvious improvement. Second, there were only tests for about 50% of the code at the time of the review.

    Some good ideas did come out of these reviews including factoring out more business logic out of the JSF backing beans and moving error messages into resource bundles.

    On reviews being a good way to even out the skills on a team, that is one of the largest goals for us at least. Our development team is improving their skills everyday, but there’s still a lot of mentoring to go around. Even these two reviews have made a small dent in sharing ideas.