-
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.
-
Results from methods can be memoized. You shouldn't have to look up objects every time you call a method to get back the same results.
def transactions
@transactions ||= Transaction.find_all_by_invoice_id(id)
end
- Don't need to use self when in a class method because self already points to the your class.. For example in invoice.rb
new() instead of self.new()
- Unnecessary use of self. I was wondering why there were so many class methods. Then I saw the following code.
def self.build_data(contents)
@invoice_items = contents.collect { |row| InvoiceItem.new(row) }
end
def self.invoice_items
@invoice_items
end
There is a separation of concerns problem here. InvoiceItem is responsible for both storage of all invoice items and providing instance methods to operate on an invoice item. In your case it's a bit confusing to store invoice items into a class instance variable. I might have created an object whose only concern was to store all data objects. Invoices, Transactions etc, which I could ask for invoice item instances by id, name or whatever. Almost like your own hand rolled db.
- customer_test.rb
I wonder if this will cause intermittent failures
def test_it_can_return_a_random_value
refute_equal Customer.random.to_s, Customer.random.to_s
end
- No need to loop over one element:
def test_it_can_find_all_customers_by_id
customers = Customer.find_all_by_id(3)
assert_equal 1, customers.count
customers.each do |customer|
assert_includes "3 Mariah", customer.to_s
end
end
- Again a lot of duplication in the tests that can be fixed with dynamic finders.