Code to update content via ajax

Question

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);

• svick: 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?
• aWebDeveloper: @svick isn't this a code review site. I just want to get my code reviewed by peers
• svick: Yeah, but you should state that explicitly, because some people who post here actually want something else.
• konijn: Seems clear to me that (s)he wants the code reviewed?
• Jamal: @tomdemuyt: See my Meta question

Answer 1

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.