I have received a project that is built on Cordova with JQueryMobile. The app is already published on three platforms and is being used by 100K+ users. Now I have to add new functionality in it. But when I checked the code, it looks less organized; poorly written; lacks code maintainability; has unnecessary code repetition; over initiation of vars or function calls.
Index.html
<body onload="onLoad();">
<!-- There are more than 30 Pages in body and each page is fragmented in the below format while its contents are dynamically embedded at the time of that page display-->
<div id="barCodeScanResultsPage" data-role="page">
<div data-tap-toggle="false" data-position="fixed" data-role="header" class="ui-header ui-bar-a ui-header-fixed slidedown ui-panel-content-fixed-toolbar ui-panel-animate ui-panel-content-fixed-toolbar-closed" role="banner">
<a class="smallIcon ui-btn-left" id="iconLoggedOut" data-transition="slide" data-role="none" href="#loginPanel"></a>
<a class="smallIcon ui-btn-right" id="iconLoggedIn" data-transition="slide" data-role="none" href="#loginPanel"></a>
<a class="smallIcon softBackButton" data-direction="reverse" id="iconBack" data-transition="slide" data-role="none" href="#"></a>
<a class="smallIcon" id="iconSettings" data-role="none" href="#"></a>
<a class="smallIcon rightMenu" id="iconMenu" data-transition="slide" data-role="none" href="#menuPanel"></a>
<h1 aria-level="1" role="heading" class="ui-title"><span data-translate="_appTitle">PAGE TITLE</span></h1>
</div>
<div data-role="panel" id="menuPanel" data-position="right" data-position-fixed="true" data-display="overlay" class="right page3Menu">
<a href="#"><div class="menuPanelIcons home"><span data-translate="_home">Home</span></div></a>
<a href="#"><div class="menuPanelIcons service"><span data-translate="_servicesCatalog">Services Catalog</span></div></a>
<a href="#"><div class="menuPanelIcons tracking"><span data-translate="_tracking">Tracking</span></div></a>
<a href="#"><div class="menuPanelIcons notification"><span data-translate="_notification">Notification</span></div></a>
<a href="#"><div class="menuPanelIcons land"><span data-translate="_myLandmyChoice">Land Reservation</span></div></a>
</div>
<!-- below Div will be used as placeholder to load dynamic contents inside this page -->
<div id="barCodeScanRessultsContentDiv"></div>
</div>
</body>
There is an "includes" folder that contains a separate .html file holding each page's content area (segment) that is defined in the index.html.
For example: below is the contents segment of above page ("includes/barCodeScanResults.html") . This HTML markup will be loaded dynamically.
<div class="ui-panel-content-wrap ui-body-c ui-panel-animate ui-panel-content-wrap-closed">
<div id="content3" data-role="content" class="ui-content" role="main">
<div class="pageHeader">
<div class="pageIcon BarCodeIcon"></div>
<div data-translate="_temp_BarcodeScanner" class="pageTitle">Barcode Scanner</div>
</div>
<div class="qr_result">
<div class="rslt_qr">
<div class="res_img"><img src="images/phone/1x/qr.png"></div>
<div class="qr_det">
<p data-translate="_temp_FormatQR">Format: QR_CODE</p>
<p data-translate="_temp_TypeGeo">Type: Geo</p>
<p data-translate="_temp_Metadata_US">Metadata: US/CA</p>
</div>
<p class="qr_subheading" data-translate="_temp_Details">Details</p>
<div class="qrDetails">
<h4 data-translate="_temp_GoogleMaps">Google Maps</h4>
<p>https://www.google.com/maps/place/a+location+map+with+some+details</p>
<div class="btns">
<a href="#" class="submit_btn" data-translate="_temp_Rescan">Rescan</a>
<a href="#" class="submit_btn" data-translate="_temp_Continue">Continue</a>
</div>
</div>
</div>
</div>
</div>
</div>
main.js
$(document).ready(function() {
jQuery.support.cors = true;
});
function onLoad() {
document.addEventListener('deviceready', onDeviceReady, false);
startTimer();
}
function onDeviceReady() {
document.addEventListener("backbutton", onBackKeyDown, false);
navigator.splashscreen.hide();
}
$(document).on("click", ".softBackButton", function(event) {
var currentPage=$.mobile.activePage.attr('id');
if( currentPage == 'page3' ){
$.mobile.changePage("#page2", { transition: "slide", reverse: true });
}
else if( currentPage == 'page4' ){
$.mobile.changePage("#page3", { transition: "slide", reverse: true });
}
// and some one ....
}
// --- each button , menu , list etc. all are bind with events like this:
$(document).on("click", "#btnLogin", function(event) {
sessionStorage.setItem("LoginPanelClicked", "1");
ulength=$("#txtUser").val().length;
plength=$("#txtPwd").val().length;
var unametemp,upwdtemp;
unametemp=$("#txtUser").val();
upwdtemp=$("#txtPwd").val();
$("#txtUser").val("");
$("#txtPwd").val("");
if((ulength<=0||ulength>25)||(plength<=0||plength>25)){
$(".errorText").text(inputLengthError);
$("#authError").popup('open');
}
else{
Authenticate(unametemp, upwdtemp);
}
});
$(document).on("click", "#btnLogout", function(event) {
sessionStorage.clear();
sessionStorage.setItem('userLoginStatus', '-1');
$('#logout').hide();
$('#signup').show();
$('#txtUser').attr("placeholder", userName);
$('#txtPwd').attr("placeholder", password);
$('#login').show();
$('#iconLoggedOut').show();
$('#iconLoggedIn').hide();
$('#iconBack').hide();
$('.redBubble').css('visibility','hidden'); //at few places css is updated in this way
$('.iconLoginStatus').css("background","url('images/phone/1x/iconuserstatusout.png') 50% 39% no-repeat #108AB1");
});
<!-- Below Js code will embed the dynamic contents inside the html page (the place holder Div) -->
$(document).on("pageinit", "#barCodeScanResultsPage", function (event) {
lang = window.localStorage.getItem('language');
$("#barCodeScanRessultsContentDiv").load("includes/barCodeScanResults.html", function (event) {
$(".menuPanel").load('includes/menuPanel.html', function () {
$(this).trigger("create");
trans(lang);
$(".menuPanelIcons").css('height', (windowHeight / 5) - 5);
$(".menuPanelIcons span").css('top', ((((windowHeight / 5) - 5) / 2) + 30));
});
$(document).on("swipeleft swiperight", $(this), function (e) {
if ($.mobile.activePage.jqmData("panel") !== "open") {
if (e.type === "swipeleft") {
$(".menuPanel").panel("open");
} else if (e.type === "swiperight") {
$(".menuPanel").panel("close");
}
}
});
// translate the page's text/labels/contents
trans(lang);
});
});
This code has many redundant calls to the trans(lang) function which is simply recreating all dictionaries again and again. It should do one time only. This is one of the improvement area.
trans(lang) is used to translate all contents of a page into the selected language preference.
function trans(lang) {
"use strict";
var dictionary, set_lang;
// Object literal behaving as multi-dictionary
dictionary = {
// each language has many words for translation
"english": {
"_appTitle": "PROJECT TITLE"
} ,
"arabic" : {
"_appTitle": "PROJECT TITLE"
},
"french": {
"_appTitle": "PROJECT TITLE"
},
"chiness": {
"_appTitle": "PROJECT TITLE"
},
"german": {
"_appTitle": "PROJECT TITLE"
}
"urdu": {
"_appTitle": "PROJECT TITLE"
}
}
set_lang = function (dictionary) {
$("[data-translate]").html(function () {
var key = $(this).data("translate");
console.log(key);
if (dictionary.hasOwnProperty(key)) {
return dictionary[key];
}
});
};
// Set initial language to English
if(lang == "EN"){
set_lang(dictionary.english);
}else{
set_lang(dictionary.arabic);
} // .... and so on for other lang checks
}
So all the elements on html pages contains (data-translate="translateMe") property and above JS language translate function will translate all those elements.
main.js contains followings functions for all pages in order to call page specific services via Ajax.
//Now on Pageshow event it calls few services
$(document).on( "pageshow", "#barCodeScanResultsPage", function( event ) {
CallAService( param1, param2 ); //this is a sample service name
});
Services.js
var NewsServiceAPI = (function() {
var _success = 0;
var _returnedXML = '';
return {
CallAService: function() {
var requestMode = 'SERVICENAMEGOESHERE';
var dataString = 'mode='+requestMode+'&vu='+ApplicationConfig.getVN()+'&vp='+ApplicationConfig.getVP();
$.ajax({
type: "POST",
url: ApplicationConfig.getVendorURL(),
dataType: "text",
data: dataString,
processData: false,
crossDomain: true,
success: this.OnNewsServiceSuccess,
error: this.OnNewsServiceError,
complete: this.OnNewsServiceComplete
});
return false;
},
OnNewsServiceSuccess: function(data, status,jqxhrobject) {
var xml = data.toString(),
xmlDoc = $.parseXML( xml ),
$xml = $( xmlDoc );
$objXML = $xml.find('ERROR');
if ($objXML.length == 0) {
this._success = 1;
this._returnedXML = data.toString();
} else {
this._success = 0;
this._returnedXML = data.toString();
var errorDesc = getErrorDetails($objXML);
}
},
OnNewsServiceError: function(request, status, error) {
//$(".errorText").text(_error + error);
//$("#page4Error").popup('open');
},
OnNewsServiceComplete: function(jqXHRObject,textStatus) {
if (this._success == 1) {
//...
} else {
//...
}
}
};
})();
Now kindly help review:
- Organize and improve it for the future
- Remove duplication or reduce redundant calls
- Avoid unnecessary creating of functions, vars or markup
- Better re-factor the code structure ( or architecture)
Improvement Suggestions:
- As
trans(lang)is multiple times called in every function so it should be called once throughout the app until the user selects another language.
Few problems:
- Pages run a bit slow on device
- Transition is bad on a few pages (jerking), especially pages with listview
dictionarybeing generated over and over I think this looks actually pretty solid. A bit too much jQuery for my taste. I would just add the new functionality and not re-factor the whole thing. – konijn Oct 17 '14 at 12:22