3
\$\begingroup\$

I wanted to create a basic interaction which does 1 outcome but with 2 possible actions: get a number from an other document, by either clicking on a button or waiting a fixed amount of time.

I wrote this, which works:

//Initialize the ajax function
(function($) {
    $.fn.getNumber = function() {
        $(this).load('number.html .giver');
    };
})(jQuery);

//Launch ajax function 3s after DOM is loaded
$(function() {
    $('.receiver').delay(3000).queue(function(nxt) {
        $(this).getNumber();
    });
});

//Launch ajax function if a button is clicked
$('button').click(function() {
    $('.receiver').getNumber();
});

Now even if it works I'm pretty sure my code is badly written, for instance my getNumber function seem redundant. I just fear I might get bad habits, any insights of how I could write this in a more elegant way?

\$\endgroup\$
4
\$\begingroup\$

The biggest problem with this code is that is not manageable at all. This setup is inefficient and will make it very hard to add features and also bug fix if things start going wrong.

  • there's no success or error handling on your AJAX request.
  • There's no way of knowing if getNumber() worked on not, which could end up spewing errors onto the page.
  • Callbacks are your friend.

Here is a more manageable setup for you.

// jQuery will wait until the DOM is ready. Then in the callback you can define all your listeners.
$('body').on('ready', function(){
    // This is the listener for the button click. $.on() should always be used from jQuery 1.7+.
    $('button').on('click', function(){
        $('.receiver').load('number.html .giver', function(response, status, xhr){
            // This is just a very basic error check. At least is there is an error you'll know.
            if (status == "error") {
                $('.receiver').html("Sorry there was an error: " + xhr.statusText);
            }
        });
    });
});

I would always advise strongly against having timed events in your code. They tend to break easily and are hard to fix. However if you really want them, use the Javascript setTimeout() function after DOM load. setTimeout() is just a bit easier to read and maintain. Instead of timing events, I would advise using callbacks to fire events. This solution allows any number of DOM listeners to be easily added to your page now without hassle.

I would make one more note here: in my experience it never ends well trying to drag AJAX responses through functions unless you have rock solid scopes set up. I would personally always say it is best practice, with adding dynamic content, to always create the AJAX request in the same scope/function that you are going to use it in. So:

// Don't do this.
var ajaxRetrieve = function(){
    $.ajax({
        url: '/gimmeAllOfThat',
        method: 'POST',
        success: function(responseData){
            return responseData;
        },
        error: function(responseData){
            alert('Check yourself');
        }
    });
}

$('button').on('click', function(){
    // This is very bad.
    var data = ajaxRetrieve();
    $('#div').html(data);
});

This config means that you will still have to use whatever you get returned from your function, even if the AJAX request fails. Which could mean you'll have to check for errors twice, instead of just once and using the data as an event in the AJAX request. Also something to note is: that AJAX return will fail so badly, as it won't get returned into the parent function scope, only the success function scope.

TL;DR: use callbacks:

// Do this.
$('body').on('ready', function(){
    var ajaxRetrieve = function(callback){
        $.ajax({
            url: '/gimmeAllOfThat',
            method: 'POST',
            success: function(responseData){
                callback(responseData);
            },
            error: function(responseData){
                alert('Check yourself');
            }
        });
    }

    $('button').on('click', function(){
        ajaxRetrieve(function(data){
            $('#div').html(data);
        });
    });
});

This setup means that that function can be reused for a number of different things, and you can just change the callback function each time you call it. It also means you get out of 'Returning Hell', trying to return data here, there and everywhere.

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