Refactoring Long Ruby Stripe Payment Class

Ruby on Rails
1

Hi Upcase community,

I am a beginner and asking for any suggestion on refactoring. Trying to follow Sandi Metz’s rule but I can’t figure out how to reduce this class under 100 lines of code with limited knowledge that I have.

Basically, it is a donations controller that is charging stripe payments.

There are four things this class is trying to achieve:

New Stripe Customers

  1. New customer donate one time gift
  2. New customer donation monthly recurring gift

Existing Stripe Customer
3) Existing customer donates one time gift
4) Existing customer donates monthly recurring gift

Everything is working as it should, but as you can see the code is very long…

Any tips and help will be greatly appreciated to become a better coder!

Thanks in advance!

Here is the code in Gist: http://goo.gl/Y4gjUE

    class DonationsController < ApplicationController

  def index
  end

  def new
  end

  def create
    monthly_donation_with_existing_stripe_customer
    one_time_donation_with_existing_stripe_customer
    monthly_donation_with_new_stripe_customer
    one_time_donation_with_new_stripe_customer

    rescue Stripe::CardError => e
    flash[:error] = e.message
    redirect_to donations_path
  end

  private

  def stripe_email_present
    Payment.where(:email => params[:stripeEmail]).present?
  end

  def stripe_email_blank
    Payment.where(:email => params[:stripeEmail]).blank?
  end

  def monthly_donation_checked
    params[:monthlyDonation] == "1"
  end

  def monthly_donation_not_checked
    params[:monthlyDonation] == nil
  end

  def monthly_donation_with_existing_stripe_customer
    if stripe_email_present && monthly_donation_checked
      create_stripe_plan
      create_stripe_subscription_to_existing_customer
      create_stripe_payment_without_email
      redirect_to @payment
    end
  end

  def one_time_donation_with_existing_stripe_customer
    if stripe_email_present && monthly_donation_not_checked
      charge_existing_stripe_customer
      create_stripe_payment_without_email
      redirect_to @payment
    end
  end

  def monthly_donation_with_new_stripe_customer
    if stripe_email_blank && monthly_donation_checked 
      create_stripe_customer
      create_stripe_plan
      create_stripe_subscription_to_new_customer
      create_stripe_payment_with_email
      redirect_to @payment
    end
  end

  def one_time_donation_with_new_stripe_customer
    if stripe_email_blank && monthly_donation_not_checked
      create_stripe_customer
      charge_new_stripe_customer
      create_stripe_payment_with_email
      redirect_to @payment
    end
  end

  def create_stripe_customer
    @customer = Stripe::Customer.create(
      :email => params[:stripeEmail],
      :card  => params[:stripeToken]
    )
  end

  def charge_new_stripe_customer
    @charge = Stripe::Charge.create(
      :customer    => @customer.id,
      :amount      => params[:amount].to_i * 100,
      :description => 'Thrive General Donation',
      :currency    => 'usd'
    )
  end

  def charge_existing_stripe_customer
    @payment_donor = Payment.find_by_email(params[:stripeEmail])
    @charge = Stripe::Charge.create(
      :customer    => @payment_donor.customer_id,
      :amount      => params[:amount].to_i * 100,
      :description => 'Thrive General Donation',
      :currency    => 'usd'
    )
  end

  def create_stripe_plan
    @stripe_plan = Stripe::Plan.create(
      :amount       => params[:amount].to_i * 100,
      :interval     => 'month',
      :name         => params[:stripeName] + ' ' + 'Online Monthly Donation' + ' - ' + params[:stripeEmail],
      :currency     => 'usd',
      :id           => SecureRandom.hex
    )
  end

  def create_stripe_subscription_to_existing_customer
    @existing_customer = Payment.find_by_email(params[:stripeEmail])
    customer = Stripe::Customer.retrieve(@existing_customer.customer_id)
    customer.subscriptions.create(:plan => @stripe_plan.id)
  end

  def create_stripe_subscription_to_new_customer
    customer = Stripe::Customer.retrieve(@customer.id)
    customer.subscriptions.create(:plan => @stripe_plan.id)
  end

  def create_stripe_payment_without_email
    @existing_customer_from_db = Payment.find_by_email(params[:stripeEmail])
    @payment = Payment.create(
      amount: params[:amount],
      card: params[:stripeToken],
      currency: 'usd',
      customer_id: @existing_customer_from_db.customer_id,
      description: 'Thrive Online Donation',
      monthly_donation: params[:monthlyDonation],
      uuid: SecureRandom.uuid
    )
  end

  def create_stripe_payment_with_email
    @payment = Payment.create(
      amount: params[:amount],
      card: params[:stripeToken],
      currency: 'usd',
      customer_id: @customer.id,
      description: 'Thrive Online Donation',
      email: params[:stripeEmail],
      monthly_donation: params[:monthlyDonation],
      uuid: SecureRandom.uuid
    )
  end

end

Here is the form code on my donation view page:

<%= form_tag donations_path, id: 'chargeForm' do %>
  <script src="https://checkout.stripe.com/checkout.js"></script>
  <div class="input-group">
    <%= text_field_tag :amount, params[:amount], :class => 'donation-amount-number-only' %>
    <%= hidden_field_tag 'stripeToken' %>
    <%= hidden_field_tag 'stripeEmail' %>
    <%= hidden_field_tag 'stripeName' %>
    <br />
    <br />
    <span class="input-group-btn">
      <button id="btn-buy" type="button" class="btn btn-block btn-donate">Donate</button>
    </span>
  </div>
  <p>
    <%= check_box_tag :monthlyDonation %>
    <%= label_tag(:monthlyDonation, "Recurring Monthly Donation") %>
  </p>
  <script>
    var handler = StripeCheckout.configure({
      key: '<%= Rails.configuration.stripe[:publishable_key] %>',
      token: function(token, arg) {
        document.getElementById("stripeToken").value = token.id;
        document.getElementById("stripeEmail").value = token.email;
        document.getElementById("stripeName").value = token.card.name;
        document.getElementById("chargeForm").submit();
      }
    });
    document.getElementById('btn-buy').addEventListener('click', function(e) {
      handler.open({
        address:        true,
        amount:         document.getElementById("amount").value * 100,
        currency:       'usd',
        description:    'Donation to Thrive',
        name:           'Thrive Ministry',
        panelLabel:     'Donate {{amount}}',
        image:          'assets/logo-thrive-stripe.jpg'
      });
      e.preventDefault();
    })
  </script>
<% end %>

Thank you for your help!

2

There is quite a lot to be done here, but take it in small steps. The controller is definitely doing too much, so I would aim to extract most of the behaviour into a service object. That will make things easier to test and refactor.

Service objects won’t have access to the params hash, so you will need to pass those values to the initializer. Also, you can’t perform a redirect from within the service object, so that logic will need to stay in the controller. It seems that redirect_to @payment happens in all four donation scenarios, so you should be able to move that call into the create method.

You should end up with something like:

def create
  handles_donation = HandlesDonation.new(params)
  handles_donation.process
  @payment = handles_donation.payment
  redirect_to @payment
rescue Stripe::CardError => e
  flash[:error] = e.message
  redirect_to donations_path
end
3

Thanks @andyw8 I am now reading about services object. Any good resources you recommend?

4

Here are a couple of useful articles:

1 Like
5

I would add that instead of HandlesDonation, I think you should consider calling this class just Donation.

1 Like
6

Thanks guys. I am going to study about service object this weekend. :smile:

7

Another awesome blog post

http://brewhouse.io/blog/2014/04/30/gourmet-service-objects.html

8

Thanks @Austin! @andyw8 what else would you recommend to better this code after implementing service objects? While controller seemed to reduce the code amount, I’m still having hard time refactoring code in service object class itself…

9

@anthony

It really depends, obviously there is stuff that CAN be done but at the same time you don’t want to spend time optimizing or refactoring unless you are just doing it to learn/have fun and you don’t have anything else more valuable that you could be doing for the app.

The next step I would take would be to abstract the payment processing and plans and then use the donation as more of a collaborator between them. The example below also uses something called Dependency Injection, which is just a fancy way of saying that i pass in an object that provides some sort of service to the receiving object. So, you can easily switch from Stripe to Recurly by just creating a proxy class that implements .charge from recurly’s perspective.

class Donation
  def initialize(subscription_type, current_user, payment_processor=StripeCharger.new)
    @transaction_type = transaction_type
    @current_user = current_user
    @payment_processor = payment_processor
  end       

  def process
@payment_processor.charge(@transaction_type, @current_user)
  end
end

Then your subscription/payment processor would be something like this.

class TransactionType
  def initialize(amount,recurrance)
    @amount = amount.to_f.round(2)
    @recurrance = recurrance
  end    

  def amount_in_cents
    @amount * 100
  end

  def interval
    @recurrance
  end

  def description
    "Thrive Online #{@recurrance} Donation"
  end
end

class StripeCharger
  def charge(transaction_type, current_user)
    #deal with your transaction and user stuff here
    #i.e. logic for creating plans, sending to Stripe, etc
    #return the status code (200, 404, etc) or true/false, etc
  end
end

Then finally in your controller your create would just do this

def create
  #note, transactiontype.new can be inlined, separated for readability
  transaction_type = TransactionType.new(amount, interval)
  response = Donation.new(transaction_type, current_user).process

  #Handle success based on the response from the donation process
end

I struggled a little bit with the naming on the transaction type, that sometimes means that things are a little too coupled. The reason is that this is sometimes a subscription and sometimes a single transaction. Those are two different things, so my next step would be to use the transactiontype to return an object. That object will either be a single transaction or a recurring transaction. On that you would implement the different things like monthly_donation, etc.

Ultimately, you will have to weigh the complexity and extend-ability of adding more objects vs the duplication in your class.

Hope this helps, happy to receive critiques on this as well. It has been my approach for SaaS subscriptions for the last 8 months or so.

10

Thanks @Austin This is so much help. Thank you so much for taking time to write this, it will help me to study deeper on dependency injection too. So much to learn! I am just learning refactoring so I can practice to be a better developer.

11

While I don’t have anything much to add about this specific piece of code, I would recommend you go through the Intermediate Rails Workshop - Intermediate Ruby on Rails Course | Upcase Tutorials. I just did and it has so much valuable information with helping you think through how to decouple stuff, and use concerns and other design patterns to keep as many methods as single responsibility as possible.

That workshop is long, but I think it will go a long way with helping you.

1 Like
12

This was really good blog post for me for understanding service object. Thanks @Austin.