Should I refactor this rails application?

Question

I have taken over the code base for a ruby-on-rails application which relies very heavily on ActiveRecord callbacks to perform domain rules.

The application can be compared to a bank application, where a customer has an Account. On each account you can register a Transaction (an amount of money that is either inserted or removed from the account). The Account has an account_balance that is updated every time a transaction is created. This update happens as an ActiveRecord callback.

class Account < ActiveRecord::Base
 has_many :transactions
 def update_account_balance
 account_balance = 0
 self.transaction_logs.each do |transaction|
 account_balance = account_balance + (transaction.amount + transaction.commission)
 end
 self.account_balance = account_balance
 end
end
class Transaction < ActiveRecord::Base
 belongs_to :account
 after_save :update_account_value
 def update_account_value
 self.account.update_account_balance
 self.account.save!
 end
end

I find that this way of coding makes it very difficult to figure out what happens (and what goes wrong when a bug occurs). Also, since the account needs to load all transactions, this will be heavier as more and more transactions are added.

Had I been developing an application like this in .NET (in which I am much more familiar) I would have created something like this:

public class Account {
 public void AddTransaction(decimal amount) {
 this.Transactions.Add(new Transaction(amount));
 this.AccountValue += amount;
 }
}

And then made sure that a Transaction object is immutable.

I am very tempted to refactor the system into something like that. But should I? Would that be the "rails way"?

The drawback, as far as I can tell, is that my controller needs to explicitly handle controlling database transactions, something which happened implicitly in the Transaction.save! call currently called in the application. Are there other factors I should be aware of?

Answer 1

The existing application logic isn't terrible. I wouldn't make any changes move logic up into the controller; that's the opposite direction from the preferred methodology of pushing logic down into the model as much as possible.

The only thing that sticks out to me as potentially (or perhaps inevitably) dangerous is that, as you mentioned, Account#update_account_balance is iterating over every transaction to find what its balance should be, every time a new Transaction is saved. This is not only a potential performance bottleneck, but also a potential race condition, if two transactions are saved at nearly the same time and one is excluded from the set returned by the call to Account#transaction_logs.

My suggested refactoring:

class Account < ActiveRecord::Base
 attr_protected :account_balance
 has_many :transactions
 def update_account_balance(transaction)
 self.account_balance = (self.account_balance + transaction.amount + transaction.commission)
 self.save!
 end
end
class Transaction < ActiveRecord::Base
 attr_readonly :amount, :commission
 belongs_to :account
end
class TransactionObserver < ActiveRecord::Observer
 def after_create(transaction)
 transaction.account.update_account_balance(transaction)
 end
end

The use of ActiveRecord::Observer here helps you to follow the Single Responsibility Principle; updating an Account isn't part of the core responsibility of a Transaction, but it does have to occur each time a Transaction is persisted.

The use of attr_readonly in Transaction protects the values from update operations, and attr_protected prevents mass assignment on :account_balance.

• Pete: That is very interesting. I didn't know about the Observer. I do like it better than the current version. What about the case where the object being observed is not immutable, can the observer compare the current updated value to the original value when an object is modified?
• Jason Lewis: Sure. An observer is just a way of extracting callbacks for cleaner design. So inside your observer, you could do def before_save(obj); diff = obj.changes; #do something with diff; end Look at the docs for ActiveRecord::Dirty