Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

First, some background. I have a Payment model that has_many :subscriptions. When a user pays (creates a new payment), I have an after_create callback that adds the newly created @payment.id to @subscriptions.payment_id. Here's what it looks like:

def update_subscription
  @unpaid_subs = Subscription.where(:user_id => self.user_id).unpaid
  @unpaid_subs.each do |sub|
    sub.payment_id = self.id
    sub.start # call to state_machine to change state from pending -> active
    sub.save
  end
end

I know that doing database queries inside a loop is generally not good for performance, but I don't know any way to update multiple records at the same time. Also, is there a way to pass the @unpaid_subs instance variable from my create action to the callback (it's the same query on both) so that I can remove the query here?

share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

Considering you have a state machine, you're probably doing the right thing by looping through the records. Although you could change all the subscriptions (including their state) in the database, you'd be bypassing the state machine and whatever checks and callbacks it has in place.

But, if you really want to bypass the state machine, you could probably do something like this:

unpaid_subs = self.user.subscriptions.unpaid # I'm assuming Payment belongs_to User
unpaid_subs.update_attributes(:payment_id => self.id, :state => 'active')

Of course, it would require you to allow mass-assignment of both payment_id and (what I assume is named) state, neither of which sound like good ideas at all.

So you'd have to bypass the state machine and ActiveRecord to directly update the records in the database with some raw SQL... ugh, gross.

So, as I said, you're probably doing the right thing already :)

share|improve this answer
    
Well, I'm glad to know I'm doing it right. I was thinking it'd be bad performance wise, but I couldn't think of any other way to do it. I definitely don't want to mass-assign :payment_id or :state. I actually might get rid of the state_machine in the next version, since it's a bit overkill for my implementation. Payment actually doesn't belong_to :user right now, though that's a great idea. I had asked a question about that a few weeks ago on SO. –  GorrillaMcD Nov 3 '12 at 22:32
    
@GorrillaMcD Well, performance-wise it's only "bad" if what you're doing is unnecessary. But in this case triggering the state transition is necessary, so you can't avoid loading the records. However, in the interest of robustness (if you keep the state machine), it might be good to tie the state transition directly to setting payment_id, so you can't transition without a payment, and can't set a payment without changing state. But that's about robustness rather than performance. –  Flambino Nov 3 '12 at 23:34
    
Thanks, I didn't think about that. I'll be sure to do that. –  GorrillaMcD Nov 4 '12 at 4:42
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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