Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I often use jQuery plugins for front-end functionality in web applications. This requires me to write code like this, where I am assigning a function to a an object property:

var trainingDiary = $("#training_diary").fullCalendar({
  dayClick: function(date, allDay, jsEvent, view) {

    var title = prompt('Event Title:');
    var dayObj = $(this);
    //console.log($(this));
    if (title) {
     $.ajax({
      url: "/app_dev.php/addtrainingday",
      global: false,
      type: "POST",
      data: "dayNo=" + date.getDate(), //returns day of month. getDay() returns day of week
      async:true,
      success: function(msg) {

               trainingDiary.fullCalendar('renderEvent',
                {
                  title: title,
                  start: date,
                  allDay: allDay,
                  backgroundColor:'#cccccc'
                },
            true // make the event "stick" across calendar pages
          )

      dayObj.css({'background-color':'#339933','background-image':'url()'})
      console.log(msg.valueOf());
      } //end ajax success
     })
   } else {
      trainingDiary.fullCalendar('unselect');
   }

  },

  theme: true,
  header: {left:'title',
            center: '',
            right: 'today prev next'},
  firstDay:1

});

The code becomes hard to read and maintain as there is now a success function within an ajax function within yet another function. My question is how can I improve the code so that 'dayClick: function(...) can call a function defined outside of the fullCalendar call. In this case, fullCalendar is a jQuery plugin.

UPDATED CODE:

// Directives for JSLint
/*global console, prompt, $, ROOT_URL */ // declaration of global variables used
/*jslint unparam: true, sloppy: true, white: true */ // disable less useful checks


//create a closure for private scope

  ( function() {

    var calendarOptions, // object, options for calendar diary
        trainingDiary,   // object, training diary using jQuery plugin fullCalendar()
        bgColour = '#339933';

    // Function: onDayClick(day, allDay, jsEvent, view)
    // Callback for the selection of a day in the calendar.
    //
    // Parameters:
    //   date - type?, description?
    //   allDay - type?, description?
    //   jsEvent - type?, description?
    //   view - type?, description?

    function onDayClick(date, allDay, jsEvent, view) {

      var date2 = new Date(date.getFullYear(), date.getMonth(), date.getDate()+1),
          todaysEvents = trainingDiary.fullCalendar('clientEvents', function(event) {
            return event.start >= date && event.start < date2;
          }),
          title,       // string, event title, input by user
          dayObj = $(this),      // object, jQuerified DOM element
          ajaxOptionsAdd, // object, options for AJAX call to addtrainingday script
          ajaxOptionsRemove; // object, options for AJAX call to addtrainingday script

      //NESTED AJAX CALLS - NESTED TO RETAIN REQUIRED PARAMS IN CLOSURE SCOPE

      // Function: onAjaxSuccessAdd(msg)
      // Callback for a successful AJAX call to server-side addtrainingday script.
      //
      // Parameter:
      //   msg - string, message received from server

      function onAjaxSuccessAdd(msg) {

        var event = {
          title: title,
          start: date,
          allDay: allDay,
          backgroundColor: bgColour
        };

        //render the event on the calendar
        trainingDiary.fullCalendar(
          'renderEvent',
          event,
          true // make the event "stick" across calendar pages
        );

        dayObj.css({
          'background-color':bgColour,
          'background-image':'url()'
        }); // added missing semicolon

        console.log(msg.valueOf());

      } // end of onAjaxSuccessAdd declaration

      // Function: onAjaxSuccessRemove(msg)
      // Callback for a successful AJAX call to server-side removetrainingday script.
      //
      // Parameter:
      //   msg - string, message received from server

      function onAjaxSuccessRemove(msg) {

        trainingDiary.fullCalendar('removeEvents',todaysEvents.id);
        dayObj.css({
          'background-color':'#FFFFFF',
          'background-image':'url()'
        }); // added missing semicolon

        console.log(msg.valueOf());

      } // end of onAjaxSuccessRemove declaration

      // AJAX CALL OPTIONS

      ajaxOptionsAdd = {
        url: ROOT_URL+"addtrainingday",
        global: false,
        type: "POST",
        // getDate() returns day of month. getDay() returns day of week
        data: "date=" + $.fullCalendar.formatDate(date, 'yyyy-MM-dd hh:mm:ss'),
          async: true,
          success: onAjaxSuccessAdd
      };

      ajaxOptionsRemove = {
        url: ROOT_URL+"removetrainingday",
        global: false,
        type: "POST",
        // getDate() returns day of month. getDay() returns day of week
        data: "date=" + $.fullCalendar.formatDate(date, 'yyyy-MM-dd hh:mm:ss'),
          async: true,
          success: onAjaxSuccessRemove
      };

      // DAYCLICK EXECUTION LOGIC

      if (todaysEvents.length > 0 ) {
         $.ajax(ajaxOptionsRemove);
         return;
      }

      title = prompt('Event Title:');
      if (title) {
        $.ajax(ajaxOptionsAdd);
      } else {
        trainingDiary.fullCalendar('unselect');
      }


    } // end of onDayClick declaration

  // CALENDAR INITIIALIZATION CODE

  calendarOptions = {
    dayClick: onDayClick,
    theme: true,
    header: {
      left: 'title',
      center: '',
      right: 'today prev next'
    },
    firstDay: 1,
    eventRender: function (event, element) {
      element.removeClass('fc-corner-right fc-corner-left'); //removes border round the event
    }
  };

  trainingDiary = $("#training_diary").fullCalendar(calendarOptions);


}() ); //end of and call of closure
share|improve this question

1 Answer 1

You should use the JavaScript Module Pattern to create a private scope for your function declarations:

// Directives for JSLint
/*global console, prompt, $ */ // declaration of global variables used
/*jslint unparam: true, sloppy: true, white: true */ // disable less useful checks

// create a closure for private scope
(function(){

  // Private declarations

  var calendarOptions, // object, options for calendar diary
      trainingDiary;   // object, training diary using jQuery plugin fullCalendar()

  function onDayClick(date, allDay, jsEvent, view) {
    // Function: onDayClick(day, allDay, jsEvent, view)
    // Callback for the selection of a day in the calendar.
    //
    // Parameters:
    //   date - type?, description?
    //   allDay - type?, description?
    //   jsEvent - type?, description?
    //   view - type?, description?

    // single var declaration
    var title,       // string, event title, input by user
        dayObj,      // object, jQuerified DOM element
        ajaxOptions; // object, options for AJAX call to addtrainingday script

    // this declaration must be nested to access date, allDay... in closure scope
    function onAjaxSuccess(msg) {
      // Function: onAjaxSuccess(msg)
      // Callback for a successful AJAX call to server-side addtrainingday script.
      //
      // Parameter:
      //   msg - string, message received from server, for logging purpose        

      var event = {
        title: title,
        start: date,
        allDay: allDay,
        backgroundColor:'#cccccc'
      };

      trainingDiary.fullCalendar(
        'renderEvent',
        event,
        true // make the event "stick" across calendar pages
      ); // added missing semicolon

      dayObj.css({
        'background-color':'#339933',
        'background-image':'url()'
      }); // added missing semicolon

      console.log(msg.valueOf());

    } // end of onAjaxSuccess declaration

    // Init
    title = prompt('Event Title:');
    dayObj = $(this);
    ajaxOptions = {
      url: "/app_dev.php/addtrainingday",
      global: false,
      type: "POST",
      // getDate() returns day of month. getDay() returns day of week
      data: "dayNo=" + date.getDate(),
        async: true,
        success: onAjaxSuccess
    };

    //console.log($(this));
    if (title) {
      $.ajax(ajaxOptions); // added missing semicolon
    } else {
      trainingDiary.fullCalendar('unselect');
    }

  } // end of onDayClick declaration

  // Initialization code

  calendarOptions = {
    dayClick: onDayClick,
    theme: true,
    header: {
      left: 'title',
      center: '',
      right: 'today prev next'
    },
    firstDay: 1
  };

  trainingDiary = $("#training_diary").fullCalendar(calendarOptions);

}()); // the function is called immediately to make the declarations effective

I also created extra variables to hold options objects, to enhance readability, and I added comments to describe the intent and parameters of functions declared.

Note that trainingDiary is now declared in the private scope as well. If you need to access it in the global scope, you will need to export it to the global object by assigning it to global this or window:

(function(){

  // Private declarations
  (...)

  // Init
  (...)

  // Public declarations
  this.trainingDiary = trainingDiary;
}());

I checked the code with latest JSLint; it is a useful tool once you know what to expect from it, e.g. by reading "JavaScript: The Good Parts" from its author Douglas Crockford. It would definitely help you to spot the missing semicolons, which can cause unexpected issues when you minify your code.

share|improve this answer
    
Wow - thanks so much for such a comprehensive answer. I will take a look into this approach and report back. Can you explain how one would '[assign it to] global this'? –  codecowboy Jul 30 '11 at 5:10
1  
This is shown in second example: this.trainingDiary = trainingDiary; A classic alternative would be to assign to window, which only works in a browser environment: window.trainingDiary = trainingDiary; –  Eric Bréchemier Jul 30 '11 at 12:21
    
yep - my bad. I realised this after I read through your answer again and the link on closures. I've updated my question with the latest code. JSLint complains about ROOT_URL - this is defined in another file. –  codecowboy Jul 30 '11 at 16:48
    
my bad again - just realised I can alert JSLint to an additional global declaration and you had already done this for jquery –  codecowboy Jul 30 '11 at 17:06

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.