1
\$\begingroup\$

I have been working on this code to meet the best OOP standards for production. The code is working but I wish to make this a more efficient code. Could someone help me out here to make this efficient using best practices of ruby.

class Item
  @@items = {}
  def initialize(name, price)
    @@items[name] = price
  end
  
  def self.all
    @@items
  end
end

class SaleItem
  @@sale_items = {}
  def initialize(name, units, price)
    @@sale_items[name] = { 'units' => units, 'price' => price }
  end

  def self.all
    @@sale_items
  end
end

class PriceCalculator
  def initiate_billing
    input = get_input.split(',').map(&:strip)
    @purchased_items = input
    if @purchased_items.any?
      quantity = count_items
      price = calculate_bill(quantity)
      display_bill(price, quantity)
    else 
      puts "First add items to generate bill"
    end
  end

  private

    def get_input
      puts "Please enter all the items purchased separated by a comma"
      response = gets.chomp
    end

    def count_items
      @purchased_items.inject(Hash.new(0)) do |quantity, item|
        quantity[item] += 1
        quantity
      end
    end

    def calculate_bill(quantity)
      quantity.map do |item,value|
        items = Item.all[item]
        sale_items = SaleItem.all[item]
        value = if sale_items.nil?
         quantity[item] * items
        else
         (((quantity[item]/sale_items['units'])) * sale_items['price']) + ((quantity[item] % sale_items['units']) * items)
        end
        [item, value]
      end.to_h
    end
    
    def display_bill(price, quantity)
      billing_items = quantity.each_with_object(price) do |(key,value), billing_items| 
        billing_items[key] = {'units' => value, 'price' => price[key]}
      end

      total_price = billing_items.inject(0) do |total, (item,value)|
        total + value['price']
      end

      actual_price = quantity.inject(0) do |total, (item,units)| 
        total + (units * Item.all[item])
      end

      puts "Item     Quantity      Price"
      puts "--------------------------------------"
      billing_items.each do |item, value|
        puts "#{item.ljust(10)} #{value['units']}           $#{value['price'].round(3)}"
      end
      puts "Total price : $#{total_price.round(3)}"
      puts "You saved $#{(actual_price - total_price).round(3)} today."
    end
end

begin
  Item.new('milk', 3.97)
  Item.new('bread', 2.17)
  Item.new('banana', 0.99)
  Item.new('apple', 0.89)

  SaleItem.new('milk',2,5.00)
  SaleItem.new('bread',3,6.00)
  
  price_calculator = PriceCalculator.new
  puts price_calculator.initiate_billing
end

```
\$\endgroup\$
2
\$\begingroup\$

I have been working on this code to meet the best OOP standards for production.

I think your biggest problem here is that PriceCalculator does too many things:

  1. Getting user input
  2. Calculating the price per checkout item
  3. Calculating the total price
  4. Printing the bill

Let's try to split this up a bit first by introducing a BillRow for each line in your Bill to which we delegate to calculate the price per row .

class Bill
  def initialize(purchased_items:)
    @purchased_items = purchased_items
  end

  def total
    rows.inject(0) { |sum, x| sum + x.total }
  end

  def discounted_total
    rows.inject(0) { |sum, x| sum + x.discounted_total }
  end

  def rows
    @rows ||= fetch_rows
  end

  private
  
  attr_reader :purchased_items

  def fetch_rows
    count_items.map do |item, quantity|
      BillRow.new(name: name, quantity: quantity)
    end
  end

    def count_items
      purchased_items.inject(Hash.new(0)) do |quantity, item|
        quantity[item] += 1
        quantity
      end
    end
end

class BillRow
  attr_reader :name, :quantity

  def initialize(name:, quantity:)
    @name = name
    @quantity = quantity
  end

  def total
    @_total ||= quantity * item_price
  end

  def discounted_total
    @_discounted_total ||= calculate_discounted_total || total
  end

  private

   def calculate_discounted_total
    return unless sale_item
  
    ((quantity / sale_item['units'])) * sale_item['price']) + ((quantity % sale_item['units']) * item_price)
   end

    def sale_item
      @_sale_item ||= SaleItem.all[name]
    end

    def item_price
      Item.all[name]
    end
end

Bill.new(['milk', 'milk', 'bread'])

Additionally we will extract the display method to it's own class.

class BillPrinter
  def initialize(bill:)
    @bill = bill
  end

  def print
    print_header
    print_rows
    print_total
  end

  private

  def print_header
    puts "Item     Quantity      Price"
    puts "--------------------------------------"
  end

  def print_rows
    bill.rows.each do |row|
      puts "#{row.item_name.ljust(10)} #{row.quantity}           $#{row.total.round(3)}"
    end
  end

  def print_total
    puts "Total price : $#{bill.total.round(3)}"
    puts "You saved $#{(bill.total - bill.discounted).round(3)} today."
  end
end

bill = Bill.new(['milk', 'milk', 'bread'])
BillPrinter.new(bill: bill).print

I will skip getting the user input as this is can basically go to the main method.

Splitting the PriceCalculator into three classes has several advantages.

  1. Easier to test: We can pass in data instead of user input
  2. Classes are smaller and have only one responsibility
  3. Easier to extend and maintain e.g. if we want to use it on a website instead the console we need to refactor the code (user input read from StdIn and bill printed to StdOut). Now we can just implement a HtmlBillPrinter for instance.
  4. We can easier memoize data (efficiency)

The code is working but I wish to make this a more efficient code

In terms of efficiency, the biggest issue is that you calculate some values several times and look it up several times. This might be fine and the most important rule when making something more efficient is to first identify the bottleneck before jumping into optimize it. Readable code is usually preferred over complicated but efficient code.

To sum it up, I think the biggest improvements can be made in a better object oriented approach which allows you to more aggressively memoize values (https://www.justinweiss.com/articles/4-simple-memoization-patterns-in-ruby-and-one-gem/) as well as working with objects instead of e.g. arrays and hashes (BillRow vs [quantity, value]) which allows you to split out more readable methods too.

Edit:

As you mention 'production' I also just want to mention that you should NEVER use float for monetary values because they're not accurate (https://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency). You should use integer and represent the price e.g. in cents. Even better would be to use a money gem (https://github.com/RubyMoney/money).

Another improvement would be to use objects in your SalesItem and Item classes.

class SaleItem
  @@sale_items = {}

  attr_reader :name, :units, :price
 
  def initialize(name:, units:, price:)
    @name = name
    @units = units
    @price = price
  end

  def initialize(name, units, price)
    @@sale_items[name] = new(name: name, units: units, price: price)
  end

  def self.all
    @@sale_items
  end
end
```
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 for the float/currency issue. No idea about that \$\endgroup\$ – Alter Lagos Jul 10 at 4:53
  • \$\begingroup\$ I don't wish to use class variables. How may I implement this with instance variables. \$\endgroup\$ – The Medevil Jul 12 at 8:17
  • \$\begingroup\$ Why do you not want to use class variables? You only use them to store the sales items and items which makes perfectly sense. \$\endgroup\$ – Christian Bruckmayer Jul 12 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.