CODEWITHSUNDEEP
rubyloops
SoSimple
SoSimple

Reputation: 701

Each loop not working as expected

I have an array called @results that is made up only of arrays. I want to iterate through @results and permanently delete any of the inner arrays that are smaller than a given size:

My code:

  def check_results limit
    @results.each_with_index do |result, index|
      @results.delete_at(index) if result.size < limit
    end
  end

Unfortunately, this only deletes the first item where the array length is less than limit. For example if limit = 4 and @results = [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1, 1], [1, 1]] then check_results returns [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1]]

I can't figure out why this is happening. Am I using the wrong loop?

Upvotes: 3

Views: 380

Answers (3)

user3373470
user3373470

Reputation:

As per the documentation, #delete_at returns the element at that index.

a = ["ant", "bat", "cat", "dog"]
a.delete_at(2)    #=> "cat"
a                 #=> ["ant", "bat", "dog"]
a.delete_at(99)   #=> nil

I added some debug statements to show you what is happening at each step, assuming limit is 4:

@results = [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1, 1], [1, 1]]
@results.each_with_index do |r, i|
  puts "RESULT: #{r.to_s}"
  puts "INDEX: #{i}"
  @results.delete_at(i) if r.size < 4
  puts "ARRAY: #{@results.to_s}"
end

RESULT: [1, 1, 1, 1]
INDEX: 0
ARRAY: [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1, 1], [1, 1]]
RESULT: [1, 1, 1, 1]
INDEX: 1
ARRAY: [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1, 1], [1, 1]]
RESULT: [1, 1, 1]
INDEX: 2
ARRAY: [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1]]
# @results == [[1, 1, 1, 1], [1, 1, 1, 1], [1, 1]]

As you can see, the element originally at index 2 has been removed. Because you are modifying @results while you are iterating through it, an index of 3 no longer exists, and an index of 2 has already been analyzed. This is why you should not modify an object while iterating through it.

Ideally, you want to use #delete_if. Similar to methods ending in !, #delete_if will modify the array (not return a copy of the result), based on conditions from a block (as an argument). The following would be how you would implement the method:

def check_results(limit)
  @results.delete_if { |arr| arr.length < limit }
end

@results = [ ['foo', 'bar'], ['bizz', 'bazz'], ['kaboom'] ]

check_results(2)
# => @results == [ ['foo', 'bar'], ['bizz', 'bazz'] ]

If you do not want to modify @results, then I suggest a similar method, #reject. Again, @results will not be modified, and instead a copy of the results will be returned.

def check_results(limit)
  @results.reject { |arr| arr.length < limit }
end

@results = [ ['foo', 'bar'], ['bizz', 'bazz'], ['kaboom'] ]

check_results(2)
# => [ ['foo', 'bar'], ['bizz', 'bazz'] ]
# => @results == [ ['foo', 'bar'], ['bizz', 'bazz'], ['kaboom'] ]

Upvotes: 2

Simone Carletti
Simone Carletti

Reputation: 176352

It's not a good idea to modify the @results array in place as that will conflict with the outer iteration.

What you should do instead is use select to build a new array.

def check_results(limit)
  @result.select { |result| result.size > limit }
end

Upvotes: 3

Wand Maker
Wand Maker

Reputation: 18762

You should do this, as delete_at modifies the array, and you will get unexpected behavior if you are deleting elements while iterating it

@results.reject { |i| i.size < limit }

Above code will exclude all array elements whose size is smaller than limit

Upvotes: 4

Related Questions