class Page < ActiveRecord::Base
def assigned_widgets=(string)
array = string.split(',')
self.widgets.clear
1.upto(array.size) do |i|
w = Widget.find(array[i-1])
w.position = i
self.widgets << w
end
end
Refactorings
No refactoring yet !
Michael Nutt
July 10, 2008, July 10, 2008 03:48, permalink
A simple refactoring you could do would be to take advantage of array's #each_with_index. It ends up putting the first item in position 0, which may or may not be a problem. Usually people are just doing :order => "position ASC" so it doesn't matter.
class Page < ActiveRecord::Base
def assigned_widgets=(string)
widget_ids = string.split(',')
self.widgets.clear
widget_ids.each_with_index do |widget_id, i|
widget = Widget.find widget_id
widget.position = i
self.widgets << widget
end
end
Anthony Green
July 10, 2008, July 10, 2008 12:57, permalink
I don't know why you'd want to clear the widgets and instantiate them just to assign them to the page object.
I'm guessing that you're missing a domain model.
class Page < ActiveRecord::Base
has_many :assignments
has_many :widgets, :through => :assigments
end
class Assignment < ActiveRecord::Base
acts_as_ordered_list :scope => :page
belongs_to :page
belongs_to :widget
end
class Widget < ActiveRecord::Base
has_many :assignments
end
in the controller
widget_ids = string.split(',')
assignments = widget_ids.enum_for(:each_with_index).map{|id,i| Assignment.new(:widget_id => id, position => i+1)}
@page.assigments = assignments
zoopzoop
July 10, 2008, July 10, 2008 15:15, permalink
Save on more line and one variable
class Page < ActiveRecord::Base
def assigned_widgets=(string)
self.widgets.clear
string.split(',').each_with_index do |widget_id, i|
widget = Widget.find widget_id
widget.position = i
self.widgets << widget
end
end
mitch
July 10, 2008, July 10, 2008 23:47, permalink
Michael and zoopzopp:
Thanks for the refactorings, I think the final result is more readable.
Anthony:
I don't like your refactoring because it made me thing about my design too much! :)
I'm really wrestling about your refactoring in my mind. I see the advantages of having an intermediate model for assignments which would create a list. On one hand I don't want that extra model, especially if it just handles the "listing" aspect of widgets. I think a many-to-one association is what I was going for here rather than the many-to-many association that you created.
On the other hand if I ever wanted to work with the assignments more or have the same widget on multiple pages I would definitely do that. In the future a simple list of widgets on a page is probably not going to be enough as a widget may have to be assigned to a column for instance.
>I don't know why you'd want to clear the widgets and instantiate them just to assign them to the page object.
As it stands now I DON'T think that I'm instantiating Widgets here and you ARE instantiating Assignments. Maybe by instantiating you mean looking up the widgets in the database? (Maybe this is a correct use of the term). Still instantiating Assignments (your technique) is probably less expensive than looking up Widgets.
Thanks for the help!
Anthony Green
July 11, 2008, July 11, 2008 17:38, permalink
Mitch
Yes if the relationship is one to many then the extra domain model is unnecessary.
However by calling Object.find you may be making database calls and instantiating objects unnecessaryly.
ActiveRecord provides methods to update association models without having to instantiate them:
@page.widget_ids = [1,2,3]
or
@page.widgets.build{[{:id => 2, :position => 1 }, {:id => 3, :position => 2 }]}
It may be worth playing with the console to see if either of these methods can generate the result you need.
Tony
@page.widget_ids = [1,2,3]
or
@page.widgets.build{[{:id => 2, :position => 1 }, {:id => 3, :position => 2 }]}
I tried playing with this a bit but had trouble even making it work. The method gets a list of widgets in a string that looks like "4,3,6,1,12" from jQuery's sortable plugin. "3,1,2" would mean that widget with id==3 is first, id==1 is second and id==2 is last.
The list must be recreated (I think). If an empty string came in that would mean that the page should now have no widgets.
This code looks cryptic to me and may be slow. Thanks for any help!