Friday, February 13, 2015

Ruby Case Statements and `kind_of?`

You’re an object – Stand up straight and act like one!

Imagine you have this code:
Code 1:
class MyView
  attr_reader :target
  
  def initialize(target)
    @target = target
  end

  def double
    case target
      when Numeric  then target * 2 
      when String   then target.next    # lazy example fail
      when Array    then target.collect {|o| MyView.new(o).double}
      else
        raise “don’t know how to double #{target.class} #{target.inspect}”
    end
  end
end
It does just what you’d expect.
Output 1:
>> MyView.new(‘aaa’).double
=> "aab"
>> MyView.new(49).double
=> 98
>> MyView.new([‘b’, 6]).double
=> ["c", 12]
>> MyView.new({‘x’=>‘y’}).double
RuntimeError: don’t know how to double Hash {"x"=>"y"}
        from (irb):73:in `double'
        from (irb):80
        from :0
You’re probably familiar with this pattern. Its everywhere in Rails and you likely use it in your own code.
I want to say, in the nicest possible way, that this style of code is wrong, wrong, wrong and that you should do a different thing.
Okay, now that I have your attention, I’m not trying to start a fight. I’m not the best Ruby person around and I’m definitely not the best OO designer, but I do have an alternative pattern to suggest.
I’m aiming to start a discussion, not a religious war. Strong opinions are welcome.
What’s happening up there?
MyView needs to operate on several other objects. It knows:
  • the classes of all the objects that it can interact with, and
  • the behavior that should occur for each of those classes.
The case statement above is really an if statement that checks ‘kind_of?’ on each collaborating object.
I object to this code because:
  • use of kind_of? is a code smell that says your code is procedural, not object oriented, and
  • if you write procedural code your application will gradually become impossible to change and everyone will hate you.
Why is it wrong?
If I change how double works on any of these classes, MyView must change, but that’s not the real problem. What happens if MyView wants to double some new kind of object? I have to go into MyView and add a new branch to the case statement. How annoying is that?
But that’s the least of it. If I’m writing code that follows this pattern, I likely have many classes that do stuff based on the classes of their collaborators. My entire application behaves this way. Every time I add a new collaborating object I have to go change code everywhere. Each subsequent change makes things worse. My application is a teetering house of cards and eventually it will come tumbling down.
Also, what if someone else wants to use MyView with their new SuperDuper object? They can’t reuse MyView without changing it since MyView has a very limited notion of what kinds of objects can be doubled.
MyView is both rigid and closed for extension.
What should I have done instead?
Something like this.

No comments:

Post a Comment

Ruby Case Statements and `kind_of?`

You’re an object – Stand up straight and act like one! Imagine you have this code: Code 1: class MyView attr_reader :target ...