3
\$\begingroup\$

What is a cleaner and/or clearer way of writing the following method

def disbursements
  @invoice = Invoice.find(params[:id])
  utc_time = Time.parse.now.utc

#check if invoice_id is not nil
  if @invoice.invoice_id == nil
    @invoice.errors[:invoice_id] << "invoice_id can not be blank"
  end
#check if trip date is now or past
  if utc_time <= @invoice.trips.first
    @invoice.errors[:trips] << "Funds can not be disbursed yet."
  end
#check if trip had been cancelled
  if @invoice.cancelled == true
    @invoice.errors[:cancelled] << "This invoice was already been cancelled."
  end
#check if this invoie has been processed before
  if @invoice.service_rendered == true
    @invoice.errors[:service_rendered] << "This invoice has already been disbursed."
  end
#check all is well
  if @invoice.errors.empty? == false
    render json: @invoice.errors, status: :unprocessable_entity
    return
  end

...

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi. Welcome to Code Review! Please change your title to reflect what your code does rather than the kind of review that you want. It can also help if you write out in words what you were trying to do with your code. As is, it can be difficult to tell if behavior is intentional or accidental. \$\endgroup\$ – mdfst13 Mar 25 '16 at 16:39
  • \$\begingroup\$ @mdfst13 updated the title as per your comment \$\endgroup\$ – CheeseFry Mar 25 '16 at 16:45
1
\$\begingroup\$

Inline conditionals would make this a bit cleaner:

def disbursements
  @invoice = Invoice.find(params[:id])

  @invoice.errors[:invoice_id] << 'invoice_id can not be blank' if @invoice.invoice_id.nil?
  @invoice.errors[:trips] << 'Funds can not be disbursed yet.' if Time.now.utc <= @invoice.trips.first
  @invoice.errors[:cancelled] << 'This invoice was already been cancelled.' if @invoice.cancelled
  @invoice.errors[:service_rendered] << 'This invoice has already been disbursed.' if @invoice.service_rendered

  return render json: @invoice.errors, status: :unprocessable_entity if @invoice.errors.any?
  ...
end

We can slim the method down and save a lot of space that way.

I removed the utc_time variable because it was only used once.

\$\endgroup\$
  • \$\begingroup\$ Will need to keep the return as there is more code following this that has business logic in it. an I add return to the end of the line? \$\endgroup\$ – CheeseFry Mar 25 '16 at 16:44
  • \$\begingroup\$ In that case you can do a return render, I'll modify the answer to show that. \$\endgroup\$ – MTarantini Mar 25 '16 at 16:50
0
\$\begingroup\$

I would turn this around and add validations on the model for the context of disbursing an invoice, the controller code is really small and only concerned with "controlling the code flow" then.

class Invoice
  # ...
  validates :invoice, presence: true, on: :disbursement
  validate :validate_trip_time, on: :disbursement 
  validate :validate_not_cancelled, on: :disbursement
  validate :validate_service_rendered, on: :disbursement 

  def disbursable?
   valid? :disbursement
  end

  private

  def validate_trip_time
    if Time.now.utc <= trips.first
      errors.add(:trips, "Funds can not be disbursed yet.")
    end 
  end

  def validate_not_cancelled
    if Time.now.utc <= trips.first
      errors.add(:cancelled, "This invoice was already been cancelled.")
    end 
  end

  def validate_service_rendered
   if service_rendered
     errors.add(:service_rendered, "This invoice has already been disbursed.")
   end 
 end
end 

# controller
def disbursements
  @invoice = Invoice.find(params[:id])

  if @invoice.disbursable?
    # disburse invoice?
    # render success response
  else
    render json: @invoice.errors, status: :unprocessable_entity
  end
end
\$\endgroup\$

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.