Let’s take the example of the depot app. In my controller I set @order.
Then in the view comes the bad stuff:
@order.items.each do |item|
item.product.title
end
Now I’m having problems specing item.product.title. A quick and dirty
fix is to trade a for for an underscore, so I create Item#product_title:
def product_title
product.title
end
The problem is that I have a few models that act the same way, so I
would find myself writing lot’s of these little instance methods, and
the solution looks a bit stupid to me. So how do you handle such issue?
I thought about composed_of, but AWDWR’s examples still break Demeter’s
law and that annoys me.
On Sat, Apr 18, 2009 at 10:17 AM, Fernando P. [email protected]
wrote:
end
And this still doesn’t satifsy the “Law” of Demeter because you’re
still calling order.items[x].product_title. Simply adding a method
call in between doesn’t obviate the fact that you need the order’s
item’s product’s title.
I would split the difference and have an orders helper that knew how
to format each component of a displayed item.
Now I’m having problems specing item.product.title. A quick and dirty
issue?
Personally, I don’t think its so bad to violate the ‘Law of
Demeter’(which of course is really just a guideline) in cases such as
these dealing with domain models. All but the simplest apps will have
entities that relate to other entities and each entity will of course
have its own attributes. Sometimes I’ll bring up a property from a
child entity if its extremely common(as may be the case with
product_title) but that’s not the case most of the time. In this case,
IF I was trying to spec a view for an ‘Order’, I’d probably use a
factory method of some sort that would handle creating the desired
object graph. If these models already exist, I usually use real
objects in place of mocks. There are quite a few Factory/Builder type
libs out there for ActiveRecord. In my case, we’re typically dealing
with Java/Hibernate domain layers through JRuby so I haven’t used any
of them, but its been simple enough to roll my own so far.
it “should show product title” do
order = new_order(:title => ‘the product title’)
item.product.title
The problem is that I have a few models that act the same way, so I
would find myself writing lot’s of these little instance methods, and
the solution looks a bit stupid to me. So how do you handle such
issue?
The risk you’ve identified here is that your views are coupled to the
relationship between an order and the title of the products that make
up that order. If you were coding down in your model layer and decided
to make a change to the structure of your models then code far, far,
away in the views would break. In a small application like a blog,
this might not be a problem, but as your codebase starts to get
bigger, it’s a worry you want to avoid.
Putting on your ‘outside-in’ hat and thinking about what the view
wants, you could code the view, which doesn’t need or want to care
about the structure of these models, more like this:
@order.product_titles do |product_title|
<%= product_title %>
end
Now you’re going to need to add this Order#product_titles method to
your model layer. Having this extra method buys you more
maintainability in the future: the structure of the model objects is
hidden from the views, and if you need to change that structure, the
code you’ll need to change to keep the views working
(Order#product_titles) is right down there next to you in the model
layer where you’re working. This sort of cohesion - keeping the things
that change together in the same part of the codebase, makes
maintenance much easier and less accident-prone.
By designing a custom protocol (think ‘API’) on your model layer for
the use by the views, rather than constraining yourself to the ones
that come out of the box with ActiveRecord, you’re putting some
lubrication in between the layers, making it easier for them to move
independently.
The trade-off, obviously, is that you’re adding extra methods to your
models. If you imagine doing this for every view on a large project,
you might end up with quite a bit of clutter on those models. This is
where you could consider adding another ‘facade’ layer that wraps the
models and presents them to the views in the appropriate manner. At
Songkick, we use a presenter layer for this purpose, and it works very
nicely.
@order.product_titles do |product_title|
<%= product_title %>
end
Another problem, is that not only do I need the title, but also a
clickable permalink which uses a url helper (not available to models),
the product price, and maybe other stuff in the future. So I might end
up with a nice messy code.
This is where I start to introduce a presenter layer in between the
view and the models.
If you have a Ruby class anywhere in rails, you can access the auto-
generated url helpers just by including ActionController::UrlWriter.
class OrderPresenter
include ActionController::UrlWriter
def initialize(order) @order = order
end
def product_titles @order.items.map{ |i| i.product.title, product_url(i.product) }
end
end
Construct the presenter in the Controller. Then in the view:
@order_presenter.product_titles do |product_title, url|
<%= link_to product_title, url %>
end
But I’ll have to think more about it, because as you said making a
little change in a model can result in having to fix places that are
far
far away and totoally unexpected.
@order_presenter.product_titles do |product_title, url|
<%= link_to product_title, url %>
end
The presentation of an order in most apps will be constructed from
additional columns (e.g., color, size, price, extended price). These
columns may well be formatted differently depending on the
circumstance (e.g, printed or on-screen).
Now, it might be a good idea to put this construction and formatting
in a helper or presenter outside the view (though it won’t be
TSTTCPW). But anywhere you put it, there will be non-Demeterian code
that needs to drill down into each order’s items’, products’
information (as well as non-product information, such as line-item
discount). If the underlying model changes, this code will have to
change - you can’t avoid it.
@order.product_titles do |product_title|
<%= product_title %>
end
Another problem, is that not only do I need the title, but also a
clickable permalink which uses a url helper (not available to models),
the product price, and maybe other stuff in the future. So I might end
up with a nice messy code.
For the time being I use delegate, so I traded a dot for an underscore.
It’s easy to spec, and after all, an item only exists in the context of
an Order, so I think it’s fine to break Demeter’s ‘guideline’ and say
that it’s cool that Order knows a little bit stuff about Item’s
internal. However a Product can exist without an Order, so Order doesn’t
need to know anything about Product and vice versa.
But I’ll have to think more about it, because as you said making a
little change in a model can result in having to fix places that are far
far away and totoally unexpected.
Now, it might be a good idea to put this construction and formatting
in a helper or presenter outside the view (though it won’t be
TSTTCPW). But anywhere you put it, there will be non-Demeterian code
that needs to drill down into each order’s items’, products’
information (as well as non-product information, such as line-item
discount). If the underlying model changes, this code will have to
change - you can’t avoid it.
Agreed. I think Dan M. post is a good one on this topic as well:
http://www.dcmanges.com/blog/37
His second to last paragraph:
“The crux of this is that webpage views aren’t domain objects and
can’t adhere to the Law of Demeter. Clearly from the examples of
behavior delegation the Law of Demeter leads to cleaner code. However,
when rendering a view, it’s natural and expected that the view needs
to branch out into the domain model. Also, anytime something in a view
dictates code in models, take caution. Models should define business
logic and be able to stand alone from views. If this “train-wreck”
method calling in your views is bothersome, there is a better solution
that I will blog about later.”
I tend to agree his thoughts on this, but be careful how you read the
sentence “anytime something in a view dictates code in models, take
caution”. He isn’t referring outside-in development practices. He’s
referring to adding methods in a model for strictly presentation
purposes, which is bad because it dilutes the richness of the model,
and makes it change for reasons it shouldn’t. Here’s another article
which discusses why methods shouldn’t be added to models for
presentation purposes:
Although if you read that article and want to try out presenters, you
should know that CachingPresenter supersedes the PresentationObject
library in the article. And you don’t need a library to use
presenters, your presenters can be POROs just like Matt W. posted.
I tend to like the CachingPresenter library (heck I wrote it) approach
so I can utilize caching, memoization, and a slightly more declarative
API and list of helper methods.
One technique I’ve employed in the past along side presenters
(although it can be done w/o presenters) is to make use of partials,
and actually treat them like little view objects. So you could take
what you had:
@order.items.each do |item|
item.product.title
end
And transform it into two views, one that is concerned with the order,
and a partial which is concerned with the product of an item. :
And then add a “items/product” partial and spec it in isolation.
Spec’ing views (and partials) in isolation gives you the ability to
have simpler view specs, and you’re able to avoid feeling the awkward
need to build up a network of objects. And you don’t have to worry
about adding a bunch of little delegate methods all over the place.
Now, it might be a good idea to put this construction and formatting
in a helper or presenter outside the view (though it won’t be
TSTTCPW). But anywhere you put it, there will be non-Demeterian code
that needs to drill down into each order’s items’, products’
information (as well as non-product information, such as line-item
discount). If the underlying model changes, this code will have to
change - you can’t avoid it.
The motivation behind demeter is to localize change. The cost is
method bloat and potential lack of cohesion on a model. Having a
single presenter object act as the one and only place that will change
besides the model itself is a good compromise between complete
localization of change in the model and method bloat on the model.
A very interesting approach to all this was presented by Allen Holub
in his talk boldly entitled “Everything You Know is Wrong,” in which
he tears apart common misunderstandings about OO.
The idea, as I understand it (but, according to his basic premise, I’m
probably wrong) is to have data importers and exporters on domain
objects in order to minimize getters and setters. These collaborators
are going to take the hit of changes to the model objects, but only
they will if you follow this approach. In the example we’re talking
about here, we’d end up with code like this in the controller:
def some_view @order = find_order.export_to(OrderPresenter.new)
end
OrderPresenter would have a bunch of setters on it, which the order,
which knows its own data structure would call. Now you don’t need a
bunch of getter methods on order.
Similarly, data importers would have a bunch of getters on them, and
you would use them to import data into an order like this:
order_form = OrderForm.new(params[:order])
order = Order.create_from(order_form)
Obviously, ActiveRecord provides the getters and setters anyway, but
the real violator of encapsulation is the consumer, not the vendor.
Just because a flasher opens his coat doesn’t mean that reaching
inside is a good idea
The Exceptional Presenter
Interesting idea too. So basically I need to totally rethink and
refactor the way my views display the information to the customer:
Let’s see how I currently display or not an add to cart button depending
whether or not the product is free.
My very first quick and very ugly procedural hard to spec solution was
to do in the view:
<%- if @product.price >= 0 -%>
<%= display_button %>
<%- else -%>
This product is free!
<%- end -%>
Then my second solution was to create an instance method so that the
view doesn’t know about the Product internal mechanism about it’s
freeness:
<%- if @product.free? -%>
This product is free!
<%- else … -%>
Yeah I thought I was an OOP master and Demeter could rest in peace!
But thinking about the GOF Builder and Exceptional Presenter design
patterns you talk about, that would mean that the html output for a
Product should therefore happen in the Model itself. And then in the
show.html.erb, I simply call:
<%= @product.display -%>
and all the magic about whether or not the product is free and to
display the button has already been handled inside the model (or another
related place) when it gets instantiated. Same applies to
item.product.title, that would be handled elsewhere than in the view.
However it might clutter the Model, so actually there is more to MVC:
each Model should have a sub class or something that handles how the
model instance will be presented to the view.
How do you handle such issue? Are there some open source rails apps that
I could learn from?
OrderPresenter would have a bunch of setters on it, which the order,
which knows its own data structure would call. Now you don’t need a
bunch of getter methods on order.
Similarly, data importers would have a bunch of getters on them, and
you would use them to import data into an order like this:
order_form = OrderForm.new(params[:order])
order = Order.create_from(order_form)
I think (but I’m probably wrong too!) that this is what people like
Nat Pryce and Steve Freeman are describing when they talk about
keeping state and behaviour separate. It’s something that functional
programming languages like Haskell force you to do, but it’s decent
practice to use day-to-day even in languages that let you mix the two
together, IMO.
Obviously, ActiveRecord provides the getters and setters anyway, but
the real violator of encapsulation is the consumer, not the vendor.
Just because a flasher opens his coat doesn’t mean that reaching
inside is a good idea
LOL. I had half-written my own reply to Mark, but that says it all
The way I go about this is, IMO, pretty straight forward. I cringe
when people start talking about generating html in models, since I
think separation of concerns is much more important than law of
demeter, and I think that the model most definitely shouldn’t be
concerned with how it needs to be displayed.
In your original example you have: @order.items.each do |item|
item.product.title
end
The way I would do this is: @order.items.each do |item|
render :partial => “product/line_item”, :locals => {:product =>
item.product}
end
In other words, I might have the following partials:
product/_line_item
product/_product (full representation of product probably for detail
page)
product/_search_item (for display in a search result)
etc.
All the partials simply take a product object. There is probably
already some smart, intention revealing name, but I call them
“nanoformats”.
I’m not exactly an Academic, so I have no idea the validity of my
approach, but this seems to get around the law of demeter enough for
me (thought, obviously not totally since order still technically
knows about order.items.products). Order doesn’t actually know
anything about products or it’s internals, except that it has some.
The partial could be reused in other places, and with semantic markup,
can be displayed in very different ways with a little CSS. A change to
Product only necessitates changes to the app/views/products/* files.
Your Orders don’t need to know anything, item probably doesn’t even
need to be updated. the partials can be spec’d independently of
wherever they are being used.
Your view and your models are always going to be tightly coupled, and
a change in your domain object should probably necessitate a change in
your views. That’s the way it should work. However, your views
shouldn’t be tightly coupled with each other but they will have to be
coupled in some way or another.
+1 what we are doing here is rendering resources in different contexts.
In
the original example we are rendering product resources in an order
context.
We should let the product define how it should be viewed - after all it
knows best. If we need different views in different contexts then just
create a new partial for product.
Generally for any resource I will create partials for following views
detail
summary
admin_summary
And create other partials for forms etc.
Another plus about this approach is that it helps guide consistency in
view
e.g. products look the same in ‘my recent orders’, ‘search for product
foo’,
‘my favourite products’ …
That’s a good example as far as it goes, but I think it makes a
distinction that isn’t quite the true distinction. For example, I
remember dragsters from the '60s that had as many as four engines. What
does car.engine.name do in that case? And the car I drive these days has
a pair of electric motors in addition to an internal combustion
engine…
Likewise with the stereo–some minivans and SUVs come with two
independent stereo/entertainment systems, one for the front seat
passengers and one for the rear. When I invoke car.stereo.has_cd_player,
am I asking about the “default” front-seat stereo, or do I only care
whether or not the car has a cd player somewhere?
The point, of course, is that digging down into the structure of an
object, even if structural information is what you’re after, requires
that you make assumptions about that structure. And that should be the
criterion. You have to ask yourself:
(a) What implicit assumptions am I making by doing this?
(b) Is is safe to make those assumptions?
If you can reasonably answer yes to the second question, then you’re in
good shape. But you still have to accept that future changes to the
design can lead to structural changes that retroactively invalidate your
answer.
Luke R. wrote something called Demeter’s Revenge which you might
want to look at. I’ve not used it though so I can’t tell you if it
will solve your problems.
I’m not sure how this ties to the application of Demeter’s law. I
think Pat’s point was that if what you’re interested in is an object’s
structure(such as to display information about the engine/engines in a
web page because customers want to see that) then applying Demeter’s
law is just “needless indirection without any benefits”. The decision
to model a car with Car#stereo Car#engine vs. Car#engines Car#stereos
has to be made(risk and assumptions considered here). If that
changes, things that renderinformation about the car engine
information WILL have to change. At that point I don’t see how
Car#engine_name, Car#engine_type is of any benefit over
Car#engine.name. Car#engine_type. This is in contrast to the same
references in the context of behavior(e.g. car.start() vs.
car.engine.start()). I’m REALLY missing how applying GoF Builder
solution from *Everything.You.Know.is.Wrong to avoid getters/setters
buys me anything at all. I guess it would prevent the possibility of
behavior type code that violates Demeter’s law, but it seems like it
comes at high cost. Unnecessary redundancy and class
explosion(importer/exporter interface for every domain class). This
seems more doable in a dynamic language with method_missing type
stuff, but I can’t even imagine this in Java. Now every time I need to
add a new attribute to a class, I have to change all the Importer/
Exporter implementations regardless of their interest in said
attribute? How does that make my code simpler and more maintainable?
On Thu, Apr 23, 2009 at 2:19 PM, Fernando P. [email protected]
wrote:
Rails definitely entices you to break Demeter’s law just so often.
So fix it. It’s usually just a matter of putting in some delegators.
If you don’t like @article.comments.build, you can declare your own
Article.build_comment() method and simply have it point to the
comment’s method. Or call it something else if you don’t like the
word “build.” (I don’t.)
Now how to cleanly spec: @comment = @article.comments.build(params[:comment]) ?
That’s backwards. If you already have a line of code and you’re
asking how to write a spec for it, you’re not honoring the behavior.
You’re honoring that line of code.
I’m going to guess from knowledge of the idiom that this is for a
‘create’ method in the CommentsController, nested under
ArticlesController. Whether it calls ‘comments.build’ or
‘comments.create’ or ‘make_me_a_comment!’ isn’t important and doesn’t
need to be specified. What’s important is the end result of the
method, which is that the article gains a shiny new comment:
Assume an article with no comments and a params structure
have already been set up in before(:each)
it “should make a new comment belonging to the article” do
post :create, :comment => @my_valid_params @my_article.should have(1).comment
end
How clean is that? Now just a couple more specs for invalid params
and for where the method directs to, and you’re basically done.
Mocking and stubbing is starting to get ugly now.
That much is true. You’ll notice I didn’t mock or stub anything up
there. I honestly think the controller code by the RSpec scaffold
generator is the wrong approach; it’s specing implementation, not
behavior. Even worse, it’s specing the implementation details of model code, which shouldn’t even be a consideration here. My way
breaks isolation (it hits the database) but it’s a lot simpler and
focuses on behavior.
Is it possible to get behavior focus and isolation? I started to
think about that. And then I realized, in accordance with the “If
it’s hard to spec you’re probably doing it wrong” principle,
that…well, Rails is probably doing it wrong. Controllers in Rails
are just doing the Wrong Thing. That’s why specing them is so
painful, and why so many of us skip trying.
How clean is that? Now just a couple more specs for invalid params
and for where the method directs to, and you’re basically done.
Amen to that.
Mocking and stubbing is starting to get ugly now.
That much is true. You’ll notice I didn’t mock or stub anything up
there. I honestly think the controller code by the RSpec scaffold
generator is the wrong approach; it’s specing implementation, not
behavior. Even worse, it’s specing the implementation details of model code, which shouldn’t even be a consideration here. My way
breaks isolation (it hits the database) but it’s a lot simpler and
focuses on behavior.
And amen to that too.
Even within an rspec example I think of each line of code as a Given
(setup), When (event) or Then (outcome). Usually when I see a line doing
more than one of these - say calling a method and verifying its result -
I
break it out into separate event and outcome lines.
The reason for this is that the givens and outcomes can tinker with
whatever
they want. They can create mocks, poke around in the database, wire up
models, whatever. The event lines though - the Whens - can only interact
with the object like client code would. In other words they can’t know
that
an object is a mock, or that a value got into a database because you
poked
it there.
Your example is great. You have an event line that interacts with the
controller through a regular HTTP POST, like a client would, and you
verify
that by checking the database via the ActiveRecord model object. There
is no
need for a mock here if your example is checking that data ends up in
the
database (although that does make it more of an integration test than a
behavioural spec). Because of rails’s evil insistence that a “model” is
just
a persistence strategy, they are pretty much the same thing.
Is it possible to get behavior focus and isolation?
Yes, but it only has value on a per-line-of-code-in-your-spec basis.
Your
givens and outcomes shouldn’t care about isolation - your events should
only
be exercising behaviour that is available in the object you are
describing.
It gets more complicated when a single line of code both exercises
behaviour
and does verification. I find breaking these out helps my sanity.
I started to
think about that. And then I realized, in accordance with the “If
it’s hard to spec you’re probably doing it wrong” principle,
that…well, Rails is probably doing it wrong. Controllers in Rails
are just doing the Wrong Thing. That’s why specing them is so
painful, and why so many of us skip trying.
