How can this 6 line method be refactored to be more readable?

Question

I'm trying to clean up this ridonkulously ugly method here, that's crying out for refactoring, but I'm not sure what kind of structure would do this best (i.e. a case statement, or simply a carefully formatted if then statements)

At first glance, it looks like it would be an ideal place for a case statement with a few well placed when's, but my understanding was that case statements can only be used to for a single variable, not two, and various fiddlings with irb to try these statements using hashes or arrays haven't shed much light here either.

How would you do this? Are there any common tricks in Ruby to avoid code like this when checking multiple booleans like this?

  def has_just_one_kind_of_thing?(item, controller)
    if (controller == 'foos' && item.widgets.blank?) || (controller == 'foos' && item.doohickeys.blank?) || (controller == 'bars' && item.widgets.blank?) || (controller == 'bars' && item.doohickeys.blank?) || (controller == 'bazes' && item.widgets.blank?) || (controller == 'bazes' && item.contraptions.blank?)
      return true
    else 
      return false
    end
  end

Answer 1

Guys - it seems there's a need for polymorphism here that just can't be solved by fiddling with the flow control within one method.

Here's my attempt starting with the expanded out version:

def has_just_one_kind_of_thing?(item, controller)
  (controller == 'foos' && item.widgets.blank?)    || 
  (controller == 'foos' && item.doohickeys.blank?) || 
  (controller == 'bars' && item.widgets.blank?)    || 
  (controller == 'bars' && item.doohickeys.blank?) || 
  (controller == 'bazes' && item.widgets.blank?)   || 
  (controller == 'bazes' && item.contraptions.blank?)
end

So there are three different behaviours - one per controller.... If each controller was smart enough to contain controller-specific decision making, and you pass in the actual controller not just a name of a controller you reduce the method to this:

def has_just_one_kind_of_thing?(item, controller)
  controller.has_just_one_kind_of_thing?(item)
end

This requires each controller to do the relevant item handling for the kind of controller it is. So let's define a method on each of foos, bars and bazes, called has_just_one_kind_of_thing?

Example for foos:

def has_just_one_kind_of_thing?(item)
   item.widgets.blank? || item.doohickeys.blank?
end

Example for bazes:

def has_just_one_kind_of_thing?(item)
   item.widgets.blank? || item.contraptions.blank?
end

On every controller that you want to return false on, just apply the "constant method" pattern:

def has_just_one_kind_of_thing?(item)
  false
end

This code will even run faster because now, we don't have to make as many checks as there are types of controllers - we just do one method dispatch against the controller.

So it's faster in interpreted ruby - and it might even go a lot faster in jruby or one of the other rubies that can heavily optimise method dispatching...

We could probably make it even smarter but I'd need to know on which class this method lives and maybe a few other things about item.

The alternative refactoring would have been to make item smart and have different methods on each type of item. Again, we'd need to know more about the object model to tell which is best...

Still it's a first cut.