CODEWITHSUNDEEP
ruby-on-railsdry
doctororange
doctororange

Reputation: 11810

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

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

Upvotes: 1

Views: 378

Answers (2)

askegg
askegg

Reputation: 664

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

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

Upvotes: 1

Senad Uka
Senad Uka

Reputation: 1131

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.

Upvotes: 2

Related Questions