fast-ruby icon indicating copy to clipboard operation
fast-ruby copied to clipboard

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

Open Oppen opened this issue 6 years ago • 6 comments

Oppen avatar Feb 01 '19 02:02 Oppen

@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

ashmaroli avatar Feb 24 '19 10:02 ashmaroli

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 [email protected] 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 .

Oppen avatar Feb 24 '19 18:02 Oppen

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?

ashmaroli avatar Feb 24 '19 18:02 ashmaroli

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

El dom., 24 feb. 2019 15:11, Ashwin Maroli [email protected] 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 avatar Feb 24 '19 20:02 Oppen

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 ([email protected]) escribió:

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

El dom., 24 feb. 2019 15:11, Ashwin Maroli [email protected] 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 avatar Feb 24 '19 20:02 Oppen

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

ashmaroli avatar Feb 25 '19 06:02 ashmaroli