First Code Review

We finally held our first code review today after having planned them more than a month ago. It was important enough to me to ignore my current stomach flu and attend the code review anyway.

The review involved myself, the tech lead on the project, and one senior developer. I’d reviewed the process with the tech lead, but as these things go some things fell through the cracks, like that there should be two days to review the code before holding the review. Anyway it was more important that we got started.

We met in the team room which was pretty much my office until about 2 weeks ago when I moved out to cubicle land. The tech lead had prepared printouts that he handed out the day before with 5 classes for the review. I setup a projector so we could jump through the code on the overhead as well and we recorded the mandatory and optional changes on the whiteboard.

I ended up leading the review process, probably because this is more of my initiative than anyone else’s. The senior developer admitted he had never gone through a code review before and was interested in seeing how they worked. Out of the five classes being reviewed most of the time was spent on a single JSF backing bean class.

To start out with I did a quick review of the process checklist (see PDF version). It’s still in rough form and bound to get cleaned up, but it covers pretty much our initial process. I asked if either of them had run a static checker like Checkstyle, or the code analysis tools in RAD or IDEA. Neither of them had, so I mentioned I had at least looked at Checkstyle and showed them the output on the latest email from a cruisecontrol build. They seemed to think it would still be useful to run the check, they just didn’t get around to it this time.

After that I started going through the first large class where we ended up spending most of our time discussing. This was an interesting class to review since it contained a mix of RAD JSF generated code and the developer’s actual code. RAD’s JSF generator violates quite a few of our coding conventions including:

  • By default creates a <div class="codecolorer-container text vibrant overflow-off" style="overflow:auto;white-space:nowrap;">
    1
    pagecode.pages

    </div>

    package</li>

    • Makes all the properties protected instead of private.
    • Puts underscores in a lots of variable names.
    • Doesn’t camel case things with multiple words.
    • Uses wonderfully descriptive defaults for variable names like text1.</ul>

    We agreed to make the attempt to clean them up in RAD’s JSF tool, but if it causes problems with say reverse engineering, then we’ll just let it slide. (RAD continues to get my hackles up with its wonderful features.)

    I hammered home that the class we were reviewing badly needed tests since it had none. This really shouldn’t have happened, but so far we’re making slow progress on the test driven development front. We agreed the generated class doesn’t really tests for all of it’s generated methods, just the ones which the developers changed or added to.

    The style for making comments was fairly open. We all discussed our thoughts on the class and its methods as we scrolled through the code on the overhead. We didn’t use the more formal process of having one person go through all of their comments before moving on to the next person.

    The main class that was reviewed was another interesting animal because it had code from both the tech lead and the senior developer in it so it really was a review for both of them. Going through some conditional navigation logic the tech lead had written the comments was made that it needed to be refactored because it was bout 40 lines long anyway, but that much of the code could be moved to the faces-confix.xml file and navigation rules. The tech lead on this project has quite a bit of experience with Struts, but seeing how the senior developer had utilized more of a JSF style for the navigation he realized it was probably a better approach anyways.

    We also found some minor things like get methods that didn’t return anything and should probably be renamed:

    public void getIrsDeductions()

    The other four classes were much faster to review as they were fairly lightweight DAOs and Service classes that called the DAOs.

    At the end of the review I went through the comments from Checkstyle. The developers had recently fixed an issue with RAD where it automatically uses tabs for indenting instead of spaces, so the remaining Checkstyle comments were of some use. Despite getting these reports they hadn’t really spent any time looking at them. It turned out 90% of the comments were about problems in the generated code sections and some older code that had been borrowed from another project that used a Java RMI strategy to attach to a proprietary legacy system which again generated a lot of non-compliant code. Still I thing they got some understanding when I explained the cyclomatic complexity warnings and the method size limit which we set to 20 lines for now.

    We wrapped up in about an hour with all of the mandatory changes written on the whiteboard and on the developer’s code printouts. We agreed that the fixes will be in within two days and then the tech lead and I will review that the changes were made. There won’t be a formal meeting, possibly just addressed at the daily Scrum in a few days.

    I think this first attempt if a little disorganized was generally a success. We managed to share knowledge and catch things a single developer wouldn’t have noticed, even little things like a misspelled variable name,

    1
    lables

    instead of

    1
    labels

    . And I don’t think anyone came away feeling negative about criticism of their code. Both of the developer’s were pretty engaged the whole time, and they both asked a lot of questions and had comments to add. So there’s still the followup, and then next week we run another code review.

Scrum and XP

There seems to be a lot of overlap between XP and Scrum in the Agile world. Somewhere I remember hearing that Bob Martin suggested at Agile 2005 that everyone was moving towards an Agile process that looked pretty much like Scrum with the XP engineering practices. I saw a little background on this on the scrum-development mailing list from Mike Beedle:

Many of us were in a room full of people — a group that included Ron Jeffries, at Snowbird 2001 when Kent Beck said: I stole Scrum and used Scrum in XP (paraphrasing).

This appears to be about where we are with Agile adoption, though we have quite a ways to go on a lot of the XP engineering practices.

The Java Posse Podcast

After a bit of a restart from the ashes of the Javacast, the Java Posse Podcast has taken up in it’s wake. I like it overall, but it can have a bit of Java boosterism to it. Still they get some pretty great interviews including Joshua Bloch recently. So if you don’t mind a bit of a Java bias it’s a pretty good development podcast. I pick up little things from the hosts every once in a while, things like one of the hosts regularly uses multiple Java IDEs instead of spending the day in a single IDE.

Haven’t Opened MS Project in a Year

I realized today I haven’t actually had to put together a project plan in MS project in the longest time. I don’t even really open other peoples project plans anymore. A few years ago working as a professional services consultant I was constantly in MS Project because, darn it, customers want their Gantt charts. I always found it a very frustrating tool for anything more than a very simple projects. Add a few resources, a few dependencies and then make a change between some linked tasks and you are headed down a rabbit hole pretty quickly. And just try taping together a Gantt chart with 500+ tasks.

MS Project and other project management software largely fades away on Scrum project. Since everything is driven from a backlog and you really track velocity on a daily basis there’s no need for the familiar Gantt chart. Even when I attempted to use XPlanner on a Scrum project it turned out to have to high of an overhead. One of the nicest things about Agile is that you work the plan everyday so you always have a good idea of where you are at least for the near future and the longer term planning is just revisited once a month.

Enjoying Refactoring Again

I shot through the first hundred pages of Martin Fowler’s classic Refactoring today. I can see why it’s still considered a classic now. Fowler’s style is very readable for a software development book and his code examples are simple steps well explained. He peppers the explanations with lots of real world anecdotes from projects. And probably most importantly he’s not afraid to explain that he didn’t start out as an expert in things like object oriented programming:

“It is hard to see what causes the technique to be less effective, even harmful. Ten years ago it was like that with objects. If someone asked me not to use objects, it was hard to answer. It wasn’t that I didn’t think objects had limitations–I’m to cynical for that. It was just that I didn’t know what those limitations were, although I knew what the benefits were.” –pg. 62

It also reinforces the notion that many of the thought leaders in the agile world are former Smalltalkers. At some point maybe I’ll take a look at Squeak, but Ruby is my current new language to learn.