Refactoring a JavaScript Single Page App - Part 1

This tutorial series breaks down how to refactor a single page web application that has lots of in-line JavaScript. I put together a simple page that contains some technical debt, so that I could highlight some of the less than ideal parts and of course, how to improve them!

I wrote this code under the following conditions:

• As fast as possible (~ 1 hour)
• No 3rd libraries or frameworks (other than Google Maps of course)
• No stopping to refactor

What the app does?

I based this app on the SF Movies example from the Uber Coding Challenge page. (I also wrote the server-side code for this, which I will save for another tutorial).

The spec is to:

Create a service that shows on a map where movies have been filmed in San Francisco. The user should be able to filter the view using autocompletion search.

You can:

• Search a movie by title.
• Select a movie for more information.
• Once a movie is selected, all the locations are plotted on the map (using Google Maps).
• You can click a marker to see more information about the location, including satellite or street views of the location.

Here’s a screen shot of the app:

There is a small video demonstrating the functionality: SF Movies App Video

The First Iteration

Here is a gist of the less than ideal code. It’s all contained in a single HTML file.

I’ve definitely written code like this earlier in my career. In fact, I would say that there’s a lot of JavaScript out there like this, especially if it was written more than a few years before all things JavaScript started to mature. If the JavaScript you are working with now resembles this example a little, then the points covered in this article will help to make the code more manageable!

Important Note: This is the first part in a series, and in this part there is more emphasis on re-structuring the application rather than analysing each line of code.

Let’s breakdown the problems with the code.

Issue #1 In-line CSS and styling

Looking at the gist, the first thing we have to scroll through is CSS. So for a quick win this can be moved into a separate file. Something that’s also not ideal is style information in the JavaScript. Here’s an example of this:

var homeControlDiv = document.createElement('div');
homeControlDiv.id = 'bottom_panel';
homeControlDiv.style.backgroundColor = 'white';
homeControlDiv.style.borderStyle = 'solid';
homeControlDiv.style.borderColor = '#ccc';
homeControlDiv.style.borderWidth = '1px';
homeControlDiv.style.width = '100%';
homeControlDiv.style.height = '150px';
homeControlDiv.index = 1;

If we extract this into the CSS file also, we can drop several lines of JavaScript:

var homeControlDiv = document.createElement('div');
homeControlDiv.id = 'bottom_panel';
homeControlDiv.className = 'bottom_panel';

This is a small change, but imagine how this problem could be amplified over a large code base? Hundreds (thousands?) of lines of JavaScript could be avoided!

For good measure, we should also use className over id to apply the CSS style to avoid CSS specificity issues.

See the git commit to track the changes made.

Issue #2 Google map usage

Usage of the Google Maps object (google.maps) is sprinkled throughout the code. What if the map provider needs to be changed? I would have to search through and find all instances of google.maps in order to replace them. There is also code that toggles certain elements of the map through the app to show/hide elements, plot/remove location markers etc. If the map logic was moved to its own abstraction, it could also maintain its own state.

The Google Maps API has a lot of functionality, but this app uses only a fraction to:

• Add div panels to the top and bottom of the map
• Add location markers to the map
• Configure map UI options e.g. what input controls are visible
• Configure map view i.e. satelite, streetview etc

Only the functionality to support this is exposed via this new abstraction which can be used as:

var map = new MapService(window.google, mapDiv);
map.addView(searchControlDiv, 'TOP_LEFT');
map.clearMarkers();
map.plotLocation(lat, lng, callback);

It’s a subtle change, but it means larger blocks of code within the main app to configure the map state, such as:

map.setCenter(pos);
map.setZoom(20);		
map.setMapTypeId(google.maps.MapTypeId.SATELLITE);		
map.setTilt(45);		
map.setHeading(90);

Can now become a single line, since such code can be moved to the abstraction:

map.sateliteView(location.geo.lat, location.geo.lng);

See the git commit to track the changes made.

Issue #3 Doing too many things

I believe in the mantra of software libraries that should do just one thing, and do it well. Something that violates this principle in this app is the functionality for the AJAX autocomplete. Logic for the autocomplete is mixed in with the logic for the app, resulting in a bunch of mixed up functions that are hard to decipher.

The main code for the autoCompleteEvent function can be found here.

The next step is to find a library that does one thing very well, which in this case is an AJAX autocomplete library. There are many to choose from, but for a little fun I wrote my own AJAX autocomplete using Karma.

See the git commit. With this change, we’ve been able to remove a lot of code from the app. All the autocomplete related logic we saw before can now be replaced with:

new window.Autocomplete(searchInput, { 
  url: '/movies/search', 
  param: 'q',
  label: 'title',
  value: 'releaseYear',
  select: function(item) { 
    clicked(item);
  }
});

I introduced Bower for front-end package management. This enables the download of the autocomplete library from Github rather than have it as part of the code base.

Side Note: I know that I said that a condition in building this app was no 3rd party libraries or frameworks. But that condition was in place to emphasise the importance of using small, single purpose libraries to grow a code-base.

Issue #4 HTML markup mixed with logic

To recap, the index page is looking a little leaner so far as we can see from the gist. In order to understand the main functionality and purpose of the app, we’ve gone from having to read 434 lines of code to read, to just 222. There’s still room for improvement.

The next problem is chunks of code that dynamically create HTML and register it to the DOM. Here’s an example:

function HomeControl(controlDiv, map) {
    // Set CSS for the control border.
    var dashBoard = document.createElement('div');
    dashBoard.innerHTML = '<h2>SF Movies</h2><div>See film locations for all movies filmed in San Fransisco. <strong>Click on a marker to see more information about a location.</strong>.</div><p>This is a sample project from <a href="http://www.bradoncode.com">Bradley Braithwaite</a>.</p>';
    dashBoard.id = 'dashboard';
    dashBoard.className = 'dashboard';

    controlDiv.appendChild(dashBoard);
}

Since the HTML is associated with the presentation, we don’t really want this mixed in with the logic of the app. If we centralised the HTML, it would be much easier for us to make HTML changes in the future.

This is a little easier on the eyes:

function HomeControl(controlDiv, map) {
    var dashBoard = document.createElement('div');
    dashBoard.innerHTML = templates.home();
    dashBoard.id = 'dashboard';
    dashBoard.className = 'dashboard';
    controlDiv.appendChild(dashBoard);
}

What does templates.home() do? Here’s the code:

templates.home = function homeTemplate() {
  var buf = [];
  buf.push('<h2>SF Movies</h2>');
  buf.push('<div>See film locations for all movies filmed in San Fransisco.'); 
  buf.push('<strong>');
  buf.push('Click on a marker to see more information about a location.');
  buf.push('</strong>');
  buf.push('</div>');
  buf.push('<p>This is a sample project from' );
  buf.push('<a href="http://www.bradoncode.com">Bradley Braithwaite</a>');
  buf.push('</p>');
  return buf.join('');
};

Note: another approach is to make use of template libraries such as handlebarsjs. Since they ultimately take HTML fragments, parse the contents and make them into strings anyway, I’ve used strings in JavaScript to keep things simple. Templating “may” be introduced in a later tutorial ;-)

See the Github commit.

Issue #5 Functionality not logically grouped

A problem we have with the current code is that it’s not easy to determine which section does what based on the app’s features.

To recap, the main features are:

• Home Screen
• Search Control
• Movie Detail
• Location Detail

This is a simple change to group the related functions under comment headings which will help us for the next step.

See the git commit to see how the code has been logically grouped.

Issue #5.1 Everything in a single file

The next step is to move these logical feature groups into their own files.

See the git commit to track these changes.

The index file now look as follows: Gist.

The entry point to the app is now a lot easier to understand:

<script type="text/javascript">
  /*
   * NOTE: this (probably) isn't how you should be doing JavaScript!
   */
  var map;

  function initialize() {
    var mapDiv = document.getElementById('map-canvas');
    var map = new MapService(window.google, mapDiv);

    var searchControlDiv = document.createElement('div');
    searchControlDiv.id = 'top';
    searchControlDiv.className = 'top';

    new SearchControl(searchControlDiv, map);
    map.addView(searchControlDiv, 'TOP_LEFT');

    var homeControlDiv = document.createElement('div');
    homeControlDiv.id = 'bottom_panel';
    homeControlDiv.className = 'bottom_panel';

    new HomeControl(homeControlDiv, map);
    map.addView(homeControlDiv, 'BOTTOM_CENTER');
  }

  google.maps.event.addDomListener(window, 'load', initialize);
</script>

Issue #6 Not using Code Analysis

Moving the code into separate files makes it easier to use code analysis tools. A popular choice for JavaScript developers is JSHint.

Running JSHint against these files yields 64 errors (which I have to admit, is less than what I was expecting). The errors range from lines that are too long, not using ‘strict’ code and missing/using unnecessary semi-colons.

See the git commit to track these changes.

Working through the issues leaves 27 jshint errors that are less trivial to fix and also reveal a problem with the architecture of the app.

The errors are all along the lines of:

app/home-control.js: line 5, col 25, 'templates' is not defined.
app/home-control.js: line 3, col 10, 'HomeControl' is defined but never used.

What does ‘is not defined’ mean? Consider the following code:

'use strict';

function HomeControl(controlDiv) {
  var dashBoard = document.createElement('div');
  dashBoard.innerHTML = templates.home();
  dashBoard.id = 'dashboard';
  dashBoard.className = 'dashboard';
  controlDiv.appendChild(dashBoard);
}

As this file is now self-contained, jshint analyses this without taking into consideration the rest of the files. This is why it reports that HomeControl is never used, even though we know that it indeed is from elsewhere in the app.

Despite these warnings, the code runs without error. This subtle error is due to global variables in JavaScript. JShint is right, the object called templates isn’t defined in this file, but when I run this code in the browser, the browser will look for templates on the global scope (the window object) where it will indeed find it, since our file templates.js creates an object called templates and places it on the global window object.

The use of document.createElement should also generate a warning, since we haven’t defined that either but it doesn’t since document is provided by the browser. JShint is configured to know about document and that this is a global object that will always be available as convention.

Issue #6.1 Not specifying global variables and functions

How to fix this? Well, it depends what you mean by “fix”! We can make the jshint errors go away by being explicit as to which variables are on the global/window object.

Here’s the updated HomeControl function with these changes. As with the document object, JSHint knows that the window object will be defined by the browser, so it doesn’t produce an error. In this updated code, we are being specific that we are adding objects to the global window object:

'use strict';

window.HomeControl = function(controlDiv) {
  var dashBoard = document.createElement('div');
  dashBoard.innerHTML = window.templates.home();
  dashBoard.id = 'dashboard';
  dashBoard.className = 'dashboard';
  controlDiv.appendChild(dashBoard);
};

Important Note: Placing things on the global window object like this is not ideal, even though it corrects the JShint errors. But, we’re moving in the right direction. Improving upon this will be covered in a later tutorial.

See the git commit to track these changes.

Issue #7 Not delivering a single JavaScript file

By fixing the problems raised so far, we’ve introduced a new one. Breaking the features out into separate files comes with some extra download bloat for the app. The number of JavaScript includes now looks as follows:

<script type="text/javascript" src="map-service.js"></script>
<script type="text/javascript" src="templates.js"></script>		
<script type="text/javascript" src="home-control.js"></script>		
<script type="text/javascript" src="location-control.js"></script>		
<script type="text/javascript" src="movie-control.js"></script>		
<script type="text/javascript" src="search-control.js"></script>		
<script type="text/javascript" src="app.js"></script>

Using a minification tools along with a task runner such as Grunt, will condense these files into a single file. The result being that we can reference the JavaScript source as a single file:

<script type="text/javascript" src="dist/sf-movies-web.min.js"></script>

See the git commit to track these changes.

Summary

There is still room for improvement, but the app is in better shape than before. Now, if a developer new to this project had to go and change the HTML for the home panel, or change what movie data is displayed once a movie is selected, they would have a much better chance of identifying where in the code-base they should be looking.

The next set of improvements will be discussed in the next tutorial (coming soon).

The Github project and branch can be found at: https://github.com/bbraithwaite/sf-movies-web/tree/refactoring

Part 2

See Part 2: Refactoring a JavaScript Single Page App - Part 2, AngularJS