Tell Don't Ask

Posted by James Mead Sat, 23 Dec 2006 20:04:00 GMT

Following on from a couple of my previous posts, here are some more comments on the Making a Mockery of ActiveRecord article…

Something that may be adding unnecessary complexity to the test is that the Message#generate method knows the structure of objects for which it does not have direct references. For example, it looks like the List and Person instances are supplied, just so that the generate method can navigate to the subscriptions in order to call create. This breaks the Law of Demeter.

A first step to improving the design would be to have a List#create_subscriptions method which would be called by Message#generate. We could then simplify the unit test as follows…

  def test_should_create_email_messages_and_subscriptions_when_include_subscribers_is_true
    list = mock()
    campaign = Campaign.new(:people => [])
    message = Message.new(:campaign => campaign)
    message.stubs(:lists).returns([list])
    message.include_subscribers = true

    list.expects(:create_subscriptions)

    message.generate
    assert_equal 1, message.email_messages
  end

Obviously we’d need to add a separate unit test for the List#create_subscriptions method, but that would also be much simpler and easier to understand. In this way we have decoupled Message from the structure of List and are just relying on its behaviour. Nat Pryce & Steve Freeman call this the Tell Don’t Ask style.

Finally, I prefer not to have much more than one assertion per test (and don’t forget verifying an expectation is effectively an assertion), so I’d probably split the test into two – one to verify create_subscriptions was called and one to check the email_messages were created.

Tags , , , , , ,  | 8 comments

Comments

  1. Luke Redpath said about 2 hours later:

    Thinking about it, the assertion of the number of email messages looks like it would belong in the create_subscriptions test as the creation of email messages is part of its behaviour

  2. Channing Walton said about 10 hours later:

    The first time I can find that the Tell Don’t Ask principle is mentioned is in the Pragmatic Programmer’s article in 1998. They seem to credit the idea to Alec Sharp in his book Smalltalk by Example (1997).

    Anyone know of any older references to this principle?

  3. Wilson Bilkovich said about 15 hours later:

    Interesting. Thanks for thinking so much about my article. I wrote it partly to help me think about whether I was going about things the right way.

    There are definitely some rough edges in AR where things are passed by ‘dup’ing, rather than by reference. Pays to keep that in mind.

    Just in case it helps, here’s the implementation that is being spec’d out:

    You’re right that the Association Proxy setup in ActiveRecord makes it hard to obey the law of demeter.

  4. James Mead said about 20 hours later:

    Channing: Thanks for the reference to the Tell Don’t Ask principle. I know less than you about its origins.

    Wilson: Thanks for your comment – it is helpful to see the code under test. I’m sorry I went a bit overboard by posting so much about your article, but I found the exercise helpful in getting some things straight in my own head. I hope you’re not offended.

  5. Wilson Bilkovich said 1 day later:

    No way. Game on. Merry Christmas. :)

  6. David Chelimsky said 2 days later:

    This is one of the big pitfalls in rails apps. AR makes it way to easy to violate TDA and LoD. Thanks for pointing this out. Please keep doing so – we all need to be chanting this like a mantra.

  7. Wilson Bilkovich said 3 days later:

    So, based on your points, I have refactored the code.

    It is now about 6 jillion times easier to test.

  8. James Mead said 3 days later:

    Cool.

RSS feed for this post

Comments are disabled