Code review revisited: something old, something new

It's (code) hammer time!
Now that we've gotten further on our DueDates project, our professor asked us to do a code review again for a (hopefully) new and improved version. Last week we reviewed three other projects by our classmates, but due to the added functionality and length of the code, we only reviewed one project this week, DueDates-Gold.

Something new
So what's new this time around? We added another library the user could get their borrowed book info from, the Hawaii Public State Library system. We also added in sorting by due date and by library name, and also a -within flag to show only the books due within a certain number of days. Finally, we tried out a new feature of Google Project Hosting that makes code review easier by tying in the review to the specific branch of code rather than the revision number.

Something old
Unfortunately, this code review has seen a bunch of old problems crop up. The DueDates-Gold project is, simply put, not modular at all. It only has three classes excluding JUnit test cases and exceptions, and one class performs almost all of the functionality of the program. The second class is simply an ADT, and the last one contains only one method. Just like the last code review, some people in the class seem to lack a fundamental understanding of how object-oriented programming works.

One thing I did learn from reviewing their project though was that it's important to make your classes modular, but it's also important to make your project settings modular too. Everyone in the class is using the Eclipse IDE, and our projects come with settings for it so anyone using Eclipse doesn't have to manually set up things such as external libraries and build paths. When I imported the DueDates-Gold project, there were a number of problems that I had to fix before I could even compile and test it, mostly having to do with missing jar files. Also, their JUnit test cases relied on reading environment variables that the user has to set up themselves, which I believe is a very bad practice. It helped to illustrate just how important it is to set up your project so that other developers can easily download your source code and work on it without having to spend hours just trying to get their development environment the same as yours.

Bad GPH, bad boy
Google Project Hosting, you're up to your old tricks again. You tempt us with the allure of a new feature that cures cancer and eliminates hunger, but really it's just the same old crap in a new box. In my last code review, I complained about how annoying the code review process is in GPH. Any comments that people make are tied into the revision number they made them on, but you can't tell if a revision has any comments when looking at the revision changelog. This means that it's not only difficult for the reviewers to find their own comments after submitting them, but it's doubly difficult for the developer to find them in the first place. Our professor had us try a new technique this time by creating a branch of our code and reviewing the code in the branch, but there is no difference in how the code review works. In the end I had to write a guide on how to keep all the comments in one revision number so that it's easy to find for us. You can view the guide here. This also illustrates yet another problem with GPH; when you create an issue, you can't edit the description of the issue, you can only add comments or change the issue title.

Strangely, we were supposed to receive a code review from the DueDates-Green group, but we didn't get any comments from them. When I looked at their project, it seems like they haven't set up their own project for this code review either.

I'll get you next time
Although we had a better idea of what we wanted our reviewers to scrutinize this time around, our review request is still pretty general. There are problems with the code that we're already well-aware of already, for example we don't have enough error checking, so asking the reviewers to take a look at that would be a waste of their time and ours. Next time we do a code review, I want to pinpoint more specific problems that I want them to look at, such as whether the project is portable enough for them to download and compile right away, which is something that is hard to test on our own systems since they're already set up correctly. Unfortunately, because we didn't receive a code review, this question will have to remain unanswered until the next code review.

One more thing: until the GPH code review problem gets resolved, it's probably a good idea to write explicit instructions for the reviewers on how to keep their reviews in one revision, unless you enjoy doing a scavenger hunt through your revision log.