• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 109
  • Last Modified:

Better way of using callback in javascript

Hi,
Here is the code which i am using ... this includes a callback... although code seems to be working fine.. Need help from experts on its design a better way to write the same or any flaws in it  :
clock.groups.list(userId, callback);
clock.roster.listContacts(userId, callback);

var callback = function(errorThrown, data, name) {
         if(errorThrown === null) {
             if(name === 'groups.list') {
                 mygroupList = data;
             }
             else if (name === 'roster.listContacts') {
                 data.forEach(function(elem){mybuddyList.push(elem)});
             }
         }
}

exports.groups = {};
exports.groups.list = function(userId, callback) {
    callMethod('groupList', userId, callback);
}
exports.roster = {};
exports.roster.listContacts = function(userId, callback) {
    callMethod('listContacts', userId, callback);
}

callMethod = function(name, userId, callback) {
    $.ajax({
        url : endpoint,
        type: 'POST',
        contentType: 'application/json',
        headers: {
            'X-user-id': userId,
        },
        dataType: 'json',
        success: function(data, textStatus, jqXHR){
            callback(null, data, name);
        },
        error: function(jqXHR, textStatus, errorThrown) {
            callback(errorThrown, null, name);
        }
    });
}

Open in new window


Thanks
0
Rohit Bajaj
Asked:
Rohit Bajaj
4 Solutions
 
RobOwner (Aidellio)Commented:
Why would you want to do that if it's working... Not sure I understand?  Is it an assignment?
0
 
Steve BinkCommented:
I did something similar awhile ago.  I found that I had a need to run the same bit of code for each AJAX call (error checking, data prep, etc.), so I wrote a wrapper to go around it.  Take a look at doAjax() and handlerDoAjax().  This allows for my common code to be run after each call, and also a list of custom callbacks passed in by the caller.  You can find some brief examples of how I used it in other sections of that file, or some of the other JS files in the repo (e.g., CartPlugin.js, order-status.js).
0
 
Rohit BajajAuthor Commented:
no its not an assignment.. but its always good to look for alternatives and improve code quality... may be my code has some design flaws...for which i need advice from experts...
0
Concerto Cloud for Software Providers & ISVs

Can Concerto Cloud Services help you focus on evolving your application offerings, while delivering the best cloud experience to your customers? From DevOps to revenue models and customer support, the answer is yes!

Learn how Concerto can help you.

 
Julian HansenCommented:
Well the trend is to use promises rather than callbacks. To that end the use of the success and error properties in your $.ajax call should probably be changed as they are deprecated - rather use the .done()

$.ajax({
  url: ...,
  data: ...
}).done(function(resp) {
});

Open in new window

0
 
BigRatCommented:
The ajax call makes a distinction between success and failure by having the two cases define functions in the object with keys "success" and "error". Why then do you 1) define a function which simply calls another function - instead of doing the action directly and 2) call that function in the error case and then ignore the error if it occurs? Furthermore I see no point in the function (defined as a var) of a  callback which then makes a distinction as to what to do.

I'd simplify all of this into two callbacks - with distinctive names like "processGroups" and "processRoster" which assign the data to the corresponding variables and has a signature of "function(data, textStatus, jqXHR)". I'd simply pass these into the ajax call which sets the success field to that function.
callMethod = function(name, userId, callback) {
    $.ajax({
        url : endpoint,
        type: 'POST',
        contentType: 'application/json',
        headers: {
            'X-user-id': userId,
        },
        dataType: 'json',
        success: callback
        },
        error: function(jqXHR, textStatus, errorThrown) {
            // DO SOME ERROR ACTION HERE
        }
    });
}

Open in new window

0
 
leakim971PluritechnicianCommented:
what is clock.groups.list and clock.roster.listContacts?
0
 
leakim971PluritechnicianCommented:
my two cents depending the two hidden functions not showed by your code snippet :
var setGroupList = function(list) {
    mygroupList = list;
}

var setContactsList = function(lists) {
    lists.forEach(function(elem){
        mybuddyList.push(elem)
    });
}

// you should call this each time userId change
// so I'm pretty sure it's not the right place for it here
$.ajaxSetup({
    headers: { 'X-user-id': userId },
    error: handleError 
});

var handleError = function(jqXHR, textStatus, errorThrown) {
    // you should create something strong to help the end user instead the following
    console.log(errorThrown); // for debugging purpose, not for production    
}

clock.groups.list(userId, setGroupList);
clock.roster.listContacts(userId, setContactsList);

exports.groups = {
    list = function() {
        $.post(endpoint, function(data) { setGroupList(data); }, "json");
    }  
};

exports.roster = {
    listContacts: function(userId, callback) {
        $.post(endpoint, function(data) { setContactsList(data); }, "json");
    }
};

Open in new window

0

Featured Post

Industry Leaders: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Tackle projects and never again get stuck behind a technical roadblock.
Join Now