7
\$\begingroup\$

I've been working on a new project and I've found myself needing to make multiple Ajax calls, often on the same page and at the same time. I decided rather than repeating code that I would try and make a reusable object that to handle these calls.

I'm very new to Object Orientated JavaScript so I'm looking for some feedback and advice on my code; am I doing it right? is there something I'm not doing? should I be doing things differently etc?

Here's my Ajax constructor:

/**
 *
 * AjaxRequest Constructor
 *
 * @param url
 * @param success_callback
 * @param limit
 * @param type
 * @constructor
 */

function AjaxRequest (url, success_callback, limit, type) {
    this.url = url;
    this.success_callback = success_callback;
    this.limit =  limit || 3;
    this.type = type || "POST";

    //init ajax
    this.init();
}

/**
 * initialise
 * ajax
 */

AjaxRequest.prototype.init = function () {

    var _self = this;

    $.ajax({
        dataType: "json",
        url: this.url,
        type: this.type,
        data: {
            member_id: 1,
            limit: this.limit
        },
        beforeSend : function (){
            console.log('loading');
            //spinner.spin(target);
        },
        success: function (response) {
            if (response.success === 1) {
                _self.success_callback(response);
            }
        },
        error: function (a, b, c) {

        }
    });
}

/**
 * change number of
 * displayed outputs
 * @param limit
 */

AjaxRequest.prototype.set_limit = function (limit) {
    this.limit = limit;
    this.init();
}

and here is how I'm calling it

/**
 * instantiate getAchievements
 * @type {AjaxRequest}
 */

getAchievements = new AjaxRequest("/ajax-member/get-achievements", function (response) {

    var i,
        container = document.getElementsByClassName('achievements-container')[0],
        achievements_data = '';

    for (i in response.achievements) {
        achievements_data += ("<div class='achievement'><div class='copy'><h2>" + response.achievements[i].t_achievement_name + "</h2><span>" + response.achievements[i].t_achievement_description + "</span></div></div>");
    }

    container.innerHTML = achievements_data;
});

/**
 * instantiate getComments
 * @type {AjaxRequest}
 */

getComments = new AjaxRequest("/ajax-member/get-comments", function (response) {

    var i,
        container = document.getElementsByClassName('comments')[0],
        comments_data = '';

    for (i in response.comments) {
        comments_data += ("<div class='comment'><div class='copy'><h2>" + response.comments[i].t_fname + " " + response.comments[i].t_lname + "</h2><span>" + response.comments[i].t_comment_content + "</span></div></div>");
    }

    container.innerHTML = comments_data;
});

It would be good to hear some feedback on this. I'm always looking to improve and find the best ways of doing things so if you spot something I should be doing differently then please shout.

\$\endgroup\$
7
\$\begingroup\$

From a quick once over:

  • JsHint.com has very little feedback, good!
  • Consider using use strict to activate strict mode
  • Avoid console.log
  • Both beforeSend , error and complete should be customizable by the caller
  • success() returns ( PlainObject data, String textStatus, jqXHR jqXHR ) you should pass all 3 to success_callback
  • JavaScript is lowerCamelCase, success_callback -> successCallback
  • member_id: 1, <- Maybe this is sample code, this magic constant deserves a comment
  • I wonder if AjaxRequest should know about "/ajax-member/, from a DRY perspective
  • for (i in response.achievements) {, response.achievements seems to be an array, please use a regular for loop to iterate over arrays, it will prevent hard to find bugs at a later stage
  • Find a good templating library, you need it, consider Handlebars
  • I think your Ajax class could be expanded, getAchievements and getComments are clearly a result of copy pasting code
\$\endgroup\$
  • \$\begingroup\$ thanks for the feedback @konijn, I'll look into making those amends you suggested, i've played with handlebars a little before so i'll see how i can integrate it into this :) \$\endgroup\$ – woolm110 May 21 '14 at 12:28
3
\$\begingroup\$

The name of your class AjaxRequest is a generic name, yet it is used for a specific kind of request being that the limit and member_id parameters are hard coded. I think what you want is a class that encapsulates specific AJAX calls. For this, the Repository Pattern would work well:

function AchievementsRepository(baseUrl, memberId) {
    this.baseUrl = baseUrl;
    this.memberId = memberId;
}

AchievementsRepository.prototype = {

    baseUrl: null,

    memberId: null,

    constructor: AchievementsRepository,

    add: function(data) {
        var promise = new Promise(function() {
            var xhr = new XMLHttpRequest(),
                url = this.baseUrl + "/add-achievement";

            ...
        }.bind(this));

        return promise;
    },

    findAll: function(limit) {
        var promise = new Promise(function() {
            var xhr = new XMLHttpRequest(),
                url = this.baseUrl + "/get-achievements";

            ...
        }.bind(this));

        return promise;
    }

};

And then to use this class:

var achievements = new AchievementsRepository("/ajax-member", 1);

achievements.findAll(10).then(function(data) {
    // All achievements fetched
}, function(error) {
    // Some sort of error occurred
});

achievements.add({
    foo: "bar",
    baz: "foo"
}).then(function(result) {
    // Achievement added successfully
}, function(error) {
    // There was a problem adding the achievement
});

This allows you to centralize all your AJAX calls, yet keep things strongly typed enough to make your code easy to follow.

Edit: This pseudo code uses the Promise/A+ API, which browsers have native support for now in ECMA Script 6. Related pollyfill: https://github.com/jakearchibald/es6-promise

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