Refactoring case-when statement [duplicate]

Question

The Product model which can have 2 price tables. The method price calculates the price of the product based on product's price table which is defined by price_table field.

class Product < ActiveRecord::Model
  NORMAL_PRICE_TABLE = 1
  PROMOTIONAL_PRICE_TABLE = 2

  validates_presence_of :price_table

  def price(quantity)
    case price_table
    when NORMAL_PRICE_TABLE
      # calculate using normal price table rules
    when PROMOTIONAL_PRICE_TABLE
      # calculate using promotional price table rules
    else
      raise 'unknown data price'
    end
  end
end

I want to refactor this code. The cleaner way I get is this:

class NormalPriceTable

  def initialize(raw_product_price, quantity)
    @raw_product_price = raw_product_price
    @quantity = quantity
  end

  def price
    # calculate
  end    
end

class PromotionalPriceTable

  def initialize(raw_product_price, quantity)
    @raw_product_price = raw_product_price
    @quantity = quantity
  end

  def price
    # calculate
  end    
end

class Product < ActiveRecord::Model
  NORMAL_PRICE_TABLE = 1
  PROMOTIONAL_PRICE_TABLE = 2

  def price(quantity)
    price_table_class.new(raw_price, quantity).price
  end

  def price_table_class
    case price_table
    when NORMAL_PRICE_TABLE
      NormalPriceTable
    when PROMOTIONAL_PRICE_TABLE
      NormalPriceTable.new(raw_price, quantity).price
    else
      raise 'unknown price table'
    end
  end
end

Two questions:

• It's possible to get rid of this case clause on Product class?
• Which are the other alternatives to refactor this code?

Answer 1

I think in the very most cases (not all?) you can replace a case condition with a hash; at least in OOP. Because what else is the case condition than choosing the right value based on the input parameter? This is exactly what a map does: Based on a key it returns a value.

But what if some logic is necessary instead of a value? Well in this case the map-value is a function / static method or a object that implements that certain logic. For example if a new object is necessary each time that method is called, the map value can be a factory class that creates such object. In strongly typed programming languages all "map value objects" should implement the same interface or extend the same class.

In your case the Product class could look like this:

class AbstractPriceTable
  def price
    raise 'unknown price table'
  end
end

class Product
  NORMAL_PRICE_TABLE = 1
  PROMOTIONAL_PRICE_TABLE = 2

  PRICE_TABLE_CLASSES = {
    1 => NormalPriceTable,
    2 => PromotionalPriceTable }

  def price(quantity)
    (PRICE_TABLE_CLASSES[price_table] || AbstractPriceTable).
      new(raw_price, quantity).price
  end
end

With such a construct, case conditions would not be necessary anymore. The advantage of the map solution is the better extensibility and its more dynamic, as you could "register" more functions during runtime. If you don't want that or need a simple fixed structure, case conditions may still be the better choice; at least better than massive if..elsif..elsif..else..end constructs!

btw: I took the idea for this refactoring from this talk.

Answer 2

Replacing conditional with polymorphism (see Refactoring, Martin Fowler, page 255) is indeed the refactoring which comes into mind.

You've already did it partially, by:

• Creating classes, one for normal price, another for the promotional one.

• Moving the business logic to the newly created classes.

The remaining part is to:

• Create the hierarchy. It makes sense to derive both normal and promotional prices tables from an abstract PriceTable class. While PriceTable class won't contain any logic, it will at least provide the common interface for other classes which use the price tables.

• Determine the location of the separation between the normal and the promotional prices. You do it in Product class, but is it the best location? Or would it be better to inject the type of the price into the Product class? What about a Product class which doesn't care about the actual type of the price table, as soon as it corresponds to the interface of the abstract PriceTable class?

  This is an open question. It depends completely on your business logic and the remaining part of the program. It looks like Product shouldn't care about the actual type of the price table, as soon as it is derived from the abstract ProductTable, but this also means that somewhere, you'll still have to do your choice of the appropriate concrete type (either in a switch or through a map as suggested by rudi). Maybe in a factory class. Maybe somewhere else.