How can I reduce repetition in this Ruby on Rails code?

Question

This is a snippet of code from an update method in my application. The method is POSTed an array of user id's in params[:assigned_ users_ list_ id]

The idea is to synchronise the DB associations entries with the ones that were just submitted, by removing the right ones (those that exist in the DB but not the list) and adding the right ones (vise-versa).

 @list_assigned_users = User.find(:all, :conditions => { :id => params[:assigned_users_list_id]})
 @assigned_users_to_remove =  @task.assigned_users - @list_assigned_users
 @assigned_users_to_add =  @list_assigned_users - @task.assigned_users

    @assigned_users_to_add.each do |user|
  unless @task.assigned_users.include?(user)
   @task.assigned_users << user
  end
 end
 @assigned_users_to_remove.each do |user|
  if @task.assigned_users.include?(user)
   @task.assigned_users.delete user
  end
 end

It works - great!

My first questions is, are those 'if' and 'unless' statements totally redundant, or is it prudent to leave them in place?

My next question is, I want to repeat this exact code immediately after this, but with 'subscribed' in place of 'assigned'... To achieve this I just did a find & replace in my text editor, leaving me with almost this code in my app twice. That's hardly in keeping with the DRY principal!

Just to be clear, every instance of the letters 'assigned' becomes 'subscribed'. It is passed params[:subscribed_ users_ list_ id], and uses @task.subscribed_ users.delete user etc...

How can I repeat this code without repeating it?

Thanks as usual

Answer 1

You don't need if and unless statements. As for the repetition you can make array of hashes representing what you need. Like this:

   [ 
    { :where_clause => params[:assigned_users_list_id], :user_list => @task.assigned_users} , 
    {  :where_clause => params[:subscribed_users_list_id], :user_list => @task.subscribed_users} 
    ] each do |list| 
        @list_users = User.find(:all, :conditions => { :id => list[:where_clause] })
        @users_to_remove =  list[:user_list] - @list_users
        @users_to_add =  @list_users - list[:user_list]

        @users_to_add.each do |user|
            list[:user_list] << user
        end
        @users_to_remove.each do |user|
            list[:user_list].delete user
        end
      end

My variable names are not the happiest choice so you can change them to improve readability.

Answer 2

I seem to be missing something here, but aren't you just doing this?

@task.assigned_users = User.find(params[:assigned_users_list_id])