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

Add `Array#extract!` #33137

Merged
merged 3 commits into from Aug 14, 2018

Conversation

Projects
None yet
7 participants
@bogdanvlviv
Contributor

bogdanvlviv commented Jun 14, 2018

The method removes and returns the elements for which the block returns a true value.
If no block is given, an Enumerator is returned instead.

numbers = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
odd_numbers = numbers.extract! { |number| number.odd? } # => [1, 3, 5, 7, 9]
numbers # => [0, 2, 4, 6, 8]

Also, I added commit Use Array#extract! where possible in order to show that we can improve Rails code base by this method.

- nils, values = values.partition(&:nil?)
- ranges, values = values.partition { |v| v.is_a?(Range) }
+ nils = values.extract!(&:nil?)
+ ranges = values.extract! { |v| v.is_a?(Range) }

Looks more readable if use Array#extract!, doesn't it?

I know we try to omit adding new methods to ActiveSupport, but I think this one has a good chance to be added since we can apply it in Rails code base.
By the way, we already added Hash#extract!
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/hash/slice.rb#L41-L47

Thanks!

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Jun 14, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Jun 14, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 14, 2018

Member

I'm ok with this one. @dhh what do you think of this name?

Member

rafaelfranca commented Jun 14, 2018

I'm ok with this one. @dhh what do you think of this name?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 14, 2018

Member
Member

dhh commented Jun 14, 2018

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Jun 15, 2018

Member

I honestly thought that this is what select! does, but apparently not. So, 👍from me too.

Member

sikachu commented Jun 15, 2018

I honestly thought that this is what select! does, but apparently not. So, 👍from me too.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jun 15, 2018

Member

FWIW, it would be named extract upstream... but as we already have Hash#extract!: 🤷🏻‍♂️

Also, I added commit Use Array#extract! where possible in order to show that we can improve Rails code base by this method.

💯. This was very helpful, thank you.

On the implementation: I'd like to avoid allocating the second array. What do you think of something like reject! { |el| extracted_elements << el if yield el } ?

Let's also avoid unless/else in favour of an early return; the double-negative of that else can be confusing, even though the code layout is nearly the same.

Member

matthewd commented Jun 15, 2018

FWIW, it would be named extract upstream... but as we already have Hash#extract!: 🤷🏻‍♂️

Also, I added commit Use Array#extract! where possible in order to show that we can improve Rails code base by this method.

💯. This was very helpful, thank you.

On the implementation: I'd like to avoid allocating the second array. What do you think of something like reject! { |el| extracted_elements << el if yield el } ?

Let's also avoid unless/else in favour of an early return; the double-negative of that else can be confusing, even though the code layout is nearly the same.

@bogdanvlviv

This comment has been minimized.

Show comment
Hide comment
@bogdanvlviv

bogdanvlviv Jun 15, 2018

Contributor

Thanks for the review guys!

@matthewd I applied your suggestions in Refactor Array#extract!.

On the implementation: I'd like to avoid allocating the second array. What do you think of something like reject! { |el| extracted_elements << el if yield el } ?

I prepared benchmarks in order to ensure that the changes speed up the method: (Not sure that these benchmarks are completely right)

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update
your Bundler"
  raise e
end

class Array
  def extract_v1!(&block)
    unless block_given?
      to_enum(:extract!) { size }
    else
      extracted_elements, other_elements = partition(&block)

      replace(other_elements)

      extracted_elements
    end
  end

  def extract_v2!
    return to_enum(:extract!) { size } unless block_given?

    extracted_elements = []

    reject! do |element|
      extracted_elements << element if yield(element)
    end

    extracted_elements
  end
end

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
end

arrays_for_partition = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v1 = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v2 = Array.new(1000) { (0..10000).to_a }

Benchmark.ips do |x|
  x.report("Array#partition")  do
    arrays_for_partition.each do |numbers|
      odd_numbers, numbers = numbers.partition { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v1!")  do
    arrays_for_extract_v1.each do |numbers|
      odd_numbers = numbers.extract_v1! { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v2!")  do
    arrays_for_extract_v2.each do |numbers|
      odd_numbers = numbers.extract_v2! { |number| number.odd? }
      numbers
    end
  end

  x.compare!
end

The result of the benchmarks:

ruby -v
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using benchmark-ips 2.7.2
Using bundler 1.16.1
Warming up --------------------------------------
     Array#partition     1.000  i/100ms
   Array#extract_v1!     1.000  i/100ms
   Array#extract_v2!     1.000  i/100ms
Calculating -------------------------------------
     Array#partition      1.390  (± 0.0%) i/s -      7.000  in   5.044843s
   Array#extract_v1!      2.781  (± 0.0%) i/s -     14.000  in   5.050589s
   Array#extract_v2!      3.151  (± 0.0%) i/s -     16.000  in   5.080608s

Comparison:
   Array#extract_v2!:        3.2 i/s
   Array#extract_v1!:        2.8 i/s - 1.13x  slower
     Array#partition:        1.4 i/s - 2.27x  slower

Let's also avoid unless/else in favour of an early return; the double-negative of that else can be confusing, even though the code layout is nearly the same.

Yeah, It will be more readable and also using of early return would improve git diff
in some cases if we needed to change this method I think.

Contributor

bogdanvlviv commented Jun 15, 2018

Thanks for the review guys!

@matthewd I applied your suggestions in Refactor Array#extract!.

On the implementation: I'd like to avoid allocating the second array. What do you think of something like reject! { |el| extracted_elements << el if yield el } ?

I prepared benchmarks in order to ensure that the changes speed up the method: (Not sure that these benchmarks are completely right)

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update
your Bundler"
  raise e
end

class Array
  def extract_v1!(&block)
    unless block_given?
      to_enum(:extract!) { size }
    else
      extracted_elements, other_elements = partition(&block)

      replace(other_elements)

      extracted_elements
    end
  end

  def extract_v2!
    return to_enum(:extract!) { size } unless block_given?

    extracted_elements = []

    reject! do |element|
      extracted_elements << element if yield(element)
    end

    extracted_elements
  end
end

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
end

arrays_for_partition = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v1 = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v2 = Array.new(1000) { (0..10000).to_a }

Benchmark.ips do |x|
  x.report("Array#partition")  do
    arrays_for_partition.each do |numbers|
      odd_numbers, numbers = numbers.partition { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v1!")  do
    arrays_for_extract_v1.each do |numbers|
      odd_numbers = numbers.extract_v1! { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v2!")  do
    arrays_for_extract_v2.each do |numbers|
      odd_numbers = numbers.extract_v2! { |number| number.odd? }
      numbers
    end
  end

  x.compare!
end

The result of the benchmarks:

ruby -v
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using benchmark-ips 2.7.2
Using bundler 1.16.1
Warming up --------------------------------------
     Array#partition     1.000  i/100ms
   Array#extract_v1!     1.000  i/100ms
   Array#extract_v2!     1.000  i/100ms
Calculating -------------------------------------
     Array#partition      1.390  (± 0.0%) i/s -      7.000  in   5.044843s
   Array#extract_v1!      2.781  (± 0.0%) i/s -     14.000  in   5.050589s
   Array#extract_v2!      3.151  (± 0.0%) i/s -     16.000  in   5.080608s

Comparison:
   Array#extract_v2!:        3.2 i/s
   Array#extract_v1!:        2.8 i/s - 1.13x  slower
     Array#partition:        1.4 i/s - 2.27x  slower

Let's also avoid unless/else in favour of an early return; the double-negative of that else can be confusing, even though the code layout is nearly the same.

Yeah, It will be more readable and also using of early return would improve git diff
in some cases if we needed to change this method I think.

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Jun 22, 2018

Member

I rerun the test case (twice, in the span of 6 days ..ha) and it is now passing. @matthewd @rafaelfranca would either of you mind merging this in if this is good to go?

Member

sikachu commented Jun 22, 2018

I rerun the test case (twice, in the span of 6 days ..ha) and it is now passing. @matthewd @rafaelfranca would either of you mind merging this in if this is good to go?

bogdanvlviv added some commits Jun 14, 2018

Add `Array#extract!`
The method removes and returns the elements for which the block returns a true value.
If no block is given, an Enumerator is returned instead.

```
numbers = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
odd_numbers = numbers.extract! { |number| number.odd? } # => [1, 3, 5, 7, 9]
numbers # => [0, 2, 4, 6, 8]
```
Refactor `Array#extract!`
Avoid allocating the second array by using `Array#reject!` instead of
`Enumerable#partition` in `Array#extract!`.

There are benchmarks in order to ensure that the changes speed up the method:
```
begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update
your Bundler"
  raise e
end

class Array
  def extract_v1!(&block)
    unless block_given?
      to_enum(:extract!) { size }
    else
      extracted_elements, other_elements = partition(&block)

      replace(other_elements)

      extracted_elements
    end
  end

  def extract_v2!
    return to_enum(:extract!) { size } unless block_given?

    extracted_elements = []

    reject! do |element|
      extracted_elements << element if yield(element)
    end

    extracted_elements
  end
end

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
end

arrays_for_partition = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v1 = Array.new(1000) { (0..10000).to_a }
arrays_for_extract_v2 = Array.new(1000) { (0..10000).to_a }

Benchmark.ips do |x|
  x.report("Array#partition")  do
    arrays_for_partition.each do |numbers|
      odd_numbers, numbers = numbers.partition { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v1!")  do
    arrays_for_extract_v1.each do |numbers|
      odd_numbers = numbers.extract_v1! { |number| number.odd? }
      numbers
    end
  end

  x.report("Array#extract_v2!")  do
    arrays_for_extract_v2.each do |numbers|
      odd_numbers = numbers.extract_v2! { |number| number.odd? }
      numbers
    end
  end

  x.compare!
end
```

The result of the benchmarks:

```
ruby -v
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
```

```
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using benchmark-ips 2.7.2
Using bundler 1.16.1
Warming up --------------------------------------
     Array#partition     1.000  i/100ms
   Array#extract_v1!     1.000  i/100ms
   Array#extract_v2!     1.000  i/100ms
Calculating -------------------------------------
     Array#partition      1.390  (± 0.0%) i/s -      7.000  in   5.044843s
   Array#extract_v1!      2.781  (± 0.0%) i/s -     14.000  in   5.050589s
   Array#extract_v2!      3.151  (± 0.0%) i/s -     16.000  in   5.080608s

Comparison:
   Array#extract_v2!:        3.2 i/s
   Array#extract_v1!:        2.8 i/s - 1.13x  slower
     Array#partition:        1.4 i/s - 2.27x  slower
```

Avoid `unless`/`else` in favour of an early return.
The double-negative of that `else` can be confusing,
even though the code layout is nearly the same.
Also using of early return would improve `git diff`
if we needed to change this method.

@matthewd matthewd merged commit 7fa2f53 into rails:master Aug 14, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:add-array-extract-method branch Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment