3
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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 && @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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – TheJKFever Jul 10 '15 at 23:45
  • \$\begingroup\$ 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. \$\endgroup\$ – papirtiger Jul 11 '15 at 2:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.