Tell Don't Ask

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.