User "Catrope" posted a comment on MediaWiki.r90818.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/90818#c20920
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:
Aaargh, please don't move functions and change them in the same commit, because
it produces a diff that's horrendously difficult to review. When you commit
something, please consider what the resulting diff would look like and split
things up in multiple commits if that's needed to preserve the sanity of your
reviewers.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
