← Back to Upcase

Law of Demeter and multiple model attributes

(Jerry Busser) #1

In a Rails app I have four models, D has one C; C belongs to B; B belongs to A. In a view template for D, we present a number of attributes off A. A typical line of code in from that template looked like

<%= @d_instance.c.b.a.attribute %>

Later, after Sandi Metz’ Four Rules dropped, we tidied things up a bit and wrapped our instance of D up in a presenter class and ended up creating methods on the presenter like

class DPresenter
  def attribute

to approximate a Facade pattern.

This cleaned up the view, which now had expressions like <%= d_instance_presenter.attribute %> (ta-da! rule four!). Some time after that, “Ruby Science” introduced me to Law of Demeter. I now look at what I did with that presenter and think I need to reconsider my approach.

Given that I need to present a half dozen attributes off A on this view, with the potential for more in the future, what can I do? Do I simply end up creating accessor methods on D that delegate to C for each model attribute I need off of A? Is there another way?

What about naming those methods? Let’s say I decide to put a new method on D that requests attribute from its neighbor, C, which, in turn, delegates all the way to A for the real value. Is there a generally accepted way to name those methods? I fear that a straightforward d.attribute will run in to namespace collisions down the road, so I am inclined to name it d.a_attribute, but that feels a little cheesy.

Relatedly, what do you all think of Rails’ has_one :through? If you squint really hard, it’s kinda-sorta addressing LoD. Do devs who try to follow LoD use it?

(Dolph Mullen) #2

It seems like you might be trying to access objects from the wrong end, so to speak. Depending on the domain of your app, rather than try to go down the chain, could you define class F which is a presenter, or makes logic in your system more explicit?

Even though the classes that you created are anonymous, I would bet that there is some implicit knowledge in the system that if you teased out into a new class would be made explicit. This will not only make it easier for others to understand the domain of the app, but I would also bet that you will have a better understanding of your own code.

(Luís Ferreira) #3

Apart from the solution @dolphorama proposed, you can also create a facade for the d and a instance:

class GoodNameThatRepresentsDAndA
  def initialize(d)
      @d = d
      @a = d.give_me_an_a

You can also delegate all methods of A you need from D to C, to B and then to A.

Another approach would be to do:

<%= render @d_instance.give_me_an_a %>

So you’ll have a partial for the instance of A.

Ultimately it seems a little odd that in a view of D you need so much of A, being that A is so distant of D. Maybe it would make sense to put them closer, maybe by adding a relationship between them.

(Jerry Busser) #4

Re: accessing objects from the wrong end, let me lay out some more specifics. I am reluctant to discuss my own domain objects because in the end, I don’t think it’s worth all the extra exposition. This provides a good approximation:

A is a Warehouse
B is a ShippingContainer
C is a Widget
D is an InspectionReport

So an instance of Warehouse has many ShippingContainers; an instance of ShippingContainer has many Widgets; a Widget will have zero or one InspectionReports. I’m in app/views/inspection_reports/edit.html.erb and the design requirements tell me I have to include a lot of contextual information in the view, e.g. this Widget belongs to this ShippingContainer, which is in this Warehouse. The user doesn’t need to edit those attributes in this view template; I just need them on the page in order to provide context. I need to display things like the warehouse’s address, GPS coordinates, general manager’s name or whatever. So in InspectionReportsController, I get an instance of InspectionReport, which feels correct because we still want a user to mutate that InspectionReport object and only that object, but I still need to get to Warehouse somehow.

What I’m trying to say is that I don’t think my models are incorrectly designed, nor do I think I’m approaching them the wrong way. Surely we all have needed to solve the same problem I just described (I have one object, and I need to display – and only display – attributes of parent objects). And I can do this just fine with the this-dot-that-dot-theotherthing notation, but I’m struggling with bringing LoD in to the mix.

As I mentioned, we already have wrapped our instance of InspectionReport up in a Facade instance, so that is in place, and that lines up with your suggestion, @zamith. We even discussed amongst ourselves yesterday the notion you suggested, where we implement InspectionReport#warehouse (or D#give_me_an_a), but that felt like violating the spirit of Law of Demter if not the letter. But it seems like the choices really boil down to

  • Create a new object that aggregates attributes of InspectionReport and Warehouse. Doesn’t seem like a good fit in my case
  • Define individual methods on InspectionReport that delegate to specific attribute accessors, e.g. InspectionReport#warehouse_address. Feels a little clunky because we have to touch two or three objects each time the design requirements change and we need to access a different attribute on Warehouse
  • Implement a Facade and individual accessors on it


(Luís Ferreira) #5

Why not? I would probably go with this one, which is an implementation of the facade pattern.

Or you might just have different partials:

<%= render 'warehouse_stuff', warehouse: inpection_report.warehouse %>
<%= render 'inspection_report_form', inspection_report: inpection_report %>

(Jerry Busser) #6

Sorry. I was being imprecise with my language. When I said in option one “create a new object,” I meant “create a new model,” as in something to represent a relationship between InspectionReport and Warehouse. Since I don’t see any relationship there, aside from requirements in the view, I dismissed the option. I agree that a Facade implementation is probably the best approach overall.