1
\$\begingroup\$

Below are a few of my handlers and helper methods. This works perfectly fine, but I'm pretty sure I'm breaking about 1001 modern conventions and possibly even optimizations. Could you provide some insights as to how I can improve the structure and design of this code?

function getSelectedId() {
    var grid = $("MainGrid").data("kendoGrid");
    var selected = grid.select();
    var data = grid.dataItem(selected);

    return data.Id;
}

function changeGrid(e) {
    var id = getSelectedId();

    if (id == 0) {
        hideSubGrid();
    } else {
        refreshSubGrid();
        showSubGrid();
        setTitle(id);
    }
}

function hideSubGrid() {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.addClass("hidden");
}

function showSubGrid() {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.removeClass("hidden");
}

function refreshSubGrid(e) {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.dataSource.read();
    subGrid.refresh();
}

function setTitle(id) {
    $("#Title").text(id);
}

Some quick searching on the subject, suggests a style similar to what I've tried in the below code. However, I don't think this looks a whole lot better. I guess it's a step in the right direction, where I won't have to repeat myself as much, though.

var gridHandler = {
    mainGrid: $("MainGrid").data("kendoGrid"),
    subGrid: $("SubGrid").data("kendoGrid"),
    title: $("#Title"),

    getSelectedId: function () {
        var selected = this.mainGrid.select();
        var data = this.mainGrid.dataItem(selected);

        return data.Id;
    }

    changeGrid: function (e) {
        var id = this.getSelectedId();

        if (id == 0) {
            this.hideSubGrid();
        } else {
            this.refreshSubGrid();
            this.showSubGrid();
            this.setTitle(id);
        }
    }

    hideSubGrid: function () {
        this.subGrid.addClass("hidden");
    }

    showSubGrid: function () {
        this.subGrid.removeClass("hidden");
    }

    refreshSubGrid: function (e) {
        this.subGrid.dataSource.read();
        this.subGrid.refresh();
    }

    setTitle: function (id) {
        this.title.text(id);
    }
}
\$\endgroup\$
1
\$\begingroup\$

The second solution has two main improvements:

  1. Fewer lookups. Every time you call your original getSelectedId method, the lookup for $("MainGrid").data("kendoGrid"); is performed again. The 2nd version caches this in gridHandler.mainGrid -- only one lookup. The same goes for subGrid.

  2. More compact. For example the subGrid local variable here was not particularly useful:

    function hideSubGrid() {
        var subGrid = $("SubGrid").data("kendoGrid");
    
        subGrid.addClass("hidden");
    }
    

    It would have been simpler and more natural this way:

    function hideSubGrid() {
        $("SubGrid").data("kendoGrid").addClass("hidden");
    }
    
  3. Reduced namespace clutter. By putting the methods in a gridHandler, you don't have many methods lying around in the global namespace, they are all neatly inside gridHandler.

\$\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.