5
\$\begingroup\$

[update: keeping this around for posterity, but we ultimately went to a pub/sub pattern, using Amplify.js. Amplify isn't strictly necessary to implement pub/sub, but it has some nice features and syntax]


I have a REST API, which naturally you can just use HTTP calls (mainly Ajax) in order to send and receive data. I am building a layer of abstraction on top of the raw REST API for JavaScript Developers. I already use jQuery in the project, so I am using $.ajax(). I feel like I have arrived at a good pattern, but I'm not absolutely certain.

var ta = ta || {};

$.ajaxSetup({
  cache: false
});

ta.api = {
  heartbeat: function(successCallback, errorCallback) {
    var callType = "heartbeat";
    // if no custom handlers are provided, wire up the default callbacks or blank callback methods
    successCallback = ta.utils.setDefaultIfNeeded(successCallback, ta.callbacks[callType].success);
    errorCallback = ta.utils.setDefaultIfNeeded(errorCallback, ta.callbacks[callType].error);

    var url = '/rs/heartbeat';

    $.ajax({
      url: url,
      success: function(data) {
        successCallback(data);
      },
      error: function(xhr) {
        errorCallback(xhr);
      }
    });
  },
  version: function(successCallback, errorCallback) {
    var callType = "version";
    // if no custom handlers are provided, wire up the default callbacks or blank callback methods
    successCallback = ta.utils.setDefaultIfNeeded(successCallback, ta.callbacks[callType].success);
    errorCallback = ta.utils.setDefaultIfNeeded(errorCallback, ta.callbacks[callType].error);

    var url = '/rs/version';

    $.ajax({
      url: url,
      success: function(data) {
        successCallback(data);
      },
      error: function(xhr) {
        errorCallback(xhr);
      }
    });
  }
}

ta.utils = {
  setDefaultIfNeeded: function (providedHandler, defaultHandler) {
    var checkedHandler = providedHandler;
    if (typeof (providedHandler !== "undefined")) {
      if (typeof (providedHandler) !== "function") {
        if (typeof (defaultHandler) === "function") {
          checkedHandler = defaultHandler;
        } else {
          checkedHandler = function () { /* intentionally empty */ };
        }
      }
    } else {
      checkedHandler = function () {  /* intentionally empty */ };
    }
    return checkedHandler;
  }
};

So, the API can accept success and error handlers on a per-call basis. The setDefaultIfNeeded will do a few pieces of logic, but one of those pieces is an optional phase of using handlers from a callback object. So the callback object is optional, but it's a handy place to store and organize callbacks:

ta.callbacks = {
  heartbeat: {
    success: function(data) { console.log(data) },
    error: function(xhr) { console.log(xhr.statusText) }
  },
  version: {
    success: function(data) { console.log(data) },
    error: function(xhr) { console.log(xhr.statusText) }
  }
}

Another addition I plan to make but haven't tested yet is to have the functions return the jqXhr. It's already being made, so why not return it? But I haven't tested that yet so I didn't want to provide sample code that might be broken.

So, there are now a few options for using the API, depending on your needs:

  1. Make calls to ta.api methods and supply success and error handlers as needed. I.e. you would define them in the methods that ultimately call the REST API.
  2. Make void calls to ta.api, but the success and error handlers are supplied in the ta.callbacks object.
  3. You can still continue to make calls to the REST service with whatever other arbitrary methods you choose.
  4. *planned - define a variable against ta.api which becomes a jqXhr. You then have full access to the XHR and can do with it as you please.

I have a suspicion that some of you will recommend deferred/promises API, and I am not against it. But if I'm being honest, I can't visualize a strong way to encapsulate the various moving parts and have them extremely readable for other devs.

\$\endgroup\$
  • \$\begingroup\$ Please consider using promises. If you find them less readable, you need to spend more time with them, as many of us did before we saw the light. Promises are great for encapsulating the various moving parts because they have a simple interface which simply propagates values and errors. They can be composed. Taking a callback is fine, but return a promise. \$\endgroup\$ – Aluan Haddad Jun 25 '16 at 11:18
  • \$\begingroup\$ Thanks for the comment, @AluanHaddad. In the end, we adopted a pub/sub pattern. Success and Error events simply publish the success (with its data) or its error (with the xhr). \$\endgroup\$ – Greg Pettit Jun 27 '16 at 17:51
2
\$\begingroup\$

Looks to me like there's a lot of duplication here. I.e. boilerplate stuff you'd probably end up copy/pasting (and having to deal with the consequences of that later).

I'd just pare it way down for now: All that really changes is the URL. A really simple version might be:

ta.api = (function ($) {
  // private helper function
  function get(url, success, error) {
    return $.ajax({
      url: url,
      success: success || ta.callbacks[url].success;
      error: error || ta.callbacks[url].error;
    });
  }

  return {
    heartbeat: function (success, error) {
      return get('/rs/heartbeat', success, error);
    },

    version: function (success, error) {
      return get('/rs/version', success, error);
    }
  };
}(jQuery));

There are two assumptions/changes here:

  1. You'd use the url as the property name for default callbacks to avoid having to maintain a separate callType variable, and
  2. It assumes that you either pass a function or a false'y value (e.g. null or just nothing) for the callbacks. I.e. don't pass something truthy that's not a function.

Similarly, it assumes that ta.callbacks[url].* is either undefined or a proper function.

However, there's a potential bug if ta.callbacks[url] itself is undefined. So a safer option would be:

  function get(url, success, error) {
    return $.ajax({
      url: url,
      success: success || (ta.callbacks[url] ? ta.callbacks[url].success : null);
      error: error || (ta.callbacks[url] ? ta.callbacks[url].error : null);
    });
  }

If you want to keep your setDefaultIfNeeded, there are few issues with it. For one it's name is not terribly decriptive: Set a default what? A string? A number? It's very specifically a type-checking function that only looks for functions, but given the name, I might think I could use it for anything. And despite its name, it doesn't actually set anything. It just returns, so it should rather be called get....

Secondly, it can be shortened considerably:

ta.utils = {
  getValidCallback: function (providedHandler, defaultHandler) {
    if(jQuery.isFunction(providedHandler)) return providedHandler;
    if(jQuery.isFunction(defaultHandler)) return defaultHandler;
    return function () {};
  }
};

I honestly don't have a good idea for a name (getValidCallback is ok, but not great), because I'd just stick to the first option of using || and simply not have this function.

As for using promises/deferreds instead, the above code allows for that automatically. The result of $.ajax is itself a promise object, and since this code returns it (your function didn't return anything), you can use it both ways:

ta.api.heartbeat(foo, bar);
// or
ta.api.heartbeat().then(foo, bar);
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for taking the time, Flambino. I'll need some time to test this all out, but it seems every effective; particularly the ability to use the $.ajax promise object but also for the streamlined code. \$\endgroup\$ – Greg Pettit Dec 7 '15 at 18:21

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.