User "Catrope" changed the status of MediaWiki.r90818. Old Status: new New Status: ok
User "Catrope" also posted a comment on MediaWiki.r90818. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/90818#c21002 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: <pre> + new RegExp( /^\d{1,3}[\.]\d{1,3}[\.]\d{1,3}[\.]\d{1,3}$/) </pre> Why would you use <code>new RegExp()</code> here? You only need that for dynamic string input, otherwise you can just pass in the regex directly. <pre> + // Replace double slashes + s = s.replace( /\/\//g, '/' ); + s = s.replace( /\/\//g, '/' ); </pre> Why would you collapse double slashes twice? Did this mean to do <code>.replace( /\/\/+/g, '/' )</code> ? _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
