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) {
hubProxy.server.requestPage(postdata);
},
ColNames: ColNames,
colModel: ColModel,
pager: '#' + pager,
ondblClickRow: function (rowid) {
$(this).jqGrid('editGridRow', rowid, editTemplate);
}
});
1 Answer 1
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 dothis.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
inself.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.
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\$