Wednesday, October 22, 2008

Code review: The Good, The Bad, The Ugly

Introduction
First off I needed to come up with clever title for the blog and I thought The Good, the Bad, and the Ugly was appropriate for reviewing code. I would like to mention that in no way is the title a reflection of my fellow classmates' programs. I will however, point out what I thought was specifically good, what was generally bad, and make a general statement about what was ugly.

When it comes to programming I am still in that in-between phase of, I know enough code to write a simple application and but do not have enough experience to tackle a full project. Because of my experience, for the review, I could only comment on portions of code I was familiar with and used minimal examples to show what I thought was proper or made more sense in my estimation.

The Good
Overall, the code I saw had good ideas the kind of stuff I wish we implemented in our DueDates. The Blue Team: Arthur and Erin's program did some cool things I especially like their interface which prompts the user to choose a library.



The Green Team: Jeho and Yasu concatenated the values from the respective website which will help to present the information in a meaningful way.
String bookInfo = "";
bookInfo = bookInfo.concat(String.format(format, "Due Dates", "Book Information" ));
for (int i = 1; i < bookinfo =" bookInfo.concat(String.format(format,table.getCellAsText(i,">
The Orange team: Aric and Daniel used an Abstract class and method which I thought was interesting and something I was not thinking of when we were going through the preliminaries of the first DueDates assignment. Using Abstract should be able to give the group the ability to reuse the code when needing to add more libraries to the application.

The Bad
I did not see code that was troubling however, I saw bad documentation and a lack of testing. One group had only one test case and a what looked like a test in the constructor of the unit test. Another group had zero test cases, other than running the code they could not check for valid or invalid values. I saw a class that had one proper Javadoc for a method.

The Ugly
Again, I did not see bothersome code except for trying to read the code to the left (sorry Aric and Daniel), its an implementation of one of their classes with html tags filling every line. My eyes hurt after looking at the file. The code was not bad it was just hard to read as you can see.




Team Purple's code
For the most part the reviewers: John, Arthur, Vincent, Ronn, Daniel, and Yasu liked our implementation with a few of them making suggestions on how to improve our code. One thing I would like to implement is an interface for an user to enter information instead of having to do it through the command line. The team needs to improve the way DueDates retrieves information from webpage with a table. I agree with Ronn's assessment of abstraction however, that should be no problem when we need to make the adjustment in future versions. We do have a nagging xbeans.jar issue that causes an error with JUnit and the verify which needs to be fixed before we can start the next the phase of the project.

Issues with the review tool
I had problems getting the comment window the code review. I was attempting to open the window by clicking on the line number when I needed to be clicking on the line of code. That bit of brain-lock prevented me from reviewing the code for a day. A couple gripes about Google's code review besides having to click a line of code to enact the comment window. With my comments only one set appeared in one of the group's code, the other teams' reviews were stuck in another page so it was difficult to know if the comments were saved. In case I need to make a change to a review it is complicated to find the comments I made to a file.

My Thoughts
Doing code review is helpful by improving the system that is being reviewed and the reviewer gets to see how other programmers are implementing their programs. By looking at people's code a developer may see a whole another way of doing things such as Team Orange's use of Abstract. So for a future project a developer can take advantage of the knowledge they gained by looking at other programmer's code.

Conclusion
We were told to spend a hour on each team's system which was barely enough time to go through the documentation, verification, and examination of the application. Because DueDates is still in its infancy the project is small enough to inspect and still understand how the application is suppose to work. Other than my small mishap with the comment portion of the code review. The only improvement that I see with reviewing code is spending more time examining programs. Also, getting the developement team and the reviewer together to go over the comments, if possible. Such a meeting maybe helpful for both the reviewer and the programmer.