JavaScript code quality

Question

I decided to put together a quick project using jquery, the Kendo UI HTML 5 framework and JavaScript.

I don't do much JavaScript development and I'd like some input on how to make this look less like a complete spaghetti hack, walking up and down DOM elements, trying to find the right event handler to hook onto.

$(window).load(function(){
var serviceUrl = "http://catalogService.asmx";

var configurationsUrl = serviceUrl + "/Configurations?format=json";

var itemAttributesUrl = serviceUrl + "/GetItemAttributes?format=json";

var updateConsumersUrl = serviceUrl + "/UpdateConsumers?format=json"; 

var dataSource = kendo.data.DataSource.create({
    schema: { model: {id: "ServiceEntityName"} },
    transport: {
        read: {
            url: configurationsUrl,
            dataType: "jsonp"            
        }
    },
    pageSize: 15
    });

$("#grid").kendoGrid({
   dataSource: dataSource,
   height: 670,
   scrollable: true,
    pageable: true,
    sortable: true,
    groupable: true,
    detailInit: detailInit,

   dataBound: function(e) { 
       this.element.find(".consumers").each(function() {
           // Get the matching grid data row.
           var id = $(this).closest("tr").data("id");

           var data = dataSource.get(id).data;

           var consumers = data.Consumers;

           var dd = $(this).kendoDropDownList({
               dataSource: consumers,
               template: $("#consumerTemplate").html()
           }).data("kendoDropDownList");

           var lastTarget = document.body;

           updateConsumersText(dd, consumers);

           // keep track of mouse up element to determine if we should close the dropdown
           dd.popup.element.delegate("*", "mouseup", function(e) { lastTarget = e.currentTarget; });

           dd.popup.bind("close", function(e) { 
               if ($.contains(this.element[0], lastTarget)) { 
                   // don't close - we haven't clicked on the dropdown element itself (or outside the dropdown)         
                   this.element.find(".consumerCheckBox").each(function(i, e) {
                       consumers[i].IsSelected = e.checked;
                   });
                   updateConsumersText(dd, consumers);
                   e.preventDefault();
               } 
               else if (data.ServiceEntityName)
               {
                  updateConsumers(data.ServiceEntityName, dd.text());
               }
               lastTarget = document.body;
           });
       });
   },
   columns: [
       { field: "Namespace" },
       { field: "Name" },
       { field: "Description" },
       { 
         name: "Consumers", 
         template: $("#consumersTemplate").html()
       }
   ],

   selectable: true
});  

$.fn.toggleCheckbox = function() {
    this.attr('checked', !this.attr('checked'));
}

String.prototype.endsWith = function(suffix) {
    return this.indexOf(suffix, this.length - suffix.length) !== -1;
};

function updateConsumersText(dropDownList, consumers)
{
    var text= "";
    for (var i = 0; i < consumers.length; i++)
    {
        var c = consumers[i];
        if (c.IsSelected)
        {
            text+= c.Name + ", ";
        }
    }
    if (text.endsWith(", "))
    {
        text = text.substring(0, text.length - 2);
    }

    dropDownList.text(text);
}

function updateConsumers(serviceEntityName, consumersText)
{
    var url = updateConsumersUrl + "&serviceEntityName=" + serviceEntityName + "&consumersText=" + consumersText;

    $.ajax({
        url: url,
        error: function(xhr,s,e) {alert("Error updating consumers: " + e); },
        tyoe: "GET",
        dataType: "JSONP"
    });

}

function detailInit(e) {
    var itemAttributesDataSourceUrl = itemAttributesUrl + "&serviceEntityName=" + e.data.ServiceEntityName;

    var itemAttributesDataSource = kendo.data.DataSource.create({
    schema: { model: {id: "ID"} },
    transport: {
        read: {
            url: itemAttributesDataSourceUrl,
            dataType: "jsonp"            
        }
    }
    });    

    $("<div/>").kendoGrid({
        dataSource: itemAttributesDataSource ,
        scrollable: false,
        sortable: true,
        columns: [ "Name", "DisplayName", "Description" ]
    }).appendTo(e.detailCell);
}

});

Answer 1

Here's my approach:

1. Reduce temporary variables when it makes sense (when they're only used once)
2. Reduce string tokens into reusable functions for url generation
3. Add comments to increase clarity
4. Add whitespace for clarity
5. Reduce the duplicate config data structures used by kendo
6. Declare variables as late as possible in the source so they are more closely associated with all of their usage (for instance, so you don't have to jump from top to bottom of the page to figure out what initDetail does)
7. Refactor logic when better approaches are available (mostly the string concatenation/endsWith stuff)

I also fixed some bugs hidden in the code - always use encodeURIComponent for url parameter data unless the values are constants and do not contain characters that need uri escaping. There was a typo or two in the $.ajax hash keys.

The code didn't reduce too much, though mostly because I'm not very familiar with kendo and don't know what parameters are defaulted to in the config groups (hopefully they contain common use case defaults.) Also, you have a lot of specific behavior mixed inside your data population code. That could probably be isolated across all behaviors in the site, reducing this data population snippet even further. I've added comments for these as TODO:'s in the code.

Since this didn't compile before, and since I don't have the same kendo environment up, I hope it will compile now. I ran it through JS Lint but it complained about a variable not being used when it actually was. The code was reduced to about 120 lines of code (minus comments) so about a 15% reduction in entropic loc's. And imo the clarity and conciseness was increased in the process so overall a net win.

$(function() {

    //generate target url for catalog service
    var target = function(page, params) {
        //reduce number of string tokens
        return "http://catalogService.asmx/" + page + "?format=json" + params;
    };

    //generate kendoConfig param structure     
    var kendoConfig = function(model, url, params, pageSize) {
        //NOTE: kendo is a big part of your spaghetti code problem
        //      this data structure is unecessarily complex...
        return {
            schema : {
                model : {
                    id : model
                }
            },
            transport : {
                read : {
                    url : target(url, params),
                    dataType : 'jsonp'
                }
            },
            pageSize : pageSize //assuming kendo can handle undefined parameters
        };
    };

    var updateConsumersText = function (dropDownList, consumers) {
        //TODO: consider using a filter iteratator
        // dropDownList.text(consumers.filter(function(c) { if (c.IsSelected) return c.Name; }).join(", "));
        var selected = [];
        for (var i = 0; i < consumers.length; i++) { 
            var c = consumers[i];
            if (c.IsSelected) selected.push(c.Name);
        }
        dropDownList.text(selected.join(", ")); 
    };

    var configs = kendo.data.DataSource.create(kendoConfig('ServiceEntityName', 'Configurations', "", 15);

    $('#grid').kendoGrid({
        dataSource : configs,
        height : 670,
        scrollable : true, //TODO: if these equal the kendo defaults remove them
        pageable : true,   //ditto
        sortable : true,   //ditto
        groupable : true,  //ditto
        selectable: true,  //ditto
        detailInit : function(e) {
            //Generate the Item Detail Grid
            $('<div/>').kendoGrid({
                dataSource : kendo.data.DataSource.create(
                    kendoConfig(
                        'ID',
                        'GetItemAttributes',
                        '&serviceEntityName=' + encodeURIComponent(e.data.ServiceEntityName)
                    )
                ),
                scrollable : false,
                sortable : true,
                columns : ['Name', 'DisplayName', 'Description']
            }).appendTo(e.detailCell);
        },
        dataBound : function(e) {
            //Implement Main Consumer Binding
            this.element.find(".consumers").each(function() {
                // Get the matching grid data row. 
                var id = $(this).closest("tr").data("id"); //this is not really that bad
                var data = configs.get(id).data; 
                var consumers = data.Consumers; 

                //Update drop down with list of consumers 
                var dd = $(this).kendoDropDownList({ 
                    dataSource: consumers, 
                    template: $("#consumerTemplate").html() 
                }).data("kendoDropDownList");

                updateConsumersText(dd, consumers); 

                // keep track of mouse up element to determine if we should close the dropdown
                //TODO: this could be abstracted into an evented behavior for any dropdown (the behavior of
                //      selecting and closing dropdown lists)
                var lastTarget = document.body;

                dd.popup.element.delegate("*", "mouseup", function(e) { lastTarget = e.currentTarget; }); 

                dd.popup.bind("close", function(e) {  
                    if ($.contains(this.element[0], lastTarget)) {  
                        // don't close - we haven't clicked on the dropdown element itself (or outside the dropdown)          
                        this.element.find(".consumerCheckBox").each(function(i, e) { 
                            consumers[i].IsSelected = e.checked; 
                        }); 
                        updateConsumersText(dd, consumers); 
                        e.preventDefault(); 
                    }  
                    else if (data.ServiceEntityName) {
                        //Update service entity name on the server

                        //TODO: consider using a toParamString() function on a json object instead of manually
                        //      compiling the parameters
                        var url = target(
                            'UpdateConsumers',
                            '&serviceEntityName=' + encodeURIComponent(data.ServiceEntityName) +
                            '&consumersText=' + encodeURIComponent(dd.text())
                        );

                        $.ajax({ 
                            url: url, 
                            error: function(xhr,s,e) { alert("Error updating consumers: " + e); }, 
                            type: "GET", 
                            dataType: "JSONP" 
                        });                             
                    }

                    lastTarget = document.body; 
                });
            });
        },
        columns: [
            { field: "Namespace" }, 
            { field: "Name" }, 
            { field: "Description" }, 
            {  
              name: "Consumers",  
              template: $("#consumersTemplate").html() 
            } 
        ] 
    });
});