# 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 => exception
render :json => { :error => 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

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 && @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 && @charged
render json: @reservable.current_use
else
errors = [@charged.transaction, @reservable]
.reject(&: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.

• Phenomenal. I have a couple questions. Can you explain the .reject method. it seems like that would work only for the @reservable, but by putting it in an array, will it call it for both objects? I'm assuming ReservationChargingService.call should return a transaction? otherwise, I'm not sure where /@charged.transaction comes from. Also one initialize method has a colon after the params, and not in another method. Was that a mistake? – TheJKFever Jul 10 '15 at 23:45
• Thats enumerable#reject, it removes elements from the array where the test is truthy. I made a little slip up as you noticed - the array should have been [@charged.transaction, @reservable]. I have omitted quite a few parts to focus on the general concepts - the ReservationChargingService could return a transaction or you could assign the transaction as a instance variable in somehere in #call. – papirtiger Jul 11 '15 at 2:35