6b5a68d41436ce28831a0e0ca6bcd124

We have a model where a person has many phones, emails, addresses, etc. It would be nice to refactor these into something more compact. I'm basically replicating the "model_attributes" and "save_model" functions over and over for each piece of contact information I add to a user.

Model

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
  def phone_attributes=(phone_attributes)
    phone_attributes.each do |attributes|
      if attributes[:id].blank?
        phones.build(attributes)
      else
        phone = phones.detect { |t| t.id == attributes[:id].to_i }
        phone.attributes = attributes
      end
    end
  end
  
  def save_phones
    phones.each do |t|
      if t.should_destroy?
        t.destroy
      else
        t.save(false)
      end
    end
  end
  
  def email_attributes=(email_attributes)
    email_attributes.each do |attributes|
      if attributes[:id].blank?
        emails.build(attributes)
      else
        email = emails.detect { |e| e.id == attributes[:id].to_i }
        email.attributes = attributes
      end
    end
  end
  
  def save_emails
    emails.each do |e|
      if e.should_destroy?
        e.destroy
      else
        e.save(false)
      end
    end
  end

Refactorings

No refactoring yet !

3f91cf60c92b20940674ebdeb46f6582

Brian

October 16, 2007, October 16, 2007 21:22, permalink

2 ratings. Login to rate!

Refactored!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
  %w{phone email}.each do |prop|
    src = <<-END_SRC
        def #{prop}_attributes=(#{prop}_attributes)
          #{prop}_attributes.each do |attributes|
            if attributes[:id].blank?
              #{prop.pluralize}.build(attributes)
            else
              #{prop} = #{prop.pluralize}.detect{ |t| t.id == attributes[:id].to_i}
              #{prop}.attributes = attributes
            end
          end
        end
        
        def save_#{prop}
          #{prop.pluralize}.each do |t|
            if t.should_destroy?
              t.destroy
            else
              t.save(false)
            end
          end
        end
    END_SRC
    class_eval src, __FILE__, __LINE__
  end
6b5a68d41436ce28831a0e0ca6bcd124

Etandrib

October 16, 2007, October 16, 2007 21:56, permalink

No rating. Login to rate!

Thanks for this. I think I understand it a little better. I need to look into the class_eval stuff you've done here. I haven't seen that before.

Avatar

Emmett

October 17, 2007, October 17, 2007 01:12, permalink

4 ratings. Login to rate!

You don't need eval to do this; I'm generally opposed to using it unless you actually can't get what you want any other way reasonably. There's a tiny amount of duplication in exchange for simplified code.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
def set_model_attributes(model_name, model_attributes)
  model_attributes.each do |attributes|
    if attributes[:id].blank?
      send(name).build(attributes)
    else
      model = send(name).detect {|t| t.id == attributes[:id].to_i}
      model.attributes = attributes
    end
  end
end

def save_models(name)
  send(name).each do |m|
    if m.should_destroy?
      m.destroy
    else
      m.save(false)
    end
  end
end

def save_phones() save_models(:phones); end
def save_emails() save_models(:emails); end

def email_attributes=(attributes) set_model_attributes(:emails, attributes); end
def phone_attributes=(attributes) set_model_attributes(:phones, attributes); end
3f91cf60c92b20940674ebdeb46f6582

Brian

October 17, 2007, October 17, 2007 02:21, permalink

No rating. Login to rate!

I think that's a good start for a plugin. Just create one method that defines both method and throw it in a lib directory ad you're good.

14b8667b3bdf64068647b96c26001e0d

Mike

December 4, 2007, December 04, 2007 02:13, permalink

No rating. Login to rate!

One small modification to the above posted code: Changed "model_name" argument for the set_model_attributes method to "name" otherwise error will be thrown for name being undefined. I like the idea, works well!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
def set_model_attributes(name, model_attributes)
  model_attributes.each do |attributes|
    if attributes[:id].blank?
      send(name).build(attributes)
    else
      model = send(name).detect {|t| t.id == attributes[:id].to_i}
      model.attributes = attributes
    end
  end
end

def save_models(name)
  send(name).each do |m|
    if m.should_destroy?
      m.destroy
    else
      m.save(false)
    end
  end
end

def save_phones() save_models(:phones); end
def save_emails() save_models(:emails); end

def email_attributes=(attributes) set_model_attributes(:emails, attributes); end
def phone_attributes=(attributes) set_model_attributes(:phones, attributes); end

Your refactoring





Format Copy from initial code

or Cancel