CODEWITHSUNDEEP
ruby-on-railsrefactoring
sam.killer
sam.killer

Reputation: 7

How would you refactor this

'def show

 @dept = Dept.find(params[:id])
    @members = @dept.members_list.collect{|a|a.name}
    dif = @dept.users.collect{|a|[a.name,a.id]}
    @admin_list = @dept.admin_users.collect{|a|a.user}
   @not_member_users = User.all_users - dif
   @not_admin_user = (@dept.list_of_deptmembers - @dept.admin_users).collect{|a|[a.user.name, a.id]}'

how can i refactor the @not_admin_user??

Upvotes: 0

Views: 60

Answers (3)

Benjamin Sullivan
Benjamin Sullivan

Reputation: 1476

It's better to deal with collections of objects, not arrays of attributes of objects. So @members should be an array of Member or User objects, not an array of strings for names.

This may not be a very good answer to your question, but I would follow the principle of outside-in development and aim to be able to get what you want like this:

def show
  @dept = Dept.find(params[:id])
  @members          = @dept.members
  @admin_list       = @dept.admin_users
  @not_member_users = @dept.non_member_users
  @not_admin_users  = @dept.non_admin_users
end

In your Dept model, set up scopes/associations that will return the appropriate results to the methods used above. Filtering results with Ruby is a last resort.

Upvotes: 0

Kevin Bedell
Kevin Bedell

Reputation: 13424

It's faster and cheaper in some cases to search tables based in id's -- this may be one of those cases.

@dept = Dept.find(params[:id])

# assuming both `members_list` and `admin_users` actually point to `User` objects
member_ids = @dept.members_list.collect(&:id)
admin_ids = @dept.admin_users.collect(&:id)
non_admin_id = member_ids - admin_ids

non_admin_users = User.where("id in ?", non_admin_id)

Upvotes: 1

pdoherty926
pdoherty926

Reputation: 10399

For starters, you can push all of that logic down into the Dept model.

def show
  @dept = Dept.find(params[:id])
  @admin_list = @dept.admin_list
  @members = @dept.members
  @not_member_users = @dept.not_member_users
  @not_admin_user = @dept.not_admin_user
end

class Dept < ActiveRecord::Base
  def members
    self.members_list.collect{ |a| a.name }
  end

  def admin_list
    self.admin_users.collect{ |a| a.user }
  end

  def not_member_users(user = User)
    dif = self.users.collect{|a|[a.name,a.id]}
    user.all_users - dif
  end

  def not_admin_user
    (self.list_of_deptmembers - self.admin_users).collect{|a|[a.user.name, a.id]}
  end
end

Upvotes: 0

Related Questions