# Saving a subscription plan with a payment

Is this acceptable for readability or would it be better to refactor it into smaller methods?

def save_with_payment
if valid?
charge = Stripe::Charge.create(:amount => (self.plan.price * 100).to_i,
:currency => 'usd', :card => stripe_card_token, :description => "Charge for #{user.email}")
self.stripe_charge_id = charge.id
self.starts_at = Date.today
self.amount = plan.price
self.expires_at = 1.years.from_now
save!
end
rescue Stripe::InvalidRequestError => e
logger.error "Stripe error while creating charge: #{e.message}"
errors.add :base, "There was a problem with your credit card."
false
end

• Is it a joke?.. – Nakilon Jul 18 '13 at 3:37
• @Nakilon, are you answering a deleted comment? otherwise I don't see the joke. – tokland Jul 18 '13 at 6:51
• @tokland, I don't the complexity nor unreadability nor hugeness in this code. – Nakilon Jul 18 '13 at 11:13
• I see. Well, I think the OP was just being modest, indeed it's not complex nor unreadable, but of course it can be improved (as any other code), I'll post an answer later. – tokland Jul 18 '13 at 11:35

• Indentation of a call with multi-line hash: That's subjective, but this style you use wastes a lot of space, and it's pretty hard to read (because of the longer lines). If the hash is long I prefer to use the one-line-per-key/value JSON style. More on this here.
• self.stripe_charge_id = charge.id: When you have a lot of sets + save! it is better just to use update_attributes!.
• save!: The method is named save_with_payment, in a ActiveRecord context that means this method won't raise an exception, so you should call save.
• if valid?: No else branch? The method seems to return a boolean so returning false in that case would be more consistent.
• rescue Stripe::InvalidRequestError => e: That's a long and subjective topic. My opinion: don't wrap a whole method that is doing lots of things with a rescue, the logic forms now some kind of spaghetti. Wrap the specific parts of the code that may raise that particular exception.
• Stripe::Charge.create: You asked if the code is too complex. I don't think so, at least not compared with typical Ruby practices, but it's probably more orthodox to create a separate method for this call.
• errors.add :base,: I don't like this mixing of calls with parens and calls without, it looks messy. DSL-style code in Rails without parens -> ok, normal code in methods -> not so sure, I'd write them. Or at least be consistent.
• self.plan: It's not idiomatic to write explicit self. to call instance methods.

I'd write:

def save_with_payment
if !valid?
false
elsif !(charge = create_stripe_charge)
errors.add(:base, "There was a problem with your credit card.")
false
else
update_attributes({
:stripe_charge_id => charge.id,
:starts_at => Date.today,
:amount => plan.price,
:expires_at => 1.years.from_now,
})
end
end

def create_stripe_charge
Stripe::Charge.create({
:amount => (plan.price * 100).to_i,
:currency => 'usd',
:card => stripe_card_token,
:description => "Charge for #{user.email}",
})
rescue Stripe::InvalidRequestError => e
logger.error("Stripe error while creating charge: #{e.message}")
nil
end


I would refactor the Charge.create to a separate class.

Because one the principies that we have respect is "our code reader should not read the details of the implementation for understand the code"

• Please explain why you would do this. (In the meantime -1) – mheinzerling Jul 18 '13 at 6:54
• I would extract it to a own method, just to get rid of the long line and give it a descriptive name. – mheinzerling Jul 18 '13 at 6:55
• I explain it, it's my opinion – Gabriel Moral Jul 18 '13 at 9:35
• That's a good point (the answer's point, not the comment's point). – Ecnalyr Jul 19 '13 at 16:22