Demeter's law (suggestion) and tell don't ask - Part 1

There are several very good explanations of this guideline for writing object oriented code. This is a supplementary post. One of the Demeter guidelines is that you can invoke a method on a parameter but not on the object that is thereby returned. For instance, the following snippet violates the guideline:

//checkout a book from a library
checkout(long bookId, long userId){
Book bookAskedFor = repository.findBookById(bookId); //repository has been injected
User borrower = repository.findUserById(userId);
bookAskedFor.setNumberOfCopiesAvailable(bookAskedFor.getNumberOfCopiesAvailable() - 1);

By invoking methods on bookAskedFor, Demeter's law is violated. It is also a flagrant violation of encapsulation that may be addressed as below:

checkout(long bookId, long userId){
Book bookAskedFor = repository.findBookById(bookId);
User borrower = repository.findUserById(userId);
bookAskedFor.lendTo(borrrower); //tell-don't-ask in action

This reads much better. lendTo nicely encapsulates the business logic of lending out a book. However it still doesn't satisfy Demeter's law because it invokes a method on a second hand object (bookAskedFor). I find this overkill. A trivial change (offering no further improvements) seems to satisfy the law:

checkout(long bookId, long userId){
Book bookAskedFor = repository.findBookById(bookId);
User borrower = repository.findUserById(userId);
lend(bookAskedFor, borrower);

lend(Book book, User borrower){

What is worse, one could satisfy Demeter without bothering to encapsulate:

checkout(long bookId, long userId){
Book bookAskedFor = repository.findBookById(bookId);
User borrower = repository.findUserById(userId);
setBorrowerId(bookAskedFor, borrower);

changeStatustoCheckedOut(Book book){

setBorrowerId(Book book, User borrower){

setReturnDate(Book book){

adjustNumberofAvailableCopies(Book book){
book.setNumberOfCopiesAvailable(book.getNumberOfCopiesAvailable() - 1);

Such examples lead me to think that 'Tell don't ask' is a better principle to keep in mind than the Law of Demeter.


KetanPadegaonkar said...

I am in complete agreement here.

If your objects do not return their friends, only talk to their immediate friends, and not friends of friends, this would enforce that objects talk to each other by tell-dont-ask.

Anonymous said...

None of your proposed alternatives is good enough. Law of Demeter is more a guideline than a law but should still be strived for.

The "law" can thus be enforced with the following code.

Loan checkOut(long bookId, long userId) {
Book bookAskedFor = repository.findBookById(bookId);
User borrower = repository.findUserById(userId);

// Create and control the relationship here; this will remain an invariant
return new Loan(bookAskedFor, borrower);

Kris said...

The "checkout" method is something that I generally consider a smell in a codebase. I've seen such methods used as a naive attempt to not violate the law of demeter without respecting the purpose behind the law.

Developers can hide the dot-dot part of law of demeter through passing to other methods, but that misses the point - that object interaction is done via the closest friend.

Sriram said...

What happens when the user returns the book? At some stage, you will need to lookup an object from the repository and send it a message that changes its state. But then as per Demeter, one shouldn't do the lookup and the message-sending in the same method. This is why I think tell-don't-ask is more useful as a principle than Demeter.

None of your proposed alternatives is good enough
The only alternative I propose is the second.

Can you elaborate why you consider the presence of a method named 'checkout' as a code-smell?

Saager Mhatre said...

I'll second @kris' comment.
IMHO, the checkout method probably belongs in the Book class like so.

However, that still doesn't solve the problem entirely. We still end up calling the checkoutFor method on the Book we 'found by Book Id'. Hmm... makes me wonder if Repository's the one in Demeter's bad books.

Basically, LoD also serves as an indicator of misplaced behaviour- if you're talking to 'aliens', you should probably be 'talking' elsewhere.

PS- never thought I'd see you passing long ids to methods. :S

Saager Mhatre said...

Just so that I don't come out as a complete idiot-

I do believe that the tell don't ask principle has tremendous value; I also believe that LoD is a fundamental principle of OOP and can stand by itself. Although, I must admit that on more than a few occasions I have overlooked it when actually writing code; but then again, I am mostly doing maintenance right now.

I just thought your example was a little off- a standalone method for an OO principle just stuck out real bad.

Here's a somewhat more non-tirvial implementation of Book that uses the State pattern while obeying LoD (I think), though I still can't eliminate call on 'found' book in main; those finder methods seem a little smelly. I think I'd prefer Book and User constructors that take ID params.

PS- sorry I don't have tests for all that code! :S

Anonymous said...

@sriram - Handling a book return would be another object's responsibility; perhaps a Return class would be appropriate if returns are significant.

It is hard to propose a viable solution without knowing the exact problem context, but having Book manage its movements (through a relationship with User) just does not sound right.


haniuthere said...

Loan checkOut(long bookId, long userId)

It is incorrect abstraction It should have being.
Loan checkOut(Book bookToLend,User userWhoAsked)

Post a Comment

Note: Only a member of this blog may post a comment.