Revisited Kata: Listening to the tests to improve the design of Ohce

A couple of weeks ago, I did the the Ohce kata in a Barcelona Software Craftsmanship event (I wrote a previous post about it).

I wasn't happy with the resulting tests for the Ohce class:

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.core.*;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.junit.Before;
	import org.junit.Test;
	public class OhceTest {
	private Mockery context;
	private GreetingsSelector selector;
	private Notifier notifier;
	private Ohce ohce;
	private PhraseReader phraseReader;
	private String userName;
	private Phrase stopPhrase;
	@Before
	public void setUp() {
	context = new Mockery();
	selector = context.mock(GreetingsSelector.class);
	notifier = context.mock(Notifier.class);
	phraseReader = context.mock(PhraseReader.class);
	String stopPhraseContent = "Stop!";
	ohce = new Ohce(stopPhraseContent, selector, notifier, phraseReader);
	userName = "Juan";
	stopPhrase = new Phrase(stopPhraseContent);
	}
	@Test
	public void greets_user() {
	String greeting = "¡Buenos días Juan!";
	context.checking(new Expectations() {{
	allowing(phraseReader);
	will(onConsecutiveCalls(
	returnValue(new Phrase("not used")),
	returnValue(stopPhrase)
	));
	oneOf(selector).selectGreeting(userName);
	will(returnValue(greeting));
	oneOf(notifier).greet(greeting);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void echoes_the_reversed_user_phrase() {
	Phrase phrase = new Phrase("hola");
	Phrase reversedPhrase = new Phrase("aloh");
	context.checking(new Expectations() {{
	ignoring(selector);
	exactly(2).of(phraseReader).read();
	will(onConsecutiveCalls(
	returnValue(phrase),
	returnValue(stopPhrase)
	));
	oneOf(notifier).echoReversedPhrase(reversedPhrase);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void identifies_palindromes() {
	Phrase palindrome = new Phrase("ana");
	context.checking(new Expectations() {{
	ignoring(selector);
	exactly(2).of(phraseReader).read();
	will(onConsecutiveCalls(
	returnValue(palindrome),
	returnValue(stopPhrase)
	));
	oneOf(notifier).echoReversedPhrase(palindrome);
	oneOf(notifier).palindromesRock();
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void says_bye_when_told_to_stop() {
	context.checking(new Expectations() {{
	ignoring(selector);
	oneOf(phraseReader).read();
	will(returnValue(stopPhrase));
	oneOf(notifier).sayBye(userName);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void keeps_asking_for_the_user_input_until_told_to_stop() {
	context.checking(new Expectations() {{
	ignoring(selector);
	exactly(3).of(phraseReader).read();
	will(
	onConsecutiveCalls(
	returnValue(new Phrase("pepe")),
	returnValue(new Phrase("moko")),
	returnValue(stopPhrase)
	)
	);
	oneOf(notifier).echoReversedPhrase(new Phrase("epep"));
	oneOf(notifier).echoReversedPhrase(new Phrase("okom"));
	oneOf(notifier).sayBye(userName);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	}

view raw OldOhceTest.java hosted with ❤ by GitHub

What I didn't like about the tests was that they weren't independent from each other.

When I started doing TDD, they were independent, but as soon as I introduced the loop to request more user phrases, I had to stub a call to the PhraseReader in each test that returned the stop phrase to avoid infinite loops.

This made the tests to be very coupled to Ohce's implementation:

	package com.dodevjutsu.katas.ohce.core;
	public class Ohce {
	private Phrase stopPhrase;
	private GreetingsSelector selector;
	private Notifier notifier;
	private PhraseReader phraseReader;
	public Ohce(String stopPhraseContent, GreetingsSelector selector, Notifier notifier, PhraseReader phraseReader) {
	this.stopPhrase = new Phrase(stopPhraseContent);
	this.selector = selector;
	this.notifier = notifier;
	this.phraseReader = phraseReader;
	}
	public void run(String userName) {
	greet(userName);
	processPhrase(phraseReader.read());
	sayBye(userName);
	}
	private void greet(String userName) {
	notifier.greet(selector.selectGreeting(userName));
	}
	private void sayBye(String userName) {
	notifier.sayBye(userName);
	}
	private void processPhrase(Phrase phrase) {
	if(shouldStop(phrase)) {
	return;
	}
	notifier.echoReversedPhrase(phrase.reversed());
	if(phrase.isPalindrome()) {
	notifier.palindromesRock();
	}
	processPhrase(phraseReader.read());
	}
	private boolean shouldStop(Phrase phrase) {
	return stopPhrase.equals(phrase);
	}
	}

view raw OldOhce.java hosted with ❤ by GitHub

Another problem was a violation of the Law of Demeter caused by asking the Phrase value object, being returned by the PhraseReader, if it was a palindrome.

This made it necessary to stub some calls to the PhraseReader returning dummy phrases to avoid getting null pointer exceptions.

These problems in the tests were a signal of problems in the design (see What the tests will tell you).

I think there were two main problems:

• Ohce code had too many responsibilities.
  • Controlling the dialog with the user.
  • Responding to the user inputs.
• Ohce code was too procedural.

How could I fix them?

Well, I started thinking about a metaphor in which Ohce was having a dialog with the user and responded to each user phrase, and how this dialog might be infinite unless the user made it stop by introducing an stop phrase.

This made me realize that there were some missing abstractions.

So I decided to explore this metaphor of an infinite dialog comprised of responses to each user phrase:

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.adapters.dialogs.InfiniteDialog;
	import com.dodevjutsu.katas.ohce.core.Dialog;
	import com.dodevjutsu.katas.ohce.core.Phrase;
	import com.dodevjutsu.katas.ohce.core.PhraseReader;
	import com.dodevjutsu.katas.ohce.core.Response;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.junit.Before;
	import org.junit.Test;
	public class InfiniteDialogTest {
	private Mockery context;
	private Dialog dialog;
	private Response response;
	private PhraseReader phraseReader;
	private String stopPhraseContent;
	@Before
	public void setUp() {
	context = new Mockery();
	phraseReader = context.mock(PhraseReader.class);
	response = context.mock(Response.class);
	stopPhraseContent = "Stop!";
	dialog = new InfiniteDialog(phraseReader, response, stopPhraseContent);
	}
	@Test
	public void keeps_asking_for_input_until_told_to_stop() {
	context.checking(new Expectations() {{
	exactly(3).of(phraseReader).read();
	will(onConsecutiveCalls(
	returnValue(new Phrase("pepe")),
	returnValue(new Phrase("moko")),
	returnValue(new Phrase(stopPhraseContent))
	));
	oneOf(response).respondTo(new Phrase("pepe"));
	oneOf(response).respondTo(new Phrase("moko"));
	}});
	dialog.start();
	context.assertIsSatisfied();
	}
	}

view raw InfiniteDialogTest.java hosted with ❤ by GitHub

InfiniteDialog only had the responsibility of going on reading and responding to the user phrases until told to stop.

Going on with the metaphor, each of those responses to the user input might be seen a sequence of two responses: one that reversed the user phrase and another one that celebrated when the user phrase was a palindrome.

To compose several independent responses, I used a decorator:

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.adapters.reponses.SequenceOfResponses;
	import com.dodevjutsu.katas.ohce.core.Phrase;
	import com.dodevjutsu.katas.ohce.core.Response;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.jmock.Sequence;
	import org.junit.Before;
	import org.junit.Test;
	import java.util.ArrayList;
	import java.util.List;
	public class SequenceOfResponsesTest {
	private Mockery context;
	private Response firstResponse;
	private Response secondResponse;
	private Response response;
	@Before
	public void setUp() {
	context = new Mockery();
	firstResponse = context.mock(Response.class, "firstResponse");
	secondResponse = context.mock(Response.class, "secondResponse");
	List<Response> responses = new ArrayList<>();
	responses.add(firstResponse);
	responses.add(secondResponse);
	response = new SequenceOfResponses(responses);
	}
	@Test
	public void uses_each_of_its_responses_in_order() {
	final Sequence responsesSequence = context.sequence("responsesSequence");
	Phrase anyPhrase = new Phrase("anything");
	context.checking(new Expectations() {{
	oneOf(firstResponse).respondTo(anyPhrase);
	inSequence(responsesSequence);
	oneOf(secondResponse).respondTo(anyPhrase);
	inSequence(responsesSequence);
	}});
	response.respondTo(anyPhrase);
	context.assertIsSatisfied();
	}
	}

view raw SequenceOfResponsesTest.java hosted with ❤ by GitHub

Then I just needed to separately code each concrete response:

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.adapters.reponses.ReversingResponse;
	import com.dodevjutsu.katas.ohce.core.Notifier;
	import com.dodevjutsu.katas.ohce.core.Phrase;
	import com.dodevjutsu.katas.ohce.core.Response;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.junit.Before;
	import org.junit.Test;
	public class ReversingResponseTest {
	private Mockery context;
	private Notifier notifier;
	private Response response;
	@Before
	public void setUp() {
	context = new Mockery();
	notifier = context.mock(Notifier.class);
	response = new ReversingResponse(notifier);
	}
	@Test
	public void echoes_the_reversed_user_phrase() {
	Phrase phrase = new Phrase("hola");
	Phrase reversedPhrase = new Phrase("aloh");
	context.checking(new Expectations() {{
	oneOf(notifier).echoReversedPhrase(reversedPhrase);
	}});
	response.respondTo(phrase);
	context.assertIsSatisfied();
	}
	}

view raw ReversingResponseTest.java hosted with ❤ by GitHub

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.adapters.reponses.PalindromesResponse;
	import com.dodevjutsu.katas.ohce.core.Notifier;
	import com.dodevjutsu.katas.ohce.core.Phrase;
	import com.dodevjutsu.katas.ohce.core.Response;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.junit.Before;
	import org.junit.Test;
	public class PalindromeResponseTest {
	private Mockery context;
	private Notifier notifier;
	private Response response;
	@Before
	public void setUp() {
	context = new Mockery();
	notifier = context.mock(Notifier.class);
	response = new PalindromesResponse(notifier);
	}
	@Test
	public void notices_palindromes() {
	Phrase palindrome = new Phrase("ana");
	context.checking(new Expectations() {{
	oneOf(notifier).palindromesRock();
	}});
	response.respondTo(palindrome);
	context.assertIsSatisfied();
	}
	@Test
	public void ignores_non_palindromes() {
	Phrase nonPalindrome = new Phrase("koko");
	context.checking(new Expectations() {{
	never(notifier).palindromesRock();
	}});
	response.respondTo(nonPalindrome);
	context.assertIsSatisfied();
	}
	}

view raw PalindromeResponseTest.java hosted with ❤ by GitHub

Notice how all the tests in InfiniteDialogTest, SequenceOfResponsesTest, ReversingResponseTest and PalindromeResponseTest were just adaptations of tests that were originally in OhceTest, only that now they were much more focused and simple.

I regard this as a sign of design improvement.

Finally, I refactored the code to make Ohce start using the InfiniteDialog by applying parallel change. It was a very smooth refactoring during which no tests were broken.

I think that this smoothness was a special case, because all the expectations in old Ohce's tests were also satisfied using InfiniteDialog. Using InfiniteDialog just added some new layers of abstraction over old Ohce's collaborators, but behind those layers the old collaborations were still happening.

In a different case, I might have probably broken some Ohce's unit tests, but that wouldn't have been a problem because the behavior would have been still protected by the end-to-end tests. So, I could have just deleted the broken unit tests and written a new one for the collaboration between Ohce and Dialog.

These were the new tests for Ohce:

	package com.dodevjutsu.katas.ohce.tests.unit;
	import com.dodevjutsu.katas.ohce.core.Dialog;
	import com.dodevjutsu.katas.ohce.core.GreetingsSelector;
	import com.dodevjutsu.katas.ohce.core.Notifier;
	import com.dodevjutsu.katas.ohce.core.Ohce;
	import org.jmock.Expectations;
	import org.jmock.Mockery;
	import org.junit.Before;
	import org.junit.Test;
	public class OhceTest {
	private Mockery context;
	private GreetingsSelector selector;
	private Notifier notifier;
	private Ohce ohce;
	private String userName;
	private Dialog dialog;
	@Before
	public void setUp() {
	context = new Mockery();
	selector = context.mock(GreetingsSelector.class);
	notifier = context.mock(Notifier.class);
	dialog = context.mock(Dialog.class);
	ohce = new Ohce(selector, notifier, dialog);
	userName = "Juan";
	}
	@Test
	public void greets_user() {
	String greeting = "any greeting";
	context.checking(new Expectations() {{
	ignoring(dialog);
	oneOf(selector).selectGreeting(userName);
	will(returnValue(greeting));
	oneOf(notifier).greet(greeting);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void starts_a_dialog_with_the_user() {
	context.checking(new Expectations() {{
	ignoring(notifier);
	ignoring(selector);
	oneOf(dialog).start();
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	@Test
	public void says_bye_when_told_to_stop() {
	context.checking(new Expectations() {{
	ignoring(selector);
	ignoring(dialog);
	oneOf(notifier).sayBye(userName);
	ignoring(notifier);
	}});
	ohce.run(userName);
	context.assertIsSatisfied();
	}
	}

view raw OhceTest.java hosted with ❤ by GitHub

And this is its new code:

	package com.dodevjutsu.katas.ohce.core;
	public class Ohce {
	private final Dialog dialog;
	private final GreetingsSelector selector;
	private final Notifier notifier;
	public Ohce(GreetingsSelector selector, Notifier notifier, Dialog dialog) {
	this.selector = selector;
	this.notifier = notifier;
	this.dialog = dialog;
	}
	public void run(String userName) {
	greet(userName);
	dialogWithUser();
	sayBye(userName);
	}
	private void dialogWithUser() {
	dialog.start();
	}
	private void greet(String userName) {
	notifier.greet(selector.selectGreeting(userName));
	}
	private void sayBye(String userName) {
	notifier.sayBye(userName);
	}
	}

view raw Ohce.java hosted with ❤ by GitHub

We've seen how listening to some problems in Ohce's tests, led me to find some design problems, that once fixed produced much more focused, clearer and less coupled tests, and how having used mocks didn't suppose any problem at all during the whole refactoring process.

--------------------

You can find all the code in this GitHub repository.