Refactoring Fest

(hosted by Naresh Jain)

What is Refactoring Fest? You can also find details about the project we used to refactor and what is the rationale behind this activity.

I guess the only variation from the standard Refactoring Fest, is we had only one computer connected to the projector and we swapped pairs to refactoring code.

We choose VQWiki, a Java based Wiki as our open source project to refactor. VQWiki does not have any unit tests or functional tests. People claim that you need a good suite of tests to refactor. In my experience you need to find an inception point to be able to write unit tests. Sometimes (in my case most of the times) I need to restructure code to be able to write unit tests around it. Also we need to understand the code well enough to be able to do this. And to understand the code, you might have to do some refactoring. This seems like a catch 22 situation where you can't refactor without having tests and you can't write tests without some basic refactoring.

We started the fest, by me (Naresh Jain) showing a suite of Selenium tests, that I created for the wiki. This is fairly easy to start with. Pick a functionality that you are interested in, walk through the UI (web UI in this case) and quickly create (record) Selenium (functional UI) tests. People agreed that this was a good starting point.

We picked the "recent changes" functionality. If you change a page, you should be able to find it under the recent changes link. If there are no changes then the recent changes link should say "No recent changes are found". We ran the test and it was Green. Great starting point. A Green Bar.

Here is the selenium tests:

Matt Scilipoti and I started pairing. We switched pairs as we went along.

We started to look at the RecentChanges? Servlet to understand what was going on.

Whole class at a glance:

The main method for a servlet is the doGet method:

doGet() was again calling the following methods

This is really what the class had when we started.

After series of refactoring we ended up with the following class

The doGet() looks pretty much the same expect is had better names and is far more communicative of what is happening

shouldIgnoreCahce() was called mustReload() It was not very clear what it meant by mustReload(), what are we reloading? All we want is to get the recentChanges? It turns out that this class maintained a cache (instance variable all) to hold the last recent changes. And if the cache was up-to date, then there was no need to fetch all the recent changes again.

You can also notice that earlier we had a method called getDeltaMillisReload() which would getRecentChangesRefreshInterval() from Environment and then do * 1000 * 60. We inlined this method coz it was not carrying its weight and we also introduced another method on Environment called getRecentChangesRefreshIntervalInMillis().

Lets have a look at the other methods:

We renamed the reload() to fetch(). What its really doing is fetching the recentChanges from a flat file. Since it is in the recentChanges servlet class, fetch() is good enough a name.

We would have loved to refactor this further. Esp. pushing the loop into the ChangeLog? and just giving it a date range to fetch changes from. Also we could have got rid of the null check, by making sure getChanges() always returned a collection, even if it was an empty collection.

We were also able to clean up the whole date logic which was getting in the way. We created a method called timeForNDaysAgo(int daysAgo) which return the required date.

This method could have been a little more clearer. Why are we doing -1 * daysAgo is not very clear.

There was also the logic for how many days you need to get the recent changes for. Certainly you don't want everything from the day the wiki was created. This logic was also mixed up in the reload(), we pulled out numberOfDaysToGetRecentChangesFor() method to make it clear. Essentially replace temp with query refactoring.

You'll notice that if the PROPERTY_RECENT_CHANGES_DAYS is not set or is set to 0, we default the value to DEFAULT_NUMBER_OF_DAYS (which is 5). This would also be a great thing to have a method on Environment which takes a default value. This is certainly a duplication that everyone who calls getIntSetting() might be checking for zero and setting a default value.

This is how far we got in 1 hour.