3
\$\begingroup\$

Following is the code to update content via ajax. Please review all aspects of the code.

var ajaxUpdate = {
    init: function(){
        ajaxUpdate.enableHistory();
    },
    _this:null,
    enableHistory: function(){
        History.Adapter.bind(window,'statechange',function() { // Note: We are using statechange instead of popstate
            var State = History.getState();

            console.log('History: '+State.url)
            if(State.data.type == 'maincontainer'){
                if(jQuery('#maincontainer').length){
                    //update content
                    jQuery('#maincontainer').html(State.data.response);

                    //select the menu element
                    jQuery('#main-menu li').removeClass('active');
                    jQuery('#main-menu li:eq('+State.data.main+')').addClass('active');
                }
                else{
                    window.location.href = State.url;
                }
            }/*
            _this.parent().addClass('active');*/
        });
    },
    updateMainContent: function(){
        var _this = ajaxUpdate._this;
        if ( !History.enabled ) {
            return ;
        }

        jQuery.ajax({
            type: 'GET',
            url: _this.attr('href'),
            beforeSend  : function(jqXHR, settings){
                jQuery('#maincontainer').addClass('loading');
            },
            complete: function(jqXHR, textStatus){
                jQuery('#maincontainer').removeClass('loading');
            },
            success: function(response){
                var data = {
                    main : _this.data('main_index'),
                    type: "maincontainer",
                    response: response
                }
                jQuery('#maincontainer').removeClass('loading');
                History.pushState(data, _this.data('title'), this.url);
            },
            error : function(jqXHR, textStatus, errorThrown){
                //processERROR();
            }
        });
        return false;
    },
    mainmenu: function(){
        ajaxUpdate._this = jQuery(this);
        ajaxUpdate._this.data('main_index', ajaxUpdate._this.parent().index());
        return ajaxUpdate.updateMainContent();
    },
    submenu: function(){
        ajaxUpdate._this = jQuery(this);
        ajaxUpdate._this.data('main_index', jQuery('#main-menu li.active').index());
        return ajaxUpdate.updateMainContent();
    },
    search: function(){
        ajaxUpdate._this = jQuery(this);
        ajaxUpdate._this.data('main_index', jQuery('#main-menu li.active').index());
        return ajaxUpdate.updateMainContent();
    }
};

ajaxUpdate.init();

jQuery('#main-menu a').click(ajaxUpdate.mainmenu);
jQuery('#maincontainer').on('click', '.nav a', ajaxUpdate.submenu);
jQuery('#maincontainer').on('submit', 'form', ajaxUpdate.search);
\$\endgroup\$
5
  • 2
    \$\begingroup\$ What exactly is your question? Does the code work for you? Are you asking us to review it? Are there any specific areas you want the reviews to focus on? \$\endgroup\$
    – svick
    Aug 2, 2013 at 18:53
  • \$\begingroup\$ @svick isn't this a code review site. I just want to get my code reviewed by peers \$\endgroup\$ Aug 5, 2013 at 12:22
  • 2
    \$\begingroup\$ Yeah, but you should state that explicitly, because some people who post here actually want something else. \$\endgroup\$
    – svick
    Aug 5, 2013 at 12:47
  • \$\begingroup\$ Seems clear to me that (s)he wants the code reviewed? \$\endgroup\$
    – konijn
    Aug 5, 2013 at 15:44
  • \$\begingroup\$ @tomdemuyt: See my Meta question \$\endgroup\$
    – Jamal
    Aug 5, 2013 at 15:49

1 Answer 1

5
\$\begingroup\$

A couple things right off the bat:

  1. You use jQuery('#maincontainer') multiple times. You should consider caching that somewhere. Either as part of your object or a separate variable.

    // Wherever you use jQuery('#maincontainer'), use ajaxUpdate.mainContainer
    ajaxUpdate.mainContainer = jQuery('#maincontainer');
    
  2. I would suggest passing the this from your event handlers directly to updateMainContent since that is the only method that uses it.

  3. Unless you have plans to significantly change the way events are handled between search, submenu, and mainmenu, you can can just have one method that handles those events.

  4. complete gets called whether it's an error or success. No need to remove the loading class on success and complete.

  5. Your beforeSend function is unnecessary since it doesn't actually manipulate any of the data before the AJAX call or cancel the AJAX call. You can move the addClass to before the jQuery.ajax. From the jQuery docs:

    A pre-request callback function that can be used to modify the jqXHR (in jQuery 1.4.x, XMLHTTPRequest) object before it is sent. Use this to set custom headers, etc. ... Returning false in the beforeSend function will cancel the request.

  6. You can chain the following lines together (to remove another call to jQuery):

    jQuery('#main-menu li').removeClass('active');
    jQuery('#main-menu li:eq('+State.data.main+')').addClass('active');
    

    Like so:

    jQuery('#main-menu li').removeClass('active')
        .filter(':eq('+State.data.main+')').addClass('active');
    
  7. Switching your AJAX call to a .get and using the promise API cleans up the code a little and makes it a bit more concise.

  8. In your success response, you don't need to define data since you're just passing it directly to History.pushstate.

Not knowing the HTML structure, I think that's the best I can give you. Here's a fiddle with all of these things implemented.

I'm more than happy to answer any questions if you have them. Hopefully this will help you out.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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