# Ruby calculator for a grocery store

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 $$$$  ## 1 Answer 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

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 = {}

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
$$$$

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