def upcase_collection(collection)
array = []
collection.each{|item| array << item.upcase}
array
end
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
orinject
which do exactly what you need - for free.
The (trivial) example I've given would be better solved using
collect
.
2 comments:
For those who aren't familiar with collect, you'd reimplement like this:
def lcase_collection(colllection)
collection.collect do | item |
item.lcase
end
end
Full documentation can be found here:
http://www.ruby-doc.org/core/classes/Array.html#M002210
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!}
Or:
>> 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"]
Post a Comment