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\$
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\$ Feb 14 '14 at 4:21
  • \$\begingroup\$ @DavidHarkness Agreed. \$\endgroup\$
    – Jivings
    Feb 14 '14 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\$

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.