sales_engine - git://github.com/Diasporism/sales_engine.git

gistfile1.md

• Class level variables are considered bad practice. Instance variables are the preferred way of storing object state.

    def self.build_customer(contents)
      @@customer = []
      contents.each {|row| @@customer << Customer.new(row)}
      @@customer.count
    end

I can feel @j3 cringing at the use of @@ hehe.

• This can be simplified in customer.rb

def self.return_customers_for_invoices(invoices)
  customers = []
  invoices.each {|i| customers << Customer.find_by_id(i.customer_id)}
  customers.flatten
end

to

def self.return_customers_for_invoices(invoices)
  invoices.collect { |i| Customer.find_by_id(i.customer_id)}
end

• Every class has find_by_* methods. Take a look at method_missing to create dynamic finders for your class attributes. This is exactly how the Rails finders work.

• Some refactoring in invoice.rb

def items
  invoice_item = InvoiceItem.find_all_by_invoice_id(id)
  item_id = []
  invoice_item.each { |item| item_id << item.item_id }
  item_list = []
  item_id.each { |id| item_list << Item.find_by_id(id) }
  item_list
end

to

def items
  invoice_items.collect do |invoice_item|
    Item.find_by_id(invoice_item.item_id)
  end
end

• Lines longer than 80-120 characters is too long. For example Merchant#revenue. You can refactor those lines to be easier to read by breaking it up into multiple lines

• Pass objects around and operate on their methods instead of hashes with calculated values

I noticed the methods that are returning hashes of invoice_id to quantity and revenue. Then you use that hash and build another hash to create a sorted hash of merchants-by revenue/quantity. Then you use the id keys to find invoices and merchants, etc. Phew. That's a lot of code just to figure how much money a merchant made.

I'm talking specifically about code like this:

     most_items = Merchant.sum_value_by_merchant_id(InvoiceItem.get_invoice_quantity(Transaction.get_successful_transaction))

     most_revenue = Merchant.sum_value_by_merchant_id(InvoiceItem.get_invoice_revenue(Transaction.get_successful_transaction))

    def self.sum_value_by_merchant_id(invoice_totals)
      merchant_revenues = Hash.new(0)
      invoice_totals.each_pair do |k, v|
        invoice = Invoice.find_by_id(k)
        key = invoice.merchant_id
        merchant_revenues[key] += v
      end
      merchant_revenues.sort_by { |k,v| v }.reverse
    end

What's easier to understand is moving revenue calculations into merchant. You want to know how much money a merchant made. So let's ask each merchant how much money it made :) The same goes for invoices.

def self.merchants_by_revenue
  Merchant.merchants.sort_by { |m| m.revenue }
end

• Comments like ############################ First_Name are unnecessary :)