4
\$\begingroup\$

I am using RequireJS JavaScript library. I have the following JavaScript module define for dataservice:

define(['jquery'], function ($) {

    var callApi = function (url, type, dataType, data, callback) {
        $.ajax({
            url: url,
            type: type,
            data: data,
            dataType: dataType,
            contentType: "application/json",
            success: function (data) {
                callback(data);
            }
        });
    };

    var getData = function (url, dataType, data, callback) {
        callApi(url, 'GET', dataType, data, callback);
    };

    var postData = function (url, data, callback) {
        callApi(url, 'POST', 'json', data, callback)
    };

    return {
        getData: getData,
        postData: postData
    };

});

Is there anything which can be improved in this module?

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

I really like this code.

Still, 2 observations:

  • The code is awfully optimistic, no default error handling ?
  • success gets data, textStatus, and jqXHR. I would pass all 3 to the callback function.
\$\endgroup\$
3
\$\begingroup\$

Very nice. I like to see some require.js around here.

  • As konijn said, you shouldn't be only passing data to the callback. In fact, you don't need an extra function here at all.

    $.ajax({
        [...]
        success: callback
    });
    
  • Since getData and postData are not used anywhere else and are public I would just include them in the return statement.

      return {
        getData: function (url, dataType, data, callback) {
           callApi(url, 'GET', dataType, data, callback);
        },
        postData: function (url, data, callback) {
           callApi(url, 'POST', 'json', data, callback)
        }
      };
    
\$\endgroup\$
2
  • \$\begingroup\$ To the second point, I agree in this two-method case, but I wouldn't once the methods grew by a line or two or if you added a method. Because that can happen easily, I would probably avoid merging them. \$\endgroup\$ Commented Feb 14, 2014 at 4:21
  • \$\begingroup\$ @DavidHarkness Agreed. \$\endgroup\$ Commented Feb 14, 2014 at 17:54
3
\$\begingroup\$

You're passing in a lot of parameters to all of these functions. This might present a usability issue as the developer who uses the functions has to remember the order in which to pass the parameters.

It might be easier to use if the functions accepted a single object parameter:

var callApi = function (options) {
    $.ajax({
        url: options.url,
        type: options.type,
        data: options.data,
        dataType: options.dataType,
        contentType: "application/json",
        success: function (data) {
            options.callback(data);
        }
    });
};

var getData = function (options) {
    options.type = 'GET';
    callApi(options);
};

var postData = function (options) {
    options.type = 'POST';
    options.dataType = 'json';
    callApi(options);
};

And then you'd call your functions like:

var options = {
    url: 'http://api.example.com/post.svc',
    data: { "id": 34021 },
    callback: success
};
dataservice.postData(options);

This also means that if the developer misses a variable, you can fall back to default options in the public facing functions without breaking the order in which parameters are passed.

var postData = function (options) {
    options.type = 'POST';
    options.dataType = 'json';

    if (!options.callback) {
        options.callback = genericSuccess;
    }

    callApi(options);
};
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.