-
Notifications
You must be signed in to change notification settings - Fork 379
compare inject block vs manual entries #105
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
Conversation
@@ -0,0 +1,19 @@ | |||
require "benchmark/ips" | |||
|
|||
User = Struct.new(:name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable
How about ARRAY.inject(:+)
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. must have been a copy paste from another enumerable one. oops
Whoops, didnt mean to unilaterally close it. My bad! |
5f15498
to
fd0f34a
Compare
Made fixes. Think @nateberkopec is right. that is covered. agreed to close? |
Just tested, seems like even when require "benchmark/ips"
ARRAY = (1..10_000_000).to_a
def fastest
ARRAY.inject(:+)
end
def fast
ARRAY.inject(&:+)
end
def slow
ARRAY.inject { |a, i| a + i }
end
Benchmark.ips do |x|
x.report('inject symbol') { fastest }
x.report('inject block') { fast }
x.report('inject sum') { slow }
x.compare!
end
So I think it's worth having a benchmark to show this—even though it's very specific and only applies to |
aah - symbol. forgot that one. Yea. there was a reason why I was visiting this code :) |
@Arcovion what version of ruby are you running? for that matter, what version of ruby should I be using for the readme? |
|
fd0f34a
to
17e494b
Compare
@Arcovion @nateberkopec Is this good for you? Any other changes? |
Thanks, LGTM. |
|
||
ARRAY = (1..1000).to_a | ||
|
||
def fastest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think naming the methods after their relative speed is a bit presumptuous. They're subject to change across runtimes and versions. I think it'd be better to name them after the type of operation being performed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. I was trying to follow the style guide but I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry. I see now that that could be read as an attack. I just meant the convention in general should change, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. we're good.
Someone had asked me about using the block short form vs manually entering the code.
Turns out, in 1.8.7, using the block was 1.7x slower, but since then, the symbol form is a little quicker.
Thanks for this repo