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


...

• 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. – mdfst13 Mar 25 '16 at 16:39
• @mdfst13 updated the title as per your comment – CheeseFry Mar 25 '16 at 16:45

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.

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

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
end
end

def validate_service_rendered
if service_rendered
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