DRY-ing up begin-rescue-end

Question

I have this create method:

def create
    ...
    grid = Tag.find_by_id(story[:tag_id]) or raise GridNotFoundError
    ...
    event = Event.find_by_id(story[:event_id]) or raise EventNotFoundError
    ...
  rescue GridNotFoundError
    flash.now[:error] = "Please select a category"
    @item = item
    @years = story[:years]
    @event_name = race[:name]
    @country = race[:country]
    @classification = race[:class]
    @events = Event.all
    @countries = Tag.countries
    @classifications = Classification.all
    @grids = Tag.grids.find(:all, :conditions => ["value != ?", "Channel Creation Grid"])
    render "home/race_updates"
  rescue EventNotFoundError
    flash.now[:error] = "Please select an event or create a new one if you don't find your event"
    @item = item
    @event = story[:event_id]
    @years = story[:years]
    @events = Event.all
    @countries = Tag.countries
    @classifications = Classification.all
    @grids = Tag.grids.find(:all, :conditions => ["value != ?", "Channel Creation Grid"])
    render "home/race_updates"
  rescue CreateEventError
    flash.now[:error] = "There has been a problem creating your event."
    params[:expand] = true
    @item = item
    @years = story[:years]
    @event_name = race[:name]
    @country = race[:country]
    @classification = race[:class]
    @events = Event.all
    @countries = Tag.countries
    @classifications = Classification.all
    @grids = Tag.grids.find(:all, :conditions => ["value != ?", "Channel Creation Grid"])
    render "home/race_updates"
  rescue ActiveRecord::RecordNotSaved, ActiveRecord::RecordInvalid, ActiveRecord::RecordNotFound
    flash.now[:error] = item.errors.full_messages.join(" ,")
    @item = item
    @event = story[:event_id]
    @years = story[:years]
    @events = Event.all
    @countries = Tag.countries
    @classifications = Classification.all
    @grids = Tag.grids.find(:all, :conditions => ["value != ?", "Channel Creation Grid"])
    render "home/race_updates"
  end

and as you can see, the rescues are almost the same. The rescues are also literal copy-pastes of the home#race_updates method.

I've got 2 questions:

1. Is there any way for me to DRY this up?
2. Is this a good pattern for controllers in general?

I've thought of separating it out as a function but I would need to pass in the flash message, item, story and race variables. I feel like it's not an elegant solution, but it would certainly be cleaner.

I've found that coding this way (i.e. raising errors and rescuing them) makes it easier for me to separate the actual business logic as it should be, and handling the different errors/cases that comes up in the business logic. So far, it works, but I want to gather opinion on if this is best practice or I'm not using begin/rescue as it is intended?

Answer 1

DRYing this up is a great idea and a good lesson in rails design (and Test Driven Development/TDD) if you can do so.

Ideally, you'd do something like this:

def create
    ...
    grid = Tag.find_by_id(story[:tag_id]) or raise GridNotFoundError
    ...
    event = Event.find_by_id(story[:event_id]) or raise EventNotFoundError
    ...
  rescue GridNotFoundError
    flash.now[:error] = "Please select a category"
    process_grid_not_found(item, story, race, etc...)
  rescue EventNotFoundError
    flash.now[:error] = "Please select an event or create a new one if you don't find your event"
    process_event_not_found(item, story, race, etc...)
  rescue CreateEventError
    flash.now[:error] = "There has been a problem creating your event."
    process_event_create_error(item, story, race, etc...)
  rescue ActiveRecord::RecordNotSaved, ActiveRecord::RecordInvalid, ActiveRecord::RecordNotFound
    flash.now[:error] = item.errors.full_messages.join(" ,")
    process_other_error(item, story, race, etc...)
  end
  render 'home/race_updates'

Then you should create the relavent new methods (process_event_not_found, etc) as separate (probably private) methods in the model.

This both makes the code much more readable, but has the great advantage be being much easier to write test code for.

So then you should write test code (using Test::Unit or rspec or whatever) that tests the isolated functionality required by each of the individual exception methods. What you'll find is that this both yields better code, as well as it also will likely break-down the exception methods into smaller, more modular methods themselves.

When you hear Ruby and Rails developers talk about the benefits of Test Driven Development, one of the main benefits of that approach is that it is much less likely to result in long, complex methods like the one you've got here. It's much more likely that you'll have code that is much DRYer, with smaller, simpler methods.

I'd also recommend that once you get through this you take another look and try to simplify it further. There will be more room for simplification, but I'd recommend refactoring it iteratively and starting with breaking it down as I've described and getting tests in place to start.

Answer 2

I combined Kevin Bedell's answer with Victor Moroz' insight, along with the fact that and and or in Ruby are flow control structures. I've come up with this:

def create
    ...
    grid = Tag.find_by_id(story[:tag_id]) or (render_race_updates(item, "Please select a category") and return)
    ...
    event = Event.find_by_id(story[:event_id]) or (render_race_updates(item, "Please select an event or create a new one if you don't find your event") and return)
    ...
    if item.save
      ...
    else
      render_race_updates item, item.errors.full_messages.join(", ")
    end
  rescue CreateEventError
    params[:expand] = true
    render_race_updates item, "There has been a problem creating your event."
  end
private
  def render_race_updates(item, message)
    flash.now[:error] = message
    # etc.
    render "home/race_updates"
  end

This way, I get to handle the exceptional cases on the line that it occurs without raising exceptions, while catching exceptions bubbled up by other methods.

I have yet to write tests, however. For now, I only write unit tests. Still getting the hang of RSpec and slowly changing my "wing it" development to TDD, but that's an entirely different conversation.

Answer 3

You'll have to verify this yourself if it's still 100% correct, cause I might have missed something, but perhaps thisis what you are looking for?

def create
    ...
    grid = Tag.find_by_id(story[:tag_id]) or raise GridNotFoundError
    ...
    event = Event.find_by_id(story[:event_id]) or raise EventNotFoundError
    ...
rescue Exception => e
    flash.now[:error] = e.is_a?(GridNotFoundError) ? "Please select a category" :
                        e.is_a?(EventNotFoundError) ? "Please select an event or create a new one if you don't find your event" : 
                        e.is_a?(CreateEventError) ? "There has been a problem creating your event." :
                        e.is_a?(ActiveRecord::RecordNotSaved) or e.is_a?(ActiveRecord::RecordInvalid) or e.is_a?(ActiveRecord::RecordNotFound) ? item.errors.full_mesages.join(", ") : e.to_s
    @item = item
    @years = story[:years]
    @event_name = race[:name] unless e.is_a?(EventNotFoundError) or e.is_a?(ActiveRecord::RecordNotSaved) or e.is_a?(ActiveRecord::RecordInvalid) or e.is_a?(ActiveRecord::RecordNotFound)
    @events = Event.all
    @countries = Tag.countries
    @classifications = Classification.all
    @grids = Tag.grids.find(:all, :conditions => ["value != ?", "Channel Creation Grid"])
    @event = story[:event_id] if e.is_a?(EventNotFoundError) or e.is_a?(ActiveRecord::RecordNotSaved) or e.is_a?(ActiveRecord::RecordInvalid) e.is_a?(ActiveRecord::RecordNotFound)

    if e.is_a?(GridNotFoundError) or e.is_a?(CreateEventError) 
        @country = race[:country]
        @classification = race[:class]

    end

    params[:expand] = true if e.is_a?(CreateEventError)

    render "home/race_updates"
end