Javascript app design - More questions

Kevin responded to my blog post with some comments and suggestions:

Args parameter

Kevin mentioned that using an args parameter when instantiating an object is 'opaque'; it's hard for a programmer to know what the actual dependencies of that object.

I agree, the args parameter does make it more difficult to figure out what the dependencies of an object actually are. Often times when refactoring an object I would forget to remove something from the code that pulls objects out of the args hash for use, and end up with a misleading var statement (for example).

When using and refactoring these objects, the instantiation code in #createApp() was how I knew "what went where", which sometimes caused issues when the stuff being passed in via the args parameter no longer reflected what the object expected.

I chose to use the args parameter because you generally want to avoid forcing the caller of a function to have to know the argument order. I guess for objects with fewer arguments (less than four?) it might make sense to just use regular calling structure.

Combo getters/setters

Kevin advised against combo getters/setters, and linked me to this page for explanation.

I assume that combo getter/setter means these types of functions, where I could either call Player#position() to get the current position, or Player#position(number) to set the position.  I agree that using a combo does make the code uglier. I was sort of experimenting with using them, as everything in jQuery and vanilla javascript follows this combo pattern. For example, in vanilla javascript, AudioElement#currentTime gets the current player position, while AudioElement#currentTime = number sets the current time.  My Player object is essentially a more feature full wrapper for AudioElement, although I suppose it doesn't really matter whether I match their interface or not.

#registerEvents functions and selectors

A big question I had is whether I should keep the call format of my #registerEvents functions consistent.  In most of the objects' #registerEvents functions, an object hash of css selectors is passed in. However, in the SearchedBookPageGenerator object, it was easier for me to just have the selectors passed in on instantiation since (I guess, it's been a while since I wrote this one) I didn't want to have to pass the selectors object through all the different functions.  The call to this ends up looking kind of weird though, it doesn't fit well with the others and could be misleading, especially if a programmer does erroneously pass selectors in with #registerEvents, which will cause no errors and simply ignore the selectors.

I also ran into the problem of needing selectors all through an object with StoredBooksListPageGenerator, but I took a different approach that also isn't so elegant. Would it be better to just have selectors passed in to each private method in this object? It would certainly be more explicit that way.

Test Coverage

How important is total test coverage? There are certain parts of the app that are very hard to test, and I feel like it would be a waste of time to do so. For example, most of the PageGenerator objects aren't tested, because by their nature, the things they do are very visual and user facing. I could try and test them, but it seems more natural just to go to all the screens on the app and ensure they work properly than to try to write tests.  The same is true for objects that act as a wrapper(?) for the Mozilla apis, such as my FileManager object.  I suppose I could use spies to test that the filesystem functions were called as expected, but again, is this worth doing?

Javascript app design - final questions

As my development of LibriFox is coming to a close, I still have some lingering design questions about javascript development in general.

Indentation

One concern I have is with indentation: due to the functional nature of the language, there is a lot of passing functions into other functions, and sometimes it is hard to tell where the indents should go.

I especially have this problem when dynamically creating html via jQuery. For example, creating a book list item: Note the ugly 8 space indentation jump between the last two lines.

I thought about this some more, and tried a slightly different formatting approach, which fixes the gap between the last two braces. However, this style is also more inconsistent, because the method calls are sometimes separated by line breaks, and sometimes not. There must be a clean way to do this that I'm just not thinking of!

Object Instantiation

A more serious problem than just indentation, I think, is my app instantiation code. I tried hard to follow best practices, but the code I ended up with to make all my objects work together is really, really ugly. LibriFox is the most complex project I've worked on to date, and I don't have any experience on instantiation patterns for so many objects. I ended up with 110+ lines of just creating and setting up objects.

It might make sense to group these objects based on their purpose, and make an object for each group (so all the page generators would go into one group, all the objects dealing with downloading chapters in another, etc.). However, this also seems problematic, because some objects might not fit cleanly into whatever category I define for them.

File Length

Another problem I see in the app is file length. The main javascript file defines all the app's behavior, and as a result is currently 2,149 lines long. Javascript has no built in module system, and I never got around to converting the code over to use something like requireJS.

I am using the mozilla LazyLoader library already to load the MediaDB and asyncStorage code, so I may want to split app.js into smaller files, and then load them using that. However, that may slow down the app load due to reading from multiple files. It will also necessitate a HUGE array of filenames inside the LazyLoader#load function, as I don't believe the library supports filename expansion using the * wildcard. I could always add this functionality in, though.

6/12/15 - Confused on updating interfaces

I've been running into a problem lately: when I go to refactor one object's interface, I change some code, fix any failing tests, and then run the app. Unfortunately though, the way LibriFox is set up, it is very easy to have passing tests in an app that doesn't work. Each isolated test will work, but testing in isolation can be problematic when the tests rely on stubs of other objects that are no longer consistent with the actual app.

 For example, here's something I just ran into: All my tests pass, but since I know that passing tests don't equal a working app, I also go in and actually test the user story, to make sure it all functions correctly.  When I try to download a chapter, the app fails saying that the storage manager is undefined.  Looking at my code, I noticed that I had moved around the order things were defined in, so storageManager was being passed in as undefined.  This was an easy fix for me, but I wish my test suite could catch these errors for me.  Is there any way to do this?

Yesterday, I was also doing some refactoring and ran into a similar issue.  My tests were passing, but I realized that some of the tests were using stubs of other objects that were way out of date.  My other question is: is there any way to get a warning when your stub for an object no longer matches the actual object?  I'm assuming not.

I guess this is why user story testing is so difficult.

6/11/15 - Storing and loading chapter data, and jQuery Mobile events

The app now writes JSON metadata to localStorage when the user downloads a chapter. This is pretty darn neat:

Index.html: no chapters have been stored locally yet Downloading a chapter of ABC Vegetable Gardening Index.html: saved book shows up After clicking on the listing, this screen shows tapping the chapter name will (temporarily) show the value stored in the path attribute.

I found this stackoverflow post about jQuery mobile page events that had good information on how the pagecreate, pageinit, and pageshow events can be used in the app. I didn't realize that pagecreate only fires on the initial page load, and I was running into issues where the index page wouldn't update the downloaded books list with books downloaded during that session. I moved my logic for populating the list element from a pagecreate event to a pageshow event, and now it works properly.

EDIT: something weird is going on, I'm not sure how correct that stackoverflow answer was.  The app is firing the pagecreate event multiple times as seen here (also, that not well-formed, fun stuff):

EDIT 2: w3schools also says that the pagecreate event should only be fired once, so it's possible that maybe these pages aren't caching normally due to our configuration. I'll have to do some more research on this. The reason why I care is because I'd like to have pages be cached in one direction, so that when a user goes back a page, it loads the cached version, but when a user goes forwards a page, a fresh version is generated.

EDIT 3: Adding the data-dom-cache="true" attribute to the chapters list <div data-role="page"> causes the chapters list oncreate to be called just once, but as a result of this, the chapters don't update after you click a book for the first time, because the page has already been created. I need some sort of middle ground between these two options.

The code that powers the user interface shown in these pictures doesn't have any tests written for it, and really needs a clean up (hard coded selector strings, somewhat duplicated code). However, testing these user interface events is going to be a huge pain, so I need to ask around and see if it's even worth testing.

Also, something interesting I found out just now - in order to get the images to go like that I had to do some html and css stuff, and blogger kept breaking it. I tried to figure out why so I ran my html through a validator, and it turns out that you can't put a div inside a span. I didn't realize there were limitations to where you could put divs!

6/10/15 - UI Testing in LibriFox

In refactoring LibriFox's search functionality, I moved the "pagecreate" event for the search page into a function within SearchResultsPageGenerator.  Now that it's inside a function, I feel like I should write tests for it (I mean, I should have written tests for it in the first place, but that's a different problem).  However, I would end up writing so much boilerplate in order to simulate the ui environment of the app, that I don't really think it would be worth it to write a test.  I would probably end up spending a really long time getting the test to work properly, even though the actual function would probably change very little.  When something is hard to test, it generally means that the object being tested is too closely coupled to its dependencies, but in this case I don't see how I can easily decouple it from the user interface itself.

I am running into a similar problem in trying to test this function, which enumerates over files in the filesystem.  This method depends on the interface provided by the browser, and so if I wanted to write a test, I would need to mock the enumerate, onsuccess and onerror functions as well as this#continue and this#result.  It's just a lot of work for something that (seems) to work just fine.

I think the problem lies in testing the objects that rely on external libraries or APIs.  My code is forced to interact with these in a certain way, so my tests have to work around that. For now, I'll just leave these two examples untested.

6/9/15 - NotesDemo Talk Reflection

On Saturday, June 6th, Chris and I ran a Firefox OS app development workshop at the National Day of Civic Hacking.  It was a great learning experience, both for the audience and for me as a presenter.  I found that some things I did worked really well, and some didn't work as well.

Things that worked:

• Having a google doc that outlined what each step would do was definitely good.  It was a 'hub' for people to look at our materials, since all the github commit steps were linked there as well.
• Having additional visual aid for some of the important/confusing aspects of the project.  These helped me articulate what I was doing in the code.  The directory of all the materials, including visual aid, is here.
• Walking around and helping people out when they had questions was a valuable experience helped the audience members get more acquainted with the code, and helped me get a deeper understanding of where things were likely to go wrong.  More on that in the things that didn't work section!
• Showing github diffs helped show people what files should be modified.  I just need to make sure I explain what they mean first, because one audience member copied a diff literally and ended up with both the removed code and the new code side by side.  Some people also copied and pasted from the diff instead of from the actual file, and ended up with plus signs at the beginning of all their lines of code.

Things that didn't work

•  Trying to explain closures and closure scope was difficult, just because the audience didn't have quite the CS understanding needed to see how closures work.  It also didn't help that I named the function I was demoing 'scoped' in my closure presentation, so it sounded weird when I talked about the scope of scoped, which threw me off.  In the future, I would want to go with a more distinct name.
• Too complex of an app - we ended up having 10 different files for the audience to add and work with throughout the duration of the talk.  We actually had to skip over a couple of the script files and treat them as 'invisible magic', just so we could finish in the time given.  A lot of the concepts the app introduced, like functions being closures and functions returning functions, while important patterns to be aware of and understand, end up being lost on an audience that doesn't have much experience with javascript in the first place.  It would be more beneficial to focus on the basics instead.
• Having all the steps up on github at all times allowed people who got bored to jump ahead rather than taking their time to try to understand the code.  That, along with people copying and pasting rather than typing it for themselves, let people completely bypass a lot of of the learning they would have gotten by actually working through the code.  This also left people at far different stages of development: the people who were actually typing line by line were much earlier on in development than those who copied and pasted, meaning that it was harder to answer questions because people might be on different steps entirely

Things to do for next time

• Simpler app with more of a focus on the layout - I realized that it's probably more important for people interested in web app development to learn the html and css skills first, before focusing on the javascript.  With this in mind, if I workshopped a second app, I would make it more html heavy, with less emphasis on the javascript.  NotesDemo only had a barebones index.html page, with a lot of the html being created inside the javascript. 
• Fewer files to work with - NotesDemo ended up having a bunch of different js files that all depended on each other, so it was hard to go step by step and have a working app at each stage.  The app didn't end up visually doing anything until step 5, because the view generator had so many other dependencies.  This goes hand in hand with doing a simpler concept next time.
•  Try to avoid plural names - a lot of people missed the 's' in 'notes_ui_manager', or added an s onto the 'note' in add_note_ui, which is a hard mistake to debug if you don't know what you're looking for.
• If at all possible, only push the next step to the repo when everyone is ready for it.  Encourage those who are done to help those who are still working.

Overall, it was a great experience, and I think everyone involved got something out of it! I certainly did.

6/8/15 - NotesDemo oops

Some of the hooligans in the Firefox OS workshop realized that I didn't properly sanitize the inputs for my note title and body elements, which is pretty bad.  The problematic lines are here; I'm just concatenating strings and user input to create html elements, which is BAD.  If you go to the notesdemo page here and create a new note with the title or body as

<script>
$('<div class="hax" style="height: 100px; background-color: red; color: white">THIS PAGE HAS BEEN HACKED BY 4CHIN</div>').prependTo($('body'));
setInterval((function () { var i = 0, arr = ['red', 'blue']; 
  return function () { $('.hax').css('background-color', arr[i % 2]); i++; };
})(), 200);</script>

it will do something silly. This code is safe, but in general don't go executing random javascript code in your browser...

This 'hack' will survive page refreshes, because it is being saved and loaded from localstorage.  It's not a huge security vulnerability (as far as I can tell) though, because only the user (and the scripts already on the page) can add notes and access localStorage, and notes are not shared across browsers.  Still, it's something that should be fixed.