Which is a better way of writing callbacks?

Question

Just by seeing what I've wrote now, I can see that one is much smaller, so in terms of code golf Option 2 is the better bet, but as far as which is cleaner, I prefer Option 1. I would really love the community's input on this.

Option 1

something_async({
    success: function(data) {
        console.log(data);
    },
    error: function(error) {
        console.log(error);
    }
});

Option 2

something_async(function(error,data){
    if(error){
        console.log(error);
    }else{
        console.log(data);
    }
});

Answer 1

They are not exactly the same. Option 2 will still log the (data), whereas Option 1 will only log data on success. (Edit: At least it was that way before you changed the code)

That said, Option 1 is more readable. Programming is not / should not be a competition to see who can write the fewest lines that do the most things. The goal should always be to create maintainable, extendable (if necessary) code --- in my humble opinion.

Answer 2

Many people will find option#1 easier to read and to maintain - two different callback functions for two different purposes. It is commonly used by all Promise Libraries, where two arguments will be passed. Of course, the question Multiple arguments vs. options object is independent from that (while the object is useful in jQuery.ajax, it doesn't make sense for promise.then).

However, option#2 is Node.js convention (see also NodeGuide) and used in many libraries that are influenced by it, for example famous async.js. However, this convention is discussable, top google results I found are WekeRoad: NodeJS Callback Conventions and Stackoverflow: What is the suggested callback style for Node.js libraries?.

The reason for the single callback function with an error argument is that it always reminds the developer to handle errors, which is especially important in serverside applications. Many beginners at clientside ajax functions don't care forget about error handling for example, asking themselves why the success callback doesn't get invoked. On the other hand, promises with then-chaining are based on the optionality of error callbacks, propagating them to the next level - of course it still needs to be catched there.

Answer 3

In all honesty, I prefer to take them one step further, into Promises/Futures/Deferreds/etc... Or (/and) go into a "custom event" queue, using a Moderator (or an observer/sub-pub, if there is good reason for one particular object to be the source for data).

This isn't a 100% percent of the time thing. Sometimes, you just need a single callback. However, if you have multiple views which need to react to a change (in model data, or to visualize user-interaction), then a single callback with a bunch of hard-coded results isn't appropriate.

moderator.listen("my-model:timeline_update", myView.update);
moderator.listen("ui:data_request", myModel.request);
button.onclick = function () { moderator.notify("ui:data_request", button.value); }

Things are now much less dependent upon one big callback and you can mix and match and reuse code.

If you want to hide the moderator, you can make it a part of your objects:

var A = function () {
        var sys = null,
            notify = function (msg, data) {
                if (sys && sys.notify) { sys.notify(msg, data); }
            },
            listen = function (msg, callback) {
                if (sys && sys.listen) { sys.listen(msg, callback); }
            },
            attach = function (messenger) { sys = messenger; };

        return {
            attach : attach
            /* ... */
        };
    },
    B = function () { /* ... */ },

    shell = Moderator(),
    a = A(),
    b = B();

    a.attach(shell);
    b.attach(shell);

    a.listen("do something", a.method.bind(a));
    b.notify("do something", b.property);

If this looks a little familiar, it's similar behaviour to, say Backbone.js (except that they extend() the behaviour onto objects, and others will bind, where my example has simplified wrappers to show what's going on).

Promises would be the other big-win for usability, maintainable and easy to read code (as long as people know what a "promise" is -- basically it passes around an object which has the callback subscriptions).

// using jQuery's "Deferred"

var ImageLoader = function () {
    var cache = {},

        public_function = function (url) {
            if (cache[url]) { return cache[url].promise(); }
            var img = new Image(),
                loading = $.Deferred(),
                promise = loading.promise();

            img.onload  = function () { loading.resolve(img); };
            img.onerror = function () { loading.reject("error"); };
            img.src = url;
            cache[url] = loading;
            return promise;
        };

    return public_function;
};

// returns promises
var loadImage = ImageLoader(),

    myImg = loadImage("//site.com/img.jpg");


myImg.done( lightbox.showImg );
myImg.done( function (img) { console.log(img.width); } );

Or var blog_comments = [ /* ... */ ],

    comments = BlogComments();

blog_comments.forEach(function (comment) {
    var el = makeComment(comment.author, comment.text),
        img = loadImage(comment.img);

    img.done(el.showAvatar);
    comments.add(el);
});

All of the cruft there is to show how powerful promises can be.
Look at the .forEach call there. I'm using Image loading instead of AJAX, because it might seem a little more obvious in this case:

I can load hundreds of blog comments, if the same user makes multiple posts, the image is cached, and if not, I don't have to wait for images to load, or write nested callbacks. Images load in any order, but still appear in the right spots.

This is 100% applicable to AJAX calls, as well.

Answer 4

Promises have proven to be the way to go as far as async and libraries like bluebird embrace node-style callbacks (using the (err, value) signature). So it seems beneficial to utilize node-style callbacks.

But the examples in the question can be easily be converted into either format with the functions below. (untested)

function mapToNodeStyleCallback(callback) {
  return {
    success: function(data) {
      return callback(null, data)
    },
    error: function(error) {
      return callback(error)
    }
  }
}

function alterNodeStyleCallback(propertyFuncs) {
  return function () {
    var args = Array.prototype.slice.call(arguments)
    var err = args.shift()
    if (err) return propertyFuncs.err.apply(null, [err])
    return propertyFuncs.success.apply(null, args)
  }
}