Tuesday, November 24, 2009

Code Review of the Ekolugical Carbonometer

Our latest task was to do a review of another group's web application. This blog is a review of the Ekolugical Carbonometer by BJ Peter DeLaCruz, Wahib Hanani, and Lyneth Peou. I purposefully did not include any of the suggestions made in class (use whitespace better, remove names at the top, and adding some information about what the system does). This should be a really short blog, right?

The review checklist I used to evaluate the system can be found here.

Review The Build

The system builds correctly without issues.

Review System Usage
  • The proper format of the date is not listed on the front page (i.e. yyyy-mm-dd).
  • If we put in a bad date, the error message should suffice (we don't need to see a table full of N/A's).
  • Inconsistent output, why is 9:00 red when 23:00 is yellow? The value at 23:00 is higher than the value at 9:00.
  • There appears to be an empty table cell at the bottom of the output.
  • The output has big bold letters compared to the "Enter a date" label and the error label, which looks really small. There should be some balance between the two.
  • I would prefer that there be a label that shows the day we're looking at.
  • Overall, the speed is slow with little feedback.
Javadocs
  • Thresholds.java has a public constructor, but the Javadoc says it's private.
Names
  • Some instance variables are named with all caps when they are not constants. This is especially noticeable in the Session class with the results lists.
  • List variables in the session should be named the other way around (i.e. todaysTimestamps instead of TIMESTAMPS_TODAY).
Testing
  • Coverage is outstanding as far as I can tell.
  • While WattDepotCommand is covered by testing the web application, I would like to see a separate test for it to make sure it works correctly. WattDepotCommand really should be a separate component (as suggested in the next section) and should have its own test.
Package design
  • WattDepotCommand is independent of Wicket and should be in a separate package.
Class design
  • Instance variables for the WattDepotCommand class should not be public. For the most part, you do not want some of these variables to be changed by other classes. Instead, they should be private with an appropriate getter. For example, noData should be private and a method like "hasNoData" should return the value. If you really want someone else to be able to modify noData, then you should have a setter "setNoData(boolean noData)" (EJS 71).
  • Also, why does WattDepotCommand have lists for the results? It doesn't seem to do anything with them.
Method design
  • When the values from WattDepot are added, there are three separate implementations of onComponentTag when a label is created. I suggest creating a class that extends Label and allows you to select a color to put into onComponentTag.
  • I also noticed that onComponentTag does not have a @Override tag associated with it, which it should to ensure you override the method.
  • WattDepotCommand#getCarbonContentData seems to work by appending to a passed in list. Thus, there is an assumption that the results parameter is an empty list. It seems that what you really want is to return a new list of 24 values, or at least one that matches up with the timestamps. The method would make more sense if you returned a new list of results instead of appending it to the results parameter. Alternatively, you can assert that the list is empty and throw an exception if it isn't. The former is the better option though.
  • In Timestamps#createTimestamps(timestamp, tstamp), there's a temp parameter that is set and incremented, but otherwise doesn't seem to be used at all. It should be removed.
Check for common look and feel
  • As in the review of the command line client, the code definitely looks consistent.
Review the documentation
  • Documentation looks good. One thing I would've liked is a link to WattDepot since you're obviously using that service to get your data.
Review Software ICU
  • Overall their stats look fine. The churn level seems to be on the increase, but it looks good otherwise.
Issue management
  • There is a relative lack of issues for this project. If I just go by the issues in the tracker, then only Lyneth appeared to do any coding and Wahib just wrote the user guide. I'm sure that's not actually the case though.
Review CI
  • There does not seem to be any commits from the 19th and the 20th. Also, the 21st has a period of 3 1/2 hours where the build was red. I recommend that you check with Hudson when you commit and make sure the build is not red.
Summary

I think you guys know what you need to do based on the discussion in class, so I won't repeat those. My additional suggestions are to remove unnecessary elements (empty table cells, extra table rows, table full of N/A's, reduce the font, etc) and use issue tracking more. Also, review the design of the code (naming, methods with side effects, etc). Obviously, you guys work well together, even if the Google Code issues doesn't reflect that. Learn to use it so that if someone else comes along, they can jump in and know what's going on.

Sunday, November 22, 2009

Wicket (Not the Ewok)

This week in software engineering (TWISE?), we were tasked with creating a web application that displays carbon data from WattDepot. I do some Rails programming, so this should be easy right? Wait, we have to use Java? Well I have some experience in Tomcat, JSP, and some JSF. Wait, Apache Wicket? Well, if it's a web framework, maybe it's like the others I've used. But as I quickly found out, it is quite different.

As a little background, I'm quite comfortable working with Javascript and HTML. I know my way around the Prototype and Scriptaculous libraries and wrote a simple web application that uses just HTML and Javascript. I think that HTML and Javascript go together almost as much as HTML/CSS. If you're a web designer, you need HTML and CSS. If you're a web programmer, you need HTML and Javascript.

So Dean, Aaron, Yichi, and I were tasked with making our web app E hoʻomaluō (means "to conserve" in Hawaiian). And we wanted to make a cool Web 2.0 application with all the bells and whistles. And I think we're mostly there with some small caveats. But the most difficult thing I had to get used to was creating this AJAX web app without writing a single line of Javascript. I think a Wicket method took a Javascript event handler as a parameter (i.e. onchange or onblur) and that was the closest I got to Javascript.

I have to give props to what Wicket does. They totally took out the code from the HTML file (Javascript, JSP tags, etc) and abstracted it out to Java. My Index.html file reflects this. This is an AJAX application without any Javascript in the HTML file. There are additional HTML ids that correspond to Wicket identifiers, but that's it. This is in stark contrast to Rails (with RHTML tags), PHP, and JSP.

So as you might imagine, I was a little bit confused at first. I knew what I wanted to do in Javascript terms (on submit, update this div, assign classes to the tags so that the CSS can do its stuff, etc). I just had to figure out how it worked in Wicket. And as Yichi can attest to, it was a source of frustration.

But in the end, the application was completed. Since Dean was off island, he took care of some administrative tasks (setting up the Google Code project, Hackystat, and doing some documentation). Yichi worked on the class that gets the carbon content from WattDepot. Aaron and I joined forces to take on Wicket. Both of us had annoying issues (Wicket and non-Wicket related), but I think we worked through it quite well. For the most part, we met during the week briefly to catch up on what each other has been doing. Dean has also been in contact with us through email. He mentioned that he'd be without a reliable internet connection, but he got a lot more done than I thought he would.


Here's our Software ICU data. Unfortunately, our coverage isn't quite as good as I'd like. The fact that our application is all AJAX makes testing slightly more difficult. There are WicketTester methods to deal with it and Aaron is looking into it as I write this entry.



You can download the source and the executable for E hoʻomaluō here. Consult the user guide and the developer guide for setting up the application.

Sunday, November 15, 2009

Version 2.0

This was a busy week in ICS 613. Over the past week, we had to do a new version of our command line interface for Watt Depot. This meant that we had to:
  • Improve our original implementation based on the feedback in the code review.
  • Install and use Hackystat and SoftwareICU to get a visualization of our group process.
  • Implement new commands.
  • Answer a few Watt Depot questions (task B) from Philip.
So, we basically took those 4 things in order. First, we needed to improve our original implementation. Yichi worked on making his code more readable. He also added better error messages. I had a clear vision on how I wanted to refactor the help command, so I spent a lot of time doing that. I added an abstract method that each command had to implement that printed out usage information. Then, I had the help command just collect all of the help strings and print them out. I also made sure that the commands conformed to the new 2.0 specification. Because the first word in the command uniquely identifies it, I simplified the CommandProcessor to just look at the first word.

The second step was to use Hackystat and SoftwareICU to visualize our group process. The setup was fairly tricky, but we eventually got it to work. I did update the Hackystat library and forgot to tell Yichi, but we resolved that fairly quickly.

The third step was to implement the new commands. This was pretty straightforward. I implemented one command while Yichi implemented two. I changed a few things in Yichi's code (I had helped a classmate work on one of the commands that Yichi was doing), but it was good for the most part.

Before I get to the questions posed by Philip, overall I'd say I was pretty pleased with the way the project went. Our design is pretty solid and our test coverage is 91% (a mere 1-2% increase over 1.0). There are some minor issues that were reported by the reviewers that we did not quite get to. The help refactoring meant that we could provide better error messages, but as of this writing we did not redo all of the commands.

Yichi and I did not meet regularly. We mainly communicated via email, which I think worked for us. I might've spent more time doing refactoring, but I guess I had the design in mind that I wanted to impelement. However, Yichi did implement 2 commands versus my one, so I guess the work was equally partitioned even if the time spent wasn't.

I'm not sure how I feel about the SoftwareICU. On the one hand, it's neat to visualize our project health. However, this revision was not a good time to get introduced to the SoftwareICU mostly because some aspects felt out of our control. For instance, many people had to refactor and/or rewrite parts of their code based on reviews. Thus, the churn reading on the SoftwareICU will probably be high for most of us. As for complexity, many students may have started out with a poor implementation and will be stuck with a high complexity reading unless they rewrite most of their code (although, I guess the implementation wasn't too complex yet). It'll be interesting to use the SoftwareICU on our next assignment, since we'll be starting with a clean slate.


Cue the vital sign beep.
So, this brings us to the questions posed to us by Philip about WattDepot. To answer them, I decided to go ahead and implement two additional commands ("energystats" and "carbonstats") to easily answer this question. I hadn't done much coding over the weekend due to illness, so it felt good getting back into it.

Robert had mentioned to us that the energy consumed by SIM_OAHU_GRID is zero for all dates since the data only represents power plants. Instead, energystats calculates the energy generated by the grid. Getting the hourly data takes a while on my internet connection, probably because it's a lot of data to gather. I split it up into 10 day increments to hopefully make a little more reliable.

>energystats generated SIM_OAHU_GRID from 2009-11-01 to 2009-11-10 hourly
Max: 951.0 MW at 2009-11-02T20:00:00.000-10:00
Min: 497.5 MW at 2009-11-02T04:00:00.000-10:00
Average: 606.3 MW

>energystats generated SIM_OAHU_GRID from 2009-11-11 to 2009-11-20 hourly
Max: 951.0 MWh at 2009-11-11T20:00:00.000-10:00
Min: 497.5 MWh at 2009-11-11T04:00:00.000-10:00
Average: 609.4 MWh

>energystats generated SIM_OAHU_GRID from 2009-11-21 to 2009-11-30 hourly
Max: 951.0 MWh at 2009-11-23T20:00:00.000-10:00
Min: 497.5 MWh at 2009-11-23T04:00:00.000-10:00
Average: 603.1 MWh

So there seems to be at least a 3 way tie for max and min, although it might happen more regularly than that. Let's look at the daily data.

>energystats generated SIM_OAHU_GRID from 2009-11-01 to 2009-11-30 daily
Max: 14764.0 MWh at 2009-11-03T00:00:00.000-10:00
Min: 14089.0 MWh at 2009-11-02T00:00:00.000-10:00
Average: 14571.1 MWh

If only you could see me twiddle my thumbs while these commands do their thing. Finally, here's the carbon emitted statistics.

>carbonstats SIM_OAHU_GRID from 2009-11-01 to 2009-11-30
Max: 29959472.0 lbs at 2009-11-04T00:00:00.000-10:00
Min: 22908808.0 lbs at 2009-11-07T00:00:00.000-10:00
Average: 27141379.3 lbs

And that wraps it up for the command line client. We're implementing a web app in the coming weeks, so stay tuned!

Wednesday, November 11, 2009

We Want Your Feedback!

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.

Sunday, November 8, 2009

Reviewing Code Part 2

In this second part, I'll be reviewing code written by Aaron Herres and Dean Kim.

A. Review the build.
Downloaded and ran 'ant -f verify.build.xml'. Seems to run fine.
B. Review system usage
To start the system, it requires the WattDepot URL. The current service could be used as a default.
Got a stack dump when getting the summary for SIM_WAIAU.
>list source SIM_WAIAU summary
Exception in thread "main" java.util.NoSuchElementException
at java.util.AbstractList$Itr.next(AbstractList.java:350)
at java.util.Collections.min(Collections.java:570)
at org.wattdepot.cli.ListSourceCommandCli.getSourceEarliestDataTimestamp
(ListSourceCommandCli.java:374)
at org.wattdepot.cli.ListSourceCommandCli.processSource
(ListSourceCommandCli.java:453)
at org.wattdepot.cli.ListSourceCommandCli.processCommand
(ListSourceCommandCli.java:487)
at org.wattdepot.cli.ListCommandCli.processCommand(ListCommandCli.java:76)
at org.wattdepot.cli.CommandLineInterface.processMainCommand
(CommandLineInterface.java:209)
at org.wattdepot.cli.CommandLineInterface.processUserInput
(CommandLineInterface.java:172)
at org.wattdepot.cli.CommandLineInterface.main
(CommandLineInterface.java:269)
I also tried the following list commands, but it said they were invalid.
>list sensordata SIM_WAIAU timestamp 2009-11-01
The input string was invalid.
>list sensordata SIM_WAIAU day 2009-11-01
The input string was invalid.
I tried the chart command, but got the following results:
>chart power
chart power error. Usage: chartpower [generated|consumed] {source} {startday} {endday} sampling-interval {minutes}
>chart power generated SIM_KAHE 2009-11-01 2009-11-02 sampling-interval 120 file test.html
No power generated values returned for source
Note that the usage string says "chartpower". SIM_KAHE might not generate power, so I tried to check it:
>list powerGenerated SIM_KAHE day 2009-11-01 sampling-interval 120 statistic max
Exception in thread "main" java.lang.NullPointerException
at org.wattdepot.cli.CommandLineInterface.processUserInput
(CommandLineInterface.java:173)
at org.wattdepot.cli.CommandLineInterface.main(CommandLineInterface.java:269)
Finally, I used the list carbon|energy command:
>list total energy SIM_KAHE day 2009-11-01 sampling-interval 120
list total error. Usage: list total [carbon|energy] generated {source} day {day} sampling-interval {minutes}
>list total energy generated SIM_KAHE day 2009-11-01 sampling-interval 120
1.5854300700617285E9
The first command is the syntax listed in the help command, so that should be changed.
C. Review the JavaDocs
The system summary looks good. The package summary could be improved since the system has changed from the initial simple example.
Interesting that you are not required to put Javadocs for some defined constants (namely in ListSourceCommandCli). I have no problem with it as long as the name of the constant is descriptive enough (SOURCES_COMMAND is not very descriptive).
Method summaries are good enough as far as I can tell.
D. Review the names
I have a minor issue with Cli (or CLi, which is probably a typo) being appended to the end of class names. Seems unnecessary (ListSourceCommandCommandLineInterface?).
ListSensorData#toCLIOutput should be named "toCliOutput" (only the first letter in an acronym should be capitalized).
E. Review the tests
Some classes are not tested at all (ChartPowerCommandCli, ListTotalCommandCli, ListPower, and ListSensorData).
ListSourceCommand seems to be only partially tested. It doesn't seem to get sources with subsources.
F. Package design
All of the files are in one package. Seems like there should be at least a separate package for the command implementations.
G. Review the class design
Some of the command implementations include a fair amount of duplication. For example, ChartPowerCommandCli and ListTotalCommandCli have similar verify and timestamp generation commands. These could be moved to a helper class that is used by the command classes to verify and parse input.
H. Review the method design
Some of the methods in the command implementations are listed as protected when they could be made private.
The system handles exceptions by printing stack traces. It should handle the exception and present a clean message to the user about what happened instead of a generic error.
I. Check for common look and feel
It is somewhat noticeable that two people worked on this. Some command implementations have protected methods while others have private methods that are hidden from the user.

Wednesday, November 4, 2009

Reviewing Code Part 1

Our latest thrilling assignment in software engineering is reviewing our classmate's implementations of the Watt Depot Command Line Interface. One of my instructors once told me a long time ago that it's surprisingly easy to catch someone cheating even if they change their name and a few variables. Everyone has their own style of coding. I'm looking forward to seeing how others wrote their programs. This blog will review an implementation written by BJ Peter DeLaCruz, Wahib Hanani, and Lyneth Peou. A later blog will be written for the rest of the members of the class.
I'll be following this review checklist kindly provided to us by Philip Johnson.
A. Review the build.
Downloaded and ran 'ant -f verify.build.xml'. Seems to run fine.
B. Review system usage
I noticed a minor issue with the following command:
> list summary foo
There is no data for foo.
Error encountered when processing source name.
Seems that there are two error messages. Foo is an invalid source, so it should be an error instead of reporting that there is no data.
I also tried the following command:
> chart power generated foo 2009-11-01 2009-11-01 sampling-interval 120 file test.html
Data was successfully written to test.html.
Looks okay, but here's the output:
There probably should be a check to see if the day is equal. It also does not seem to be able to handle my bad source name.
C. Review the JavaDocs
The system summary and the package summaries look good to me.
Note that all command classes inherit the main method of CommandLineInterface according to the Javadoc.
In AbstractTestCommandLineInterface, the Javadoc seems to imply that the class contains test cases when it does not.
I noticed that TestSourceSummaryInformation checks that the number of subsources for "SIM_OAHU_GRID" is four. Because the WattDepot service is still in development, things might change on the server. If a subsource is added to SIM_OAHU_GRID on the server (i.e. SIM_OAHU_WIND), this test might fail.
D. Review the names
AbstractCommandLineInterface is not an abstract class, but instead is an interface. Perhaps it would be better named as "CliCommandInterface".
The parser method of CommandProcessor should be named "parse", as parser is a noun (EJS 23).
CommandLineInterface defines some constants like "SOURCE_MESSAGE" and "STRING_MESSAGE" for error messages. They could be more descriptive to say that they are errors (i.e. "SOURCE_ERROR" or "CONVERSION_ERROR").
The names of the command classes could be changed to better match the commands. The commands tend to start with "list" or "chart", so they could be named to match up better with the actual command (i.e. ListPowerDayStatistic or ListPowerTimestamp).
E. Review the tests
For this section, I ran their emma.build.xml, which uses the Emma build tool to check test coverage.
Getting the chart for power consumed does not seem to get tested.
SourceInformation, SourceListing, and SourcePowerGenerated are not covered at all. They don't seem to have any tests (questionable if Help needs a test).
SourceSummaryInformation for sources with properties does not seem to get tested.
Many tests test for bad input, but they don't seem to cover "good" input.
Also, the tests do print out a lot of information by default. Perhaps they could be toggled with a system property.
F. Package design
AbstractCommandLineInterface is in a different package from the classes that implement it.
G. Review the class design
In the CommandLineInterface, there are several constants defined, but they are not used by the CommandLineInterface. Perhaps these constants and the AbstractCommandLineInterface could be combined into an abstract class.
The main method in CommandLineInterface does a lot of input handling that could be handled in CommandProcessor. Most of it probably shouldn't be in the main method.
Be wary of public instance variables. isDebugging probably should not be changed by other classes. A "debug" instance variable and a isDebugging() getter would be better. Not sure what isError is used for, but a similar approach can be used (perhaps with "setError(boolean error)" if needed).
The command classes have methods other than doCommand() that are also publicly accessible and they tend to have similar arguments. Should a user be able to invoke some of the commands independent of doCommand()? Those methods should probably be made private.
Note that the Help class overrides CARRIAGE_RETURN when it doesn't need to.
H. Review the method design
In CommandLineInterface#main, the last continue statement is not needed at all. Again, most of the conditions should be handled in CommandProcessor.
Chart#chartPowerToHTML might be better implemented as two methods, one that creates the Google Chart and another that writes the file.
I. Check for common look and feel
Seems like the code was pretty uniform. I couldn't really see any discernible differences between the implementations.
Phew, I think I'm done now. Check back later for part 2!

Tuesday, November 3, 2009

Lucky 13

I think that there isn't enough group work in computer science classes. Granted, I've taken most of my classes here at UH, so I don't know how other schools are. But it seems like we have students who graduate and think they can hack it out themselves when 90% of the time they need to work in a team of other programmers. Maybe it's my limited view, but I hope more and more students learn how to work in groups. It also helps if the students have access to code repositories and maybe even continuous integration.

As part of working on a command line interface for Watt Depot, we learned about CI. More importantly though, we started working with one or two group partners and applied the all of the tools we learned about in class to complete this assignment. My partner for this project was Yichi Xu and our group name was "umikumakolu" which translates to 13 in Hawaiian. We decided to split the 10 commands in half where we each implement 5. I did take some of the harder ones, but I had few issues implementing them.

At first, we got to a rocky start. Because we were modifying the same source file, we encountered Subversion merge conflicts constantly. After a few pointers from Philip though, we refactored the code to separate the commands to individual files. In hindsight, our initial design was pretty bad, if not terrible. Because each command ended up in a separate file, we saw few merge conflicts after that. We later refactored to follow some design patterns suggested by Philip. Honestly, I don't know very many design patterns (singleton and now dispatch tables). I should read up on that more because these design patterns simplify my code and may even make it more stable.

I applied test driven development to create a few of the commands. Hence, my tests are somewhat comprehensive where they test for all sorts of bad input. I did find that the tests sometimes took a while because they needed to make multiple requests to the Watt Depot server. Then again, that was when all of the tests were in one file, so I had to run all of the tests each time I made a small change. With the program's current structure though, I think TDD would go a lot more smoothly.

I'm happy to say that we completed the commands as outlined here. I uploaded our distribution to the WattDepot-CLI project, which you can find here.

Sunday, November 1, 2009

Everything's Shiny Captain

I mentioned before that one reason why having a code repository is great is because we can always go back to a stable state. Things become more complicated if other users are committing code as well. How do you know if the code in the repository is stable? And if it's not, how do you know whose change to roll back?

During my time in the LILT lab, I quickly became familiar with CruiseControl, which is a Continuous Integration (CI) tool. Whenever we committed code, the unit, functional, and acceptance tests would all run. And when everything was successful (cause I always checked my code... not) it would be green. And if someone broke something, it would be red. I actually was kind of afraid to commit code at times, since I was afraid of breaking the build and having the blame log be pointed at me (who was still very new to this whole programming thing). But I grew to accept it and became more comfortable using it. And I now understand how important it was to the overall health of the project.

In our software engineering class, we are creating a command line interface for WattDepot, which is part of a research project here at UH. This gives us the chance to apply a lot of the things we've learned over the past few weeks (automated QA, code repositories, and style guidelines) while working with fellow classmates. We were also introduced to Hudson, which is another CI tool that integrates with tools we already use in class (namely JUnit and Ant). It was pretty easy to set up so that it checked for a new commit every 5 minutes. At first, it failed the build since the code did not pass Checkstyle. Once that was fixed though, the weather cleared up and we haven't failed builds since. Make sure you run verify before committing!

So as Miss Kaylee Frye from Firefly/Serenity would say...