Clean object oriented code

Question

I'm trying to be a better developer and I always ask myself If there is a better way to do the things. Is not the first time that I have to deal with this problem so I decided to ask what you think about it.

Let's say I have to implement a class that represent a product.

class Product 
  def initialize (name, net_price)
    @name = name 
    @net_price = net_price
    @gross_price = nil
  end

  def set_gross_price
    @gross_price = heavy_gross_price_calculation
  end

  def export
    @gross_price.nil? && set_gross_price
    return product.to_hash
  end

  def heavy_gross_price_calculation
    #  This function calculate the gross price but let's say that this is 
    #  pretty onerous operation that involves maybe also an external API
    #  request
  end
end

Let's say that the work flow for this class is create a product, calculate the gross price and export it for future use. Is it correct to not call the set_gross_price method in the initialize? The fact is that when you export a product the gross price have to be calculated but I don't think that the correct choise is to force the developer to call set_gross_price before export but also i'm not sure about the first line of the export method beacause the set should concern about set the gross price and not check if it is null. Do you have some better way to implement this?

Thanks

Answer 1

I see several approaches:

• Make Product a stupid object which just holds the information about a Product, calculate the price externally
• Make gross_price a method which memoizes the result of the calculation

Stupid Object

class Product
  def initialize(name, net_price, gross_price)
    @name ...
    ...
  end
end

class GrossPriceCalculator
  def initialize()

  end

  def call(net_price)
    # complicate operation goes here
    # ...
  end
end

Memoization

class Product
  def initialize(name, net_price)
    @name = name
    @net_price = net_price
  end

  def gross_price
    @gross_price ||= calculate_gross_price
  end

  private

  def calculate_gross_price
    # here is the complicate operation..
    #
  end
end

I prefer first solution. It does not trigger a potentially expensive call hidden by some completely different operation (like export)

Answer 2

Completely agree that if the heavy computation is only needed for the export, then you shouldn't do it in the initialize (as a concrete case, think of writing unit tests; why should all the tests pay the price of a heavy computation when it is needed for a single method?).

One way to avoid things like @gross_price.nil? && set_gross_price is to do a one-time assignment if the variable is empty by using ||= (see e.g. What does ||= (or-equals) mean in Ruby?). At the same time, by using attribute accessors you can abstract yourself form when that computation is performed, and lazy compute that on the getter:

class Product

  attr_reader :name, :net_price

  def initialize (name, net_price)
    @name = name
    @net_price = net_price
    @gross_price = nil
  end

  def gross_price
    @gross_price ||= heavy_gross_price_calculation
  end

  def export
    return to_hash
  end

  def heavy_gross_price_calculation
    puts "heavy calculation"
    1000
  end

  def to_hash
    Hash[instance_variables.map do |var_name|
      # Remove the "@"
      name = var_name.to_s[1..-1]
      [name, send(name)]
    end]
  end

end

I'm giving here a possible to_hash implementation, but of course you can use a different one, as long as it uses the accessors and not the inst vars directly.

Then in a console you can do (see that "heavy calculation" is printed only once, the first time export is called):

2.2.3 :001 > p = Product.new('X', 100)
 => #<Product:0x0000000104e128 @name="X", @net_price=100, @gross_price=nil> 
2.2.3 :002 > p.export
heavy calculation
 => {"name"=>"X", "net_price"=>100, "gross_price"=>1000} 
2.2.3 :003 > p.export
 => {"name"=>"X", "net_price"=>100, "gross_price"=>1000} 
2.2.3 :004 > 

One advantage of this is that any other method besides export that needs gross_price can also get it, with all the benefits export already has and the computation only needs to be done once.

Answer 3

I'm trying to be a better developer ...

You're on the right track.

You have detected the scent of SRP. Your Product is doing at least 2 things:

1. Being a product with name and price
2. Exporting
3. Calculating gross prices

I think if you create 1 or 2 more classes the smell will go away.

You may want to read POODR. And write specs.