Is this efficient?

Could you tell me if there is a better way.

Models:

class Skill
  has_many :tags
  has_many :positions
end


class Tag
  belongs_to :skill
  has_and_belongs_to_many :positions
end


class Position
  belongs_to :skill
  has_and_belongs_to_many :tags
end

I want to list all skills and the tags of their positions. Like this:

skill - tag tag tag tag
skill - tag tag
...

I ma开发者_如何学JAVAnaged to acheive it like this:

<% @skills.each do |skill| %>
  <%= skill.name %>   
  <% skill.positions.collect{|p| p.tags}.flatten.uniq.each do |t| %>
    <%= t.name %>
  <% end %>   
<% end %>

And my skills_controller:

def index
  @skills = Skill.all
end

Is this the right way?

Since tagging is a pretty common problem I'd recommend taking a look at acts-as-taggable-on, a widely used and very good gem for adding tags to any Rails model.

Regardless, your models look pretty good (except that has_and_belongs_to_many is often eschewed in favor for has_many :through), but I do see room for improvement here:

<% skill.positions.collect{|p| p.tags}.flatten.uniq.each do |t| %>
  <%= t.name %>
<% end %>  

Firstly, this is a lot of business logic to put in your view. You should do this in your controller instead. Secondly, it would be more performant to do it the other way around:

@tags = Tag.all :conditions => [ "tag.id IN (?)", skill.positions.map &:id ]

There are more efficient ways still to do this, but this ought to give you an idea.

First off - with any performance question you should measure the performance. Create twice as many skills, tags, and positions as you expect to need in a rake task. Then measure the page load times. If they're OK for your needs, cool. Otherwise, keep reading.

This is not particularly efficient since you will go across the network to the database for every skill to position mapping, and then again for every position to tag mapping. You can use the includes method to load using fewer queries - see the Rails guide on querying.