10

I'm trying to follow the Law Of Demeter ( see http://en.wikipedia.org/wiki/Law_of_Demeter , http://misko.hevery.com/code-reviewers-guide/flaw-digging-into-collaborators/ ) as I can see the benefits, however I've become a little stuck when it comes to domain objects.

Domain objects do naturally have a chain and sometimes it's necessary to display the information about the entire chain.

For instance, a shopping basket:

Each order contains a user, delivery info and a list of items Each order item contains a product and quantity Each product has a name and price. Each user contains a name and address

The code which displays the order information has to use all the information about the order, users and products.

Surely it's better and more reusable to get this information through the order object e.g. "order.user.address.city" than for some code higher up to do queries for all the objects I listed above then pass them into the code separately?

Any comments/suggestions/tips are welcome!

Tom B
  • 2,735
  • 2
  • 24
  • 30
  • 1
    I know there's not a specific question here ... I placed the bounty because there *could be* and the subject matter is worth discussing. Perhaps the OP could clarify the question a bit? –  Nov 26 '12 at 16:32
  • @Tom your problem is you don't like injecting a class User, class Info, and an array of Product classes in class Order? they are rather independant though –  Nov 26 '12 at 18:52
  • Hey! thanks for adding the bounty. I'll clarify the problem. Andrés Fortier's answer below summed up the problem: Combining different data sets in one place for display. These objects are essentially just data structures so although they break the law a demeter, they are related data and required in the same place. I suppose my question is, is breaking the law a demeter on these data objects really a bad thing? There won't be a hard and fast rule but it'd be interesting to weigh up the pros/cons. – Tom B Nov 29 '12 at 09:32

5 Answers5

10

One problem with using chained references, such as order.user.address.city, is that higher-order dependencies get "baked into" the structure of code outside the class.

Ideally, in cases when you refactor your class, your "forced changes" should be limited to the methods of the class being refactored. When you have multiple chained references in the client code, refactoring drives you to make changes in other places of your code.

Consider an example: suppose that you'd like to replace User with an OrderPlacingParty, an abstraction encapsulating users, companies, and electronic agents that can place an order. This refactoring immediately presents multiple problems:

  • The User property will be called something else, and it will have a different type
  • The new property may not have an address that has city in cases when the order is placed by an electronic agent
  • The human User associated with the order (suppose that your system needs one for legal reasons) may be related to the order indirectly, - for example, by being a designated go-to person in the definition of the OrderPlacingParty.

A solution to these problems would be to pass the order presentation logic everything that it needs directly, rather than having it "understand" the structure of the objects passed in. This way you would be able to localize the changes to the code being refactored, without spreading the changes to other code that is potentially stable.

interface OrderPresenter {
    void present(Order order, User user, Address address);
}
interface Address {
    ...
}
class PhysicalAddress implements Address {
    public String getStreetNumber();
    public String getCity();
    public String getState();
    public String getCountry();
}
class ElectronicAddress implements Address {
    public URL getUrl();
}
interface OrderPlacingParty {
    Address getAddress();
}
interface Order {
    OrderPlacingParty getParty();
}
class User implements OrderPlacingParty {
}
class Company implements OrderPlacingParty {
    public User getResponsibleUser();
}
class ElectronicAgent implements OrderPlacingParty {
    public User getResponsibleUser();
}
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Thank you! This is the problem I was trying to explain in my comments in TarkaDaal's answer but you put it far more eloquently than I was able. This is certainly a better solution! I'll try to put this into practice, thanks for pointing me in the right direction! – Tom B Dec 03 '12 at 19:48
2

I think, when chaining is used to access some property, it is done in two (or at least two) different situation. One is the case that you have mentioned, for example, in your presentation module, you have an Order object and you would like to just display the owner's/user's address, or details like city. In that case, I think it is of not much problem if you do so. Why? Because you are not performing any business logic on the accessed property, which can (potentially) cause tight coupling.

But, things are different if you use such chaining for the purpose of performing some logic on the accessed property. For example, if you have,

String city = order.user.address.city;
...
order.user.address.city = "New York";

This is problematic. Because, this logic is/should more appropriately be performed in a module closer to the target attribute - city. Like, in a place where the Address object is constructed in the first place, or if not that, at least when the User object is constructed (if say User is the entity and address the value type). But, if it goes farther than that, the farther it goes, the more illogical and problematic it becomes. Because there are too many intermediaries are involved between the source and the target.

Thus, according to the the Law of Demeter, if you are performing some logic on the "city" attribute in a class, say OrderAssmebler, which accesses the city attribute in a chain like order.user.address.city, then you should think of moving this logic to a place/module closer to the target.

Nazar Merza
  • 3,365
  • 1
  • 19
  • 19
1

You're correct and you'll most likely model your value objects something like this

class Order {
    User user;
}

class User {
    Address shippingAddress;
    Address deliveryAddress;
}

class Address {
    String city;
    ...
}

When you start considering how you will persist this data to a database (e.g. ORM) do you start thinking about performance. Think eager vs lazy loading trade offs.

Brad
  • 15,186
  • 11
  • 60
  • 74
  • Thanks! Yep, that's what I thought. The other issue which is obvious here is encapsulation. The order object shouldn't really know the implementation details of the user or product... but the application has to touch the ground somewhere. Should objects which are primarily there to serve as data structures be treated differently? – Tom B Sep 05 '12 at 15:25
  • It's good design to approach *all* classes (data and functional) in this manner. In fancy computer talk, this means striving to have classes with low [Cyclomatic Complexity](http://en.wikipedia.org/wiki/Cyclomatic_complexity) and a high level of [Cohesion](http://en.wikipedia.org/wiki/Cohesion_%28computer_science%29) – Brad Sep 05 '12 at 15:43
1

Generally speaking I adhere to the Law of Demeter since it helps to keep changes in a reduced scope, so that a new requirement or a bug fix doesn't spread all over your system. There are other design guidelines that help in this direction, e.g. the ones listed in this article. Having said that, I consider the Law of Demeter (as well as Design Patterns and other similar stuff) as helpful design guidelines that have their trade-offs and that you can break them if you judge it is ok to do so. For example I generally don't test private methods, mainly because it creates fragile tests. However, in some very particular cases I did test an object private method because I considered it to be very important in my app, knowing that that particular test will be subject to changes if the implementation of the object changed. Of course in those cases you have to be extra careful and leave more documentation for other developers explaining why you are doing that. But, in the end, you have to use your good judgement :).

Now, back to the original question. As far as I understand your problem here is writing the (web?) GUI for an object that is the root of a graph of objects that can be accessed through message chains. For that case I would modularize the GUI in a similar way that you created your model, by assigning a view component for each object of your model. As a result you would have classes like OrderView, AddressView, etc that know how to create the HTML for their respective models. You can then compose those views to create your final layout, either by delegating the responsibility to them (e.g. the OrderView creates the AddressView) or by having a Mediator that takes care of composing them and linking them to your model. As an example of the first approach you could have something like this (I'll use PHP for the example, I don't know which language you are using):

class ShoppingBasket
{
  protected $orders;
  protected $id;

  public function getOrders(){...}
  public function getId(){...}
}

class Order
{
  protected $user;

  public function getUser(){...}
}

class User
{
  protected $address;

  public function getAddress(){...}
}

and then the views:

class ShoppingBasketView
{
  protected $basket;
  protected $orderViews;

  public function __construct($basket)
  {
     $this->basket = $basket;
     $this->orederViews = array();
     foreach ($basket->getOrders() as $order)
     {
        $this->orederViews[] = new OrderView($order);
     }
  }

  public function render()
  {
     $contents = $this->renderBasketDetails();
     $contents .= $this->renderOrders();     
     return $contents;
  }

  protected function renderBasketDetails()
  {
     //Return the HTML representing the basket details
     return '<H1>Shopping basket (id=' . $this->basket->getId() .')</H1>';
  }

  protected function renderOrders()
  {
     $contents = '<div id="orders">';
     foreach ($this->orderViews as $orderView)
     {
        $contents .= orderViews->render();
     }
     $contents .= '</div>';
     return $contents;
  }
}

class OrderView
{
//The same basic pattern; store your domain model object
//and create the related sub-views

  public function render()
  {
     $contents = $this->renderOrderDetails();
     $contents .= $this->renderSubViews();
     return $contents;
  }

  protected function renderOrderDetails()
  {
     //Return the HTML representing the order details
  }

  protected function renderOrders()
  {
     //Return the HTML representing the subviews by
     //forwarding the render() message
  }
}

and in your view.php you would do something like:

$basket = //Get the basket based on the session credentials
$view = new ShoppingBasketView($basket);
echo $view->render();

This approach is based on a component model, where the views are treated as composable components. In this schema you respect the object's boundaries and each view has a single responsibility.

Edit (Added based on the OP comment)

I'll assume that there is no way of organizing the views in subviews and that you need to render the basket id, order date and user name in a single line. As I said in the comment, for that case I would make sure that the "bad" access is performed in a single, well documented place, leaving the view unaware of this.

class MixedView
{
  protected $basketId;
  protected $orderDate;
  protected $userName;

  public function __construct($basketId, $orderDate, $userName)
  {
    //Set internal state
  }


  public function render()
  {
    return '<H2>' . $this->userName . "'s basket (" . $this->basketId . ")<H2> " .
           '<p>Last order placed on: ' . $this->orderDate. '</p>';
  }
}

class ViewBuilder
{
  protected $basket;

  public function __construct($basket)
  {
    $this->basket = $basket;
  }

  public function getView()
  {
     $basketId = $this->basket->getID();
     $orderDate = $this->basket->getLastOrder()->getDate();
     $userName = $this->basket->getUser()->getName();
     return new MixedView($basketId, $orderDate, $userName);
  }
}

If later on you rearrange your domain model and your ShoppingBasket class can't implement the getUser() message anymore then you will have to change a single point in your application, avoid having that change spread all over your system.

HTH

BenMorel
  • 34,448
  • 50
  • 182
  • 322
Andrés Fortier
  • 1,705
  • 11
  • 12
  • This is certainly a good approach when the view can be truely distinct. But what about something as trivial as "Hi $name, your last order was placed on $lastOrderDate" or any other case where datasets are mixed. In this case, you need data from two sets, the customer and their latest order. And although I can see some advantage to this approach, it's potentially a lot of extra code for minimal benefit. It certainly seems like a better solution but I'm sure there's a middle ground somewhere. – Tom B Nov 29 '12 at 09:44
  • If that was the problem (one single attribute to be accessed in a chain of objects) I would delegate internally and eventually be flexible on the Law of Demeter (as I said in the answer). So, `User` would implement `getName()` and `getLatestOrder()` and in your view you would ask the order for its date. If that was a one-time case in your system then I wouldn't worry much. For the mixed data case, if you see no possible way of factoring it in subviews, then you should make sure that you break the Law of Demeter in a single, well documented place (I'll edit the answer to exemplify this) – Andrés Fortier Nov 29 '12 at 11:24
0

The Law Of Demeter is about calling methods, not accessing properties/fields. I know technically properties are methods, but logically they're meant to be data. So, your example of order.user.address.city seems fine to me.

This article is interesting further reading: http://haacked.com/archive/2009/07/13/law-of-demeter-dot-counting.aspx

TarkaDaal
  • 18,798
  • 7
  • 34
  • 51
  • But, the spirit of the law of demeter is there to avoid chains of dependencies via encapsulation. By only knowing the API of the nearest neighbour encapsulation is increased because the dependency can be replaced, e.g. with a mock object in testing, without breaking the code. In that example, replacing the user with a mock user object would cause things to break or at least require a significant amount of mock set up in order to achieve it, which kind of misses the point. – Tom B Dec 03 '12 at 16:38
  • Admittedly, views aren't going to be tested in that way, but it is useful to load a mock object into a real view for testing purposes. If the API of the user was changed, e.g. to have both a delivery address and a shipping address instead of a single address, things would break unexpectedly in places which don't actually list "address" as a dependency, which is far from ideal – Tom B Dec 03 '12 at 16:38
  • @TomB Yes, the spirit of it is about data hiding. Your question is explicitly about data *showing*. The view (or whatever) is presumably going to have labels for "User", "Address", "City", etc... By definition, it *has* to know about this stuff. I don't see the problem in getting this information in the simplest way possible. – TarkaDaal Dec 03 '12 at 18:02