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.
Post a Comment