User "Catrope" posted a comment on MediaWiki.r93351.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20617
Commit summary:
AjaxCategories rewrite:
Solving syntax problems, performance improvements and applying code conventions:
* Replaced sprite image with separate images and letting ResourceLoader embed
them with @embed (@embed means 0 http requests, less maintenance, none of the
known limitations with sprites, and more readable code (named files rather than
pixel offsets)
* Many functions were floating in the global namespace (like
window.makeCaseInsensitive). A statement ends after a semi-colon(;). All
functions declared after "catUrl" were assigned to the window object. I've
instead turned the semi-colons back into comma's, merged some other var
statements and moved them to the top of the closure. Changed local function
declarations into function expressions for clarity.
* fetchSuggestions is called by $.fn.suggestions like ".call( $textbox,
$textbox.val() )". So the context (this) isn't the raw element but the jQuery
object, no need to re-construct with "$(this)" or "$(that)" which is slow and
shouldn't even work. jQuery methods can be called on it directly. I've also
replaced "$(this).val()" with the value-argument passed to fetchSuggestions
which has this exact value already.
* Adding more function documentation. And changing @since to 1.19 as this was
merged from js2-branch into 1.19-trunk and new features aren't backported to
1.18.
* Optimizing options/default construction to just "options = $.extend( {},
options )". Caching defaultOptions is cool, but doesn't really work if it's in
a context/instance local variable. Moved it up to the module closure var
statements, now it's static across all instances.
* In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that
fail in IE. Shortcuts (like '<foo>' and '<foo/>') are only allowed for
createElement triggers, not when creating longer fragments with content and/or
attributes which are created through innerHTML, in the latter case the HTML
must be completely valid and is not auto-corrected by IE.
* Using more jQuery chaining where possible.
* In buildRegex(): Using $.map with join( '|' ), (rather than $.each with +=
'|'; and substr).
* Storing the init instance of mw.ajaxCategories in mw.page for reference
(rather than local/anonymous).
* Applied some best practices and "write testable code"
** Moved some of the functions created on the fly and assigned to 'this' into
prototype (reference is cheaper)
** Making sure at least all 'do', 'set' and/or 'prototype' functions have a
return value. Even if it's just a simple boolean true or context/this for
chain-ability.
** Rewrote confirmEdit( .., .., .., ) as a prototyped method named
"doConfirmEdit" which takes a single props-object with named valuas as
argument, instead of list with 8 arguments.
* Removed trailing whitespace and other minor fixes to comply with the code
conventions.
** Removed space between function name and caller: "foo ()" => foo())
** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want
a Boolean here, not a Number.
** Renamed all underscore-variables to non-underscore variants.
== Bug fixes ==
* When adding a category that is not already on the page as-is but of which the
clean() version is already on the page, the script would fail. Fixed it by
moving the checks up in handleCategoryAdd() and making sure that
createCatLink() actually returned something.
* confirmEdit() wasn't working properly and had unused code (such as
submitButton), removed hidden prepending to #catlinks, no need to, it can be
dialog'ed directly from the jQuery object without being somewhere in the
document.
* in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input
field after adding a category, so that when another category is being added it
doesn't start with the previous value which is not allowed to be added again...
Comment:
Reviewing the entire file, adding comments here in lieu.
<pre>
if ( s !== undefined ) {
</pre>
Since <code>s.replace()</code> can fail even on things that aren't undefined,
shouldn't you really be checking for <code>typeof s == 'string'</code> ?
<pre>
+ title = page.title.split( ':', 2 )[1];
</pre>
Eww. This is kind of hackish and works only because all the returned titles are
in the category namespace. Would the overhead of using mw.Title be bad?
<pre>
+ * @param id
</pre>
Undocumented, as is pretty much everything else in that function BTW.
<pre>
text = text.replace( matches[i], id + i + '-' );
</pre>
This will do fun unexpected things if <code>id</code> happens to be a Number,
you may want to force it to a string. Also, why is there a dash at the end?
This whole function doesn't make much sense to me.
<pre>
* Makes regex string caseinsensitive.
</pre>
Sounds nice and generic. But then it does this:
<pre>
+ if ( $.inArray( 14, mw.config.get( 'wgCaseSensitiveNamespaces'
) ) !== -1 ) {
</pre>
which is not very generic and totally undocumented. Generally, the
documentation for this function is terse and would be clearer with an explicit
example.
<pre>
+ categoryNSFragment = $.map( mw.config.get( 'wgNamespaceIds' ),
function( id, name ) {
+ if ( id === catNsId ) {
+ return makeCaseInsensitive( $.escapeRE( name )
);
}
</pre>
This seems to rely on the fact that returning null from a <code>$.map()</code>
callback removes the element (which is documented but not immediately
intuitive), but what's worse is that what it really does is 1) rely on the fact
that not returning anything from a function will make it return undefined,
which is evil and 2) rely on the fact that <code>$.map()</code> will treat
undefined the same as null, which is undocumented as well. Please add an
explicit <code>return null</code> with a short comment explaining why you're
returning null (so <code>$.map()</code> will remove the element).
<pre>
+ categoryRegex = '\\[\\[(' + categoryNSFragment + '):' + '[ _]*'
+ titleFragment + '(\\|[^\\]]*)?\\]\\]';
</pre>
If you're accounting for spaces after the colon, account for spaces before the
colon too:
<pre>
> echo Title::newFromText('Category __ : _ Foo')->getPrefixedText();
Category:Foo
</pre>
<pre>
if ( matchLineBreak ) {
categoryRegex += '[ \\t\\r]*\\n?';
</pre>
So this could make the regex potentially match a bunch of spaces after the
link, but not a line break? That's confusing. Document this if it's intended,
or fix it if it's not.
<pre>
+ $button.addClass( 'icon-parent' ).append( $icon
).append( text );
</pre>
This means <code>text</code> is treated as HTML. Document if intended (or
rename the var), fix if not.
I only got down to line 180 or so before lunchtime, will continue after I eat.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
