I am starting my studies with Ruby and OO and I have received a test to do about OO. I am looking for new ways to improve the scenario following object-oriented concepts. Is there a better way to develop this design applying polymorphism?

Here is my problem:

1. If the product is physical, I have to generate a shipping label.




2. If it's a book, I have to mention that this product doesn't have taxes.




3. If the product is a membership, I have to activate the signature and notify the buyer via email.




4. If the product is digital, I have to send an email for the buyer and give a $10 discount for this product.

class Product

  attr_reader :name, :description, :type, :amount

  include Shipping

  include Discount



    def initialize(name, description, type, amount)

        @name = name

        @description = description

        @type = type

        @amount = amount

    end 

end



class Physical < Product

    def initialize(name, description, type, amount)

        super(name, description, type, amount)

    end



    def shipping

        case @type

            when :book

                create_shipping_label

                notify_buyer_product_without_taxes

            else

                create_shipping_label

            end

    end



    def discount

        discount_for_physical_product = 0.0

      return discount_for_physical_product

    end

end



class Membership < Product

    attr_reader :membership_status



    def initialize(name, description, type, amount)

        super(name, description, type, amount)

    end



  def activate_membership()

    @membership_status = true;

  end



  def shipping

    notify_buyer_via_email

    activate_membership

  end



  def discount

    discount_for_membership_product = 0

    return discount_for_membership_product

  end

end



class Digital < Product



    def initialize(name, description, type, amount)

        super(name, description, type, amount)

    end



  def shipping

    notify_buyer_via_email

  end   



  def discount

    discount_for_digital_product = 10.00

    return discount_for_digital_product

  end

end



module Discount



    def prepare_discount(order)

        total_discount = 0

        order.get_items.each do |item|

             total_discount += item.product.discount

        end

        return total_discount

    end



    def discount

        discount_default = 0.0

        return discount_default

    end

end

You have a conditional based on the type of object. You can apply the Replace Conditional with Polymorphism refactoring pattern.

In this case, you might have a base class Product, with subclasses DigitalProduct and PhysicalProduct which each implement discount(). I don't think it's sensible to have a MembershipProduct class - membership should be related to the buyer. From an OO perspective, it should feel wrong when you have methods like discount_for_X() - the method should be part of the respective class so that you can make use of polymorphism.

Disclaimer: I'm not a Ruby expert

Type parameter

The main thing that bothers me with you code is the type parameter given when constructing a Product's instance. This goes against OO principles and leads to bad code for the following reason: you have to manage conditional explicitely in code with control structures (if, switch) instead of delegating this responsibility to the compiler/interpreter by using polymorphism.

Removing this would need to introduce a both Book class and a GenericProduct class which is basically representing all physical products but books.

Common discount method

Another thing is that the discount method is common to all products and differs only in the data (and not in the behavior). This can be implemented in Product and request the discount value in the constructor.

Create local variable and directly return it

This is useless and only introduce visual noise. You can directly return the value.

Similitude between Membership and Digital Product

It looks like Membership is also a special kind of digital product and that a relationship between them can exist (they both require to send an email when shipping). However I'm not entirely sure about that so feel free to ignore the end of the resulting code listing.

Reviewed code

class Product

  attr_reader :name, :description, :amount, :discount

  include Shipping

  include Discount



    def initialize(name, description, amount, discount)

        @name = name

        @description = description

        @amount = amount

        @discount = discount

    end 



    def discount

        return @discount

    end

end



class PhysicalProduct < Product

    def initialize(name, description, amount)

        super(name, description, amount, 0.0)

    end



    def shipping

        create_shipping_label

        notify_buyer_product_without_taxes if free_of_taxes

    end

end



class GenericProduct < PhysicalProduct

    def initialize(name, description, amount)

        super(name, description, amount)

    end



    def free_of_taxes

        return false

    end

end



class Book < PhysicalProduct

    def initialize(name, description, amount)

        super(name, description, amount)

    end



    def free_of_taxes

        return true

    end

end



class DigitalProduct < Product



    def initialize(name, description, amount, discount = 10.00)

        super(name, description, amount, discount)

    end



    def shipping

        notify_buyer_via_email

    end

end



class Membership < DigitalProduct

    attr_reader :membership_status



    def initialize(name, description, amount)

        super(name, description, amount, 0.0)

    end



    def activate_membership()

        @membership_status = true;

    end



    def shipping

        super

        activate_membership

    end

end