Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: make variable names more descriptive #30

Closed
wants to merge 1 commit into from

Conversation

jordalgo
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Sep 22, 2016

Current coverage is 100% (diff: 100%)

Merging #30 into master will not change coverage

Powered by Codecov. Last update 38fbf44...90280bd

@Avaq
Copy link
Member

Avaq commented Sep 22, 2016

Hi @jordalgo, thank you for your contribution! 👍

I think this is a desirable change, in order to make it easier to contribute. However, one of my reasons for using short variable names, is to keep the bodies of functions below 517 characters (including indentation), which is the (seemingly arbitrary) limit that determines whether a function will be in-lined by v8 or not.

Many functions have already surpassed this limit anyway, so for those it no longer matters to keep the variable names short. For the functions where the longer variable names cause the length of the body to tipple over the limit however, we may need to reconsider: It makes quite a difference for users of the library.

I'm currently enjoying the stunning Norway nature, and I'm with a really slow internet connection. Once I'll get back I'll have a look at the function lengths and choice of variable names. :)

@jordalgo
Copy link
Author

@Avaq Thanks for the quick response while you are on vacation! (Github should really have a "maintainer out of office" feature 😄 ).

We can definitely discuss more once you are back but my thought on the subject is why not use a nice minifier like uglify if you're trying to gain the V8 performance benefits of small function bodies (it can also do the nice conditional and boolean optimizations for you). You can always supply a min or production version of Fluture in the package while keeping some really clear and understandable source code. (FYI: Uglify requires a transpile down to es5).

@Avaq
Copy link
Member

Avaq commented Oct 17, 2016

I think this PR is superseded by #34 - I named most of the variables logical names in the class implementations. Mind taking a look @jordalgo? :)

@Avaq
Copy link
Member

Avaq commented Oct 21, 2016

Do you think we can close this PR @jordalgo? Again, I appreciate the work, but a lot of it is superseded by #34. I also created a project on this repo for adding a build step in response to your remarks about uglify, and to support rollup tree-shaking.

@jordalgo
Copy link
Author

@Avaq Sure, I'll close. Are you working on that particular project or are you looking for some added resources :) ? Also, if there is going to be a build step to minify, how do you feel about removing some of the v8 optimizations you made in favor of more readable code?

@jordalgo jordalgo closed this Oct 21, 2016
@Avaq Avaq mentioned this pull request Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants