Push the query down: Tell, don’t ask

"Feature envy" is a typical code smell in Object Oriented Programming. It can be appreciated when code talking to an object accesses directly to that object's fields or properties, from the outside, breaking the encapsulation.

  1. // feature envy:
  2. fullName = person.name + " " + person.surname;
  3.  
  4. // possible solution:
  5. fullName = person.fullName();

The "Tell, don't ask" principle helps to avoid feature envy when used properly, by using a declarative approach instead of a procedural one. This is, rather than asking a method for a value to make a decision based on the answer, we tell the method to perform an action. Any decisions based entirely upon the state of one object should be made "inside" the object itself. This principle promotes encapsulation, and low coupling. Query methods (method returning values) can't be totally avoided, in fact they shouldn't, but they can be pushed down to places where coupling is affordable, thus avoiding coupling high level classes, reducing the impact of changes.

"Tell, don't ask", and "Law of Demeter" were explained  by Andy Hunt and Dave Thomas in a IEEE Software column and on their site. Also recently by Martin Fowler.

Recently, I came across an "extract method" refactoring promoting this principle that makes the code easier to read and maintain:

Before:

  1.  
  2. // Service class:
  3. ...
  4. public virtual void RequestPin(User user) {
  5. var newPin = PinBuilder.CreateNewPin();
  6. PinRepository.SaveNewPinFor(newPin, user.UniqueIdentifier);
  7. SmsProxy.SendSms(user.Phone, CreateSMSMessage(newPin));
  8. }
  9. ...
  10.  
  11.  

After:

  1.  
  2. // Service class:
  3. ...
  4. public virtual void RequestPin(User user) {
  5. var newPin = PinBuilder.CreateNewPin();
  6. user.ResetPIN(newPin, PinRepository);
  7. user.RequestSMS(CreateSMSMessage(newPin), SmsProxy);
  8. }
  9. ...
  10.  
  11. // User class:
  12. ...
  13. public void ResetPIN(string newPin, PinRepository pinRepository) {
  14. pinRepository.SaveNewPinFor(UniqueIdentifier, newPin);
  15. }
  16.  
  17. public void ReceiveSMS(string message, SmsProxy smsProxy) {
  18. smsProxy.SendSms(Phone, message);
  19. }
  20. ...
  21.  

This refactoring brings several advantages. First, we remove feature envy, because properties "UniqueIdentifier" and "Phone" are used inside the User object. So the data and the behavior are in the same place. If I have to change the field in the future, it will be clear what is it affecting to. Second, the service code is easier to read and encapsulates details that should not be visible there.

It looks weird at first, sending a repository or a service instance as a parameter to an entity's method. I usually do it as I refactor, not upfront. The tests don't need to be changed if they are well-written.

Enjoyed reading this post?
Subscribe to the RSS feed and have all new posts delivered straight to you.
  • Guillermo Pascual

    I don’t understand this principle in the same way. I wouldn’t want User to grow methods for everything you might want to do with a User, as that results in unrelated changes touching the same code, and the “we’ll have to put it in there, as it needs those private fields” mentality, giving rise to huge classes. If you are using the result of a method call to make decisions elsewhere in the program, then you are not violating Tell Don’t Ask. If, on the other hand, you’re making decisions for an object based on a method call to that object, then you should move those decisions into the object itself to preserve encapsulation.

  • mgryszko

    After your refactoring, what about SRP of your User class? Now it’s concerned with coordination of persistence (calling the repository) and notifications (SMS).

    Was this refactoring a double-edged sword?

  • carlosble

    Thanks for the reply. Am not sure I understand what you mean by not violating the principle. It would be nice if you could provide an example. Also if you can give me an example of how to avoid feature envy with that code, it would be appreciated.
    On the other hand, I am not saying that the User class should grow methods for everything. In the code base from where this example has been extracted, the user didn’t grow much.

  • carlosble

    The SRP is not violated in the methods. The methods do exactly one thing. At the class level, you must know the problem domain more to assert that it is violating the SRP which I believe is not. User is not actually persisting or sending sms, it is delegating.
    Given the problem domain, those methods in the user make sense to a stackholder.
    Without these methods, it would be just an anemic model.

  • Guillermo Pascual

    I think there is no violation of feature envy or tell don’t ask in your before example. The fundamental rule of thumb is to put things together that change together. Data and the behavior that references that data usually change together, but there are exceptions. When the exceptions occur, we move the behavior to keep changes in one place. It’s okay to use accessors to get the state of an object, as long as you don’t use the result to make decisions outside the object. Any decisions based entirely upon the state of one object should be made ‘inside’ the object itself.

    Ask way:

    Order order = orderRepository.findById(100);
    double orderPrice = 0.0;
    foreach (OrderLine line : order.getLines()) { // ask
    orderPrice += line.getPrice(); // ask
    }

    Tell way:

    Order order = orderRepository.findById(100);
    double orderPrice = order.calculatePrice(); // tell

  • carlosble

    Thanks for the example, I’d like to feature one of your sentences:

    “It’s okay to use acessors to get the state of an object, as long as you don’t use the result to make decisions outside the object”

    The code in the example might not be violating the “Tell, don’t ask” principle but I do believe it suffers from feature envy. I see the User as a higher level of abstraction than the repository in this case, so I let the User be the one who decides which fields are being used for persistence, rather than other objects. Tomorrow, instead of “UniqueIdentifier” it might be a method.
    If the repository took the whole user for persistence, than I’ll be fine with the service invoking the repository.
    I suspect that in later refactorings, that method might move to another specialized class handling persistence details. But I haven’t reached that point yet with the current code.

  • Diego Güemes

    I agree with Carlos. The problem is we aren’t used to inject colaborators in our entities, but there’s nothing bad with that. As Carlos said above, without them you will probably end with an Anemic Domain Model.

    I don’t think moving methods to the User class is a violation of the SRP. In fact, the same SRP violation is present in the two code snippets, I mean, what happen if the messaging system changes? However, solving this without necessity is over-engineering.

    It’s impossible to accomplish with all principles and the second snippet reads better than the second one.

    My question is, why do you create the method RequestPin in a service and not in the User class? I think it would read pretty well: user.requestNewPin();

  • mgryszko

    You took the words right of my head – I think User should be responsible for resetting the PIN. You put relevant behaviour on the data this behaviour concerns. Hence you are not going towards Anemic Domain Model 🙂

  • carlosble

    Thanks for the comment. That might be the next refactor, I don’t know yet 🙂 Code is always changing!

  • carlosble

    A “similar” discussion happen in the Clean Code discussion list. This is Uncle Bob’s answer:
    https://groups.google.com/d/msg/clean-code-discussion/latn4x6Zo7w/bFwtDI1XSA8J

  • http://www.carlosble.com/ Carlos Ble

    Time has passed since this post and its discussion. Having to maintain the code I wrote this way, looking at it after several months, has made me find one issue: when I am searching for code, I am going to the services rather than the entities. So the code is not where I am expecting it to be. I don’t know whether it’s because I am not used to it yet, but I guess in some cases this indirection is harder to follow.

  • Vicenç García-Altés

    Thanks for the explanation Guillermo. It clarifies me Tell Don’t Ask a little bit more.

  • http://www.carlosble.com/ Carlos Ble

    After some months, I can say I prefer the code in the “before” snippet than the code in the “after” snippet. I mean, I prefer the original code.
    Because the most fundamental principle in software development, is to me, the Least Astonishment:

    http://c2.com/cgi/wiki?PrincipleOfLeastAstonishment

    Basically I want my code to behave as a reader would expect. I have come back to maintain this code months later I refactored it and when I saw it, I felt a bit surprised. To me this is the symptom than the code should be written the other way for this particular situation. Fortunately I’ve reverted the change using the “Inline method” automated refactor.

    Thanks 🙂