Ruby .each with removal of items in collection

Question

I am currently working with an app that allows for runtime addition and removal of items in a drop down list via a rub script. The Ruby looks like this

SAFE = ;
return control if control.CyclesCount == 0;
control.Items.each{|item| control.Items.Remove(item) if item.Value.index('|').nil?};
return control;

control is custom user control and its Items is a ListItemCollction. I am running via unit tests to get my Ruby code correct and running into some trouble. The ListItemColletion I am passing in looks like this ..

var lic = new ListItemCOllection {
  new ListItem {Text = "Item2", Value = "8"}, 
  new ListItem {Text = "Item1", Value = "1"},
  new ListItem {Text = "Item3", Value = "3|1"},
  new ListItem {Text = "Item4", Value = "4"},
  new ListItem {Text = "Item5", Value = "5|2"},
  new ListItem {Text = "Item6", Value = "6"}
}

Instead of leaving the 2 items with the pipe in them, this code always seems to leave 3 items in the items collection. The 3 depend on the order that I put the items in (while in this order, Item1, Item3, Item5 get left)which leads me to believe that its the remove that is messed up. I have also tried to take a copy of the collection, loop through it, removing from the original so that I was not removing from the collection I was iterating through. I am a relatve noob to Ruby so go easy on me ... but I could use some advice.

Thanks

Answer 1

If you just want to remove items from an array based on a condition, you should use Array#reject!:

control.Items.reject! {|item| item.Value.index('|').nil? };

In order to properly debug this, however, we need to know what control.Items looks like on the Ruby end.

Answer 2

It is not advisable to change an array while iterating over it. There are some Iterators whose purpose is to change the array.

a= [1,2,3]
b= [1,2,3]
a.delete_if { |x| x == 2 } # => [1,3]
b.reject! { |x| x == 2 } # => [1,3]
a # => [1,3]
b # => [1,3]

Array#delete_if deletes elements of an array. There is only a minor difference to Array#reject

a= [1,2,3]
b= [1,2,3]
a.delete_if { |x| false } # => [1,3]
b.reject! { |x| false } # => nil
a # => [1,2,3]
b # => [1,2,3]

Array#delete_if always returns the remaining array. Array#reject! returns nil instead in case the array remains unchanged.

Some more modifiieng iterators which, do not change the original array:

a= [1,2,3]
a.reject { |x| x == 2 } # => [1,3]
a # => [1,2,3]

Array#reject returns an array without the rejected elements, but does not modify the original array.

a= [1,2,3]
a.select { |x| x != 2 } # => [1,2,3]
a # => [1,3]

Array#select returns an array of only the selected elements, but does not modify the original array.

Answer 3

Modifying a collection while you are iterating through it is never a good idea. If you do that, all hell breaks loose. (Preferably, it would raise some kind of exception, but that's life ...)

However, that's not actually the biggest problem in your code. The biggest problem is that you have commited the mortal sin of Ruby: not knowing Enumerable. (Don't worry: everybody commits that sin. All the time.)

If you want to reject all elements from a collection that satisfy a condition, there's a method for that, and it's called exactly what you would expect: Enumerable#reject!.

So, let's clean this up, shall we?

SAFE = ;

What's that semicolon doing there? It seems you got your C# and Ruby mixed up :-) (Oh, and also, that line doesn't do anything useful anyway, does it?)

return control if control.CyclesCount == 0;

Again, useless semicolon.

control.Items.each{|item| control.Items.Remove(item) if item.Value.index('|').nil?};

This is where it gets interesting:

control.Items.reject! {|item| item.Value.include?('|') }

Much better, isn't it?

return control;

I personally like to reserve the return keyword for "pure" methods (i.e. methods that don't have side-effects), so I wouldn't use one here since the code modifies control.Items but that is a style choice. Putting it all together, this is how I would write it:

return control if control.cycles_count == 0
control.items.reject! {|item| item.value.include?('|') }
control

Note: I don't have a working install of IronRuby right now, so I am making a few assumptions that I can unfortunately not test:

• method name transliteration (CyclesCount -> cycles_count) works,
• Value is some kind of String or collection and
• ListItemCollection mixes in Enumerable

The latter should be the case if ListItemCollection implements IEnumerable (otherwise I would consider that a bug in IronRuby). If ListItemCollection does not implement IEnumerable (which I would probably consider a bug in ListItemCollection), that is still easily fixed:

class ListItemCollection; include Enumerable end

[BTW: I would also introduce a cycles? method (or a bool HasCycles property on the .NET side), so that you can get rid of the cycle_count == 0 test.]