Method that reserves a reservable entity and charges the user This method works for what I need it to do, but I just don't think it's extremely readable, and could be abstracted into different methods. I have a feeling I'm going against some best practices. Please let me know your suggestions:

  def reserve
  return render json: { error: "This entity is not available to reserve" }, status: :forbidden if @reservable.status != Reservable::STATUS[:available]
  Reservable.transaction do
    @reservable.status = Reservable::STATUS[:reserved]
    @reservable.current_use = Use.create(
      user_id: @user.id, 
      reservable_id: @reservable.id, 
      start_location: @reservable.location, 
      start_time: DateTime.now, 
      status: Use::STATUS[:progress])
    # Check payment
    payment_type = params[:payment_type]
    if payment_type === Transaction::METHODS[:subscription]
      # put subscription logic here
    else
      return render json: { error: @user.errors }, status: :payment_required, location: new_payment_path unless @user.validates_has_payment_and_good_standing
      if payment_type === Transaction::METHODS[:prepay]
        @reservable.current_ride.trans = Transaction.charge_user_for_use(@user, @reservable.current_use, payment_type)
      else # :per_minute
        # put pay_per_minute logic here
      end
    end
    @reservable.save!
  rescue ActiveRecord::RecordInvalid =&gt; exception
    render :json =&gt; { :error =&gt; exception.messages }, status: :unprocessable_entity
    raise ActiveRecord::Rollback #force a rollback
  end
end
Brandon Keepers has a really good tip in his talk Why our code smells.


  Write a top level decription of your class without using the words "and" or "or". 


If you can't do that then there is a risk that your class might be doing to much. Since ActiveModel and AR and ActionController gives models and controllers such superpowers it's easy to fall into the trap of making godlike objects.

So when you have a method which has an AND in the description then you have definitely have a code smell.

The first major code smell here is @reservable.status != Reservable::STATUS[:available] this is just a leaky class with the internals hanging out like loose wires. Fortunately it's really easy to fix:

class Reservable
  def available?
    status == Reservable::STATUS[:available]
  end
end


But an even better way would be to crack out the awesomeness of ActiveRecord::Enum, as it will take care of the wiring for you:

class Reservable
  enum status: [:available, :reserved]
end 


This gives use reservable.available?, .reserved?, .reserved! etc.

One good pattern to refactor the above would be Service objects. Service objects are dirt simple plain old ruby objects that just do one job - tops. Most service objects just have a single public method - often #call

So lets see if we can start splitting this up into distinct tasks.

# Reserves a reservable
class ReservationService 

  def initialize(user:, reservable:)
    @user = user
    @reservable = reservable
  end

  def call
    unless @reservable.available?
      raise Reservable::UnavailableError.new(object: @reservable)
      return false
    end
  end
end


# Charges a user for services rendered
class ReservationChargingService

  attr_reader :transaction 

  def initialize(charge, user){
    @charge = charge
    @user = user
  }

  def call(amount, payment_type)
    # @todo charge user for amount
    # @todo return true / false or a status code for the payment
  end
end


This gives us nice detached pieces which can be tested in isolation - which is really nice since testing methods with DB transactions can be problematic. 

Then in your controller you can boil things down a bit:

def reserve

  Reservable.transaction do
    begin
    @reserved = ReservationService.new(
                    user: @user, 
                    reservable: @reservable
                ).call
    @charged = ReservationChargingService.new(
                    user: @user, 
                    reservable: @reservable
               ).call
    raise ActiveRecord::Rollback unless @reserved &amp;&amp; @charged
    rescue Reservable::UnavailableError
      raise ActiveRecord::Rollback, 'Reservable not available.'
    end
    rescue ActiveRecord::RecordInvalid
      raise ActiveRecord::Rollback, 'Validation error'
    end # @todo rescue other errors?
  end

  if @reserved &amp;&amp; @charged 
     render json: @reservable.current_use
  else 
     errors = [@charged.transaction, @reservable]
                 .reject(&amp;:valid?).map { |o| o.errors.full_messages }
     render json: { errors: errors }, status :unprocessable_entity
  end
end


As you can see here you want to delegate as much work to services as possible. Also prefer using ActiveModel::Errors instead of creating error hashes in your controller.

Our controller only has to deal with two contingencies here two which is really good: either the complete transaction passes and we return a success object or we return a errors hash.