Refactorings - CMPUT301F12T01/classproject GitHub Wiki

Refactorings will be described here. It would be preferred if the author of the commits would add these as they occurred. Should be in order, newest at the bottom.

An example from one of Mitchell's commits:

Made DeviceStorage an observer (to store new tasks and reports), added a ReportManager (to handle the reports). (taken from the commit message)

It was growing apparent that other classes other than TaskManager needed a concrete implementation of the ObservableCollection. The implementation in TaskCollection was a wrapper (has-a) for a HashMap<UUID, Task> instance and an ArrayList<UUID> instance. Instead, all of the code dealing with manipulating the dual-indexed ObservableCollection instance was moved to a generic class name DualIndexedObservableCollection. All of the methods, save for the constructor were extracted and made generic.

Used JDeodorant to find Long Methods, and followed the wizard to extract this method, with the specified name. The final refactoring was hand-tweaked.

Used Eclipse's refactoring tools to move a for-block from onFormCompletion to its own method.

The following line of code was found in TaskAdaptor.getView():

List<Report> reports = ReportManager.getInstance().getReports(task);
if (reports.size() < 0) {
    // ...
}

The intent is not immediately obvious, and it exposes too much about the ReportManager.

A method was added to the ReportManager called taskHasReports. Given a Task, it returns if that Task has any reports at all. Thus, the code in getView turns into:

boolean hasReports = ReportManager.getInstance().taskHasReports(task);
if (!hasReports) {
    // ...
}

Now to fix that ugly singleton stuff...

More than one piece in the code needed to refresh the BaseAdaptor. Both the update() method implemented from Observer and the onResume() method overridden from Fragment. The code was placed in the method called refreshListAdaptor and called by both methods that otherwise would have had duplicated code.

So we realized that for a good chunk of our UI screens that we were using fragments in the same way we would use a normal activity. In another words, we were not using any special properties of fragments in defining a new task, viewing a task and fulfilling a task (though we still need fragments here for inflating the proper fields to fulfill a task). So the goal of this refactoring was to remove un-need fragments and move all of their work to their activities to reduce useless classes (activities just to start up their fragment) and make code clearer (what is the purpose of fragments if we don't use their properties?). This also helped improved interaction with the app, such as being able to scroll if you enter a very long description.

I won't get into all of the move methods because I don't think it's necessary. If you also look at the commit messages there was a revival of the dummy tasks. We probably should have never gotten rid of them, because there was another refactoring of the model which temporarily took out the ability to store tasks, so we had to use dummy tasks again.

• Moved Methods -- from DefineTaskFragment to DefineTaskActiviy commit 14c205c
• Moved Methods -- from TaskDetailFragment to TaskDetailActiviy commit 16e46fe
• Moved Methods -- from FulfillTaskFragment to FulfillTaskActiviy commit c739031

addTask needed to figure out which TaskCollection to place a Task in. This method is implemented using a rather ugly if/else statement. This got refactored to the following code:

public void addTask(Task newTask) {
    TaskCollection appropriateCollection;

    /* Figure out which collection to dump the task in. */
    appropriateCollection = getCollectionForTask(newTask);
    appropriateCollection.add(newTask);
}

The TaskSourceApplication handled many singleton instances. It has getters for these singleton instances; clients would call these getters and then call the appropriate method of that instance. Now, the most common actions have been delegate to static methods of TaskSourceApplication. Less dots means cleaner code, with clear intent, and more resistant to change. For example, clients do not have to worry about what instance manages Tasks -- they just ask the "global service handler" to get them the desired Task.

This was aided by Eclipses "Generate Delegate Methods" feature.

So, code that once looked like this:

String user = ((TaskSourceApplication) getApplication()).getUserID();
Task newTask = new Task(user);

// ...

TaskManager tm = ((TaskSourceApplication) getApplication()).getTaskManager();
tm.addTask(newTask);

Now looks like this:

Task newTask = TaskSourceApplication.newTaskForCurrentUser();

// ...

TaskSourceApplication.addTask(newTask);