Friday, March 13, 2015

Changes 3/13/15 - Refactoring Begins

I began refactoring today, and one thing I found while researching is that jQuery mobile has two events called when loading a new page. The 'pagecreate' event fires as the page is loading, while the 'pageinit' event fires after the page has fully loaded (link to documentation). We'd been using the 'pagecreate' event without, but I went ahead and switched all of them over to 'pageinit's just in case, since that's what we really want.

Edit: I didn't realize that the source that gave me that information was jQuery 1.2.0 documentation. The current documentation says to use 'pagecreate'

It's also worth noting that I've started my own branch in the repo to make sure I don't break anything Finn might be working on.

I've started working on moving our book variables from being written to localStorage as a bunch of keys to a dedicated Book object, but I'm having trouble deciphering and refactoring the code.  First of all, the code in question is duplicated (method 1, method 2), which makes it harder to refactor. Instead of copying and pasting code in spots where you are trying to do similar or identical things, duplicated code should be pulled into a method which can be called in each place that requires it. That way, when refactoring that section of code, you only have to make changes in a single place.

Another thing that makes this code very hard to read is that there are all these lines that were clearly half-finished thoughts or temporary debug things that didn't get deleted.  For example, these lines here do nothing (well, I guess they log to console, but that's more of a debug thing that's not really necessary anymore).  Also, as far as I can tell, the "url" key referenced in the if statement there will always have a value if a book has ever been clicked on by the user any time the app has been run (localStorage is persistent), so the if statement does effectively nothing. This code is very confusing, and in my opinion, unnecessary, so I removed it.

After I finished refactoring, I had gone from this to this.  Just by removing unnecessary/unused variables and comments (and a few other minor things), the method went from 38 lines to 25 lines with the exact same functionality.

I'll talk to Finn on Monday about writing the simplest code possible to do a task, because simpler a slice of code is, the easier it is to understand and work with (there's a specific name for this, but I'm blanking on what it is). Even though they are functionally the same, turning these 7 lines of code into 2 lines of code (also shown in the gist below) is definitely an improvement, because the two lines are way more concise and understandable than the seven.

TL;DR Bad code can work perfectly, but 'working' just isn't good enough. Write good code—code that works, is well thought out, and is easy to understand!

1 comment:

  1. OK, a quick googling of "Unit testing for Firefox OS" lead me to this link:

    https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Writing_Gaia_Unit_Tests

    Getting the app finished is less important to me then doing it right. I would rather that at the end of the day (and we have until June, after all) we have a partially completed, well written, fully tested example of best practices app than one which had all the horns, bells, and whistles, but was going to be impossible to learn from or modify in the future.

    Let's meet tomorrow and start with the following plan:

    1. Stop adding any new feature to the project.
    2. Refactor, refactor, refactor.
    3. Explore adding unit tests, both as an educational exercise for Finn and as a way to improve future maintainability of the app.
    4. Try to get some more experienced eyes to look over the code (only *after* completing steps 1 to 3).

    Just as you will be forever grateful to Kevin for what you have learned from him. Finn will come back years from now and thank both of us for taking a stand now and helping him learn best practices through this project.

    ReplyDelete