Ah, the smell of a fresh codebase.

I’m certain most software developers who have been around for any amount of time have had to work on “The Project”. This project has probably been going for years, has had a fair amount of turnover on the development team, and has had a handful of architects (with their own way of doing things).

Projects like this may work wonderfully and be very successful from a business standpoint, and the individuals working on them are likely quite competent. Yet, from a development perspective, they are a nightmare. They contain difficult to understand code, lots of unused (or commented out) code, little testing, little adherence to good development practices, few comments, and a general feeling of brittleness.

Code like this may work exactly as intended, but when a codebase gets disorganized, chaotic, and sloppy it can lead to other problems, such as:

• An unpleasant experience for developers

• High cost to refactor code

• More bugs

I’ve worked on a few projects like this over the last 5 years or so, and was naturally excited when I found we were going to be starting with a fresh codebase. At last, all of this cruft that has accumulated over the half a decade this project has been going will no more!

Imagine my surprise (and dismay) a month after the new project started when I found most of the symptoms of “The Project” starting to present themselves in the new codebase. Looking through our project and the reports we generate (PMD, CPD, Clover), I saw:

• Lots of untested code

• Lots of bad practices and sloppy coding (as shown by PMD – we really pared down the ruleset so we don’t get a ton of false positives)

• Unused code (already, after less than a month!?)

• Commented-out code

• Methods that were too big, hard to understand, or uncommented

Part of this was due to spikes being done for the new project, which led me to conclude that spikes or experimental work of any kind should always be done in a branch. I also became interested in how code gets to this state so quickly, and of far more importance, how to keep it from reaching this state.

I don’t think this happens because people are bad developers necessarily, but because it requires a large degree of diligence and policing to keep a codebase clean and well-organized. It is easy to let organizational or quality issues fall to the wayside when working on functionality, and timelines sometimes leave little time for these tasks.

Some people offer up pair programming as a way to avoid this, but I don’t particular care for it (see my comments on pair programming). I think situational pair programming is an excellent practice, but I think full-time pair programming is, besides a terrible waste of time, largely ineffective.

After doing some research and analysis of how we work, I’m beginning to think that peer reviews are an excellent solution and practice. They provide most of the benefits of pair programming with few of the drawbacks. You still have someone examining your code, asking questions, and offering feedback, but instead of doing it for an entire iteration or whatever (where one of you will likely be bored for a good portion of the time), you do it for an hour or whatever is necessary. The only potential drawback is that if someone is totally off on their approach and it isn’t noticed until the end, it could be costly to refactor (so costly, in some cases, that it may be left in its sub-optimal state).

With that in mind, I set out to attempt to come up with good practices for conducting code reviews, and also best practices we should be following to keep our codebase clean and easy to work with. I’m wondering what other organizations are doing for peer reviews, and what I can do to improve the stuff I came up with.

It should be noted that a peer review is largely orthogonal from a QA review – a particular block of work can pass the QA review with flying colors and totally fail the peer review. The way I see it, QA reviews inspect the interface, peer reviews inspect the implementation.

Here is the document I came up with:

–

The peer review should occur prior to resolving a story or bug. At least 30 minutes should be set aside for a peer review, to allow for a thorough review. Once the review has been completed and the reviewer is satisfied, they should leave a comment on the JIRA entry stating that they have peer reviewed the unit of work.

Besides following this checklist, you both should go over all the code you wrote for this story. The reviewer should not hesitate to point out how something could have been done in a better way. If the reviewer cannot figure out what a particular piece of code does in a relatively short amount of time, it should be re-written or comments should be added in the code.

1. Metrics / Standards

- (any coding metrics – code coverage, static analysis, etc)

- Test coverage should be meaningful – ie, don’t just check that a method executed without throwing an exception

- Use the right kind of test for your code (ie, some code may need just unit tests, others may need integration tests, and some might need functional tests)

- Business logic is properly tested with unit tests

- If this is a bug, was a unit test specifically for that bug created?

2. Code Style

- Properly formatted Java code

- No commented-out code

- No unused code

- JPA standards followed (we have a separate document for this)

- If you have to explain a piece of code to your reviewer, that piece of code should be documented

- If your code is meant to be re-used throughout the app (ie, a utility class) it should have descriptive and useful JavaDocs

- Classes should be fewer than 500 lines of code, and any over 250 lines should be scrutinized closely

3. View

- All JSF controls have explicit ids

- Everything works in IE (firefox support is strongly recommended, as well)