Topic: DRYing up my controller's exception catching

I started putting in some rescue clauses to catch exceptions, and I am finding myself repeating some of the code. Here's an example with the edit action:

def edit
    begin
      @course = Course.find(params[:id])
      if is_instructor && @course.owner_id != logged_in_user.id
        respond_to do |format|
          flash[:error] = 'You are not the owner of that course, cannot edit'
          format.html { redirect_to courses_path }
        end
      else
        if is_admin
          @users = User.load_user_hash
          @institutes = Institute.load_institutes_hash
        elsif is_instructor
          institute_id = logged_in_user.institute_id
        end
        @campuses = Campus.load_campuses_hash
        @areasofstudy = AreaOfStudy.load_areasofstudy_hash
        respond_to do |format|
          format.html
        end
      end
    rescue ActiveRecord::RecordNotFound:
      flash[:error] = 'Could not find a course by that id'
      redirect_to courses_path
    rescue Exception:
      flash[:error] = 'An error occured trying to display all the course information, please try again'
      redirect_to courses_path
    end
  end

So I am wondering how I would best separate that into its own method. I know I could for instance do:

def check_record(not_found_msg, exception_msg, not_found_path, exception_path)
  rescue ActiveRecord::RecordNotFound:
    flash[:error] = not_found_msg
    redirect_to not_found_path
  rescue Exception
    redirect_to exception_path
  end
end

And that would be usable across the board. However, I am wondering if there is a better way that I am not familiar with. I'm not sure if that may conflict with if statements or not. As well as how should I go about integrating it with the begin statement and such that I currently use. Thanks for your input.

Last edited by agm_ultimatex (2009-12-02 23:57:27)

Re: DRYing up my controller's exception catching

agm_ultimatex wrote:

I know I could for instance do:

Will that actually work? Not sure you can put the rescue clause in a separate method, it feels wrong somehow. According to 'Uncle' Bob Martin in his book Clean Code, it is a good idea to separate error handling from regular code to make it easier to see what's going on. Although his examples are Java based, the same principle could be used. He advocates putting the code that does the error handling into one method, and calling 'clean' methods that actually do the work and potentially throw the exceptions from there. But this is hard to generalise without turning every method call into a call_with_error_handling(object, method, *params) type of thing. So although you could claim that it is DRYing things up, it is also cluttering your code with hard-to-understand reflective method calls.

I'm relatively new at Ruby, so if this is hogwash, someone please correct me...

Re: DRYing up my controller's exception catching

You could be right. I havent done too much exception handling. Some simple examples I ran through in humble little ruby book showed using them in methods, however at the same time had variables, method calls, etc to run them against.

Re: DRYing up my controller's exception catching

This could benefit from a wrapper. Something like:

def edit
    catch_my_exceptions do
      @course = Course.find(params[:id])
      if is_instructor && @course.owner_id != logged_in_user.id
        etc. etc.
    end
end

private

def catch_my_exceptions(&block)
    begin

        yield  # This is the important point where your edit code is actually run.

    rescue ActiveRecord::RecordNotFound:
        flash[:error] = 'Could not find a course by that id'
        redirect_to courses_path
    rescue Exception:
        flash[:error] = 'An error occured trying to display all the course information, please try again'
        redirect_to courses_path
    end
end


To generalize this more, I think you would set some messages in instance variables (@err_msg = 'An error occurred etc'). It's also possible to set the redirect path that way.

Re: DRYing up my controller's exception catching

You can use rescue_from in controllers to define common exception handling behaviour. Keep in mind a few things though:

- exceptions should be exceptional, so ideally you should be writing code that doesn't raise any and use something like the exception notifier plugin to be told when something goes wrong in production

- rescuing the top level Exception is generally a bad idea as it can hide errors in your code (bit me more than once in my early Ruby days big_smile)

- rescuing exceptions is slower than handling situations in regular code, ok so it's milliseconds but performance may be important to you in the future

So for example, an alternative would be to use find_by_id and then check if the return value is nil, rather than using find and then catching an exception.

Last edited by rob-twf (2010-03-20 04:46:47)

Rob Anderton
TheWebFellas