Unless your entire coding career has consisted of nothing more than "Hello world" programs, chances are you've looked over your code to see if there are any areas that can be improved and optimized. However, since you wrote it yourself, chances are you also think it's the bomb diggity when it's actually more along the lines of what your dog did on your front yard this morning. This is where having a second opinion comes in really handy, and to give us some practice in code reviewing, our professor had us review three other DueDates projects done by the other groups in our class. If you're unfamiliar with the DueDates project, refer to this post. I was assigned the following projects to review:
DueDates-Purple
DueDates-Violet
DueDates-Red
DueDates-Purple
This project, in my opinion, could use a lot of work. All of their methods are in one class file which to me seems like they either rushed to get it done or don't really understand what objected-oriented programming is. For example, their command line parser is very easily broken; it assumes that the ID number will always be 9 characters long and that the dash will always be the 6th character. The parser also only checks to see if there are 3 or 4 command-line arguments, and if it's 4, it expects '-verbose' to be the first argument. This will become a problem once we start to use multiple login credentials in one command where we'll be using multiple flags. I realize this is only the first revision of the project but to me it seems like they'll just have to end up rewriting everything anyway because in its current form it's not modular at all.
DueDates-Violet
This project was the best of the three that I reviewed. I was asked to only look at the JUnit test cases class for the ErrorLogger class, a class that keeps track of which errors occurred and then outputs them to the console. Strangely, I was not asked to look at the ErrorLogger class itself, which may have been helpful in figuring out if the test cases were good or not. Fortunately, the test cases were very well-written and I was only able to bring up some minor cases that they could've added. Also, I didn't look at the other classes, but the file names makes me feel like they understand the concept of object-oriented programming and are striving to make their code modular.
DueDates-Red
This project, like DueDates-Purple, put all of their methods into one class. I was asked to only look at their getHawaiiStateInfo method, which logs the user into the Hawaii State Library system, gets the user's borrowed items, and prints them out to the console. Apart from the fact that this one method is doing too many things by itself, it should also be put into a new class by itself and the return type should be changed to something more modular rather than outputting directly to the console. This project will also require significant rewrites.
DueDates-Orange, our own project
Likewise, we also received code reviews from everyone that was assigned to look over our project. The reviews generally pointed out one thing: that we didn't spend enough time on error reporting. Other than that, most of the comments were for minor issues, but nobody pointed out any major faults. We even received some high praises for making the system easily expandable! You can view our project here.
Some interesting observations
Throughout the reviews, I've noticed some interesting trends. First of all, people actually wrote nice comments such as "I like how you did this" or "This is what I wanted to do too". I was surprised by this because I thought a code review was supposed to point out only the bad parts of the code, although I suppose some words of encouragement would be nice to hear for the code writers. However, I think this leads to another problem that I've noticed, which is that people generally left only a few comments here and there and a good number of them were 'nice' comments. In my opinion, a code review is a critical examination of the code and any problems should be pointed out. By being nice and holding back potentially 'hurtful' comments, it pretty much renders the review useless because it sidesteps the entire point of doing a code review in the first place. The reviewers need to be brutal and say what needs to be said instead of dancing around the point.
It also seems like some people don't really grasp the concept of object-oriented programming. When we started this project, we were told that we'll be expanding it as we go along, adding more and more features over time. Therefore, it was to our best interest to make the project as modular as possible from the get-go so we don't have to end up rewriting big portions of it. Unfortunately, 2 out of the 3 projects I reviewed wrote all of the code in one class, and it was written to specifically perform only what the requirements wanted but had no future expandability in mind. I hope this is because they rushed to get the project done by the due date, but I have to say it's sort of shameful to see code still written like this in a 400-level course.
Google Project Hosting, I love to hate you
In order to do our code review, we used Google Project Hosting. In my previous blog post, I praised how GPH was a good all-in-one package for programmers who were looking for all the tools they needed to manage their projects. After this code review experience, I'm going to have to retract that statement. For reviewers, commenting on code is easy enough; you open up the source file in GPH, double-click on the line you want to edit, then type in your comments and save them. What GPH doesn't tell you, however, is that your comments don't get posted until you tell GPH to publish them, but the kicker is that there's no 'Publish comments' link anywhere to be found! To get to it, you need to click on the revision number in the changelog that your comment was for and then publish it from there, but this is completely non-obvious and no instructions are provided. I lost and entire project's worth of comments because I didn't know this.
Where's the "Publish comments" button/link?
Here it is! (on the right side)The second issue is for people trying to look at code review comments. The comments themselves are assigned to a revision number and by default, all comments submitted are associated with the most recent revision. Since you can only view one revision at a time, this presents a problem if the comments are spread across multiple revisions. To make matters worse, the revision browser doesn't show which revisions have comments and which ones don't.
Can you tell which revisions have comments?In our case, what happened was that some people reviewed our code and submitted comments, then we made some more revisions and some other people reviewed our code after that. Because by default GPH submits the comments to the most recent revision, the comments were split across two different revisions and it made reading through them a pain.
Bruises licked and lessons learned
So what are some of the lessons that I got out of this code review? First of all, make sure you tell your reviewers to look at the right classes. I was asked to only look at a JUnit test class, but was not asked to look at the class that it tests! Obviously I can't really evaluate a test case class very well if I'm not to look at the class it tests too. Secondly, if you're doing a code review, be as brutally honest as possible. It does you and the reviewer no good to play 'nice' and beat around the bush when there are glaring problems with the code. If the code sucks, just say it and save everybody a lot of time. Lastly, write all of your comments for a single revision, and the code writers need to tell the reviewers which revision number to write their comments for. Otherwise it makes it hard for the code writers to view the comments and a super helpful comment might go by unread, lost forever in the internet void. If these tips are followed, it should make the review process much easier for both the coders and the reviewers and prevent you from pulling out your hair in sheer frustration. Now if you'll excuse me, I need to get a haircut...
No comments:
Post a Comment