Rails: Law of Demeter Confusion

Question

I am reading a book called Rails AntiPatterns and they talk about using delegation to to avoid breaking the Law of Demeter. Here is their prime example:

They believe that calling something like this in the controller is bad (and I agree)

@street = @invoice.customer.address.street

Their proposed solution is to do the following:

class Customer

    has_one :address
    belongs_to :invoice

    def street
        address.street
    end
end

class Invoice

    has_one :customer

    def customer_street
        customer.street
    end
end

@street = @invoice.customer_street

They are stating that since you only use one dot, you are not breaking the Law of Demeter here. I think this is incorrect, because you are still going through customer to go through address to get the invoice's street. I primarily got this idea from a blog post I read:

http://www.dan-manges.com/blog/37

In the blog post the prime example is

class Wallet
  attr_accessor :cash
end
class Customer
  has_one :wallet

  # attribute delegation
  def cash
    @wallet.cash
  end
end

class Paperboy
  def collect_money(customer, due_amount)
    if customer.cash < due_ammount
      raise InsufficientFundsError
    else
      customer.cash -= due_amount
      @collected_amount += due_amount
    end
  end
end

The blog post states that although there is only one dot customer.cash instead of customer.wallet.cash, this code still violates the Law of Demeter.

Now in the Paperboy collect_money method, we don't have two dots, we just have one in "customer.cash". Has this delegation solved our problem? Not at all. If we look at the behavior, a paperboy is still reaching directly into a customer's wallet to get cash out.

EDIT

I completely understand and agree that this is still a violation and I need to create a method in Wallet called withdraw that handles the payment for me and that I should call that method inside the Customer class. What I don't get is that according to this process, my first example still violates the Law of Demeter because Invoice is still reaching directly into Customer to get the street.

Can somebody help me clear the confusion. I have been searching for the past 2 days trying to let this topic sink in, but it is still confusing.

Answer 1

Your first example does not violate the law of Demeter. Yes, with the code as it stands, saying @invoice.customer_street does happen to get the same value that a hypothetical @invoice.customer.address.street would, but at each step of the traversal, the value returned is decided by the object being asked - it's not that "the paperboy reaches into the customer's wallet", it's that "the paperboy asks the customer for cash, and the customer happens to get the cash from their wallet".

When you say @invoice.customer.address.street, you're assuming knowledge of customer and address internals - this is the bad thing. When you say @invoice.customer_street, you are asking the invoice, "hey, I'd like the customer's street, you decide how you get it". The customer then says to its address, "hey I'd like your street, you decide how you get it".

The thrust of Demeter is not 'you cannot ever know values from objects far away in the graph from you"; it is instead 'you yourself are not to traverse far along the object graph in order to obtain values'.

I agree this may seem like a subtle distinction, but consider this: in Demeter-compliant code, how much code needs to change when the internal representation of an address changes? What about in non-Demeter-compliant code?

Answer 2

The first example and the second are actually not very same. While the first talking about general rules of "one dot", the second talks more about other things in OO design, especially "Tell, Don't ask"

Delegation is an effective technique to avoid Law of Demeter violations, but only for behavior, not for attributes. -- From the second example, Dan's blog

Again, "only for behavior, not for attributes"

If you ask for attributes, you are supposed to ask. "Hey, guy, how much money do you have in pocket? Show me, I'll evaluate if you can pay this." That's wrong, no shopping clerk will behave like this. Instead, they'll say, "Please pay"

customer.pay(due_amount)

It'll be customer's own duty to evaluate if he should pay and if he can pay. And the clerk's task is finished after telling customer to pay.

So, does the second example prove the first is wrong?

In my opinion. No, as long as:

1. You do it with with self-constraint.

While you can access all of the customer's attributes in @invoice by delegation, you rarely need that in normal cases.

Think about a page showing an invoice in a Rails app. There will be a section on top to show the customer's details. So, in invoice template, will you code like this?

#customer-info
  = @invoice.customer_name
  = @invoice.customer_address
  ....

That's wrong and inefficient. A better approach is

#customer-info
  = render partial: 'invoice_header_customer', 
           locals: {customer: @invoice.customer}

Then let the customer partial to process all attributes belongs to customer.

So generally you don't need that. But you may have a list page showing all recent invoices, there is a briefing field in each li displaying the customer's name. In this case, you do need the customer's attribute to show, and it's totally legit to code the template as

= @invoice.customer_name

2. There is no further action depending on this method call.

In above case of list page, the invoice asked the customer's name attribute, but it's real purpose is "show me your name", so it's basically still a behavior but not attribute. There is no further evaluation and action based on this attribute like, if your name is "Mike" I will like you and give you 30 days more credit. No, invoice just say "show me your name", no more. So that's totally acceptable according to the "Tell Don't Ask" rule in example 2.

Answer 3

Read further in the second article and I think the idea will become clearer. The idea just have customer offer a capability to pay and completely hide where the case is kept. Is it a field, a member of a wallet, or something else? The caller doesn't know, doesn't need to know and does not change if that implementation detail changes.

class Wallet
  attr_accessor :cash
  def withdraw(amount)
     raise InsufficientFundsError if amount > cash
     cash -= amount
     amount
  end
end
class Customer
  has_one :wallet
  # behavior delegation
  def pay(amount)
    @wallet.withdraw(amount)
  end
end
class Paperboy
  def collect_money(customer, due_amount)
    @collected_amount += customer.pay(due_amount)
  end
end

So I think your second reference is giving a more helpful recommendation.

The "one dot" only idea, is a partial success, in that it hides some deep detail, but still inbcreases coupling between separate components.

Answer 4

Sounds like Dan derrived his example from this article: The Paperboy, The Wallet, and The Law Of Demeter

Law Of Demeter A method of an object should invoke only the methods of the following kinds of objects:

1. itself
2. its parameters
3. any objects it creates/instantiates
4. its direct component objects

When and How to Apply The Law Of Demeter

So now you have a good understanding of the law and it's benefits, but we haven't yet discussed how to identify places in existing code where we can apply it (and just as important, where NOT to apply it...)

1. Chained 'get' Statements - The first, most obvious place to apply the Law Of Demeter is places of code that have repeated get() statements,

   value = object.getX().getY().getTheValue();

   as if when our canonical person for this example were pulled over by the cop, we might see:

   license = person.getWallet().getDriversLicense();

2. lots of 'temporary' objects - The above license example would be no better if the code looked like,

   Wallet tempWallet = person.getWallet(); license = tempWallet.getDriversLicense();

   it is equivalent, but harder to detect.

3. Importing Many Classes - On the Java project I work on, we have a rule that we only import classes we actually use; you never see something like

   import java.awt.*;

   in our source code. With this rule in place, it is not uncommon to see a dozen or so import statements all coming from the same package. If this is happening in your code, it could be a good place to look for obscured examples of violations. If you need to import it, you are coupled to it. If it changes, you may have to as well. By explicitly importing the classes, you will begin to see how coupled your classes really are.

I understand that your example is in Ruby, but this should apply across all OOP languages.