4
\$\begingroup\$

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('bread', 2.17)
  Items.new('banana', 0.99)
  Items.new('apple', 0.89)

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

  pc = PriceCalculator.new
  puts pc.start_billing
end
```
\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

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.