Refactoring - CMPUT301F13T13/StoryHoard GitHub Wiki

Refactorings Applied

1. Devious Duplicate code

Problem: We noticed a terrifying amount of duplicate code in EditChapterActivity and ViewChapter, by a terrifying amount we mean the functions insertIntogallery(), takePhoto(), browseGallery(), and OnActivityResult() were exactly the same in both activities.

Solution: We made a new abstract class, MediaActivity.java, which extended the Activity class. We moved all that duplicate code to MediaActivity, and simply had EditChapterActivity and ViewChapter extend that class so they could both use the needed functions without redundantly defining them in each activity.

2. Blubby Blob class

problem: We thought we were being clever when creating the GUIMediaUtilities class. There were functions we were using and we didn't know where to put them, so we just stuck them in GUIMediaUtilities. But the number of functions we began putting into it grew and grew, until we knew that something had to be done.

solution: Looking over the functions in GUIMediaUtilities, we noticed that the only classes that were actually using these methods were EditChapterActivity and ViewChapter. All the methods were related to dealing with images: getUri(), insertImage(), getRealPathFromURI(), calculateInSampleSize(), decodeSampledBitmapFromUri(). Since they were only needed by the the two activities mentioned before, we decided to move them to the new MediaActivity class we had made before. Now the images functions were together and ready to be used, and it allowed us to banish GUIMediaUtilities from our project and into the land of Recycle Bin.

3. Blob class and the ManagerFactory

problem: Ah yes. The SHController class. Although now it is nothing more than a simple interface, it used to contain all of the functions dealing with manipulating images, stories, chapters, choices, and media. At first it seemed like a good idea to have all the necessary functions in one class so every activity could use it, however, the class became so monstrous in size it took you an inacceptable amount of time to scroll down through it to find the method you were looking for. Every time we added a new functionality to the application, we added in just "one more function" to the SHController.

solution: We decided to make a controller interface (what is now SHController.java), and split up the class into separate controller classes that implemented the interface and could deal with each type of object specifically. We ended up with ServerStoryManager to deal with published stories and other server related functions like getRandomStory(), LocalStoryController, ChoiceController, MediaController, ChapterController. These new classes are much smaller and its methods more related making it much easier to read, understand, and scroll through. If you look at the SHController interface, you would also notice there is no retrieve method defined. This is because each controller class has different types of retrieval methods, eg. getAllStoryChapters, searchById, or getRandomStory, etc.

This refactoring also allowed us to get rid of the ManagerFactory class and the ObjectType enum class (and with it a lot of if and switch statements). We had been using one function for things like inserting, and passing it an OjectType of whatever the object (either published story, cached story, created story, chapter, choice, or media). At first, we had no ManagerFactory and had switch statements everywhere to use the right Manager class (the classes that do the actual interacting with the database). When the manager factory was added, we were able to eliminate almost all of the switch statements in the SHController, but alas, this was still not the best because we still had the big conditional in ManagerFactory that decided what Manager class to make. By having separate classes deal with specific objects, we could eliminate both the ObjectType and ManagerFactory and just sleep a little better at night.

3.5 Broken MVC

problem: We had built a system that we thought was MVC, but we were really confused and it was actually a system where the models and views were taking on controller roles as well. We also did not have an on memory model, only the database.

solution: We got rid of the old controller classes, as well as SHController interface. We moved all operations that manipulated the database into the Manager classes, so that they act as controllers to the database. We then made three new real controllers, StoryController, ChapterController, and ChoiceController. We took out methods from the data classes that were really controller classes (for example, the addChapter() method that used to be in the Story class), and moved them into their corresponding controller classes. All the views now call methods from the Controller classes to do any updating of information of the models.

4. Casting dangerous spells and not so generic interfaces

problem: Our two main interfaces were: StoringManager and SHController. Both of these defined the main type of operations that the classes implementing it could perform (mainly insertion, deletion, retrieval, and updating). In order to make them general, all we could think of was to define their parameters as java objects and just cast the objects in each method to the corresponding class we needed (chapter, choice, story, media). What we did not know how to use or even knew existed were generic types in java. Casting worked for the StoringManager, because we made a utilities function that converted arrays of Objects to arrays of Stories, or Arrays of Choices.

But for the SHController, we got stuck trying to return Arrays of specific objects like Story, or Chapter, instead of an array of Objects. Our options were to either return the arrays of objects and have the gui code call the Utility functions, but we didn't want to do this. Our other option was to make new functions that were wrappers; they would call the methods that returned the array of objects, and then they would call the Utility functions and return the array of proper story objects to GUI classes. This would have required wrapping every single function.

Solution: Then came the discovery of a lifetime: generic types. We realized then that all our casting was unnecessary and our problems could be fixed in a much more elegant way than the solutions we came up with above. We replaced the casting with generic types defined in the interfaces. We were also able to get rid of all the ObjectTo____ methods in the Utility class.

5. Crowding the poor onCreate methods

problem: The onCreate methods in the activity classes were doing everything and they were one big unreadable blob. Needless to say, figuring out what was happening in the code gave you a headache.

solution: We split up the code at places where it was accomplishing specific tasks, like setting onCLickListeners, initializing fields, and saving user inputs. We made them into separate methods, named them for better understanding, and also called them in the onResume method rather than the onCreate as a lot of our null pointer exceptions were happening due to everything being in onCreate. The code became, well, not beautiful, but at least much cleaner, easier to read and understand, and it helped to squish a lot of bugs that kept happening.

6. An expensive calculation

problem: This time, there wasn't technically anything wrong with the code. To get a random story, we were retrieving all the stories on the server, and picking one at random with java's random library. This was ok when there was a couple stories on the server, but the more we added, the slower the get random story button seemed to take...

solution: After a lot of looking aroud to understand elastic search queries, we were able to write a query that randomly sorts the stories on elastic search using a custom_score, and then only retrieves 1 story (or none if there are no stories on the server). This was much faster than actually retrieving all the stories on the server and converting each one to a Story object through json, and then picking one at random.

7. ESClient wanted to be a blob too

problem: JDeodorant thought the previous ESClient class was a god class. It was nearing 400 lines, and although the methods in it were related (delete, insert, searchByKeywords, searchById, getEntityContent), it recommended taking searchById and searchByKeywords out and moving them into a new class.

solution: As recommended, both methods that dealt with retrieving objects from server as well as the getEntityContent method which they needed were moved into a new ESRetrieval class, while the two methods that dealt with changing data on the server (insertion and deletion) were left in ESCLient, but the class was renamed to ESUpdates. Both these classes are called by the ServerManager now.

8. Some envious classes

problem: Thanks to the JDeodorant, we saw that the Syncher class' methods such as syncStory, syncChapter, syncMedia, and so on were using a lot of methods from the MediaManager, ChapterManager, StoryManager, and ChoiceManager classes, mainly existsLocally, insert, and update. The controller classes do this as well, but they are supposed to. The Syncher class though we decided could benefit from less feature envy.

solution: We moved the sync___ methods into the manager classes, and replaced them with a call to those methods. In other words, delegation. Now for example, the Syncher class just call storyManager.syncStory() to have the story synched instead of doing all the work itself.

9. An Iffy Situation (type checking)

problem: Type checking on the onActivityResult() method in MediaActivity was leading to a lot of duplicated code. The method first checked whether the request code was for taking a photo or browsing the gallery, and then within each of those blocks, it checked whether the result code was RESULT_OK, CANCELED, or something else resulting in an error. Most of the code under both RESULT_OK blocks was the same.

solution: We couldn't do much with the first set of conditional checking to see the result code was RESULT_OK, but we did switch the order of the conditionals. We first checked to see that the result code was equal to RESULT_OK, and if it was, then check to see what the request code was. This took away the duplicate checks for result code. We also moved the parts of code under each block of checking the request code so there wasn't any code duplication happening. Still ugly in my opinion, but much better than before.

10. Another blubbery blob

problem: JDeodorant identified a blob method in the EditChapterActivity: the illustration dialog builder that asks whether the user wants delete an illustration.

solution: We applied the JDeoderant solution of class extraction and created the DeleteImageDialog class in the code for the dialog building was moved into. The EditChapterActivity was very grateful for the changes.

11. Passing data via Intents refactored into Application class, then Singleton

Problem: When passing around complex objects via storing values inside an intent, we could not pass objects that were not serializable. To overcome this with the intent method, we would have to transform every complicated non serializable object into a serialized state, pass it through the intent, and transform it back into the standard state of the object. This was a significant performance hit for large objects (stories with large illustrations and photo media objects), therefore we needed a simpler alternative.

Solution v1: We originally subclassed the Android Application class to hold our data. This solution, although it worked for our application, broke all of our j-unit tests. The android activity class needed to be instantiated with preexisting variables, but we could not set up the activity because the j-unit tests depended on the activities in the first place.

Solution v2: Instead of subclassing the application class, a singleton class was created. Singletons remained throughout the lifecycle of the application, therefore it was an ideal place to hold values required across different activities. This reduced the amount of gui code clutter, and allowed the objects to be stored in an unmodified state (no transforming an object into a serializable form and back).

12. Improving performance with async tasks

Problem: Every database and server operation was being done on the main UI thread. This was leading to slow performance and a lot of lag. For the server operations, we had used methods to allow the main UI thread to perform networking too, which was a bad smell.

solution: Whenever possible, we created an async task to deal with database and server operations. It did make the activity classes longer, but performance was improved and the main UI thread was left to mostly only deal with GUI tasks. We were not always able to delegate off to an async task, but we were at least able to do so for the heavier operations like publishing or updating stories and chapters, and downloading stories from the server.

Refactorings Not applied:

god class problems according to JDeodorant

  1. JDeodorant recommended moving all the fields, the getStringFromBitmap method, and the getSearchCriteria method out of the Media class and into its a new class. The Media class was created with the purpose of holding this information, we did not see the point of making a class that holds this information and put it into the Media class rendering the Media class kind of useless. It actually recommended doing this for the chapter and story classes as well, mostly to get the getSearchCriteria method into a different class. This just all seemed pointless to us and it would only create another 4 extra classes we would need to maintain and test for.

  2. After refactoring #1, JDeodorant also wanted us to move the insert and delete methods now in ESUpdates into a new class. We found this useless, since the would still end up in the same class together, the only different would be a random class in the middle calling them. ESUpdates already is meant to take care of the uglier code that deals with server so ServerManager doesn't have to. Moving the only two functions left in ESUpdates to a new class that will probably still have the same issues (according to JDeodorant)...yeah, we didn't think it was worth it.

  3. JDeodorant wanted us to move a whole bunch of methods out of ServerManager, for example, searchById, update, insert, etc. These methods already call ESRetrieval and ESUpdates to do the real dirty work, moving them out would defeat the purpose of the class. These methods are already pretty short anyway, they are made longer by try catch statements needed.

  4. JDeodorant wanted us to move fields out of the LifecycleData class. This is a class we specifically made to hold data that can be passed around through the activities and that will remain for the entire life cycle of the application. Moving out the fields to different classes would just defeat the purpose of it and create a lot of unneeded complexity in choosing the right "holder" class to get and set the correct data from.

  5. JDeodorant wanted us to move methods out of the Syncher class. It wanted to move syncStoryFromServer, prepareStory, syncDeletions, syncChoice, and syncChapter. We didn't want to do this because we wanted this class to contain all the needed functionality to send a story to the server and to cache a story from the server. It serves as a "syncher" (would have never guessed right?) by overwriting local data by the new story data retrieved from the server. We decided keeping the related functionality together was more important for us than making the class smaller.

  6. In the SearchActivity class, the spinner code was being identified as a god class. We didn't see any way we could fix this, and whenever we clicked to see the refactoring JDeoderant would apply, it just showed up as "an error has occurred while attempting to refactor", so we thought it was safer to just leave it...

Long Methods according to JDeodorant

  1. In the chapterController, we were recommended to split up getFullStoryChapters() and have it call a new methods with half of the code that is currently inside it. This method is 9 lines long including spaces, curly braces, and the function name...we saw no need to follow JDeodorant's advice here.

Feature Envy problems according to JDeodorant

  1. ServerManager contains an ESRetrieval class and an ESUpdates class. In this case, it wanted us to move methods that use these two classes outside to another class, and just have ServerManager call that class. Because we already have the ServerStoryController which delegates off to ServerManager, we didn't find this necessary. ServerManager is basically acting as an adapter between the ServerStoryController and the ESRetrieval and ESUpdates classes. Adding a class in between just seems like more worth than it is worth.

  2. JDeodorant wanted us to move the getRandomChoice method from ChoiceController to ChoiceManager. Since the controllers are mostly acting as adapters to the StoringManager interface, we thought it made more sense to just leave the randomChoice in Controller. Also, the ChoiceController is MEANT to call all the functions from the ChoiceManager, that is what we made the controllers for.

  3. JDeodorant wanted to move the setSearchCritera() methods in the classes that implement StorignManager into another class. Apparently it thought using the get(), size(), and keys() methods of the HashMap was a bad smell, but we didn't think so, and it made more sense to leave that very specific method only used in each corresponding manager class.

  4. JDeodorant then told us we should move the syncStoryFromServer() method in the Syncher class into the mediaManager class. It probably said this because after refactoring #8 with the sync methods, we call mediaMan.syncMedia() twice in the syncStoryFromServer() class; once for a photo and once for a media. It would make no sense however to move syncStoryFromServer() into the media class since it calls all of the sync methods from all the managers, and even by the name it does not belong in the MediaManager class.

  5. A lot of feature envy was identifies in all the activity classes because they are using the singleton of LifecycleData to pass around data between the activities. So, we chose to ignore JDeodorant on this one because it was on purpose. The same thing occurred with the controller classes. They were meant to have all their methods used by the activity classes, so there was no need to "fix" the feature envy.

Type Checking problems according to JDeodorant

  1. In the MediaActivity class, the onActivityResult() method has a pretty ugly conditional. It checks whether the result code matched the one for browsing the gallery or taking a photo. We applied refactoring #9, but could not use the proposed JDeodorant solution of using a state variable since the onActivityResult() seems to be a very specific method that shouldn't be tampered with. Also, every time we clicked to preview the refactoring, JDeodorant would scream and give an error, so it really didn't seem like a wise idea to actually apply that refactoring.