3

I have a model called Coupon, which can either be set to have a money_off or percent_off attributes (it can only have one set at a time).

Also depending on whether the Coupon is money_off or percent_off changes which methods are used.

Im wondering if i should be using Single table inheritance to eseentially sub class Coupon and have a sub class that deals with percent off coupons and another dealing with money off coupons?

I would then like to know how a user would be able to select this from the view.

Sam Mason
  • 1,037
  • 1
  • 8
  • 29
  • Well, it all depends. Inheritance is one way. [Injectable] strategies - another. Branching (`if coupon_type == :money_off`) all over the place - yet another. Hard to say what's better without knowing more about your app and its requirements. – Sergio Tulentsev Dec 21 '15 at 13:24
  • Thanks for that, I would lik to try and avoid branching if I can do keep my code clean. I have never heard of injectable strategies before. – Sam Mason Dec 21 '15 at 13:25
  • Any links to where I can find more info an "Injectable Strategies"? – Sam Mason Dec 21 '15 at 13:26
  • I just made up that term :) Let me see if I can whip up a quick answer that expands on that. – Sergio Tulentsev Dec 21 '15 at 13:26
  • Wouldn't say this calls for inheritance. What happens if you get another thing based on which to branch off? – ndnenkov Dec 21 '15 at 13:26
  • Thanks @ndn, my concern I would be if'ing lots in my model to figure out which type of discount the coupon was. – Sam Mason Dec 21 '15 at 13:32
  • Thanks for that @ndn, just so that Im getting the terminology correct would this be the right place to use a concern or maybe just a PORO? – Sam Mason Dec 21 '15 at 13:36

3 Answers3

4

Here's an example that illustrates the usage of strategies (about which Yam posted a more detailed answer):

class Coupon < Struct.new(:original_price, :amount_off, :type)
  def price_after_discount
    discount_strategy.call(self)
  end

  private

  def discount_strategy
    # note: no hardcoding here
    klass = type.to_s.camelize # :money_off to 'MoneyOff'
    "Coupon::#{klass}".constantize.new       
  end

  class MoneyOff
    def call(coupon)
      coupon.original_price - coupon.amount_off
    end
  end

  class PercentOff
    def call(coupon)
      coupon.original_price * (1.0 - coupon.amount_off / 100.0)
    end
  end
end  

Coupon.new(150, 10, :money_off).price_after_discount # => 140
Coupon.new(150, 10, :percent_off).price_after_discount # => 135.0

Now, instead of creating a strategy internally, we can accept it in constructor, thus making the strategy "injectable".

Sergio Tulentsev
  • 226,338
  • 43
  • 373
  • 367
3

What you can do is maintain the strategy internally, and provide methods such as price, discounted?, discounted_price. In addition, whether or not the admin chose to enter percentages or fixed units, you can still supply both methods: discount_pct, discount_units which would internally realize how to compute their return values.

This way the original class still supports the concept (same as the data model), yet is also flexible enough to allow various ways of providing it the necessary input. Whether you wish to show customers the pct off, or the fixed price units, you can do so independently of the admin's preferred method of input.

Even internal methods can use these abstractions. And if it turns out you're if/elsing all over the place internally, you can create nested classes for strategies and instantiate the right one once you get the record from the DB.

Yam Marcovic
  • 7,953
  • 1
  • 28
  • 38
  • If I wanted to check to see if a coupon was percent of money off, how could I check for that with a method. if .coupon_type is :money-off self.calc_money_off else self.calc_percent_off end Would something like that work? – Sam Mason Dec 21 '15 at 13:31
  • If you have a separate table for coupons, then the model for Coupon can have a type. As for the Items, you can have a `.coupon` method returning its associated coupon (or `nil`, or some default degenerate coupon), and the item's pricing methods could rely on the type of coupon it has (or not). Then you can still use `item.price`, `item.discounted_price`, and if you wanted to know the type, `item.coupon.type if item.discounted?`. – Yam Marcovic Dec 21 '15 at 13:36
  • Ah there is no Item model I will be performing the checks directly with the Coupon there wont be any associated models its not a typical shopping system – Sam Mason Dec 21 '15 at 13:39
3

The best way is to determine which functionality you require for each class. If you only need a small amount of changes, then stick to a single class with an enum:

#app/models/coupon.rb
class Coupon < ActiveRecord::Base
   enum type: [:percent, :money]

   def value
      if type.percent?
         # ...
      elsif type.money?
         # ...
      end
   end
end

This will allow you to use the type in your instance methods, which shouldn't cause such a problem if you didn't have a lot of changes to make within the class.

This would allow you to call:

@coupon = Coupon.find x
@coupon.value #-> returns value based on the type

--

The alternative (STI) would be more of a structured change, and would only work if you were referencing each class explicitly:

#app/models/coupon.rb
class Coupon < ActiveRecord::Base
end

#app/models/percent.rb
class Percent < Coupon
   def amount
      # ...
   end
end

#app/models/money.rb
class Money < Coupon
   def takeout
      # ...
   end
end

An important factor here is how you call these.

For the above classes, you have to reference the subclassed classes on their own:

@percentage_coupon = Percent.find x
@money_coupon      = Money.find y

This will obviously be more cumbersome, and may even cause problems with your routes & controllers etc.

.... so it may be best going with the single class :)

Richard Peck
  • 76,116
  • 9
  • 93
  • 147