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    save!  endrescue Stripe::InvalidRequestError => e  logger.error Stripe error while creating charge: #{e.message}  errors.add :base, There was a problem with your credit card.  falseend
Refactor or keep this high-complexity method?
ruby
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?    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,    })  endenddef 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})  nilend