Prev: Builder::XmlMarkup + output encoding
Next: [QUIZ] Random Points within a Circle (#234)
From: Rein Henrichs on 19 Jun 2010 14:10
On 2010-06-19 09:20:26 -0700, Michael Fellinger said:
> def open_window
> if location.window.respond_to?(:open=)
> location.window.open = true
> else
> raise("The location doesn't have any windows you can open")
> end
> end
> end

This is a Law of Demeter violation. The home should be responsible for
opening its own windows.

This code makes assumptions about both the house and its windows on the
behalf of the person (that it has exactly one window, that windows are
opened by setting their 'open' accessor) and even then it deals with
these assumptions inconsistently.

1) It checks for the behavior of the window without first checking for
the existance of the window. If you really must program defensively, at
least be consistent. Better still to not program defensively.

2) It raises a misleading (or at least ambiguious) error. Far better to
simply try it and let the interpreter tell you where you failed. Is a
NoMethodError on a window object really any less informative? Given the
unnecessarily complex code path (because of the Demeter violation), I
would argue that it is more informative.

In any event, you shouldn't be violating Demeter in the first place.
The following Law of Demeter violation better illustrates the problem:

class Person
attr_accessor :wallet
end

class Store
def accept_payment(person, amount)
person.wallet.amount -= amount
end
end

NB: I actually originally wrote it as person.wallet.withdraw(amount)
(because I instinctively hide state and expose behavior in cases like
these) and then realized that this is one less Demeter violation than
the original example code.

People should be allowed to make payments themselves. I don't want the
store reaching into my wallet and just taking my money. I *certainly*
don't want the store reaching into my wallet and then telling me how
much money I have when it's done.

class Store
def accept_payment(person, amount)
person.pay(amount)
end
end

This lets me decide where I keep my money, how I pay and if I can pay at all.

Furthermore, allowing assignment to the window's 'open' accessor lets
client code do `window.open = [1,2,3]` and other such nonsense. A
window has exactly two states and its behavior should be modeled in a
way that enforces this relationship and keeps its state hidden behind
an interface, not by allowing client code to set its state in arbitrary
ways. Window#open and Window#close.

The home should delegate window opening and closing to its window. The
person should ask the home to close its window. Or the person should
know about the window, which makes more sense in the real world but
less sense in this OOP system.

Hopefully we can avoid making concrete OOP mistakes even while we're
answering completely hypothetical OOP questions.

Also, can someone please write me a Person#sudo_make_me_a_sandwich!

--
Rein Henrichs
http://puppetlabs.com
http://reinh.com

First  |  Prev  | 
Pages: 1 2
Prev: Builder::XmlMarkup + output encoding
Next: [QUIZ] Random Points within a Circle (#234)