We’re well into a year of introducing code reviews and I’ve noticed a continued evolution over that year.

• At first we used no tools, printed out the code ahead of time, and then walked through it with an overhead and a projector in a formal meeting.
• Then we introduced tools, first Jupiter and then Crucible. We’re still using Crucible.
• We also discovered that static analyzers like Checkstyle helped avoid a lot of more basic issues with the codebases before the review.
• Initially we hoped to do at least one review on a project every week or two.
• Now we aim for one review per Sprint on a project. It goes in as a general task each Sprint. Of course this means you might only review a few classes every month.
• We’ve gradually reduced the number of classes reviewed from 5+ to 2-3 per review.
• As we’ve adjusted to Crucible and it’s lightweight online style, we’ve dropped almost all the formal meetings unless we need to meet to discuss a design issue that crops up.

I feel like we still have a ways to go, but we are getting a lot of mentoring out of the effort and I think it helps everyone to know there code is going to be read by a few developers at some point, so they’re more likely to keep things clean along the way.

Buddy style review might be the next experiment or something akin to Kevin Klinemeier’s idea of sitting down and making any changes that come up with the code under review to be done right there in the code review. I really like the idea of completing the review and being done/done with the agreed to fixes.

As per our experience, code reviews have been a gradual experimentation and evolution towards a balance of lightweight approaches and thorough reviews. I don’t think we’re they’re yet, but we’re getting closer.

I picked up Ruby on Rails: Up and Running at the local Borders a few weeks back with the intent of forcing myself through a second book length tutorial on Rails. Reasons included:

• Long ago I learned it always helps to learn things from multiple independent sources, so I wanted to see how someone like Bruce Tate approached Rails.
• I have the beta PDF of Agile Web Development with Rails, but it’s over 600 pages now and not quite as approachable as a 167 page book.
• I like short, deep, vertical dives into topics.
• I have two small children, a full time job, and the occasional weekend day of paintball, so being able to pick it up for only an hour or so a night and make it through a chapter or so is nice.

Overall I’m down to the second to last chapter on AJAX now and the online photo app has been pretty good for solidifying some of my rails understandings. Since the authors do assume a good portion of their readers aren’t familiar with TDD the unit testing chapter is pushed to the very last chapter of the book and they don’t do the tutorial TDD style. I went ahead and used autotest and tested my way through the whole thing.

Five chapters in, I understand better now how integration tests work, and especially how to start with scaffolding code and build it out to a nice looking application. I’m expecting the AJAX chapter to be enjoyable. Not nearly as comprehensive as Agile Web Development with Rails, but not a bad introduction.

Recently I got asked by two of my developers about what to do with testing private methods. They explained they wanted to test some of their private methods that contain logic. The options they were considering included:

• Making the private methods public.
• Extracting out the methods into new classes.

As a pragmatic approach, both seemed reasonable, but I remembered reading multiple bits of advice on only testing the public interface of a class. Testing private methods is wrong and dangerous.

Simon Harris makes the common argument that you shouldn’t test private methods:

Now, I almost never test private methods. I say almost, just in case i’ve either done it once before and forgotten or I need to change my mind at some point in the future. But as far as I’m aware, I never test private methods.

I know I’ve wanted to test private methods before. I like to keep most of my public java methods around 10 lines or so if possible, so I’m constantly extracting out well defined private methods. Some of these contain conditional logic I’d like to test as a single unit, but I have to suffice testing the conditionals through the public method that calls them.

Hunting around for about 30 minutes turned up some insights on this issue.

Bill Venners defines four possible approaches to unit testing private methods:

• Don’t test private methods.
• Give the methods package access.
• Use a nested test class.
• Use reflection.

He does a good job of exploring each one, but his ultimate conclusion is that usually you shouldn’t test private methods, but if you need to any of the last three approaches will work.

Cedric Beust of TestNG fame has a stronger opinion:

My answer to the question “Should you test private methods” is a resounding YES!

When it comes down to testing, I follow a very simple rule: “if it can break, test it”.

Bruce Eckel agrees with Bill Venners that you should probably go ahead and test private methods if it makes sense:

I’m with Bill on private method testing. Unit testing allows you to test at the granularity of the method, and the reason you break code up into private methods is to manage complexity. But if you can’t test at that level, it’s defeating.

JB Rainsberger even has a recipe for testing private methods:

Recipe 17.6 Test a private method if you must

“You have a private method that is complex enough to warrant its own tests, but nevertheless is not important enough to promote to the public interface or refactor to a collaborating class. You could test the method indirectly through the public methods that invoke it, but you would rather test it in isolation, which is a laudable goal.

What’s so bad about testing private methods, anyway? Perhaps the greatest problem is that private methods are private for a reason: the author intended for nothing but this one class to have any knowledge of the particulars of its implementation. … In that sense private methods support refactoring very well by providing the design with a degree of freedom of change.

—Junit Recipes

The Pragmatics also lean towards testing private methods if you have to:

If there is significant functionality that is hidden behind private or protected access, that might be a warning sign that there’s another class in there struggling to get out. When push comes to shove, however, it’s probably better to break encapsulation with working tested code that it is to have a good encapsulation of untested, non-working code.

— Pragmatic Unit Testing

Bob Lee argues that package level scope should be used for testing:

Package scoping particularly shines during unit testing. Some programmers argue that you should only test through the public API. Don’t be silly. Limiting your tests to the public API contradicts the spirit of unit testing and subjects you to unnecessary dependency pain. I prefer to isolate and limit the amount of code I test at one time, and test as close to the code as possible.

Conclusion

So lots of thoughts, lots of options, but what to do? My conclusions are:

• Try to avoid testing <div class="codecolorer-container text vibrant overflow-off" style="overflow:auto;white-space:nowrap;">

  1

  private

  </div>

  methods.</li>

  • When you do need to test a <div class="codecolorer-container text vibrant overflow-off" style="overflow:auto;white-space:nowrap;">

    1

    private

    </div>

    method because it has enough logic to warrant unit testing just switch it to package scoping from

    1

    private

    .</li> </ul>

You know your web framework is facing some problems when projects like Reasonable Server Faces crop up.

At some point choice is overwhelming. Bruce Tate talks about this in From Java to Ruby:

Certain tiers of development may be fairly well established, but others like the web presentation tier, offer a staggering lineup of choices–Struts, JavaServer Faces, WebWork, Tapestry, Rife, Spring Web MVC, and many others. On this tier, the safe choices are among the least productive.

The really sad part is this has been the situation since about 2004 after Struts or Struts Classic as it is now called had become dominant. JSF was offered as the obvious successor and instead we ended up with dozens of successors. In comparison Ruby has the dominance of Rails.