2
\$\begingroup\$

I have the following function which runs as expected and is defined in the window object:

function myFunction(url, timeout, type, sc, ec) {
    return $.ajax({
        url: url,
        timeout: timeout,
        type: type,
        success: function (data) {
            if (typeof sc === 'function') {
                sc(data);
            }
        },
        error: function (jqXhr) {
            if (typeof ec === 'function') {
                ec(jqXhr);
            }
        }
    });
}

Which works as expected.

However, I am now expanding this function to be a whole lot more complex. As a result to this, I was wondering whether I could get some style guides (I'm a bit OCD when it comes to things like this in JS).

Additional features I am adding are as follows:

  • showLoader - the ability to show a modal loader
  • loaderMessage - the message displayed if showloader === true
  • storeAs - how to store the downloaded data

The new function now looks as follows:

(function (window, document, $, undefined) {
    function myFunction(settings, sc, ec) {
        var options = $.extend({
            showLoader: false,
            loaderMessage: 'Loading...',
            storeAs: null,
            timeout: 60000,
            url: 'http://www.google.co.uk',
            type: 'GET',
            success: function (data) {
                if (options.storeAs) {
                    localStorage.setItem(options.storeAs, JSON.stringify(data));
                }

                if (typeof sc === 'function') {
                    sc(data);
                }
            },
            error: function (jqXhr) {
                if (typeof ec === 'function') {
                    ec(jqXhr);
                }
            }
        }, settings);

        if (options.showLoader) {
            showLoader(options.loaderMessage); // external function to show loader
        }

        return $.ajax(settings);
    }

    window.app = {
        comms: {
            ajax: function (options, sc, ec) {
                return myFunction(options, sc, ec);
            }
        }
    };
}(this, this.document, jQuery));

Anonymous function

I like to now define all my code within anonymous functions since it frees up the global name space.

However, I have two questions:

  1. Is it recommended/do I need to make use of the JavaScript new keyword? Is there any advantage to doing so?
  2. If so, then how would I go about using it? I have got to grips with the majority of the language, just not the new constructor/keyword?

Parameters

My second change will be to address the parameters - five parameters in JavaScript is quite readable. However, any more isn't.

I am thinking of using a settings object as seen above:

Couple of questions surrounding this however:

  1. Is it wise to use just one options object, as I am already doing here? $.ajax receives this settings object, but my settings object contains not just jQuery.ajax settings, but function settings - obviously jQuery will ignore these since it will use just the necessary options, but is there any reason to avoid this?
  2. I could combine the success callback, and error callback into the options object but this looks quite messy. Is it better to provide the callbacks as separate parameters or move those into the options callback too?

What is the difference between:

var options = $.extend({}, {
    showLoader: false,
    loaderMessage: 'Loading...',
    storeAs: null
}, settings);

And:

var options = $.extend({
    showLoader: false,
    loaderMessage: 'Loading...',
    storeAs: null
}, settings);

Is there anything else I should be doing which I am missing, which is generally recommended in JavaScript?

\$\endgroup\$
  • \$\begingroup\$ Is it really named myFunction? \$\endgroup\$ – 200_success Jun 12 '14 at 11:03
  • \$\begingroup\$ jquery, I assume? (If not, please fix the tags.) \$\endgroup\$ – 200_success Jun 12 '14 at 11:05
  • \$\begingroup\$ No it's not - I just mocked up a quick example because my original code would be meaningless out of context. And yes correct - thank you. Chrome was being a bit buggy and not showing the recommendations for some reason. \$\endgroup\$ – keldar Jun 12 '14 at 11:09
  • 1
    \$\begingroup\$ Placeholder names such as myFunction would normally make a question hypothetical and therefore off-topic for Code Review. This is not the worst I've seen, but I'd still encourage you to post the question using realistic identifiers. \$\endgroup\$ – 200_success Jun 12 '14 at 11:15
2
\$\begingroup\$

I would prefer a setting object for each function you want to call instead of merging them together into one, because then it is clear who needs this parameters. Otherwise this information gets lost. And who knows what jquery will do with this options in future releases? Maybe something useful, maybe not.

I think it is okay to add functions to objects but you can it make a little nicer when you define these not directly in the options object, instead only referencing.

(function (window, document, $, undefined) {

    function successHandler 
    ...

    function myFunction(settings, sc, ec) {
    ...

      {
         success: successHandler
      }

Also I would prefer real names and no two character abbrevations. That makes the code more readable for you and the rest of the world.

\$\endgroup\$
2
\$\begingroup\$

Is it recommended/do I need to make use of the JavaScript new keyword? Is there any advantage to doing so?

If so, then how would I go about using it? I have got to grips with the majority of the language, just not the new constructor/keyword?

Using new in JavaScript is for constructors and get the advantage of using prototypes instead of only functions.

In this case I didn't see the necessity of use it. Your approach is more near to declare a jQuery plugin.

If you have more punctual doubts, I strongly recommend take a look on Stack Overflow.

Is it wise to use just one options object, as I am already doing here? $.ajax receives this settings object, but my settings object contains not just jQuery.ajax settings, but function settings - obviously jQuery will ignore these since it will use just the necessary options, but is there any reason to avoid this?

It's good practice with plugins in jQuery to pass a bunch of things as options objects. For me, it's a code smell and I prefer to analyze if it's possible to split responsibilities or thing in terms of view/model, but that is a personal choice.

Sometimes it's hard to avoid the options objects because we have a lot of configs to set and fallback to defaults. In this case, you are wrapping in other function, and your extra params are only for you plugin convencion, so I'm think it's not wrong.

I could combine the success callback, and error callback into the options object but this looks quite messy. Is it better to provide the callbacks as separate parameters or move those into the options callback too?

"It depends" is the best answer. Dou need that level of configuration? Normally all jQuery plugins has a fallback options. I think could be a good idea move to options, if you are going to reconfigure, or need reconfigure. If not, that doesn't make sense.

What is the difference between options

According with the documentation of $.extend the first param is the target. So, it's a good practice to not override the default parameters. You should have 2 objects options / settings and merge into internal options and use it.

var anEmptyObject = {}
var defaults = { anOption: "default option"}
var internalOptions = $.extend(anEmptyObject, defaults, settings}

About your code, global non-declared variables are attached to window.

So, instead of

(function (window){
    window.app = {}
})(this);

try

(function (){
   app = {}
})(this);

Check similar questions on Stack Overflow about the use of var.

Also, I've made some improvements in your code and left a few comments.

In this example I see you are trying to encapsulate ´myFunction´ but I don't see where are defined showLoader when you call it. It should be a good practice to send inside of options instead of ask and execute.

(function ($) {
    function myFunction(settings, sc, ec) {

        var anErrorcb = typeof ec === 'function' ? ec : function() {}
        var aSucccesCb = typeof sc === 'function' ? sc : function() {}

        var defaults = {
            showLoader: false,
            loaderMessage: 'Loading...',
            storeAs: null,
            timeout: 60000,
            url: 'http://www.google.co.uk',
            type: 'GET',
            success: function (data) {

                if (options.storeAs) {
                    localStorage.setItem(options.storeAs, JSON.stringify(data));
                }

                sc(data);
            },
            error: anErrorcb 

        };

        var options = $.extend({}, defaults, settings);

        if (options.showLoader) {
            showLoader(options.loaderMessage); // external function to show loader
        }

        return $.ajax(options);
    }

    window.app = {
        comms: {
            ajax: myFunction
            }
        }
    };
}(jQuery));
//Remove aditionan parameters, unused
//remove the reference of all functions inside of "ajax
\$\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.