I'm writing a jQuery plugin.

I'm allowing the user to pass options (optionally-- argument 2 to extend) but I also require some defaults (argument 1), and some things user is not allowed to modify (argument 3).

I follow the global override pattern recommended by most jQuery plugins. But I'm running into something I've never seen discussed which is where you have defaults that need to be set in a specific scope (caption being set to self._tableName, for example).

This forces you to do an extend pretty much everywhere you need options + internal state. It's not that terrible, but my code review sense is tingling.

Anything in the snippet is up for debate. Leave your thoughts, please.

For context, how I'm doing options in the constructor

       this.options = $.extend(true, $.fn.myPlugin.Defaults, options);

And here is the argument object I'm creating to be passed to a plugin. To reiterate, I'm forming this object much later when it's time to create the jqgrid.

       $.extend({ caption: self._tableName }, 
                self.options ? self.options.jqgrid : {},
                    datatype: function (postdata) {
                    ColNames: ColNames,
                    colModel: ColModel,
                    pager: '#' + pager,
                    ondblClickRow: function (rowid) {
                        $(this).jqGrid('editGridRow', rowid, editTemplate);
  • 1
    \$\begingroup\$ Could you provide a little more context? From the above it's hard to tell if there are improvements to be made or not \$\endgroup\$ – Flambino Mar 29 '14 at 20:58
  • 1
    \$\begingroup\$ It would just be more of the same. After considering it a bit more, I'm not sure there is a way to do things differently. How else could you do it but to $.extend 3 separate objects? As long as you order them correctly, the result is achieved. The base, options, and defaults all combine to form the behavior the user expects without letting them shoot themselves in the foot. \$\endgroup\$ – brian Mar 29 '14 at 22:24
  • \$\begingroup\$ You may well be right. I was thinking there might be stuff to find elsewhere in the code, before you reach the extend calls. But without seeing more of the code, it's impossible to tell. That said, your solution may well be the best. If you believe that's the case, feel free to self-review (or close) the question (you can always pick a different answer, if one appears). The fewer unanswered questions hanging around, the better for the site. \$\endgroup\$ – Flambino Mar 29 '14 at 23:42
  • \$\begingroup\$ Eh I'm not going to close it especially since there's a bug in my code no one caught; the first call to $.extend will overwrite all the global options with the object's options when there's a matching property. Vaguely insinuating I am yet to get a good code review. \$\endgroup\$ – brian Mar 30 '14 at 18:17

Not a lot ( of context ) to review;

  • this.options = $.extend(true, $.fn.myPlugin.Defaults, options); <- This will modify the defaults of the plugin, if you did not want that, then you should do

    this.options = $.extend(true, {}, $.fn.myPlugin.Defaults, options);
  • You are talking about passing a third parameter for immutable settings/options/parameters, the 3rd parameter in your $.extend call will do no such thing, you will not even know which properties come from the 3 object.

  • myPlugin <- as plugin names go, not very original ;)

  • datatype is probably an unfortunate name, I would expect a function with that name to return a data type, also I would expect it to follow lowerCamelCase ( dataType )

  • It is risky to not check for jqGrid in self.options ? self.options.jqgrid : {},, I would have gone for (self.options && self.options.jqgrid) ? self.options.jqgrid : {},

  • Finally, I have to say that the last code block looks like a large amorphous blob, I would have much rather seen chained setters.

All in all, I am sure that you could have gotten more out of codereview if you had posted a bit more code.

|improve this answer|||||

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.