Improving PhoneGap/JavaScript application

Question

I have a PhoneGap application that I wrote some time ago. After looking Doug Crockford's video seminar JavaScript: The Good Parts.

I was just wondering if the code could be improved for better maintainability and readability as it now could be hard for others to understand what's happening. Maybe taking advantage of using the module pattern and closures?

I know there's a big chunk of code in this post, so any help on implementing these patterns is well appreciated.

I'm pretty much following Rohit Ghatol's book Beginning PhoneGap code examples and design. That is, my app has the following execution order:

document.addEventListener('deviceready', onDeviceReady, false);
function onDeviceReady() {
 $(document).ready(function() {
 // Initiate the application
 bind();
 });
}
function bind() {
 initiateDatabases();
 initiateDashboardPage();
 initiateReserveBtn();
 initiateReservePage();
 initiateCentersBtn();
 initiateCentersPage();
 initiateDonationsBtn();
 initiateDonationsPage();
 initiateSettingsBtn();
 initiateSettingsPage();
 initiateQuestionsBtn();
 initiateQuestionsPage();
 initiateInformationBtn();
 initiateInformationPage();
 initiateMapPage();
}

In each of those initiateX() function I bind different jQuery Mobile page and button handling events, which will then call other functions. For example, initiateReservePage() looks like this:

function initiateReservePage() { 
 // When reserve page is shown
 $('#reserve-page').live('pageshow', function () {
 populateReserveList(); 
 });
 var did_user_swipe = false;
 // Bind mousedown/mouseup/mousemove events to 'refresh reserve' button
 $('#refreshReserveBtn').bind('vmousedown', function () {
 did_user_swipe = false;
 }).bind('vmousemove', function() {
 did_user_swipe = true;
 }).bind('vmouseup', function (e) {
 if(!did_user_swipe && e.which === 0) { 
 // DOWNLOAD THE RESERVE 
 downloadReserve();
 }
 });
 did_user_swipe = null;
}

As it could be seen, that function either call downloadReserve() or populateReserve() function depending on the event being fired.

Could some sort of pattern be used here to combine those 2 functions?

Here are those 2 functions in question:

function downloadReserve() { 
 // URL to download reserve from
 var reserveURL = 'http://myremotesite.com/file.xml';
 try { 
 $.ajax({
 type: 'GET',
 url: reserveURL,
 dataType: "xml",
 contentType: "text/xml; charset=utf-8",
 beforeSend: function (XMLHttpRequest) {
 $.mobile.loadingMessage = 'Loading...';
 $.mobile.showPageLoadingMsg();
 },
 complete: function(jqXHR) {
 $.mobile.hidePageLoadingMsg();
 },
 success: function(data, textStatus, jqXHR) {
 // This is a global variable to cache the fetched data
 cachedReserve = [];
 // Reserve descriptions for different states
 var reserveDescription = ['Critical', 'Adequate', 'Normal', 'Good'],
 // Reserve item from XML
 $item,
 // Group for item
 group = '',
 // State for item
 state = '',
 // Description for item
 desc = '',
 // Reserve object for item
 tmpReserveObj = {};
 $(data).find('item').each(function() {
 $item = $(this);
 group = $item.find('group').text();
 state = $item.find('state').text();
 if (state == 'A') {
 desc = reserveDescription[0];
 } else if (state == 'B') {
 desc = reserveDescription[1];
 } else if (state == 'C') {
 desc = reserveDescription[2];
 } else if (state == 'D') {
 desc = reserveDescription[3];
 }
 // Create a temporary reserve object
 tmpReserveObj = { 'group':group, 'state':state, 'desc':desc };
 // Push temporary object into global cached array
 cachedReserve.push(tmpReserveObj);
 });
 $item = null;
 group = null;
 state = null;
 desc = null;
 tmpReserveObj = null;
 reserveDescription = null;
 // Insert reserve array into local storage and populate list
 insertReserve(cachedReserve);
 },
 error: function (xhr, ajaxOptions, thrownError){ 
 // Show an alert here
 }
 }); 
 } catch (e) {
 console.log('Error while performing ajax get call to download the reserve ' + e);
 }
 reserveURL = null;
}

...and...

function populateReserveList() {
 try {
 // Get the reserve from local storage
 var reserve = JSON.parse(window.localStorage.getItem('reserve'));
 } catch (e) {
 alert('error getting the reserve from local storage: ' + e);
 return false;
 }
 if (reserve !== null && reserve.length > 0) {
 // If more than a week old
 if (isReserveDataOld()) {
 $.mobile.hidePageLoadingMsg();
 // Show alert box to ask user whether to download reserve again
 navigator.notification.confirm(
 'Your downloaded reserve seems to be old. Do you want to download it again?',
 function(buttonIndex) {
 // If 'OK' button clicked
 if (buttonIndex === 2) {
 // DOWNLOAD THE RESERVE AGAIN
 downloadReserve();
 }
 },
 'Download the reserve',
 'Cancel,OK');
 } else {
 // Reserve list container
 var list = document.getElementById('reserveList'),
 // Timestamp for when reserve was inserted into local storage
 timestamp = '',
 // Date based on timestamp
 _date;
 // Empty the reserve list container and remove all event handlers from child elements
 $(list).empty();
 try {
 timestamp = window.localStorage.getItem('reserveInserted');
 timestamp = ((timestamp != null && timestamp != 'undefined' && timestamp.length > 0) ? timestamp : 0);
 _date = new Date(timestamp - 0);
 } catch (e) {
 alert('Error while getting reserveInserted from local storage ' + e);
 }
 var infoBarText = '<h3>Downloaded' +
 '<span>' +_date.getDate() + '. ' + monthNames[_date.getMonth()] + 'ta ' + _date.getFullYear()+ '</span></h3>';
 var infoBarElem = document.createElement('div');
 infoBarElem.setAttribute('id', 'infoBar');
 infoBarElem.className = 'downloaded';
 infoBarElem.innerHTML = infoBarText;
 list.appendChild(infoBarElem);
 infoBar = null;
 infoBarText = null;
 var reserveDetailsContainer = document.createElement('div');
 var reserveListContainer = document.createElement('div');
 var reserveHeadingElem = document.createElement('h3');
 reserveDetailsContainer.className = 'detailsContainer';
 reserveListContainer.className = 'listContainer';
 list.appendChild(reserveDetailsContainer);
 list.appendChild(reserveListContainer);
 // Global variable to cache reserve data
 cachedReserveData = [];
 var htmlData = '';
 var reserveHeadingContainer;
 var did_user_swipe = false;
 // Iterate through all reserve items
 for ( var index = 0 ; index < reserve.length ; index++ ) {
 var item = reserve[index];
 cachedReserveData.push(item);
 reserveHeadingContainer = reserveHeadingElem.cloneNode(false);
 reserveHeadingContainer.setAttribute('id', index);
 reserveHeadingContainer.setAttribute('name', item.group.toLowerCase());
 htmlData = 'Group: ' + item.group + '<span>Show group</span>';
 reserveHeadingContainer.innerHTML = htmlData;
 reserveHeadingContainer.className += ' state_' + item.state;
 reserveListContainer.appendChild(reserveHeadingContainer);
 $(reserveHeadingContainer).bind('vmousedown', function(event) {
 did_user_swipe = false; 
 }).bind('vmousemove', function () {
 did_user_swipe = true;
 }).bind('vmouseup', function(e) {
 // If triggered manually or by user
 if (passDataBetweenPages == 'trigger' || (!did_user_swipe && e.which == 0)) {
 try { 
 var id = parseInt($(this).attr('id'));
 // Empty the reserve details container and remove all event handlers from child elements
 $(reserveDetailsContainer).empty(); 
 htmlData = '<div>' +
 '<p>' +cachedReserveData[id].desc+ '</p>' +
 '</div>';
 // Replace contentContainer's content with clicked list items's data
 reserveDetailsContainer.innerHTML = htmlData;
 var newHeading = $(this).clone();
 newHeading.find('span').html('State');
 $(reserveDetailsContainer).prepend(newHeading);
 $(reserveListContainer).find('h3.active').removeClass('active');
 $(this).addClass('active');
 newHeading = null;
 // Scroll to top
 scroll(0, 0);
 } catch (e) {
 alert('Error while clicking listItem with id = ' + $(this).attr('id') + ' ' + e);
 }
 }
 });
 }
 // Get group from local storage
 var group = getItemFromLocalStorage('group');
 // Open corresponding group or if not found, the first item on the list
 if (group && group != '0') {
 var listItem = $(reserveListContainer).find('h3[name="' +group+ '"]').get(0);
 passDataBetweenPages = 'trigger';
 $(listItem).trigger('vmouseup');
 passDataBetweenPages = null;
 listItem = null;
 } else {
 var listItem = $(reserveListContainer).find('h3').get(0);
 passDataBetweenPages = 'trigger';
 $(listItem).trigger('vmouseup');
 passDataBetweenPages = null;
 listItem = null;
 }
 group = null;
 }
 } else {
 $.mobile.hidePageLoadingMsg();
 // Show alert box to ask user whether to download the reserve
 navigator.notification.confirm(
 'The reserve has not been downloaded yet. Do you want to download it now?',
 function(buttonIndex) {
 console.log(buttonIndex);
 if (buttonIndex === 2) {
 // DOWNLOAD THE RESERVE
 downloadReserve();
 }
 },
 'Download the reserve',
 'Cancel,OK');
 }
}

Answer 1

I think your code looks fine (except for populateReserveList).

• I would look into replacing .live() calls with .on() calls as .live() has been deprecated since a while.
• Your success function is so large, it ought to be a function on it's own

• You could put the state -> description mapping in an object

  var stateDescriptionMap = { 'A' : 0 , 'B' : 1 , 'C' : 2 , 'D' : 3 }
  

  and then

  desc = reserveDescription[ stateDescriptionMap[ state ] ];
  

• I am not sure what you want to accomplish with ( your code should work without this? )

   $item = null;
   group = null;
   state = null;
   desc = null;
   tmpReserveObj = null;
   reserveDescription = null;
  

• populateReserveList is far too large and has to be broken up in logical parts.