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.