Opened 10 years ago
Closed 10 years ago
#12229 closed feature (fixed)
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
andArray.prototype.func
. In$.fn.domManip
, a new array is even created on each call.===
/!==
used almost everywhere (except fornull
), but some places where==
/!=
is used for no specific reason:$.event.dispatch
(cur != this
)$.ajax
(when settingcrossDomain
)$.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 thethis
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
, theif(!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
, thetypeof
check could be placed after the functions check as it's not used by the function check at all. Perhaps it could be replaced witharguments[0]
instead, so that truthy/falsy values are allowed as well.
Change History (11)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Owner: | set to gibson042 |
---|---|
Priority: | undecided → low |
Status: | new → assigned |
Some of the suggestions would be breaking, but many of them are good. Thanks!
comment:3 Changed 10 years ago by
Also, anonymous, now might be a good time to consider registering for Trac ;)
comment:4 follow-up: 5 Changed 10 years ago by
@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 the80
/443
part). - The
/source/.source
really is redundant. cur != this
can be!==
becausethis
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 Changed 10 years ago by
Replying to anonymous:
- You missed the third
!=
in$.ajax
, I think (after the80
/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!==
becausethis
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 window
s.
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 toeventToggle
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 10 years ago by
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 10 years ago by
That one I'm willing to leave because there's no size, clarity, or performance benefit to changing it.
comment:8 Changed 10 years ago by
Component: | unfiled → misc |
---|
comment:9 Changed 10 years ago by
Type: | enhancement → feature |
---|
Bulk change from enhancement to feature.
comment:10 Changed 10 years ago by
Milestone: | None → 1.8.2 |
---|
comment:11 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fix #12229, size/consistency improvements. Close gh-887.
Changeset: 15b5dbfe2386d67b8c1df3305812b35abc04458c
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 alwaysundefined
because it has never been set after the creation ofproxy
.For the last item of the list, the
typeof
is an optimization, and the replacement a feature request.