0

enter image description here

As I wrote this method I knew it could be written better, but it's an MVP. Now I am trying to learn what the experts feel the approach to be and how to solve the cops. Constant Learning!

Here is the controller method in question:

  def show
    if params[:id] && numeric?(params[:id])
      @job = Job.find_by(id: params[:id])
      if @job
        if @job.start_at
          if @job.end_at && @job.end_at >= Date.today
            @company = Company.find_by_id(@job.company_id)
            render 'jobs/show', layout: 'nested/job/show'
          else
            route_expired
          end
        else
          route_not_paid_for
        end
      else
        route_not_found_error
      end
    else
      route_not_found_error
    end
  end

In turn these call a series of routes to direct the user where to go based on the type of error they obtained:

  private

  def route_not_found_error
    flash[:error] = t('flash_messages.jobs.id.not_found')
    redirect_to new_job_url
  end

  def route_expired
    flash[:error] = t('flash_messages.jobs.id.expired')
    redirect_to new_job_url
  end

  def route_not_paid_for
    flash[:error] = t('flash_messages.jobs.id.not_paid_for')
    redirect_to job_payment_url(@job)
  end

To the experts, how would you approach this? I know this must be cleaner.

Chris Hough
  • 3,389
  • 3
  • 41
  • 80

1 Answers1

2

First, I would move some of the logic into the Job model

def started?
  self.start_at.present?
end

def finished?
  self.end_at.present?
end

def expired?
  finished? && self.end_at < Date.today
end

Next, checking for the existence of :id and whether it is numeric is unnecessary in my opinion, you can just use rescue_from within your controller. Putting that together, you can refactor the show as follows

class JobsController < ApplicationController
  rescue_from ActiveRecord::RecordNotFound, with: :route_not_found_error

  def show
    @job = Job.find params[:id]

    route_not_paid_for unless @job.started?
    route_expired if @job.expired?

    @company = Compandy.find_by_id(@job.company_id)
    render 'jobs/show', layout: 'nested/job/show'
  end

  private

  ...

end
Bart Jedrocha
  • 11,450
  • 5
  • 43
  • 53
  • Yo. This is pretty awesome. I'm not sure there's more to say, but I may or may not try anyway. – jvillian Feb 27 '16 at 01:25
  • I noticed you changed this to find instead of find_by, was there a reason for this? do they work the same? – Chris Hough Feb 27 '16 at 20:27
  • 1
    `find` is explicitly find by id. There is a key difference between `find` and `find_by` and that is that `find_by` will return `nil` if it can't find the record while `find` will throw an `ActiveRecord::RecordNotFound` exception if it can't find it. This convention allows us to use `rescue_from` as shown in my example to execute a handler when the resource can't be found. Hope that explains it. – Bart Jedrocha Feb 27 '16 at 20:34
  • thank you for the detailed explanation, a great lesson learned today – Chris Hough Feb 27 '16 at 22:56