# Ruby price calculator for groceries

I am good at logic and most of the time I write working code. But I want to learn writing code which follows best practices and is very efficient. I tried to implement some of them in my code but it still looks very unclean. Could anyone please explain how I can make this code better and make it production standard code.

This is my ruby code:

class Items
@@items = {}
def initialize(name, price)
@@items[name] = price
end

def self.all
@@items
end
end

class SaleItems
@@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 start_billing
input = get_input
@purchased_items = input.split(',').map(&:strip)
if !@purchased_items.empty?
quantity = count_items
price = calculate_bill(quantity)
billing_items = quantity.each_with_object(price) { |(k,v), billing_items|
billing_items[k] = {'units' => v, 'price' => price[k]}
}
display_bill(billing_items, quantity)
else
puts "Sorry! no items were given to process the bill"
end
end

private

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

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

def calculate_bill quantity
price = {}
quantity.each { |item,value|
if SaleItems.all[item].nil?
price[item] = quantity[item]*Items.all[item]
else
price[item] = (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
end
}
price
end

def display_bill billing_items, quantity
total_price = billing_items.inject(0){ |tot, (item,v)|
tot + v['price']
}

actual_price = quantity.inject(0){ |tot, (item,units)|
tot + (units * Items.all[item])
}

puts "Item          Quantity          Price"
puts "------------------------------------------"

billing_items.each{ |item, v|
puts "#{item.ljust(20)} #{v['units']}           \$#{v['price']}"
}
puts "Total price : #{total_price.round(3)}"
puts "You saved #{(actual_price - total_price).round(3)} today."
end

end

begin
# creating inventory of items
Items.new('milk', 3.97)
Items.new('banana', 0.99)
Items.new('apple', 0.89)

# creating sale items
SaleItems.new('milk',2,5.00)

pc = PriceCalculator.new
puts pc.start_billing
end
$$$$


## Rubocop Report

There is a nice topic on Class vs Instance variables online. Are you sure you would like to use class variables in classes Items and SalesItems?

class Items
@@items = {}
..

class Items
@items = {}
..


Class PriceCalculator has a couple of issues that need to be addressed.

billing_items = quantity.each_with_object(price) { |(k,v), billing_items|


billing_items inside the each_with_object hides the member from the outer scope. To avoid confusion which variable is accessed, consider changing the name of the variable inside the inner call.

def get_input


Ruby guidelines don't like these java-style accessors. However, since the method isn't actually an accessor, it is acceptable (Clarification).

price[item] = (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])


Try to keep the length of your lines below 80 characters. Ruby is meant to read vertically.

Other:

• Method start_billing might have many lines. Consider splitting up methods if they take more than 10 lines. Ruby likes short methods.
• The complexity of method calculate_bill is too high. Consider splitting up its content into multiple methods, each doing their specific part.

In addition to what @dfhwze has said, I wanted to give some suggestions on general ruby best practice and readibility:

## Multiline Blocks

In PriceCalculator#start_billing, PriceCalculator#count_items and in a couple other methods you use muliline blocks like so:

foobar.each { |*args|
# do stuff
# more stuff
}


The use of {} is really only intended for single line blocks -- when you have more than one line inside of the block, it's best to use do ... end. This will make your code a bit easier to read. From my example:

foobar.each do |*args|
# do stuff
# more stuff
end


## Method Definitions

In ruby, while it's valid syntax to define methods like def some_method argument, it's really easy to misread that as def some_method_argument, and so it's considered better practice to put parentheses around your method definitions, like so: def some_method(argument).

## Other

I just wanted to take a look at this method:

def calculate_bill quantity
price = {}
quantity.each { |item,value|
if SaleItems.all[item].nil?
price[item] = quantity[item]*Items.all[item]
else
price[item] = (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
end
}
price
end


Specifically, in the if statement, you could take advantage of a common ruby idiom for generating hashes, and instead of generating a price hash just return the hash you want:

def calculate_bill quantity
quantity.map { |item,value| [item, SalesItems.all[item].nil? ? quantity[item]*Items.all[item] : (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])] }.to_h
end


(note how I used the {} for a one line block). If you wanted to expand this to make it a bit more readable:

def calculate_bill quantity
quantity.map do |item,value|
v = if SalesItems.all[item].nil?
quantity[item]*Items.all[item]
else
(((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
end
[item, v]
end.to_h
end
`

I'd also recommend breaking up that massive formula into some smaller pieces with better variable names to make it easier to tweak it later if you need to.