Zepto.qsa & some other ternary operator changes #626

Closed
wants to merge 29 commits into
from

Conversation

Projects
None yet
3 participants
@shalecraig
Contributor

shalecraig commented Oct 25, 2012

I mentioned in #623 that I would submit a pull request that reduced the number of ternary operators that we use inside Zepto.

Here are two of these changes.

Uglify.js seems to be smart enough to inline if statements into ternary operators when they are used to build return statements, but it isn't smart enough to inline variables into ternary operators.

The code/style guide reflects these changes.

shalecraig added some commits Oct 25, 2012

Made this more readable. I messed around for a while with this
change, but this is the best I could do. It's not identical in the
minified source.
this one is a bit trickier. I saw an opportunity to minify the on/off…
… function, and went for it. Next commit will kill the ternary operators
I made three changes here that I've been making in tons of places.
We might be able to reduce the size of Zepto by making some of these
into a macro.
Removed another ternary operator inside src/zepto.js.
Uglify.js isn't very smart, and doesn't do variable inlining like
this may take advantage of.

That being said, I think it's worth the extra 10-15 bytes for more
readability and the ability to blame without fearing "death"
@shalecraig

This comment has been minimized.

Show comment
Hide comment
@shalecraig

shalecraig Oct 25, 2012

Contributor

Ok, I lied. It's more like 29 changes.

Overall, src/zepto.js (the default rake task) is reduced to 9.122K [gzipped].

Contributor

shalecraig commented Oct 25, 2012

Ok, I lied. It's more like 29 changes.

Overall, src/zepto.js (the default rake task) is reduced to 9.122K [gzipped].

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Oct 25, 2012

Collaborator

Wow that's a lot of changes.

I'll take a look more closely when I have time. For now, I see a lot of
incosistent style: some 1-line conditionals blocks you wrap in curly
braces, and some not (we avoid braces). You split a 1-line conditional
statement into 2 lines, even when it would fit on one line nicely (we
prefer that).

I'll cherry-pick your changes that I want, and leave the ones that replace
ternary operators a little too aggressively. I like one 1-line functions
that have a single short ternary operator inside. Nothing wrong with that
(I wouldn't call it abuse.)

Abuse is what we did in qsa

Collaborator

mislav commented Oct 25, 2012

Wow that's a lot of changes.

I'll take a look more closely when I have time. For now, I see a lot of
incosistent style: some 1-line conditionals blocks you wrap in curly
braces, and some not (we avoid braces). You split a 1-line conditional
statement into 2 lines, even when it would fit on one line nicely (we
prefer that).

I'll cherry-pick your changes that I want, and leave the ones that replace
ternary operators a little too aggressively. I like one 1-line functions
that have a single short ternary operator inside. Nothing wrong with that
(I wouldn't call it abuse.)

Abuse is what we did in qsa

@shalecraig

This comment has been minimized.

Show comment
Hide comment
@shalecraig

shalecraig Mar 12, 2013

Contributor

Looks like it wasn't looked at. Closing pull request.

Contributor

shalecraig commented Mar 12, 2013

Looks like it wasn't looked at. Closing pull request.

@shalecraig shalecraig closed this Mar 12, 2013

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Mar 12, 2013

Collaborator

That doesn't mean it won't be for the next release. We just didn't have time while baking the 1.0 to do so many code style & quality changes. For the same reason we also left speed optimizations for the future v1.1.

To save us some time, you chould rebase your changes to the latest master, and cleanup the history in this PR by squashing the "oops" commits into the previous commit they are fixing. Each commit should only describe 1 change to a particular piece of code.

Collaborator

mislav commented Mar 12, 2013

That doesn't mean it won't be for the next release. We just didn't have time while baking the 1.0 to do so many code style & quality changes. For the same reason we also left speed optimizations for the future v1.1.

To save us some time, you chould rebase your changes to the latest master, and cleanup the history in this PR by squashing the "oops" commits into the previous commit they are fixing. Each commit should only describe 1 change to a particular piece of code.

@mislav mislav reopened this Mar 12, 2013

@madrobby

This comment has been minimized.

Show comment
Hide comment
@madrobby

madrobby Sep 13, 2013

Owner

@shalecraig Feel free to reopen or submit a new pull request if you still want to get this in.

Owner

madrobby commented Sep 13, 2013

@shalecraig Feel free to reopen or submit a new pull request if you still want to get this in.

@madrobby madrobby closed this Sep 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment