Ruby method that is validating the payload received

Question

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

...

• mdfst13: 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.
• CheeseFry: @mdfst13 updated the title as per your comment

Answer 1

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.

• CheeseFry: 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?
• MTarantini: In that case you can do a return render, I'll modify the answer to show that.

Answer 2

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