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

Question

Hi everyone,

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

Something such as this, perhaps?

def has_just_one_kind_of_thing?(item, controller)
    return case controller
      when 'foos', 'bars'
        item.widgets.blank? || item.doohickeys.blank?
      when 'bazes'
        item.widgets.blank? || item.contraptions.blank?
      else 
        false
    end
  end

The outer return may not be necessary (not entirely sure what Ruby requires, I'm still fairly new to it myself), but I prefer leaving it in so the intent is obvious.

Answer 2

First, Ruby always returns the value of the last statement by default, so we can get rid of the if/else:

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

Now let's reformat for readability.

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

There, now it's a lot easier to see patterns in the algorithm. It looks like there are two questions being asked here: is the controller one of [foos|bars|bazes] and are either widgets or doohickeys blank. Let's factor out the first question:

def has_just_one_kind_of_thing?(item, controller)
  %w[foos bars bazes].include?(controller)  &&
    (item.widgets.blank? || item.doohickeys.blank?)
end

This gets the method down to a managable size. But I infer from the method name that you are looking for the case where either widgets or doohickeys has items, but not both and not neither. If that is so, an XOR might be more appropriate:

def has_just_one_kind_of_thing?(item, controller)
  %w[foos bars bazes].include?(controller)  &&
    (item.widgets.blank? ^ item.doohickeys.blank?)
end

Answer 3

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.

Answer 4

de Morgan's boolean logic theorem has two statements.

1. (A and B) is equiv to not(notA or notB)

not(A and B) is equiv to (notA or notB)

2. (A or B) is equiv to not(notA and notB)

not(A or B) is equiv to (notA and notB)

..

1. buy sandwich if it is (less than $2 and it is salmon) = do not buy sandwich if it is (not less than $2 or not salmon).

2. watch tv if (V is on or Avatar is on) = do not watch tv if (V is off and Avatar is off).

Further boolean algebra, among others,

1. (A and B) or (A and C) = A and (B or C)

2. (A or B) or C = A or B or C

Original logic:

  (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?)

Reduction
(foos and widgets) or (foos and hickeys) = foos and (widgets or hickeys):

  (
    (controller == 'foos' &&
      (item.widgets.blank? || item.doohickeys.blank?)
    ) || 

    (controller == 'bars' &&
      (item.widgets.blank? || item.doohickeys.blank?)
    ) || 

    (controller == 'bazes' &&
      (item.widgets.blank? || item.contraptions.blank?)
    )
  )

Reduction
(foos and items) or (bars and items) = (foos or bars) and items:

  (
    (controller == 'foos' || controller == 'bars') &&
    (item.widgets.blank? || item.doohickeys.blank?)
  ) ||

  (controller == 'bazes' &&
    (item.widgets.blank? || item.contraptions.blank?)
  )

The logic reduction has reduced to 3.5 lines from the original 6 lines. Even though, this exercise did not involve de Morgan's but plain boolean algebraic manipulation, de Morgan's is frequently applicable in other situations.

You might say, why should I have to go thro such trouble every time we write a bunch of logic statements? The reduced logic does not bear any semblance to the original and that is not good self-documenting code.

Exactly! The reduced logic let's you see it in simpler terms and the original bears no semblance to the simpler logic you should have had in the first place.