Tell, Don't Ask

Upcase
1
In this episode, Ben and Joe discuss the an OO design principle known as Tell Don't Ask. They walk through a number of examples, discuss its subtleties when using MVC, and cover query and command methods. Original blog post PragProg blog post on...
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/tell-don-t-ask
2

Good stuff!
Could you guys give tips on how to spec/test NullObjects?

3

I test them just like other objects. A NullObject has specific things it’s supposed to return when you send it certain messages, so I write tests for those things.

4

@alexbush I’ve sometimes written tests to check that a null object’s interface matches the interface of the object it stands in for. This can help with the problem Joe and Ben talked about in the video, where interfaces get out of sync.

There are details in this blog post: Testing Null Objects

5

@benorenstein Does it really make sense to test a NullObject, apart from making sure it complies with the interface of the object it should stand in for?

I tend to think of a NullObject as something too trivial to test (unless when it’s not). Do you test it for coverage sake, for documentation?

6

Where is the place that we usually put the conditional to return null object if the object to be accessed is not presence? Should AR model know this knowledge to return null object when its attribute missing value? In the example of NullAddress, I feel it is used for removing view presentation logics only.

address || NullAddress.new
7

Not just view logic. You can take advantage of it anywhere an address is used, on the views, controlles, services, other models, etc… You get the benefit of not having to check if it is nil anymore.

8

Sure, why not? It’s just another object that responds to messages. I like to test my objects.

9

@zamith @benorenstein sometimes I test my null objects, but I only best them to compile with the interface of the object that it stands for. In addition, I test full behaviors of special case objects. I like the ideas of differentiate between special case objects and null objects. Null objects usually just compile to the interface and do nothing like NullLogger, but on the other hand Special case objects compile to their interface and can also have some behaviors Like GuestUser. So basically, null object pattern is just a specialize form of special case pattern.

http://martinfowler.com/eaaCatalog/specialCase.html

10

The thing is that I tend to weight the value of having a test on roughly these aspects:

  • Design feedback
  • Regression testing
  • Documentation
  • Overall confidence and joy it gives me

And most Null objects, given that they are really simple and have no logic in them, will at most fulfil the 2nd and 3rd, IMO. I do understand why you would test them, but still I would always question myself how much value is it really adding, mostly according to these parameters.

Thanks for the feedback, btw.

11

That’s fair. I tend to just stick to just always stick to a TDD-based process unless the circumstances are particularly weird.

3 Likes
12

For the first example in Tell, Don’t Ask which relates to showing a welcome message to the user, better code puts view related code into the model object. As a general principle I try to keep the model objects as simple thin database wrappers as opposed to them being junk drawers for random view code. The alternative to the implementation in the video is to have a helper object that is responsible for building the welcome message for a given user however I can’t think of a clever way of not violating Tell Don’t Ask and keeping the logic out of the model.

module UserHelper
  def welcome_message(user)
    (user.admin?) ? "Welcome Admin!" : "Welcome User!"
  end
end

In your experience is it more important to obey Tell Don’t Ask or keeping models only as thin database wrappers and keeping extra logic out of them altogether.

It feels to me that the example is really a view concern rather than a model concern and the logic should not be added to the model itself even though it is technically violating Tell Don’t Ask.

13

What you are doing here is fine. Tell, don’t ask is more about don’t ask about the internals of an object and then reach into that object to do something. That is in fact why this shouldn’t on the user model. You shouldn’t have to “do” something to the user model to accomplish this, you just need to query it’s role.

14

Right. The motivation for my question was that I’ve seen a few systems where the User model becomes a junk draw with a lot of code that relates to messages show in the view and it becomes messy over time. I see you’re point though, in my implementation I am asking for the role of the user but unlike the example in the video I am not reaching into the user to get any of its internals. However what If I were to modify the example to be the following:

module UserHelper
  def welcome_message(user)
    if user.admin?
      "Welcome #{user.display_name}, you have phenomenal cosmic powers!"
    else
      "Welcome #{user.display_name}!"
    end
  end
end

I think this makes it closer to the example in the video where I am asking a question about the user and then reaching into the object to learn about its state in order to craft the welcome message.

15

Then I would reach into my bag of tricks and pull out Polymorphism over Conditionals:

module AdminWelcomeMessage
  extend self
  def message(user)
    "Welcome #{user.display_name}, you have phenomenal cosmic powers!"
  end
  alias_method :call, message
end

module UserWelcomeMessage
  extend self
  def message(user)
    "Welcome #{user.display_name}!"
  end
  alias_method :call, message
end

module UserHelper
  def welcome_message(user)
    user.admin? ? AdminWelcomeMessage.(user) : UserWelcomeMessage.(user)
  end
end
16

What’s the purpose of the alias_method? Personally I like the semantics of calling AdminWelcomeMessage.message(user) or UserWelcomeMessage.message(user). Perhaps that’s just a coding style thing.

The main idea here is pulling the code that accesses the user state into a separate class that has semantic meaning instead of inlining it into a branch of the conditional. Moving it into separate modules doesn’t fix the tell don’t ask violation though it just provide semantic grouping around it. It does provide a cleaner way to generate view text based on model state without having to put it in the model itself.

1 Like
17

@Dreamr Should your code example demonstrate the use of polymorphism instead of conditionals? You still have exactly the same conditional @ajorgensen had in his code which confuses me a lot. If you want to achieve true polymorphism you need to put welcome_message on the user object and have two different user classes User and Admin. UserPresenter and AdminPresenter would work as well.

18

@timhabermaas that’s one way of doing it but in one of my follow ups I said:

The motivation for my question was that I’ve seen a few systems where the User model becomes a junk draw with a lot of code that relates to messages show in the view and it becomes messy over time.

I think the important lesson here is that our job as programmers is to manager trade-offs and that these rules are more like guidelines than hard and fast rules. It’s damn near impossible to always follow all of them. Gary Bernhardt has a Destroy All Software screencast about conflicting principles.

Personally I would rather violate tell don’t ask in this case rather than stick the view concern on the user model where it definitely doesn’t belong. For this example I think SRP trumps Tell Don’t Ask. Polymorphism would work but I don’t think its the best solution even thought it gets rid of the Tell Don’t Ask violation.

19

@ajorgensen

I agree, don’t put welcome_message on User. I was just utterly confused by Dreamr’s “bag of tricks” response and tried to suggest a straight-forward way of using polymorphism without changing any of the existing implementation.

We can at least minimize these violations by pushing the decision further up:

a) Violate SRP, obey TDA:

class User
  def presenter
    admin? ? AdminPresenter.new(self) : UserPresenter.new(self)
  end
end

b) Obey SRP, violate TDA:

class PresenterBuilder
  def wrap(record)
    return record unless record.is_a? User
    record.admin? ? AdminPresenter.new(record) : UserPresenter.new(record)
  end
end

I prefer one of these solutions to the one discussed in this thread, because they make sure user.admin? isn’t repeated several times. So if you later decide to implement two subclasses instead of a boolean flag you only have to go to one place.

\edit: Bad example. You could always quickfix it by doing this:

class Admin
  def admin?
    true
  end
end
20

I like the idea of removing the admin? call from multiple places. If you were to ever have multiple types of admin (super, etc) then you’d have to re-do all that logic in so many different views. Gems like cancan can also help with this a little by moving that sort of logic into a central place. Personally I’m more of a fan of your b) example.