Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Not some much a problem, I am curious to know if my code can be cleaned up here a little. One annoyance is im calling a function three ( 3 ) times. I would like to know how this could be improved, ie: less calls to the function.

loadComments is the function being called in this particular object, however other object functions are similar.

For instance when loadlist is called it will run the function initially, then call it again at set interval, how can I improve this?

Regards, Phil

(function($){
    /**
     * =============================================================================================
     * Load pagination li's into a scope(table id)
     * =============================================================================================
     * 
     */
    var loadPagination = function(scope, pag){
        var pagination = $(scope).find("ul.pagination");
            pagination.empty();
            pagination.append(pag);
    }
    /**
     * =============================================================================================
     * Load table body rows into a scope(table id)
     * =============================================================================================
     * 
     */
    var loadTableBody = function(scope, data){
        var tblBody = $(scope).find("tbody");
            tblBody.empty();
            tblBody.append(data);
    }
    /**
     * =============================================================================================
     * load ajax loading icon
     * =============================================================================================
     * 
     */
    var loadLoader = function(scope){
        var loadingSpan = $(scope).find("tbody"),
            loadingIcon = $("<span class='loader'>&nbsp;</span>"),
            span = $("<td style='text-align:center' colspan='8'>");

            loadingSpan.empty();
            span.append(loadingIcon).appendTo(loadingSpan);
    }
     /**
     * =============================================================================================
     * load the list function at set interval
     * =============================================================================================
     * 
     */
    var loadList = function(func, interv){
        func();
        window.setInterval(function(){
            func();
        }, interv);
    }

    /**
     * =============================================================================================
     * Comments Object
     * =============================================================================================
     * 
     */
    var comments = {

        init : function(){
            if(document.getElementById("comments")){
                this.loadCommentsList();
            }
        },
        loadCommentsList : function(){

            var scope = $("table#comments"),
                tmplId = $("#comments-list"),
                loadComments = function(offset){
                    var data = {'offset' : (offset) ? offset : 0}
                    $.ajax({
                        url      : BASE_PATH + 'admin/comments/get',
                        type     : 'POST',
                        data     : data,
                        dataType : 'json',
                        beforeSend : function(){
                              loadLoader(scope);
                        },
                        success  : function(cb){

                            window.setTimeout(function(){


                                loadTableBody(scope, tmplId.tmpl(cb.comments));

                                loadPagination(scope, cb.pagination);


                            }, 300);
                        }
                    });
                }

            //load initial comments list ( 1 ) AND call at interval ( 2 ) 
            loadList(loadComments, 60000);


            //listen for pagination click events and make ajax request ( 3 )
            //TODO : remove function from pages object
            pages.PaginationListner(loadComments, scope);


        }
    }



    /**
     * =============================================================================================
     * Call each object's init function
     * =============================================================================================
     * 
     */
    $(function(){
        comments.init();
    });

})(jQuery);
share|improve this question

2 Answers

up vote 1 down vote accepted

I was intrigued by your problem. And in an exercise of coding, I probably went a bit overboard. I am sorry if I've misstepped or if I'm not answering your question specific enough. But thank you none-the-less for the challenge.

In looking at your code, a few things stick out.

  1. scope is being passed or set in many of the functions.
  2. the comments objects structure seemed to make things more complex.
  3. "load" having two meanings: get from server and build a DOM Element.
  4. I didn't know why it was waiting for 300 milliseconds before processing the json.

Having scope throughout the code made me think that this should really be it's own object with a single 'id' passed in. Perhaps a jQuery plugin would be nice, but I went with a standard javascript function which I knew could easily morph to a plugin.

To further the single object idea, it seems that you are planning to do the same behavior again and again but with a different scope.

So with these thoughts in mind, I began to experiment a bit.

Some other more personal style differences:

  • the use of setInterval eliminates any chance of pausing or restarting the updating.
  • if using jQuery, why use getElementById?
  • use jQuery functions to make DOM objects and not strings of HTML.
  • use .html() instead of .empty() and .append().
  • differences in naming.

At some point I realized I needed to expose some customization to the elements being used, so I extracted them into some defaults.

Here's what I came up with. Let me know if it helps or if you need me to explain what I was thinking. Also, it is completely untested. It is probably littered with syntax errors. There really is no good place for me to test without setting up a special environment.

(function($){
    $(function(){
        var comments = new PaginatedList("table#comments", {
                url: BASE_PATH + 'admin/comments/get'
            });

        comments.start();
    });

    var PaginatedList = function(element, options) {
        var settings = $.extend({}, defaults(), options);

        var instance = this,
            timeout = null,
            page = settings.startPage;

        var $scope = $(element),
            $listElement = $scope.find(settings.listElement),
            $pagination = $scope.find(settings.paginationElement);

        this.start = function() {
            getJSON();
        }

        this.pause = function() {
            stopTimer();
        }

        this.loadPage = function(number) {
            page = parseInt(number);
            getJSON();
        }

        var getJSON = function(){
            stopTimer();
                $.ajax({
                    url: settings.url,
                    type: settings.ajaxMethodType,
                    data: {'offset': page},
                    beforeSend: displayLoadingMessage,
                    success: processJSON,
            complete: waitBeforeLoadingAgain
                });
            }

            var stopTimer = function() {
                clearTimeout(timeout);
                timeout = null;
            }

        var displayLoadingMessage = function() {
            if (prependLoaderWhenReloading) {
                $listElement.prepend(settings.loaderElement);
            } else {
                $listElement.html(settings.loaderElement);
            }
            }

        var processJSON = function(json) {
            buildListItems(json);
                buildPagination(json);
        }

            var buildListItems = function(json) { 
                var $listItems = $(settings.templateElement),
                    data = json[settings.jsonItemKey];

                if (data && $listItems.tmpl) {  
                    $listElement.html($listItems.tmpl(data));
                    settings.setupListItemEvents.apply(instance);
                }
            }

            var buildPagination = function(json) {
                var data = json[settings.jsonPaginationKey];

                if (data) {
                    $pagination.html(data);
                    settings.setupPaginationEvents.apply(instance);
                }
            }

        var waitBeforeLoadingAgain = function() {
            timeout = setTimeout(getJSON, settings.timeBetween);
        }

        var defaults = function() {
            return {
                ajaxMethodType: 'POST',
                jsonItemKey: 'comments',
                jsonPaginationKey: 'pagination',
                listElement: "tbody",
                loaderElement: buildDefaultLoader(),
                paginationElement: "ul.pagination",
                prependLoaderWhenReloading: false,
                setupListItemEvents: function() {},
                setupPaginationEvents: function() {},
                startPage: 0,
                templateElement: "#comments-list",
                timeBetween: 60000,
                url: '/comments'
            }
        }

            var buildDefaultLoader = function() {
                var $loader = $('<span />').addClass('loader');

                return $("<td />")
                        .css(textAlign:'center')
                        .prop('colspan','8')
                        .html($loader);
            }
   }
})(jQuery);
share|improve this answer
Thank you both for your extensive answers, @nate thank you for taking the time to go over the code, I will give you a proper responce as soon as I go over your answer in full, regards. – Philip Mar 4 '12 at 10:18

In my mind:

  • You could use jQuery events and fire an application specific event when you finish loading a table/listing. This would implement the data bus pattern.

The event handler takes the event carrying the data and binds it to a HTML template.

jQuery has a nice Event object.

You'd pass around a ref to a 'bus' object that you publish these events on.

  • Rescheduling: again, with the bus pattern, you can have an event handler that takes an event/message stating a callback and datetime. The scheduler then calls that callback. In your case, it would call loadComments.

Then, from the success callback, you'd just fire the event with the data.

This allows other components of your page to easily update their state based on things going on (more specifically, domain/logic-specific events that are not application/infrastructure events. E.g. 'MailBodyLoaded', not 'PageLoaded')

  • DOM manipulation

You could probably use a templating library instead of raw HTML. It depends though; if this is all the page does, then don't use templates. If it does even a single thing more, use a templating engine. Spine? Backbone? I'm not updated on JS well enough nowadays.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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