Create Action in Rails 3.2

Question

I posted a question last week about refactoring an after_create callback. The code in this question is for the create action. I'm using Stripe to handle credit card payments and am roughly following the railscast on Stripe integration.

Here's my create action on the Payment model:

def create
  @unpaid_subs = Subscription.where(:user_id => current_user.id).unpaid.includes(:course)
  @payment = Payment.new(params[:payment])

  @payment.user_id = current_user.id
  @payment.email = current_user.email
  @payment.kind = @payment.determine_kind
  @payment.amount = calculate_cost(@unpaid_subs)

  if @payment.save_with_payment
    redirect_to dashboard_url, :notice => "Thank you for Paying!"
  else
    render :action => 'new'
  end
end

I am going to change the model associations so that payment belongs_to :user, so setting the user_id and email will go away. Here's the save_with_payment method:

def save_with_payment
  if valid?
    charge = Stripe::Charge.create(amount: amount, description: "Charge for #{email}", card: stripe_card_token, currency: "usd")
    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

determine_kind is a simple if conditional for setting the kind attribute. calculate_cost is in a module right now and included where needed.

def calculate_cost(unpaid) # Used to return amount total in cents for payment
  @amount_in_pesos = BigDecimal.new(unpaid.size) * COURSE_COST
  @amount_in_cents = (@amount_in_pesos.div(EXCHANGE_RATE, 10).round(2) * 100).to_i
  @amount = @amount_in_cents
  @amount
end

So, I'm mainly wanting general suggestions for improvement of this code. I was a little confused as to where calculate_cost should go (maybe as a method on the Payment model, even though it doesn't use an instance of Payment). Is there any better way to handle the call to the Stripe API?

Answer 1

Most of the following can be summed up as the ol' Rails rule-of-thumb: Skinny controller, fat model

---

1. Move logic to the model, and make it automatic

If you're doing this:

@payment.kind = @payment.determine_kind

it means that the logic should move to the model itself. If the model's doing the logic already and the result is saved to the very same model, why involve the controller at all?

Instead, you should do this:

class Payment < ActiveRecord::Base
  before_create :set_kind # determine and set the :kind attribute
  # ...

  private

  def set_kind
    self.kind = ... code ...
  end
end

However, there may be an even cleaner solution. As far as I can tell, determine_kind probably relies solely on information already in the payment record.

If that's the case, there may not be a reason to have a kind attribute/column at all. Just add a kind method to the Payment model:

class Payment < ActiveRecord::Base
  # ...
  def kind
    # same code as `determine_kind`
  end
  # ...
end

Now you can say some_payment.kind and get the kind without storing it in the database at all.

Note: this won't work if kind must be "kind as determined at the time of creation" or if you're doing database queries that involve the kind column. If either of those is the case, you can still use the before_create approach above.

---

2. Add calculate_cost to the collection

Here's what I'd do:

First, I'd move COURSE_COST to a COST constant on the Course model (accessible as Course::COST). That seems the most logical place to keep it. It'd probably be smart to also move EXCHANGE_RATE to Payment::EXCHANGE_RATE while you're at it.
Oh, and "exchange rate" is a little ambiguous: It's the rate from what to what? Pesos to USD? Or the other way around? Would be good to make the name more descriptive.

Then, assuming "user has_many subscriptions", you can first of all do this:

@unpaid_subs = current_user.subscriptions.unpaid.includes(:course)

but it also means that you can define modules that should be added to the collection. In your case, you make a module like so:

# in app/modules/subscription_collection.rb
module SubscriptionCollection
  def cost
    pesos = count * Course::COST
    usd   = pesos / Payment::EXCHANGE_RATE
    (usd * 100).round
  end
end

(I skipped the BigDecimal stuff because I didn't think it necessary, but as covered in the comments, it might be. I'll leave the code as is, though. BigDecimal or not, the procedure's the same.)

And then include that module in the relation declaration

class User < ActiveRecord::Base
  has_many :subscriptions, :extend => SubscriptionCollection
  # ...
end

Now, whenever you get a collection of subscriptions via a user model you can simply call cost on the collection itself, and get the total cost.

---

Now you should be able to say:

def create
  @unpaid_subs = current_user.subscriptions.unpaid.includes(:course)
  @payment = current_user.payments.new(params[:payment])

  @payment.amount = @unpaid_subs.cost

  if @payment.save_with_payment
    redirect_to dashboard_url, :notice => "Thank you for Paying!"
  else
    render :action => 'new'
  end
end

Last little thing: This is just me, but the name save_with_payment is a little iffy, since the model's already called payment. So, "save payment with payment" seems semantically messy. I'd suggest something like charge_and_save instead. You might also want to raise an exception if someone tries to call the method on an already-charged payment. You don't want to accidentally charge people multiple times!