Saving a subscription plan with a payment Is this acceptable for readability or would it be better to refactor it into smaller methods?

Tips?  Comments? Examples?

def save_with_payment
  if valid?
    charge = Stripe::Charge.create(:amount =&gt; (self.plan.price * 100).to_i,
                                   :currency =&gt; 'usd', :card =&gt; stripe_card_token, :description =&gt; "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 =&gt; e
  logger.error "Stripe error while creating charge: #{e.message}"
  errors.add :base, "There was a problem with your credit card."
  false
end
Some notes about your code:


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 =&gt; 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 =&gt; charge.id,
      :starts_at =&gt; Date.today,
      :amount =&gt; plan.price,
      :expires_at =&gt; 1.years.from_now,
    })
  end
end

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