CODEWITHSUNDEEP
ruby-on-railsactiverecord
Ryan Florence
Ryan Florence

Reputation: 13483

Is there a better way to get this data?

Photographers "have_many" clients.

Clients "have_many" events.

Is there a better way to assign @events here if the user is a photographer?

  def index
    if @current_user.photographer?
      @events = []
      @current_user.clients.each do |client|
        @events << client.events
      end
    else
      @events = @current_user.events
    end
  end

Edit: More code

# user.rb
class User < ActiveRecord::Base

  has_many :client_associations, 
      :foreign_key => 'photographer_id', 
      :class_name => 'Association', 
      :dependent => :destroy
  has_many :clients, :through => :client_associations

  has_one :photographer_association, 
    :foreign_key => 'client_id', 
    :class_name => 'Association', 
    :dependent => :destroy
  has_one :photographer, :through => :photographer_association

  has_many :events

  def photographer?
    self.role == 'photographer'
  end

end

# association.rb
class Association < ActiveRecord::Base
  belongs_to :client, :class_name => "User"
  belongs_to :photographer, :class_name => "User"
end

# event.rb
class Event < ActiveRecord::Base
  belongs_to :user
  has_many :images      
end

As you can see my users are all in one model with a field called "role".

Upvotes: 2

Views: 126

Answers (2)

Alessandra Pereyra
Alessandra Pereyra

Reputation: 2640

That kind of logic, IMHO, would be more properly setup on the model layer.

You could create a new model method, like current_events on the User model, and move your logic there: 

def current_events
    if self.photographer?
         self.clients.find(:all, :include => :events).map(&:events).flatten
    else
         self.events
    end
end

Then, on your controller you could just add 

def index
  @events = @current_user.current_events
end

Thus, your logic is encapsulated on your model (and can later me improved, added with mor e complexity, tested) and your controller doesn't needs to know (and care) about what it is, just calls and show the current_events of the user.

Upvotes: 0

amitkaz
amitkaz

Reputation: 2712

From the db point of view, you should load all events at once and not have the N+1 problem.

  def index
    if @current_user.photographer?
      @events = @current_user.clients.find(:all, :include => :events).map(&:events).flatten
    else
      @events = @current_user.events
    end
  end

Upvotes: 2

Related Questions