Coding

This popped up on a technical discussion site recently. The original question was how to write tests for code that invokes a method on particular values in a list. The problem was that the tests were messy, and the author was looking for a cleaner alternative. Here’s the example test, it asserts that the even-positioned elements in the parameters are passed to bar in the appropriate sequence.

public void testExecuteEven() {
  Mockery mockery = new Mockery();

  final Bar bar = mockery.mock(Bar.class);
  final Sequence sequence = new NamedSequence("sequence");

  final List<String> allParameters = new ArrayList<String>();
  final List<String> expectedParameters = new ArrayList<String>();

  for (int i = 0; i < 3; i++) {
    allParameters.add("param" + i);
    if (i % 2 == 0) {
      expectedParameters.add("param" + i);
    }
  }
  final Iterator<String> iter = expectedParameters.iterator();

  mockery.checking(new Expectations() {
   {
     while (iter.hasNext()) {
       one(bar).doIt(iter.next());
        inSequence(sequence);
      }
    }
  });

  Foo subject = new Foo();
  subject.setBar(bar);
  subject.executeEven(allParameters);
  mockery.assertIsSatisfied();
}

The intentions of the test are good, but its most striking feature is that there’s so much computation going on. This doesn’t need a new technique to make it more readable, it just needs to be simplified.

A unit test should be small and focussed enough that we don’t need any general behaviour. It just has to deal with one example, so we can make it as concrete as we like. With that in mind, we can collapse the test to this:

public void testCallsDoItOnEvenIndexedElementsInList() {
  final Mockery mockery = new Mockery();
  final Bar bar = mockery.mock(Bar.class);
  final Sequence evens = mockery.sequence("evens");

  final  List<String> params = 
    Arrays.asList("param0", "param1", "param2", "param3");

  mockery.checking(new Expectations() {{
    oneOf(bar).doIt(params.get(0)); inSequence(evens);
    oneOf(bar).doIt(params.get(2)); inSequence(evens);
  }});

  Foo subject = new Foo();
  subject.setBar(bar);
  subject.executeEven(params);
  mockery.assertIsSatisfied();
}

To me, this is more direct, a simpler statement of the example—if nothing else, there’s just less code to understand. I don’t need any loops because there aren’t enough values to justify them. The expectations are clearer because they show the indices of the elements I want from the list (an alternative would have been to put in the expected values directly). And if I pulled the common features, such as the mockery and the target object, into the test class, the test would be even shorter.

The short version of this post is: be wary of any general behaviour written into a unit test. The scope should be small enough that values can be coded directly. Be especially wary of anything with an if statement. If the data setup is more complicated, then consider using a Test Data Builder.

At the recent Software Craftsmanship conference in London, Willem and Marc ran a session on Responsibility-Driven Development with Mocks for about 30 people. Nat Pryce and I were sitting at the back watching and occasionally heckling.

The first striking thing was that when Willem and Marc asked who was using “Mock Objects” most everyone put their hand up (which was nice), but then only a handful also said they were thinking about Roles and Responsibilities when they did (which was frustrating). We first wrote up these ideas in our paper “Mock Roles Not Objects” and much of the difficulty we see people have with the technique of Mock Objects comes from focussing on classes rather than relationships.

As it happens, an example popped up in the rest of the session, which was run as a Coding Dojo. What was interesting to me was how the group managed to turn around its design ideas. Here’s what I can remember about how it worked out.

The domain was some kind of game, with a hero who moves around an environment slaying dragons and so forth. The first couple of stories were to do with displaying the current room, and then moving from one room to another. It was a little difficult getting started because the limitations of the event didn’t allow enough time to really drive the design from outer-level requirements, but the group managed to get started with something like:

describe Hero do
  it "should describe its surroundings" do
    hero = Hero.new(room)  
    
    room.stub!(:description).and_return("a room with twisty passages")
    
    console.should_receive(:show).with("in a room with twisty passages")
    hero.look(console)
  end
end

The expectation here says that when looking, the hero should write a text describing the room to the console. This was a place to start, but it doesn’t look right. Why is a hero attached to a room? And hero.look(console) just doesn’t read well, it’s hard to tell what it means. The tensions became clearer with the next feature, which was to have the hero move from one room to another. If we write

hero.move_to(other_room)

how can we tell that this has worked? We could ask the hero to look() again, but that means making an extra call for testing, which is not related to the intent of the test. We could ask the hero what his current room is, but that’s starting to leak into Asking rather than Telling. There may be a need for the hero to hold on to his current location, but we haven’t seen it yet.

Suddenly, it became clear that the dependencies were wrong. We already have a feature that can be told about the hero’s situation, which we can build on. If the feature were to be told about what is happening to the hero, we could use that to detect the change in room. So, our example now becomes:

describe Hero do
  it "should move to a room" do
    hero = Hero.new(console)  
    
    room.stub!(:description).and_return("a room with twisty passages")
    
    console.should_receive(:show).with("in a room with twisty passages")
    
    hero.move_to(room)
  end
end

That’s better, but it’s not finished. The term Console sounds like an implementation, not a role. Most of the sword-wielding adventurers that I know don’t know how to work a Console, but they’re quite happy to tell of their great deeds to, say, a Narrator (as David Peterson suggested). If we adjust our example we get.

describe Hero do
  it "should move to a room" do
    hero = Hero.new(narrator)  
    
    room.stub!(:description).and_return("a room with twisty passages")
    
    narrator.should_receive(:says).with("in a room with twisty passages")
    
    hero.move_to(room)
  end
end

The whole example now reads as if it’s in the same domain, in the language of a D&D game. It doesn’t refer to implementation details such as a Console—we might see that code when we get to the detailed implementation of a Narrator. Obviously, there’s a lot more we could do, for a start I’d like to see more structured messages between Hero and Narrator, but the session ran out of time at about this point.

Some lessons:

1. Naming, naming, naming. It’s the most important thing. A coherent unit of code should have a coherent vocabulary, it should read well. If not, I’m probably mixing concepts which will make the code harder to understand and more brittle to change than it needs to be.
2. When I’m about to write a test, I ask “if this were to work, who would know”. That’s the most revealing question in B/TDD. If there’s no visible effect from an event, except perhaps for changing a field in the target object, then maybe it’s worth waiting until there is a visible effect, or maybe there’s a concept missing, or maybe the structure isn’t quite right. Before writing more code, I try to make sure I understand its motivation.

Willem’s (and many other people’s) approach is slightly different. He likes to explore a bit further with the code before really sorting out the names, and he’s right that there’s a risk of Analysis-Paralysis. I do that occasionally, but my experience is that the effort of being really picky at this stage forces me to be clearer about what I’m trying to achieve, to ask those questions I really ought to have answers to, before I get in too deep.