2
\$\begingroup\$

I have been working on a javascript heavy project. There are sales, purchases, reports etc. What I've done is created a separate module for each i.e. a separate module for each i.e. a separate one for sale, one for purchase and so on. I am posting one of my modules, all the others have same structure. Can anyone please review it. Is there anything wrong with it or is everything okay. Any changes that I may want to consider to make it more better i.e. mantainable:

var navigation = {

init : function (){
    navigation.bindUI();

    $('.ReportViews').addClass('active');
    $('.stockNavReports').addClass('active');
},

bindUI : function () {
    $("#btnReset").on('click', function (){
        navigaiton.resetReport();
    });

    $("#btnShow").on('click', function (){

        var fromEl = $('#txtStart');
        var toEl = $('#txtEnd');
        var groupBy = navigation.getGroupingCriteria();         

        if (navigation.validDateRange(fromEl.val(), toEl.val())) {
            navigation.fetchReportData(fromEl.val(), toEl.val(), groupBy);
        }
        else{
            fromEl.addClass('input-error');
            toEl.addClass('input-error');
        }

    });
},

getGroupingCriteria : function () {
    return $('input[name="grouping"]:checked').val().toLowerCase();
},

fetchReportData : function (startDate, endDate, groupBy) {

    if (typeof navigation.dTable != 'undefined') {
        navigation.dTable.fnDestroy();
        $('#navigationRows').empty();
    }       

    $.ajax({
            url: base_url + "index.php/report/getNavigationReportData",                
            data: { startDate : startDate, endDate : endDate, groupBy : groupBy },
            type: 'POST',
            dataType: 'JSON',
            beforeSend: function () {
                // console.log(this.data);
             },
            complete: function () { },
            success: function (result) {

                if (result.length !== 0) {
                    $("#datatable_StockNavigationRows").fadeIn();


                    var prevDate = "";
                    var prevVrnoa = "";
                    var prevGodown_1 = "";
                    var prevGodown_2 = "";

                    if (result.length != 0) {

                        var navigationRows = $("#navigationRows");

                        $.each(result, function (index, elem) {

                            var obj = { };

                            obj.SERIAL = navigationRows.find('tr').length+1;
                            obj.VRNOA = elem.VRNOA;
                            obj.REMARKS = (elem.REMARKS) ? elem.REMARKS : "Not Available";
                            obj.QTY = (elem.QTY) ? elem.QTY : "Not Available";
                            obj.GODOWN_1 = (elem.GODOWN_1) ? elem.GODOWN_1 : "Not Available";
                            obj.GODOWN_2 = (elem.GODOWN_2) ? elem.GODOWN_2 : "Not Available";
                            obj.VRDATE = (elem.VRDATE) ? elem.VRDATE.substring(0,10) : "Not Available";

                            if (groupBy === 'date') {

                                if (prevDate != obj.VRDATE) {

                                    // Create the heading for this new voucher. 
                                    var source   = $("#report-dhead-template").html();
                                    var template = Handlebars.compile(source);
                                    var html = template(obj);

                                    navigationRows.append(html);

                                    // Reset the previous voucher to current voucher.
                                    prevDate = obj.VRDATE;
                                }
                            }
                            else if (groupBy === 'vrnoa') {

                                if (prevVrnoa != obj.VRNOA) {

                                    // Create the heading for this new voucher. 
                                    var source   = $("#report-vhead-template").html();
                                    var template = Handlebars.compile(source);
                                    var html = template(obj);

                                    navigationRows.append(html);

                                    // Reset the previous voucher to current voucher.
                                    prevVrnoa = obj.VRNOA;
                                }
                            }

                            else if (groupBy === 'godown_1') {

                                if (prevGodown_1 != obj.GODOWN_1) {

                                    // Create the heading for this new voucher. 
                                    var source   = $("#report-g1head-template").html();
                                    var template = Handlebars.compile(source);
                                    var html = template(obj);

                                    navigationRows.append(html);

                                    // Reset the previous voucher to current voucher.
                                    prevGodown_1 = obj.GODOWN_1;
                                }
                            }

                            else if (groupBy === 'godown_2') {

                                if (prevGodown_2 != obj.GODOWN_2) {

                                    // Create the heading for this new voucher. 
                                    var source   = $("#report-g2head-template").html();
                                    var template = Handlebars.compile(source);
                                    var html = template(obj);

                                    navigationRows.append(html);

                                    // Reset the previous voucher to current voucher.
                                    prevGodown_2 = obj.GODOWN_2;
                                }
                            }

                            // Add the item of the new voucher
                            var source   = $("#report-item-template").html();
                            var template = Handlebars.compile(source);
                            var html = template(obj);

                            navigationRows.append(html);
                        });
                    }
                }

                navigation.bindGrid();
            },

            error: function (result) {
                //$("*").css("cursor", "auto");
                $("#loading").hide();
                alert("Error:" + result);
            }
        });     

},

bindGrid : function() {
    // $("input[type=checkbox], input:radio, input:file").uniform();
    var dontSort = [];
    $('#datatable_StockNavigationRows thead th').each(function () {
        if ($(this).hasClass('no_sort')) {
            dontSort.push({ "bSortable": false });
        } else {
            dontSort.push(null);
        }
    });
    navigation.dTable = $('#datatable_StockNavigationRows').dataTable({
        // Uncomment, if datatable not working
        // "sDom": "<'row-fluid table_top_bar'<'span12'<'to_hide_phone' f>>>t<'row-fluid control-group full top' <'span4 to_hide_tablet'l><'span8 pagination'p>>",
        "sDom": "<'row-fluid table_top_bar'<'span12'<'to_hide_phone'<'row-fluid'<'span8' f>>>'<'pag_top' p> T>>t<'row-fluid control-group full top' <'span4 to_hide_tablet'l><'span8 pagination'p>>",
        "aaSorting": [[0, "asc"]],
        "bPaginate": true,
        "sPaginationType": "full_numbers",
        "bJQueryUI": false,
        "aoColumns": dontSort,
        "bSort": false,
        "iDisplayLength" : 100,
        "oTableTools": {
            "sSwfPath": "js/copy_cvs_xls_pdf.swf",
            "aButtons": [{ "sExtends": "print", "sButtonText": "Print Report", "sMessage" : "Stock Navigation Report" }]
        }
    });
    $.extend($.fn.dataTableExt.oStdClasses, {
        "s`": "dataTables_wrapper form-inline"
    });
},

validDateRange  : function (from, to){
    if(Date.parse(from) > Date.parse(to)){
       return false
    }
    else{
       return true;
    }
},  

resetReport : function (){
    $(".printBtn")
    $("#datatable_navigationRows").hide();
}
}
navigation.init();
\$\endgroup\$
  • \$\begingroup\$ Do you intend for each module to use it's own global namespace object, rather than just have one master namespace object that each module could plug its own object into? \$\endgroup\$ – jfriend00 Dec 22 '13 at 9:29
  • \$\begingroup\$ I have placed each of the object in a separate file and each page that intends to use that object, loads the file and uses it. \$\endgroup\$ – Kamran Ahmed Dec 22 '13 at 9:34
  • 2
    \$\begingroup\$ Your $.ajax success callback is way to big and cries out for refactoring. \$\endgroup\$ – meagar Dec 22 '13 at 13:57
2
\$\begingroup\$

In short,

your code looks good until $.ajax where far to many things happen, more specifically too many things happen in success.

The funny thing there is that it would look already a lot better if you took a generic approach to deal with the grouping.

Maybe you can have a groupingConfig object :

var groupingConfigs = 
{
  date     : { template : '#report-dhead-template'  , fieldName : 'VRDATE'   },
  vrnoa    : { template : '#report-vhead-template'  , fieldName : 'VRNOA'    },
  godown_1 : { template : '#report-g1head-template' , fieldName : 'GODOWN_1' },
  godown_2 : { template : '#report-g1head-template' , fieldName : 'GODOWN_1' }
}

then, before the $.each( you can get the groupingConfig

var groupingConfig = groupingConfigs[groupBy];

and within the $.each( you can simply

if (groupingConfig)
{
  if (previousValue != obj[groupingConfig.fieldName])
  {
    // Create the heading for this new group. 
    var source   = $( groupingConfig.template ).html();
    var template = Handlebars.compile(source);
    var html = template(obj);

    navigationRows.append(html);
    //Reset the previous group value
    previousValue = obj[groupingConfig.fieldName];
  }
}

Also, the below creation of the dataTable looks all kind of wrong:

navigation.dTable = $('#datatable_StockNavigationRows').dataTable({
    // Uncomment, if datatable not working
    // "sDom": "<'row-fluid table_top_bar'<'span12'<'to_hide_phone' f>>>t<'row-fluid control-group full top' <'span4 to_hide_tablet'l><'span8 pagination'p>>",
    "sDom": "<'row-fluid table_top_bar'<'span12'<'to_hide_phone'<'row-fluid'<'span8' f>>>'<'pag_top' p> T>>t<'row-fluid control-group full top' <'span4 to_hide_tablet'l><'span8 pagination'p>>",
    "aaSorting": [[0, "asc"]],
    "bPaginate": true,
    "sPaginationType": "full_numbers",
    "bJQueryUI": false,
    "aoColumns": dontSort,
    "bSort": false,
    "iDisplayLength" : 100,
    "oTableTools": {
        "sSwfPath": "js/copy_cvs_xls_pdf.swf",
        "aButtons": [{ "sExtends": "print", "sButtonText": "Print Report", "sMessage" : "Stock Navigation Report" }]
    }
});
  • Hungarian notation, don't do that
  • sDom is a super long string without any comment
  • the commented sDom has a superfluous comment
  • bJQueryUI ?

Finally,

validDateRange  : function (from, to){
    if(Date.parse(from) > Date.parse(to)){
       return false
    }
    else{
       return true;
    }
},  

should really be

validDateRange  : function (from, to)
{
  return !(Date.parse(from) > Date.parse(to));
}, 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.