2

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 something different?

Andrew
  • 42,517
  • 51
  • 181
  • 281

3 Answers3

1

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.

BaroqueBobcat
  • 10,090
  • 1
  • 31
  • 37
  • Awesome suggestion, I had no idea about association extensions. Also thank you for clearing up the Demeter issue for me. I was thinking along the lines of, Photos only know they have land uses, they shouldn't be concerned with how land uses format themselves. But I see your point about it's just interacting with the content it has. This was very helpful! – Andrew Aug 21 '11 at 21:16
0

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 !

Cydonia7
  • 3,744
  • 2
  • 23
  • 32
  • The goal is to create a consistent helper method that returns, instead of an array of land use objects, a list of their names, from the LandUse class. This suggestion doesn't help me do that. I am familiar with to_s and that does potentially save one command, but the idea is to be able to specify to land_uses that I want to return the list of them as a concatenated string rather than an array of instances. – Andrew Aug 21 '11 at 19:11
0

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.

Paul.s
  • 38,494
  • 5
  • 70
  • 88
  • Ok, so I should move this method to LandUse -- but the problem is, how? I can't call .each within a class method, and calling methods on self doesn't allow me access to the scoped instances being called from a photo. Do you know how to get around that? – Andrew Aug 21 '11 at 19:12
  • I would probably leave the implementation in the controller and just move it into a private method so I don't have to type `.map(&:name).join(', ')` multiple times. If you want a more concrete solution then maybe extending ActiveRecord but I am not sure where to start looking for monkey patching your method into it – Paul.s Aug 21 '11 at 19:25