2
\$\begingroup\$

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

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
1

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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