Order processing

Question

I have recently built a plain old Ruby program using TDD with RSpec to model this. I wanted to get feedback.

Repo

For the company whose application we’re looking at, order processing looks something like:

If we accept an order over the web, then we have to wait for payment to arrive, unless it’s a credit-card order. In the case of credit card orders, we can process the order immediately and ship the goods, but only if the goods are in stock. If they are not currently in stock, we have to delay processing credit card orders until the become available again.

We can receive a check in payment of an existing order, in which case we ship the goods (unless they are not in stock, in which case we hold the check until the goods become available).

We can receive a purchase order (PO) for a new order (we only accept these from companies with an established credit account). In this case we ship the goods (assuming they are in stock) and also generate an invoice against the PO. At some point in the future we’ll receive payment against this invoice and the order will be complete.

At any time before the goods are shipped the customer may cancel an order.

Each step in this process may occur many days after the previous step. For example, we may take an order over the web on Monday, receive a check for this order on Thursday, then ship the goods and deposit the check when the item is received from our own suppliers on the following Tuesday.

How can we design an application to support these kinds of rules?

class Customer
 VALID_TYPES = [:web, :company]
 attr_reader :name, :type
 def initialize(name, type)
 @name = name.to_s
 @type = type.to_sym
 validate!
 end
 private
 def validate!
 raise "This is not a valid type of customer" unless VALID_TYPES.include? self.type
 end
end

---

class Product
 attr_reader :title, :price
 attr_accessor :in_stock
 def initialize(title, price)
 @title = title.to_s
 @price = price.to_f.round(2)
 @in_stock = true
 end
 private 
 def in_stock?
 in_stock
 end
end

---

class Order
 VALID_STATUSES = [:pending, :paid, :shipped]
 attr_reader :customer
 attr_accessor :status, :total, :products
 def initialize(customer)
 @products = {}
 @status = :pending
 @customer = customer
 end
 def add_product(product, quantity = 1)
 @products[product] = (@products[product] ? @products[product] + quantity : quantity )
 puts "You haved added #{quantity} #{product.title}'s to your order"
 end
 def show_order
 return products.map { |product, quantity| "Item: $#{product.price} / Quantity: #{quantity}\n" }
 end
 def order_total
 self.total = products.inject(0){|memo, (product, quantity)| (product.price * quantity) + memo}.to_f.round(2)
 end
 def change_status(status)
 raise "This is not a valid order status please try again" unless VALID_STATUSES.include? status
 self.status = status
 end
 def confirm_order(method_of_payment)
 if credit_card_but_products_out_stock?(method_of_payment)
 raise "Cannot make credit card payment now, as some products are out of stock" 
 end
 order_total
 Payment.new(method_of_payment, self.total)
 end
 def self.from_purchase_order(purchase_order)
 new(purchase_order.customer) do |order|
 order.products = purchase_order.products.clone
 end
 end
 private
 def credit_card_but_products_out_stock?(method_of_payment)
 true if method_of_payment == :credit_card && !all_products_in_stock?
 end
 def all_products_in_stock?
 count = @products.select { |product| product.in_stock == false }.size
 count == 0 ? true : false
 end
end

---

class Invoice
 attr_reader :customer
 attr_accessor :total, :products
 def initialize(customer, products)
 @products = {}
 @customer = customer
 @payment_recieved = false
 end
 def show_invoice
 return products.map { |product, quantity| "Item: $#{product.price} / Quantity: #{quantity}\n" }
 end
 def self.from_purchase_order(purchase_order)
 new(purchase_order.customer, purchase_order.products.clone)
 end
end

---

class PurchaseOrder
 attr_reader :customer, :products
 attr_accessor :total
 def initialize(customer)
 @products = {}
 @customer = customer
 validate!
 end
 def add_product(product, quantity = 1)
 @products[product] = (@products[product] ? @products[product] + quantity : quantity )
 puts "You haved added #{quantity} #{product.title}'s to your purchase order"
 end
 def show_purchase_order
 return products.map { |product, quantity| "Item: $#{product.price} / Quantity: #{quantity}\n" }
 end
 def purchase_order_total
 self.total = products.inject(0){|memo, (product, quantity)| (product.price * quantity) + memo}.to_f.round(2)
 end
 def confirm_purchase_order 
 purchase_order_total 
 raise "Your PO appears to be empty! Add some products and try again." unless self.total.to_f.round(2) > 0
 create_order
 create_invoice
 return "We have generated an Invoice and created an order."
 end
 def create_order
 Order.from_purchase_order(self)
 end
 def create_invoice
 Invoice.from_purchase_order(self)
 end
 private
 def validate!
 raise "Customer must be a company account" unless customer.type == :company
 end
end

---

class Payment
 VALID_METHODS_OF_PAYMENT = [:credit_card, :check]
 attr_accessor :method_of_payment, :amount
 def initialize(method_of_payment, amount)
 @method_of_payment = method_of_payment
 @amount = amount
 validate!
 end
 private
 def validate!
 raise "This is not a valid payment method" unless VALID_METHODS_OF_PAYMENT.include? self.method_of_payment
 raise "Uh oh, your order appears to be empty" unless self.amount.to_f.round(2) > 0
 end
end

Answer 1

I only spotted a few lines of code that could be nicer, otherwise it's readable:

• count == 0 ? true : false is count == 0, which is count.zero?.
• Similarly, true if method_of_payment == :credit_card && !all_products_in_stock? should simply be method_of_payment == :credit_card && !all_products_in_stock?.
• If this was production code I'd strongly advice you to use proper decimal numbers (i.e. BigDecimal) instead of floats for monetary values.

The formatting is sometimes inconsistent and the choice of printing messages vs. returning strings is also not consistently applied - I'd go for one or the other and stick with it.

Otherwise I think looks okay; maybe consider refactoring common code into some shared thing, i.e. the show_ methods.

• Ben Hawker: Thanks for your points. I had not realised the importance of using BigDecimal (or rather not using floats for monetary values before) so have been reading up on that. Good resource on it (spin.atomicobject.com/2014/08/14/currency-rounding-errors).