This week in class, we did code reviews on other implementations of the WattDepot Command Line Interface. To be honest, most of the code reviews I've been involved with were other people reading my code. For the first time in a long time, I'm reading other people's code to try and find ways they can improve it. At the same time, my code was reviewed by others in the class.
Some of the criticisms of our code were warranted. In some ways, our program did not provide enough feedback. Aaron Herres and Dean Kim provided command usage information when they reported an error, which is something we really need to do. Some of the classes also did not provide enough information about what occurred. Dean also pointed out something in the specs that we did not cover. As far as functionality goes, that was the main issue other than BJ pointing out that something didn't quite line up as stated in the help command. We do need to do a better job of common look and feel as well. I'm not sure what the best approach is, since I rarely look at what Yichi does. This seems to imply that we should be doing our own little code reviews on the side before the actual due date.
Some of the other issues that came up were non-issues though. Some commented about the length of the file names, which is more a part of my naming convention more than anything else. Some suggested that the contents of the help file be moved to a separate file. While a good suggestion, I had a different idea (have each command implement a help string and then have the help command just collect them all). But there was also a fair amount of praise, which kind of surprised me since I simply followed Philip's recommendations over the weekend. I could say that the code structure was all our idea, but that would be a lie.
But the more interesting experience was reading other people's code. BJ, Wahib, and Lyneth's implementation was pretty solid in terms of functionality. They also followed Philip's recommendations, yet their organization was different from ours. Aaron and Dean followed some of Philip's recommendations, but they didn't go all the way with it in terms of packages and the like. Neither team was very comprehensive with their tests either. Yichi and I were both surprised when we looked at our coverage report, since we weren't even shooting for high coverage (ours is about 89%).
All in all, the feedback was welcome. The entire process has me thinking of better ways to organize our code. Well, it has me thinking about that more than usual.