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 => (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
rescue Stripe::InvalidRequestError => e
  logger.error "Stripe error while creating charge: #{e.message}"
  errors.add :base, "There was a problem with your credit card."
  • \$\begingroup\$ Is it a joke?.. \$\endgroup\$ – Nakilon Jul 18 '13 at 3:37
  • \$\begingroup\$ @Nakilon, are you answering a deleted comment? otherwise I don't see the joke. \$\endgroup\$ – tokland Jul 18 '13 at 6:51
  • \$\begingroup\$ @tokland, I don't the complexity nor unreadability nor hugeness in this code. \$\endgroup\$ – Nakilon Jul 18 '13 at 11:13
  • \$\begingroup\$ 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. \$\endgroup\$ – tokland Jul 18 '13 at 11:35

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 => 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?
  elsif !(charge = create_stripe_charge)
    errors.add(:base, "There was a problem with your credit card.")
      :stripe_charge_id => charge.id,
      :starts_at => Date.today,
      :amount => plan.price,
      :expires_at => 1.years.from_now,

def create_stripe_charge
    :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}")

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"

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

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.