Some inconsistencies/optimizations

[anonymous]

Here are some optimizations and inconsistencies I spotted in 1.8RC1. They are low priority (no functional change) but may be worth looking at/fixing. Thanks!

• $.proxy: proxy.guid can be removed.
• $.trim: .toString is used here, but + "" is used everywhere else.
• $.camelCase and $.nodeName can be private, also makes for smaller gzip size.
• [].func used in several places; it could be cached. Also inconsistent uses between [].func and Array.prototype.func. In $.fn.domManip, a new array is even created on each call.
• ===/!== used almost everywhere (except for null), but some places where ==/!= is used for no specific reason:
  • $.event.dispatch (cur != this)
  • $.ajax (when setting crossDomain)
  • $.removeData (cache != cache.window)
  • $.fn.undelegate (arguments.length == 1), also no space before ?
• There is $.guid and $.uuid. These could perhaps be melted together as they are both used in a .guid++/.uuid++ fashion.
• core_pnum can be a string; a regexp with .source are unnecessary. Sizzle uses strings for partial regexps as well.
• $.fn.toArray = core_slice; would work equally well because the this value is inferred.
• indexOf is inconsistently used with > -1, >= 0 and ~.
• In the json jsonp prefilter, indexOf is used directly with a ! which may not be what the code is intended to do (?)
• In $.clean, the if(!elem) bailout can be placed before the number to string conversion as there is no number that evaluates to the empty string. This would make for an earlier bailout.
• In $.fn.toggle, the typeof check could be placed after the functions check as it's not used by the function check at all. Perhaps it could be replaced with arguments[0] instead, so that truthy/falsy values are allowed as well.

[anonymous]

Two corrections which I now realize:

For the first item of the list, I meant proxy.guid on the right hand side of the assignments. It is always undefined because it has never been set after the creation of proxy.

For the last item of the list, the typeof is an optimization, and the replacement a feature request.

[gibson042]

Some of the suggestions would be breaking, but many of them are good. Thanks!

​https://github.com/jquery/jquery/pull/887

[ajpiano]

Also, anonymous, now might be a good time to consider registering for Trac ;)

[anonymous]

@comment 2: Thanks for your time! I revisited my points and indeed some of them have side effects. However, I'm not sure why you didn't include some side-effect-less points (not trying to be rude; just curious to know the reasoning if you didn't include them on purpose).

• You missed the third != in $.ajax, I think (after the 80/443 part).
• The /source/.source really is redundant.
• cur != this can be !== because this is always an object in non-strict mode.
• The same goes for cache != cache.window.

I also spotted an additional redundancy: one of the $.fn.toggle functions has a check for two functions:

if ( jQuery.isFunction( state ) && jQuery.isFunction( fn2 ) ) {

However, this check is already done in a replacement function for $.fn.toggle:

( !i && jQuery.isFunction( speed ) && jQuery.isFunction( easing ) ) ?
cssFn.apply( this, arguments ) :

Instead of applying cssFn, it may rather directly go to eventToggle instead in this special case (it would require some additional code there, but then the first check can be removed altogether).

Thanks again for your time!

[gibson042]

Replying to anonymous:

• You missed the third != in $.ajax, I think (after the 80/443 part).

So I did. I've updated the pull request.

• The /source/.source really is redundant.

You are correct, but gzip compression is better against the regular expression than a backslash-heavy string, and we care very much about compressed size.

• cur != this can be !== because this is always an object in non-strict mode.
• The same goes for cache != cache.window.

I didn't touch either of those because I seem to remember oldIE having issues with strict comparison against windows.

I also spotted an additional redundancy: one of the $.fn.toggle functions has a check for two functions:

if ( jQuery.isFunction( state ) && jQuery.isFunction( fn2 ) ) {

However, this check is already done in a replacement function for $.fn.toggle:

( !i && jQuery.isFunction( speed ) && jQuery.isFunction( easing ) ) ?
cssFn.apply( this, arguments ) :

Instead of applying cssFn, it may rather directly go to eventToggle instead in this special case (it would require some additional code there, but then the first check can be removed altogether).

This method gets double duck-punched, and at least one definition is in an optional module. We're leaving this alone until 1.9/2.0, when the ​(handler, handler, ...) signature goes away.

Thanks again for your time!

No problem! I hope this helps.

[anonymous]

Thanks for the very clear explanation on each point!

Just one last thing, after that I will keep quiet, I promise :)

In $.fn.proxy, return undefined; can just be return;.

[gibson042]

That one I'm willing to leave because there's no size, clarity, or performance benefit to changing it.

[dmethvin]

Bulk change from enhancement to feature.

[Richard Gibson]

Fix #12229, size/consistency improvements. Close gh-887.

Changeset: 15b5dbfe2386d67b8c1df3305812b35abc04458c