Code Reviews

Many have noted that it has been quiet here at CtJ HQ. This has been due to vacation, summer activities, and partly the giant vacuum left behind by Twitter (I have resisted the autonomous collective so far). Bloggers tend to play off each other, but the wealth of ideas have been reduced to traikus (an attempt to use the haiku form to communicate, a la "nice try") and other short status updates.

Someone must keep the home fires burning, and so your humble host writes from his quiet perch, while the twittering masses party in the streets below.

This article is a good conversation piece: it lists some elements of code reviews. I could leave a comment, but in the classic tradition, I'll blog mine here.

Prime Directive

I work in open war-room that ranges from 4 to 8 people. The benefits of a war-room are outside the scope of this post, but it may shed light on this element.

The prime directive of an effective code review is to examine the code with an eye to the team philosophy, both from a domain and technical standpoint.

Let's consider technical philosophy first: a team should have a solid, unified view on a variety of issues. Are unit tests required? Do we test getters and setters? How do we handle exceptions? Where do we place inner classes in a file? And so on. (Note that this presumes that the team has read the classics (Effective Java, etc) and that the issues therein are 'settled'.)

The key is to maintain consistency. If there is no consistency or stated philosophy from the tech lead, it should be discussed. But once it has been decided, it should not be debated. All too often, a code review can re-open a debate, but for efficiency, avoid this (unless something isn't working out). IMO, the philosophy should not be written down (the code is the example).

Establishing consistency is a major benefit in a war-room. If a team is still "partying like it's 1999" in cubes, then the stand-up meeting can help here.

The domain philosophy is similar. Questions include: if we upgrade from a previous version, and have 2 Foo objects with the same id, what happens? Should a user be allowed to delete a Foo when it is still linked to a Bar?

Code Coverage

As a reviewer, it is natural to ask if the person wrote unit tests. A good follow-up is to ask them about code coverage. The coverage should meet the level established by the team philosophy. Ditto for code complexity metrics.

Internationalization

Beware of hard-coded strings. Reviewers should always be aware of the context where strings are used, and understand which contexts require locale bundles.

Concurrency

Ask the developer if the code is thread-safe, or if it runs in a multi-threaded section of the code. There is a high probability that s/he will look up at the ceiling involuntarily (that's where all the answers are), and pause for a moment. Hijinx ensues.

Versioning

Universities do not have classes called "Versioning 401" and yet it is one of the biggest challenges we face. With respect to data, the team should be aware of versioning issues. e.g. The lifecycle of data across releases, the migration algorithm, and so on.

This is a giant trap for new persistent objects because often the ramifications are not seen for a long time.

The Upshot

Team philosophy and consistency is important, and there is no better place to see it in action than code reviews. Well, there is one better place: pair-programming. But that's another post.