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

Hash#fetch with argument vs Hash#fetch + block #39

Closed
mnarayan01 opened this issue May 20, 2015 · 5 comments

Comments

@mnarayan01
Copy link

commented May 20, 2015

This one seems a bit misleading. Simply switching the default to e.g. a symbol or constant-defined string makes both versions perform essentially identically (the argument version is actually slightly faster).

The difference in the current version is (AFAIK) exclusively due to the time taken to allocate the string (which is elided in the block version). This makes the displayed time comparison quite misleading (at least from my perspective), as the ratio can be increased/decreased at will be changing the runtime of the default expression.

@mnarayan01

This comment has been minimized.

Copy link
Author

commented May 20, 2015

Also I guess it's worth noting that if the fetch misses, the argument version is always faster.

@ixti

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2015

@mnarayan01 Difference in those two forms is that in case of:

obj.fetch("foo", "bar")

you are creating two String objects always, while in case of

obj.fetch("foo") { "bar" }

one String is created, and second one created only if Hash has no such key.

So, of course if your default value is a constant, block-less form is a pure win:

BAR = "bar".freeze

obj.fetch("foo", nil)
obj.fetch("foo", :bar)
obj.fetch("foo", BAR)

# etc...
@JuanitoFatas

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2015

@ixti Thanks for your explanations! 🎵

@mnarayan01 Thank you for bringining this! Does @ixti's reply clear your concerns? Do you have suggestions on improving current example code? Thanks! 🙇

@ixti

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2015

@JuanitoFatas yeah, I guess link to this issue is the simplest way :D Thanks!

@mnarayan01

This comment has been minimized.

Copy link
Author

commented Aug 13, 2015

Yea think it's perfect now. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.