Lately we have been doing a lot of JavaScript development and have been very happy about the improvements in libraries and frameworks, long gone are the days of seeing JavaScript just as a necessary evil. Unfortunately there is still legacy code lying around even in our codebase.
As an example I present an application that was developed by us: Semantic.hri.fi, it provides a Linked Open Data view of www.aluesarjat.fi statistics. What we will be looking at is the SPARQL search page and mainly its source code.
Even with a quick glance it is easy to spot several issues which are nowadays handled with a different approach:
- String prototype is monkey patched with a function that is only used once
- HTML is constructed in code via string concatenation
- Application is not properly layered due to rendering happening in back-end data fetches
- Hard to tell which functions are managing which area of the screen
- File is very long
- Global namespace is polluted with several variables
Now that we have identified some of the problem spots, how would we fix them?
String prototype is monkey patched with a function that is only used once
Monkey patching causes compatibility issues and thus is not very sensible. Luckily an easy fix is just to use a function that accepts the string as an argument. Another option of course is to use an external library for handling string manipulation, such as Underscore.string.
HTML is constructed in code via string concatenation
Instead of manually constructing HTML like this:
//... html.push("<thead><tr>"); for (var i = 0; i < vars.length; i++){ html.push("<th>" + vars[i] + "</th>"); } html.push("</tr></thead>"); // ..
I would use a templating library such as Handlebars. With Handlebars we would have a template declared in its own file like this:
... <thead> <tr> {{#each vars}} <th>{{this}}<th> {{/each}} </tr> </thead> ...
And the code to populate it with:
var template = Handlebars.compile(tableTemplate); $('#results').html(template({vars: vars}));
Application is not properly layered due to rendering happening in back-end data fetches
Instead of doing DOM manipulation inside an AJAX success callback like this:
success: function(data){ var defaultNamespaces = []; var bindings = data.results.bindings; for (var i = 0; i < bindings.length; i++){ var binding = bindings[i]; namespaces[binding["prefix"].value] = binding["ns"].value; defaultNamespaces.push("PREFIX ", binding["prefix"].value, ": <", binding["ns"].value, "> </br>\n"); } $("#namespaces").html(defaultNamespaces.join("")); init(); }
I would rather just fire an event to inform anyone who is interested that we are done loading. For this we use a common event aggregator called vent, which is just a JavaScript object with Backbone.Events mixed in. With vent in use I would change the above to something like this:
success: function(data) { vent.trigger('query:success', data.results.bindings); }
This enables the success function to only care about retrieving the data. How the data is processed later on is left to the subscriber(s).
Hard to tell which functions are managing which area of the screen
Looking at the code I am having a really hard time understanding which functions are responsible for managing which areas of the screen, they should definitely be somehow bundled. A great way to do this is with Backbone.Views.
I would probably first create a SparqlView which is composed of SearchView (one on the left) and SavedQueriesView. After that I would probably split SearchView into two separate views QueryView and ResultsView. Below is a pseudocode example of the structure:
SparqlView SearchView QueryView ResultsView SavedQueriesView
Backbone.Views would probably be used for smaller elements as well, but these changes alone would give us a better understanding of the structure, plus the benefit of all the goodies Backbone.View provides.
File is very long
This can be of course fixed by splitting the file into separate files and then loading them separately. Luckily for us, we have already split the code into separate Backbone entities so moving them into their own files is really easy:
View | Filename |
---|---|
SparqlView | js/views/sparql.js |
SearchView | js/views/sparql/search.js |
QueryView | js/views/sparql/search/query.js |
ResultsView | js/views/sparql/search/results.js |
SavedQueriesView | js/views/sparql/saved_queries.js |
When the subview has only one parent I find it a good practice to put the subview into a subdirectory with the name of their parent.
Global namespace is polluted with several variables
We have split the code into separate files, but have not really done anything about polluting the global namespace and our HTML file is full of script sources. I would bring in RequireJS to sort out this mess.
With RequireJS we declare only our application entry point in the HTML:
<script data-main="main" src="js/libs/require/require.js"></script>
data-main specifies which file is used as the entry point, in our case it is main.js and its contents is something like the following:
require(['app'], function(App) { App.initialize(); });
require is a function that takes an array as its first argument and a function as its second. The values in the array are RequireJS modules which are fed into the function as arguments. So in this case the argument App is module app.js. Inside the function we call App's method initialize, let's have a look at app.js:
define(['js/libs/jquery/jquery', 'js/views/sparql.js'], function($, SparqlView) { var initialize = function() { new SparqlView({el: $('#sparql')}); } return {initialize: initialize}; });
Here we have defined a RequireJS module that has two dependencies: jQuery and SparqlView and we only expose a hash containing one function, initialize. When called, initialize creates a new SparqlView, SparqlView is defined as a RequireJS module as all of its subviews.
We now hopefully have a clear view on how to take old JavaScript code to the present.