In my app, Photo has_and_belong_to_many :land_uses
I have this helper method in the Photo
model:
def land_use_list
land_uses.map(&:name).join(', ')
end
This strikes me as a code smell (demeter), but I haven't been able to figure out how to move it to the LandUse model. What I'd like to do is something like:
class LandUse < ActiveRecord::Base
...
def self.list
self.map(&:name).join(', ')
end
...
end
So that instead of calling photo.land_use_list
I could call photo.land_uses.list
.
But that doesn't work, because it gets called against the class instead of being called against the scoped instances belonging to a particular photo.
Is there a way to do what I'm thinking of? And, more generally, how do you approach issues like this in your app? Is moving the list code to the LandUse model the right approach, or would you recommend som开发者_如何转开发ething different?
First, I don't think this is violating the Law of Demeter per se. You have a method on an object that calls one method on an attribute to create a temporary variable, and then act on the temporary variable.
It would be a violation of the Law of Demeter, if you were doing this from a different class entirely. eg
class User
def names_of_lands_ive_known
photos.map(:land_uses).map(:name).join ', '
end
end
As it is, it's just good information hiding. But, if you wanted to be able to write photo.land_uses.names
, you could add an extension to the association to do what you want.
class Photo
has_and_belong_to_many :land_uses do
def names_as_list_string
all.map(:name).join ', '
end
end
end
For more information on association extensions, check out the docs.
The best way to conform to the law of demeter is to do more or less what you are doing though, because by adding your method on Photo
, it means that the methods that interact with Photo
, don't also need to know about the LandUse
class, just that photo has a method that returns a string of the names of land uses.
I am not in front of a rails app but I believe
photo.land_uses
with return an Array of LandUse
objects
So you just need to move your map down to that array like:
photo.land_uses.map(&:name).join(', ')
which is what you had originally - just in your other model. I think you may be right and it means that Photo
knows too much about LandUse
therefore I would move it out.
You can use :
class LandUse
def self.list_for_photo(id)
LandUse.find_by_photo_id(id).join(', ')
end
def to_s
self.name
end
end
Hope it helps !
精彩评论