Sunday, March 15, 2015

Changes 3/14/15 to 3/15/15 - The Refactoring Continues

Over the weekend I've continued to refactor the code, although it's pretty slow going because the code is very hard to change.

I have to be honest here: the code that made our app work was very poorly thought out.  As I mentioned earlier, using localStorage in the manner we were would only allow for one book at a time to be saved and read. As I was looking over the code for refactoring, I also noticed that there was no data persistence in regards to the information pulled from the Librivox server.  Any time we needed any information about a book (which is quite often, as you would expect) we would send another request to the server for the book json, even if an identical request had been sent seconds before.  And as I mentioned in an earlier blog post, these requests were copied and pasted almost line for line from one method to another.

This is what I was getting at in the TL;DR (that means 'too long; didn't read' for all you old folks out there) of my previous post: that working code and good code are not the same thing.  Yes, this code worked, but it wouldn't be called good code by any stretch.

Here's something to think about in relation to web requests: let's say we get to the point where our app almost has persistent book storage between app restarts.  And let's suppose a user has downloaded an audiobook to listen to on his commute to work (or whatever).  However, he doesn't have access to a data connection while in transit, and suddenly finds that he can't see the book he has downloaded, except for maybe the filename of his downloaded audio file because suddenly the web requests required to generate the chapters list and book player pages aren't going through.  Relying on web requests for every page that needs book data is a bad idea, because it makes our app all but unusable without an internet connection.

In order to fix this, I first created Book and Chapter classes, where args are passed in and a Javascript object is created. (If you need to brush up on Javascript objects, here's the w3schools explanation.)  Additionally, I moved the logic for stripping the <CDATA[[]]> tag from the rss' chapter heading text into the Chapter class, which cleaned up the chapter list pagecreate method a bit.

So I changed a couple of important things between this commit and the last one.  In order to make the book and chapter data more persistent (it doesn't survive app restarts, but doesn't have to be downloaded from LibriVox each time), I created a global bookCache. This object functions as a hash, with book ids as keys and Book objects as values. That way, given the id of a book, and assuming it has been added to the bookCache, the actual book object associated with that id can be retrieved
The reason why I've structured the code like this is because we need to have a book show up in search results, then, if clicked on, show chapters and other data in a completely separate method. The book object needs to be accessible to both methods, and it doesn't seem like there's a way to pass a variable directly from one pagecreate to another, which is disappointing, because I don't find the solution I've come up with to be a very elegant one.

Another thing I noticed when refactoring, is that I ended up with a function within a function, which suggests that this method's responsibility should probably be refactored into an object of some sort.

I wasn't able to refactor all the code, but I've made a start.  I actually didn't get a chance to start on the time storage stuff that I had mentioned a couple days ago in this post, so I'll probably work on that on Monday.

1 comment:

  1. I love the graphic! ;-) I do want to caution you about letting the perfect be the enemy of the good (see http://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good). Your desire to strive to have the best code you can is admirable. Since this project will hopefully be a solid part of your resume going forward, it is practical as well.

    That said, it is important to deliver customer value on a regular basis in an agile process, and some of the problems you discuss here do not derive from any of our current user stories (see https://trello.com/b/XOMldjmv/librifox). I don't have any stories about listening to streaming books yet, so we shouldn't be trying to solve that problem. It may be the case that streaming books effectively is a story that will arise, but for now, I've only asked to be able to download books to my SD card, see which books are downloaded, listen to books that are on my SD card, delete books that are on my SD card, and remember where I left off between listening sessions.

    Let's solve those problems first, and following the YAGNI principle (see http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it), actively avoid implementing features that have not been requested.

    ReplyDelete