9c5f7685501ec87178055f41147c75c5

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.

I'm interested in making this as readable as possible, rather than as brief as possible.

Any ideas?

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

Refactorings

No refactoring yet !

Bffc8a5d9d659b4e65c0e7e93b624ef3

Vamsee

November 23, 2009, November 23, 2009 02:04, permalink

1 rating. Login to rate!

How about this?

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

chrisadams.me.uk

November 23, 2009, November 23, 2009 02:52, permalink

No rating. Login to rate!

Thanks Vamsee, that's much, much more readable, and a variant of this approach is what I went with in the end after [posting this on stackoverflow][1] too, to see how the submissions on each site would
differ.

The most popular refactor over there, made by [Donnie][2] is very similar to yours. I've pasted it here too to show how similar they are.

[1]: http://stackoverflow.com/questions/1780672/how-can-this-6-line-method-be-refactored-to-be-more-readable"
[2]: http://stackoverflow.com/users/215568/donnie

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
880cbab435f00197613c9cc2065b4f5a

danielharan

November 23, 2009, November 23, 2009 04:42, permalink

No rating. Login to rate!

The people on StackOverflow should have voted for cartoonfox, who's closer to a better solution.

def has_only_widgets_or_doohickeys?
  @item.widgets.blank? || @item.doohickeys.blank?
end

class FoosController
  before_filter :load_item
  before_filter :has_only_widgets_or_doohickeys?
end

Your refactoring





Format Copy from initial code

or Cancel