Bug Tracker

#12229 closed feature (fixed)

Opened August 09, 2012 09:59AM UTC

Closed September 18, 2012 10:04PM UTC

Some inconsistencies/optimizations

Reported by: anonymous Owned by: gibson042
Priority: low Milestone: 1.8.2
Component: misc Version: 1.8rc1
Keywords: Cc:
Blocked by: Blocking:
Description

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.
Attachments (0)
Change History (11)

Changed August 09, 2012 10:07AM UTC by anonymous comment:1

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.

Changed August 09, 2012 02:10PM UTC by gibson042 comment:2

owner: → gibson042
priority: undecided → low
status: new → assigned

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

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

Changed August 09, 2012 02:21PM UTC by ajpiano comment:3

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

Changed August 10, 2012 05:31PM UTC by anonymous comment:4

@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!

Changed August 10, 2012 06:59PM UTC by gibson042 comment:5

Replying to [comment:4 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.

Changed August 11, 2012 09:46AM UTC by anonymous comment:6

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;.

Changed August 13, 2012 12:48AM UTC by gibson042 comment:7

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

Changed September 05, 2012 03:22AM UTC by mikesherov comment:8

component: unfiled → misc

Changed September 09, 2012 01:11AM UTC by dmethvin comment:9

type: enhancement → feature

Bulk change from enhancement to feature.

Changed September 14, 2012 04:13PM UTC by gibson042 comment:10

milestone: None → 1.8.2

Changed September 18, 2012 10:04PM UTC by Richard Gibson comment:11

resolution: → fixed
status: assigned → closed

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

Changeset: 15b5dbfe2386d67b8c1df3305812b35abc04458c