Bug Tracker

Modify

Ticket #12229 (closed feature: fixed)

Opened 21 months ago

Last modified 19 months ago

Some inconsistencies/optimizations

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

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.

Change History

comment:1 Changed 21 months ago by 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.

comment:2 Changed 21 months ago by gibson042

  • Owner set to gibson042
  • Priority changed from undecided to low
  • Status changed from new to assigned

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

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

comment:3 Changed 21 months ago by ajpiano

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

comment:4 follow-up: ↓ 5 Changed 21 months ago by 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!

comment:5 in reply to: ↑ 4 Changed 21 months ago by 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.

comment:6 Changed 21 months ago by 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;.

comment:7 Changed 21 months ago by gibson042

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

comment:8 Changed 20 months ago by mikesherov

  • Component changed from unfiled to misc

comment:9 Changed 20 months ago by dmethvin

  • Type changed from enhancement to feature

Bulk change from enhancement to feature.

comment:10 Changed 20 months ago by gibson042

  • Milestone changed from None to 1.8.2

comment:11 Changed 19 months ago by Richard Gibson

  • Status changed from assigned to closed
  • Resolution set to fixed

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

Changeset: 15b5dbfe2386d67b8c1df3305812b35abc04458c

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.