CODEWITHSUNDEEP
ruby-on-railsactiverecordrefactoring
Sean Magyar
Sean Magyar

Reputation: 2380

rails refactoring AR query

I'm trying to refactor my scope, and my controller, but I feel it could be better. From the controllers perspective the new code is definitely better, but I think the alltasks scope and the all_uncompleted_tasks_ordered_by_deadline instance methods are kinda weird. Any ideas?

user:

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

task:

belongs_to :assigner, class_name: "User"
belongs_to :executor, class_name: "User"
scope :alltasks, -> (u) { where('executor_id = ? OR assigner_id = ?', u.id, u.id) }
scope :uncompleted, -> { where(completed_at: nil) }

tasks controller

def index
  @tasks = Task.alltasks(current_user).uncompleted.includes(:executor, :executor_profile, :assigner, :assigner_profile).order("deadline DESC")
end

NEW CODE:

user.rb

def all_uncompleted_tasks_ordered_by_deadline
  Task.alltasks(self).uncompleted.includes(:executor, :executor_profile, :assigner, :assigner_profile).order("deadline DESC")
end

tasks controller

def index
  @tasks = current_user.all_uncompleted_tasks_ordered_by_deadline
end

Upvotes: 1

Views: 21

Answers (1)

jethroo
jethroo

Reputation: 2124

I rather think that the all_tasks wants to be in the User model like the assigned and executed are already. Maybe you can move it there as method and apply the needed Task scopes in the controller.

say:

#user.rb
def all_tasks
  Task.where('executor_id = ? OR assigner_id = ?', id, id)
end

#tasks_controller.rb
def index
  @tasks = current_user.all_tasks
                       .uncompleted.includes(:executor, :executor_profile, :assigner, :assigner_profile)
                       .order(deadline: :desc)

end

Upvotes: 1

Related Questions