HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript API for REST

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptrestapifor

Problem

[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 (provide

Solution

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:

  • You'd use the url as the property name for default callbacks to avoid having to maintain a separate callType variable, and



  • 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);

Code Snippets

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));
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);
    });
  }
ta.utils = {
  getValidCallback: function (providedHandler, defaultHandler) {
    if(jQuery.isFunction(providedHandler)) return providedHandler;
    if(jQuery.isFunction(defaultHandler)) return defaultHandler;
    return function () {};
  }
};
ta.api.heartbeat(foo, bar);
// or
ta.api.heartbeat().then(foo, bar);

Context

StackExchange Code Review Q#95121, answer score: 2

Revisions (0)

No revisions yet.