Sign in through GitHub

Please read for an updated status on RailsCasts:

Learn more or hide this

Adrian Mugnolo's Profile

GitHub User: xymbol

Comments by Adrian Mugnolo

Avatar

I don't think that would be good practice. Some questions:

  • What about route path translation?

  • Why would you need a parameterized route when you won't be adding new models in runtime? It's just 3 cases, not infinite associations.

Lastly, this could potentially clash with other routes in your routes.rb file depending on your routes order.

Avatar

Alain, to me and in this case, being verbose is a feature, not a bug. Back to your example, I don't think a commentable_id really belongs to the controller. Most of my other comments apply to your code too.

Avatar

Ryan,

I think the load_commentable before filter used in this RailsCasts episode is somewhat brittle and, probably too complicated to be used as an example, especially when demonstrating this feature to new users.

I would rather go with a simpler alternative. Something like:

def load_commentable
  @commentable = if params[:article_id]
    Article.find(params[:article_id])
  elsif params[:photo_id]
    Photo.find(params[:photo_id])
  elsif params[:event_id]
    Event.find(params[:event_id])
  end
end

Or even more verbose:

def load_commentable
  if params[:article_id]
    @commentable = Article.find(params[:article_id])
  elsif params[:photo_id]
    @commentable = Photo.find(params[:photo_id])
  elsif params[:event_id]
    @commentable = Event.find(params[:event_id])
  end
end

I see several benefits in this kind of code:

  • Code is less clever, which is a good thing for controller code.

  • Filter code is decoupled from changes to routes. For example, if a scope or namespace was added or the path option was used to produce easier to read routes, you would not need to change filter code or tests.

  • Scopes can be added independently to each find call. Articles could be published and events active, without the need for aliases.

  • Code produces no temporary objects nor relies on string processing which, over time, add up to app execution time.

  • Tests are easier to follow and coverage easier to check.

I would also like to mention that, in real world, some of the most popular uses of polymorphic associations, like comments, are a bad pattern.

In this example, article comments will never be queried as event comments nor -most probably- listed together and, they even might end up having very different requirements. In Ruby, behavior can always be shared using modules.

Still, there are some valid uses for polymorphic associations but, way less than what one would think of first.

P.S. thanks so much for your work and contribution! I admit this is one of the very few cases where I don't agree with the code presented. :-)