fastruby / fast-ruby

:dash: Writing Fast Ruby :heart_eyes: -- Collect Common Ruby idioms.
https://github.com/fastruby/fast-ruby
5.67k stars 376 forks source link

Add Array#reverse.first vs Array#last.reverse and vice-versa. #168

Closed Oppen closed 1 year ago

ashmaroli commented 5 years ago

@Oppen You've got the methods reversed in the benchmark script. The label says one thing but the method does the opposite. Please correct!

Also, it would be clearer to others if the labels stated that this relates to capturing multiple elements. e.g. 'Array#last(n > 1).reverse' instead of 'Array#last.reverse' The latter label implies that you're calling .reverse on the last item of the Array

Oppen commented 5 years ago

I'll fix the benchmark when I have time. While it's true the label could be more informative, note it actually applies to the last element, too, so it would be n >= 1.

El dom., 24 feb. 2019 07:52, Ashwin Maroli notifications@github.com escribió:

@Oppen https://github.com/Oppen You've got the methods reversed in the benchmark script. The label says one thing but the method does the opposite. Please correct!

Also, it would be clearer to others if the labels stated that this relates to capturing multiple elements. e.g. 'Array#last(n > 1).reverse' instead of 'Array#last.reverse' The latter label implies that you're calling .reverse on the last item of the Array

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuanitoFatas/fast-ruby/pull/168#issuecomment-466762904, or mute the thread https://github.com/notifications/unsubscribe-auth/AA45Z86ozdk2_AGOQwWJmzk_q0MW_Gjvks5vQm7UgaJpZM4adhYV .

ashmaroli commented 5 years ago

note it actually applies to the last element

@Oppen Consider the following:

array = %w(alpha beta gamma delta)
p array.last.reverse
p array.reverse.first

Are they the same?

array = %w(alpha beta gamma delta)
p array.last(2).reverse
p array.reverse.first(2)

Now are they the same?

Oppen commented 5 years ago

Right. I didn't realize the types would change. Thanks!

El dom., 24 feb. 2019 15:11, Ashwin Maroli notifications@github.com escribió:

note it actually applies to the last element

@Oppen https://github.com/Oppen Consider the following:

array = %w(alpha beta gamma delta)p array.last.reversep array.reverse.first

Are they the same?

array = %w(alpha beta gamma delta)p array.last(2).reversep array.reverse.first(2)

Now are they the same?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuanitoFatas/fast-ruby/pull/168#issuecomment-466801125, or mute the thread https://github.com/notifications/unsubscribe-auth/AA45ZyAGiM274EsPEYw12hkCr9ipzmdQks5vQtXrgaJpZM4adhYV .

Oppen commented 5 years ago

However, I run a simpler test: ["alpha"].first(1) And the result was ["alpha"], so it actually is a matter of whether you pass a parameter or not, not about the number of elements. I'm not sure how I should express that in the labels, but that's useful mostly because n may come as a parameter to the caller.

El dom., 24 feb. 2019 a las 17:29, Mario Rugiero (mrugiero@gmail.com) escribió:

Right. I didn't realize the types would change. Thanks!

El dom., 24 feb. 2019 15:11, Ashwin Maroli notifications@github.com escribió:

note it actually applies to the last element

@Oppen https://github.com/Oppen Consider the following:

array = %w(alpha beta gamma delta)p array.last.reversep array.reverse.first

Are they the same?

array = %w(alpha beta gamma delta)p array.last(2).reversep array.reverse.first(2)

Now are they the same?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuanitoFatas/fast-ruby/pull/168#issuecomment-466801125, or mute the thread https://github.com/notifications/unsubscribe-auth/AA45ZyAGiM274EsPEYw12hkCr9ipzmdQks5vQtXrgaJpZM4adhYV .

ashmaroli commented 5 years ago

I'm not sure how I should express that in the labels

I think taking a cue from your sample method is sufficient. Either ways, thank you for reporting this.

Benchmark.ips do |x|
  x.report('Array.last(n).reverse')  { ARRAY.last(5).reverse }
  x.report('Array.reverse.first(n)') { ARRAY.reverse.first(5) }
  x.compare!
end