Collections, each and the violation of encapsulation

Every so often I come across a bit of code which looks something like this:
def upcase_collection(collection)
array = []
collection.each{|item| array << item.upcase}

Yeah, I know you language punters are looking at this and going 'yech'; but folks new to blocks and used to for loops aren't likely see the problem.

Here's the deal: when using blocks with collections in Ruby, always respect the sanctity of a block passed to a collection. Never misuse the closure created with the block to manipulate objects outside the scope of the block - in this example using array as an accumulator. There are always better ways to do this without violating encapsulation, the other collection methods available in Enumerable being a case in point.

There are a couple of smells you always come across in such situations:
  • The presence of objects whose states are changed from within a block in a different scope
  • The fact that you need a temporary variable at all in the first place
  • The availability of methods like collect, partition or inject which do exactly what you need - for free.

The (trivial) example I've given would be better solved using collect.


Anonymous said...

For those who aren't familiar with collect, you'd reimplement like this:

def lcase_collection(colllection)
collection.collect do | item |

Full documentation can be found here:

Anonymous said...

It really looks like code that my indian co"workers" wright. How about not passing anything and just do something like this?

>> arr = ["a","b","c"]
>> arr.each{|v| v.upcase!}


>> class Array; def upcase!; each{|val| val.upcase!};end;end
=> nil
>> arr = ["a","b","c"]
=> ["a", "b", "c"]
>> arr.upcase!
=> ["A", "B", "C"]
>> arr
=> ["A", "B", "C"]