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!

• GorrillaMcD: Wow, thanks a bunch for the detailed answer. Payment.kind indicates the payment method (we have 2 available so far). It will be set using a hidden_field in the future (just haven't done it yet). One question though: I thought BigDecimal was needed for accuracy of calculating decimals. Do the standard arithmetic functions not use a float unless you specifically convert to a float?
• Flambino: @GorrillaMcD Makes sense with regard to kind. As for float versus BigDecimal, you're probably right. I forgot for a moment that we were dealing with money - sorry. However, it might be a lot easier to simply always use cents and thus integers. No BDs or floats to contend with. You're storing amount as an int anyway, so it might be useful to make the course-cost an int as well. Here's a discussion on BD vs float that also mentions the Money gem, which might be worth looking into.
• GorrillaMcD: Yeah, I should look at it and use cents where I can, especially since Stripe only accepts amount in cents and db is set for that as well. The problem arises with the conversion between pesos and dollars, since that almost always produces a decimal number. I need to look at the money gem, but I wanted to do it myself this time so I could learn and understand for myself what's going on.
• Flambino: @GorrillaMcD Well, if all the amounts and costs are always in cents, there's little room for rounding errors - you don't need precision to the nth decimal, since cents don't have decimals anyway. And I think it's great you're trying it yourself! But be sure to write some very robust tests; don't blunder through like I just did :)