5
\$\begingroup\$

I have a Rails app where Loan has paid attribute. Loan should be marked as paid if total sum of payments is equal or higher than loan amount_to_pay. Problem is that it is obligatory to have paid attribute and not checking it dynamically by joining payments to loans and selecting whether it is paid or not (long story...). So I have several options.

Option 1

Creating payments via Loan class. It helps to wrap it into transaction to be sure, that create and update would be executed. But as I understand I'm breaking single-responsibility principle here and loan have to worry about payment creation.

class Loan
  has_many :payments

  def pay!(payment_params)
    transaction do
      payments.create(payment_params)
      update(paid: true) if payments.total >= amount_to_pay
    end
  end
end

class Payment
  belongs_to :loan

  def self.total
    pluck(:amount).reduce(:+)
  end
end

loan = Loan.create(amount_to_pay: 1000)
loan.pay!({ amount: 500 })
loan.pay!({ amount: 500 })
loan.paid? #=> true

Option 2

Payments here are created directly, but there is no 100% guarantee that after_create will be executed. More over callbacks are difficult to test.

class Loan
  has_many :payments
end

class Payment
  belongs_to :loan

  after_create :mark_loan_as_paid

  def mark_loan_as_paid
    if loan.payments.total >= loan.amount_to_pay
      loan.update(paid: true)
    end
  end
end

loan = Loan.create(amount_to_pay: 1000)
Payment.create(amount: 500, loan_id: loan.id)
Payment.create(amount: 500, loan_id: loan.id)
loan.paid? #=> true

So, what is the best option to handle such cases (when some action in one class should change something in another).

\$\endgroup\$
1
\$\begingroup\$

Your current approach isn't terrible. Yes, there's some coupling between the two models, but that isn't "illegal". Records in Rails, by virtue of being relational data models, typically have conceptual coupling already. And while functional coupling is different, it's sometimes the best solution. Heck, Rails even has a few features that rely on such coupling like delegating methods to an associated record, or the (similar to your setup) accepts_nested_attributes_for method, and the counter_cache option for associations, which does indeed update the "owner" record after create and destroy operations.

So for what it's worth, I'd say option 2 makes the most sense. If a Payment is created - however it's created - update its associated Loan. I wouldn't say it's difficult to test either: Create a loan, create sufficient payments, then reload the loan and check that it's been paid. Transactions are trickier only if your models a distributed across multiple databases, but even then they can be added.

I'd just make sure to also run mark_loan_as_paid after an update or destroy operation - if a payment is cancelled or changed, it's no good if the loan still appears as paid. And you can run periodic sanity checks on the data (e.g. with a rake task), if need be. For anything financial, that's probably a good idea no matter what.

But all that being said, it is of course nice to avoid coupling when you can. Again, in this case, I wouldn't bother. But if I was forced to, I'd say it's a matter of thinking beyond the Loan and Payment classes, because if the logic doesn't quite fit in either of those, then it's only logical that it fits somewhere else entirely.

One option is simply to do everything in the controller. It's precisely the sort of thing controllers are meant to do: Control access to and modification of data. In Rails you typically end up with a 1:1 structure of controllers and models (e.g. a PostsController for Post records), but that isn't a law. A user account system might have a SessionsController and a SignupsController and so on that all work on the same User model.

So you could easily have your PaymentsController both create a Payment record and update the Loan record. Nothing wrong with that. In fact, your controller is probably already loading the Loan anyway in order to do @loan.payments.create(payment_params).

Last option is to make Downpayment a "model" (just a plain class, not an ActiveRecord model) or a service object. Its responsibility would be the same as the controller in the previous description: Create a Payment record, and update the Loan record. So it's essentially the same logic, just extracted out of the controller.

In either case, you're back to a single responsibility: Pay down a loan. There are two models involved, but conceptually it's one task.

The tradeoff with either, however, is that you could technically create Payment without the Loan being updated by simply not using the controller/service object. But on the other hand you can do that too by just writing to the database without going through Rails.

An example:

def downpay(loan, amount)
  loan.transaction do
    loan.payments.create!(amount: amount)
    loan.update_attributes!(paid: loan.payments.total >= loan.amount)
  end
end
\$\endgroup\$
0
\$\begingroup\$

It's not the real Rails Way, but for such kind of work I favour Service objects. With service objects your able to test you business logic as unit tests without database or controllers.

But like I said, it's not the Rails Way.

\$\endgroup\$
0
\$\begingroup\$

I would say go with option 1, but make the pay! method accept an amount to pay. No need to pass Hashes around.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.