CODEWITHSUNDEEP
sqlruby-on-railsactiverecordchaining
Sean Magyar
Sean Magyar

Reputation: 2380

which rails query/chain is better?

I have a rails app with the models below. I have both assigned_tasks and executed_tasks for a given user. I would like to know which option is better for getting all the tasks (executed and assigned as well) for that given user.

task.rb

belongs_to :assigner, class_name: "User"
belongs_to :executor, class_name: "User"

user.rb

has_many :assigned_tasks, class_name: "Task", foreign_key: "assigner_id", dependent: :destroy
has_many :executed_tasks, class_name: "Task", foreign_key: "executor_id", dependent: :destroy

Solution 1:

task.rb

scope :completed, -> { where.not(completed_at: nil) }
scope :uncompleted, -> { where(completed_at: nil) }

user.rb

def tasks_uncompleted
  tasks_uncompleted = assigned_tasks.uncompleted.order("deadline DESC")
  tasks_uncompleted += executed_tasks.uncompleted.order("deadline DESC")
  tasks_uncompleted.sort_by { |h| h[:deadline] }.reverse!
end

tasks_controller:

@tasks = current_user.tasks_uncompleted.paginate(page: params[:page], per_page: 12)

Solution 2:

task.rb

scope :completed, -> { where.not(completed_at: nil) }
scope :uncompleted, -> { where(completed_at: nil) }
scope :alltasks, -> (u) { where('executor_id = ? OR assigner_id = ?', u.id, u.id) }

tasks_controller

@tasks = Task.alltasks(current_user).uncompleted.order("deadline DESC").paginate(page: params[:page], per_page: 12)

Upvotes: 2

Views: 79

Answers (2)

Jordan Running
Jordan Running

Reputation: 106027

You should define an association on User that will return all of the Tasks associated by either executor_id or assigner_id:

class User < ActiveRecord::Base
  has_many :assigned_and_executed_tasks,
           ->(user) { where('executor_id = ? OR assigner_id = ?', user, user) },
           class_name: 'Task',
           source: :tasks
end

user = User.find(123)
user.assigned_and_executed_tasks
# => SELECT tasks.* FROM tasks WHERE executor_id = 123 OR assigner_id = 123;

Then you can do as you do in "Solution 2," but instead of the unfortunate Task.alltasks(current_user) you can just do current_user.assigned_and_executed_tasks (of course you could give it a shorter name, but descriptive names are better than short ones):

@tasks = current_user.assigned_and_executed_tasks
           .uncompleted
           .order("deadline DESC")
           .paginate(page: params[:page], per_page: 12)

Upvotes: 2

Devon C. Estes
Devon C. Estes

Reputation: 171

Solution 2 will be the more efficient way of retrieving the records from your database. In most Rails apps, calls to the database are a frequent cause of bottlenecks, and in solution 2 you make one call to the database to retrieve all the records, but in solution 1 you make two calls to the database to retrieve the same information.

Personally, I also think this solution is much more readable, easily testable, and maintainable, so solution 2 is better in many ways beyond speed!

Upvotes: 0

Related Questions