Ruby reverse.each_with_index and delete_at causing problems in latest Ruby/Rails

Question

So, I'm well aware of the perils of removing items in an iteration block (this is looping in reverse), and I know Matz mentioned something about mutations in iterations causing stability problems, but I can't seem to figure this one out.

This example is a little convoluted and I'm not sure even solving it will replicate the example exactly, but I'll have to try.

arr1 = [1, 2, 3, 4, 5]
arr2 = [3, 4, 5]
puts arr1.inspect
puts arr2.inspect
arr2.each do |i|
  arr1.reverse.each_with_index do |j, index|
    if i == j
      arr1.delete_at(index)
    end
  end
end
puts arr1.inspect
puts arr2.inspect

Outputs:

[1, 2, 3, 4, 5]
[3, 4, 5]
[4, 5]
[3, 4, 5]

when it should be:

[1, 2, 3, 4, 5]
[3, 4, 5]
[1, 2]
[3, 4, 5]

changing delete_at(index) to delete(j) fixes this, but didn't work when the array was objects. I'm also copying the objects to a temp array to make matters more complicated.

In my real life scenario, I have two arrays filled with Model objects of different type but share a common attribute (could probably use a join here, but I'm trying to avoid a special join table). What I want is to remove any objects in array1 that have a common attribute in array2. I've tried a number of different things with no solution... too many to put here.

@arr1 = []
original_arr1 = Model1.where(...)
original_arr1.each { |original| @arr1 << original.dup }
@arr2 = Model2.where(...)
@arr2.each do |object1|
  @arr1.reverse.each_with_index do |object2, index|
    if object1.color == object2.color
      @arr1.delete_at(version_index)
    end
  end
end

Without the extra copying above, the Model associations will remain and I will end up deleting the record from the table, which should not happen. It's just a temporary list. This seems like a stupid problem and I've spent way too much time on it.

Answer 1

You're deleting using the reversed index, but from the original array.

To get the "real" index, instead of one counting from the end of the array, you need to flip it around:

arr1.delete_at(-index - 1)

---

... but you should almost certainly be using reject! or delete_if instead:

require "set"

unwanted_colors = @arr2.map(&:color).to_set
@arr1.reject! { |el| unwanted_colors.include?(el.color) }

Answer 2

There are multiple solutions for your problem, an clean example is shown by @matthewds answer. However the two solutions below also resolve your issue.

First of I wanted to let you know that you can reduce the first few lines of code:

@arr1 = []
original_arr1 = Model1.where(...)
original_arr1.each { |original| @arr1 << original.dup }
# to
@arr1 = Model1.where(...).map(&:dup)
# but since you're not saving the Model1.where(...) result in a variable
# (enabling one to use them later), there is not need to dup at all
@arr1 = Model1.where(...)

The issue

The actual result you're getting back is correct. Here is why:

a1 = [1, 2]
a2 = [2]

a2.each { |n2| a1.reverse.each_with_index { |n1, i| a1.delete_at(i) if n2 == n1 }  }

# a1 = [1, 2]
# a2 = [2]
# iterate over a2
# n2 = 2
# create an new array with the reversed elements of a1
# ra1 = a1.reverse (eq [2, 1] and a1 is still [1, 2])
# iterate over ra1 with index
# n1 = 2, i = 0
# does n2 (2) equals n1 (2)? yes
# delete in a1 ([1, 2]) at the index i (0)
# resulting in a1 = [2]
# next iteration ra1
# n1 = 1, i = 1
# does n2 (2) equals n1 (1)? no
# ra1 iteration finishes
# a2 iteration finishes
# resulting in a1 = [2]

#1 Keeping your current code structure

If you want the most simple solution for your current code structure just removing the #reverse call should be enough. It seems there is no need to reverse the array anyway since you don't save the result, or use in in the #each_with_index code block.

#2 Only fetch the records that you need

This second solution solves the issue on database query level. If you don't want the records from Model1 with the same color as the colors in your current set, then don't fetch them from the database.

@arr2 = Model2.where(...)
@arr1 = Model1.where(...).where.not(color: @arr2.pluck(:color))

If color is not an attribute, but an association instance use :color_id instead.

Note: You can use .select(:color) instead of .pluck(:color). Using the select method will result in a subquery, however since you probably going to use @arr2 the records need to load in full anyway. Plucking the values from @arr2 and providing them as an plain colors instead of a subquery saves the database some work. If you're not going to use @arr2 any further I would use the select variant.

Answer 3

I did not test it with complex data structures, but maybe this could be another way.

a = [1, 2, 3, 4, 5]
b = [3, 4, 5]

p a+b-(a&b)
p a&b

# [1, 2]
# [3, 4, 5]

Which works also when:

a = [3, 4, 5]
b = [1, 2, 3, 4, 5]