User "Dantman" posted a comment on MediaWiki.r90818.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/90818#c21003
Commit summary:
Review and clean up of jquery.tablesorter.js + applying code conventions
* Dropping redundant "new function(){}" construction, instead using the closure
we already have for the local scope.
* Adding more function documentation
* Whitespace contentions
* Passing jQuery object to the $.tablesorter.construct and creating jQuery
plugin function in $.fn.tablesorter (per jQuery convention, this allows
$.tablesorter.construct to be used directly without having to use .apply() to
get the this-context right).
* Making use of the arguments that jQuery.fn.each passes by default in
$.tablesorter.construct (this/that => table)
* Renaming a few single-letter variables to something more descriptive (c =>
$row, r => regex etc.)
* Changed order of local functions in order of calling and dependancy (fixed
JSHint potential implied globals). Also makes reading/understanding them easier.
* Removed commented out code
* Added more line breaks to separate blocks
* Merge var-statements together
* Declare 'ts' shortcut on top of the local scope, define 'ts' shortcut
directly after definition of $.tablesorter
* Adding public member placeholders, populated by private functions, to the
initial object instead of inside the functions. This way the members can be
seen centrally (dateRegex, monthNames)
* Syntax/JSHint fixes (Passes 100% now; Globals: mw, jQuery)
- 'list' used out of scope. In function buildParserCache: "var list" was
defined under an if-condition, but returned outside of it, this fails in the
'else' case. Moved var list outside the if-condition
- Strict === comparison to 0, null, undefined, false, true, '' etc.
- Require curly braces around all blocks
* Performance improvements (see also http://www.mediawiki.org/wiki/JSPERF)
- Using dot notation (obj.member = {}), instead of $.extend(obj, { member: ..
}) for single additions. No need for $.extend
- Object literal {foo}, instead of new function(){ this.foo }
- Strict/fast comparison to undefined, without typeof and/or string evaluation
- cacheRegexs() is uselses if if re-creates the cache every time. Using static
cache instead (which was likely the intention)
Comment:
...regexps using literal <code>/regexp/i</code> syntax are compiled around when
the script is parsed, unless you are using <code>new RegExp</code>, which you
should ONLY be using if you absolutely have to build a regexp dynamically by
concatenating strings together.
Of course <code>new RegExp(/regexp/i)</code> completely negates every built in
performance optimisation that browser vendors have worked into the language and
makes absolutely no sense.
...as a ES dev... I weep...
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
