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  endendclass Transaction < ActiveRecord::Base  belongs_to :account  after_save :update_account_value  def update_account_value    self.account.update_account_balance    self.account.save!  endendI 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?
Should I refactor this rails application?
refactoring;ruby on rails
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!  endendclass Transaction < ActiveRecord::Base  attr_readonly :amount, :commission  belongs_to :accountendclass TransactionObserver < ActiveRecord::Observer  def after_create(transaction)    transaction.account.update_account_balance(transaction)  endendThe 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.