It’s time to wrap up the Yatzy program and draw some conclusions from my BDD adventures.

There are some disturbing issues regarding the design. The usage of Maps, Lists etc. I’ve been discussing this with a colleague but decided to forge ahead without doing anything about it at the moment. I don’t think it’s that disturbing just yet.

Instead, I’ve come up with another test to move forward and towards a possible end to this.

public void testShouldAddYatzyBonusWhenYatzyRollsAreAddedToLowerSection() throws Exception {
	game.addRollToLowerSection("Player", new int[] { 1, 1, 1, 1, 1 });
	assertThat(game.score("Player"), is.eq(50));
}

Ctrl-F11. RED, what a surprise. <5> does not pass the constraint <eq(<50>)>. Yeah yeah, it’s quite obvious. No bonus. And that’s really what we wanted … right?

(If someone’s wondering what all the Ctrl-this and Ctrl-thats are. It’s my way of educating you, my audience, in the noble art of Eclipse hot keys). Back to the topic and the implementation.

After a few minutes I come up with this:

private void checkForYatzy(int[] roll) {
	Set<Integer> values = new HashSet<Integer>();
	for (int i = 0; i < roll.length; i++) {
		values.add(roll[i]);
	}

	if (values.size() == 1) {
		gotYatzy  = true;
	}
}

And use that method in addRollToLowerSection.

public void addRollToLowerSection(String playerName, int[] roll) {
	List<int[]> rolls = getRollsForPlayer(playerName, playerScores);

	checkForYatzy(roll);

	rolls.add(roll);
	playerScores.put(playerName, rolls);
}

I’ll give it a go. And … BAM … red.

What?! Two tests break.

One is testShouldComputeAllRollsForAGivenplayer which appears to roll a yatzy. Yeah, it’s a yatzy all right. Although it doesn’t have to be. It can be anything, really. I’ll change that immediately.

public void testShouldComputeAllRollsForAGivenplayer() throws Exception {
	addRoll(new int[] { 1, 2, 3, 4, 5 });
	game.addRollToLowerSection("Player", new int[] { 1, 2, 1, 1, 1 });
	assertThat(game.score("Player"), is.eq(21));
}

Back to the yatzy test. <5> does not pass the constraint <eq(<50>)>. The roll itself seems to be included in the final score. That is: +5 for an all ones Yatzy and +50 for the bonus. After thinking quite a lot about where the design is heading I decide that it’s time for another class. Namely: Roll.

The reasoning behind this: I want to keep the history behind rolls and I also want to give them special ‘meaning’ regarding score. So I’m thinking; keep the original values AND assign a score as I add them to the scores maps.

Time to Roll up the sleeves. Here comes the pre-factoring lady.

I’ll comment out the failing test and start my work.

Some 10 minutes later I’ve introduced a Roll class. As an inner class. Now I, hate inner classes and would’ve moved it if it weren’t for readability for this blog. Check out the code below.

public class Game {

	private Map<String, List<Roll>> playerScores = new HashMap<String, List<Roll>>();

	private Map<String, List<Roll>> playerUpperScores = new HashMap<String, List<Roll>>();

	private final int bonusLimit = 63;

	private final int bonus = 35;

	private boolean gotYatzy = false;

	public void addRollToUpperSection(String playerName, int[] roll) {
		Roll r = new Roll(roll);
		List<Roll> rolls = getRollsForPlayer(playerName, playerUpperScores);

		rolls.add(r);
		playerUpperScores.put(playerName, rolls);
	}

	public void addRollToLowerSection(String playerName, int[] roll) {
		Roll r = new Roll(roll);
		List<Roll> rolls = getRollsForPlayer(playerName, playerScores);

		checkForYatzy(roll);

		rolls.add(r);
		playerScores.put(playerName, rolls);
	}

	private void checkForYatzy(int[] roll) {
		Set<Integer> values = new HashSet<Integer>();
		for (int i = 0; i < roll.length; i++) {
			values.add(roll[i]);
		}

		if (values.size() == 1) {
			gotYatzy  = true;
		}
	}

	public int score(String playerName) {
		List<Roll> rolls = getRollsForPlayer(playerName, playerScores);
		List<Roll> upperRolls = getRollsForPlayer(playerName, playerUpperScores);

		int upperScore = sumScoreFor(upperRolls);
		int score = sumScoreFor(rolls);

		if (bonusLimitReached(upperScore)) {
			upperScore += bonus;
		}

		if (gotYatzy) {
			score += 50;
		}

		return upperScore + score;
	}

	private boolean bonusLimitReached(int upperScore) {
		return upperScore >= bonusLimit;
	}

	private int sumScoreFor(List<Roll> rolls) {
		int score = 0;
		for (Roll r : rolls) {
			int [] roll = r.getRoll();
			for (int i = 0; i < roll.length; i++) {
				score += roll[i];
			}
		}
		return score;
	}

	private List<Roll> getRollsForPlayer(String playerName, Map<String, List<Roll>> playerScores) {
		List<Roll> rolls = new ArrayList<Roll>();
		if (playerScores.containsKey(playerName)) {
			rolls = playerScores.get(playerName);
		}
		return rolls;
	}

	private class Roll {

		private final int[] roll;

		public Roll(int[] roll) {
			this.roll = roll;
		}

		public int[] getRoll() {
			return roll;
		}
	}

}

Rather messy but that’ll do for now. Next step. Comment back test failing test-case. And start implementing.

I’m realizing at this moment that the Roll-class will take over some responsibilities I didn’t like the Game-class having in the first place. Lets see… clickety clack. A few more strokes at the keyboard.

OK. It’s working. But I’m not nearly happy with the code. Here’s what it looks like.

public class Game {

	private Map<String, List<Roll>> playerScores = new HashMap<String, List<Roll>>();

	private Map<String, List<Roll>> playerUpperScores = new HashMap<String, List<Roll>>();

	private final int bonusLimit = 63;

	private final int bonus = 35;

	private boolean gotYatzy = false;

	public void addRollToUpperSection(String playerName, int[] roll) {
		Roll r = new Roll(roll);
		List<Roll> rolls = getRollsForPlayer(playerName, playerUpperScores);

		rolls.add(r);
		playerUpperScores.put(playerName, rolls);
	}

	public void addRollToLowerSection(String playerName, int[] roll) {
		List<Roll> rolls = getRollsForPlayer(playerName, playerScores);

		Roll r = new Roll(roll);
		if (rollIsYatzy(roll)) {
			r = new Roll(50, roll);
		}

		rolls.add(r);
		playerScores.put(playerName, rolls);
	}

	private boolean rollIsYatzy(int[] roll) {
		Set<Integer> values = new HashSet<Integer>();
		for (int i = 0; i < roll.length; i++) {
			values.add(roll[i]);
		}

		return values.size() == 1;
	}

	public int score(String playerName) {
		List<Roll> rolls = getRollsForPlayer(playerName, playerScores);
		List<Roll> upperRolls = getRollsForPlayer(playerName, playerUpperScores);

		int upperScore = sumScoreFor(upperRolls);
		int score = sumScoreFor(rolls);

		if (bonusLimitReached(upperScore)) {
			upperScore += bonus;
		}

		if (gotYatzy) {
			score += 50;
		}

		return upperScore + score;
	}

	private boolean bonusLimitReached(int upperScore) {
		return upperScore >= bonusLimit;
	}

	private int sumScoreFor(List<Roll> rolls) {
		int score = 0;
		for (Roll r : rolls) {
			score += r.getScore();
		}
		return score;
	}

	private List<Roll> getRollsForPlayer(String playerName, Map<String, List<Roll>> playerScores) {
		List<Roll> rolls = new ArrayList<Roll>();
		if (playerScores.containsKey(playerName)) {
			rolls = playerScores.get(playerName);
		}
		return rolls;
	}

	private class Roll {

		private final int[] roll;

		private int score = 0;

		public Roll(int[] roll) {
			this.roll = roll;
		}

		public Roll(int score, int[] roll) {
			this(roll);
			this.score = score;
		}

		public int getScore() {
			if (score == 0) {
				for (int i = 0; i < roll.length; i++) {
					score += roll[i];
				}
			}
			return score;
		}

		public int[] getRoll() {
			return roll;
		}
	}

}

I’m not happy with the decision the Game-class is making whether or not it’s a Yatzy. Nor the creation of Roll instances. The ‘sections’ contains some duplication. It’s time for some more refactoring.

On my ToDo:
1. Roll is a separate class (not inner class)
2. Remove getRoll from Roll class
3. Introduce Section?
4. Renaming of methods

The reasoning behind this; Private methods are an indication of a possible SRP violation. Please observe indication AND possible.

Some larger refactorings including all items on my TODO made the code look like this. Game, Roll and Section classes in that order below.

public class Game {

	private Section upperSection = new Section();

	private Section lowerSection = new Section();

	private final static int BONUS_LIMIT = 63;

	private final static int BONUS = 35;

	public void addRollToUpperSection(String playerName, Roll roll) {
		upperSection.addRoll(playerName, roll);
	}

	public void addRollToLowerSection(String playerName, Roll roll) {
		lowerSection.addRoll(playerName, roll);
	}

	public int score(String playerName) {
		int upperScore = upperSection.scoreForPlayer(playerName);
		int score = lowerSection.scoreForPlayer(playerName);

		if (bonusLimitReached(upperScore)) {
			upperScore += BONUS;
		}

		return upperScore + score;
	}

	private boolean bonusLimitReached(int upperScore) {
		return upperScore >= BONUS_LIMIT;
	}
}

public class Roll {

	private final int[] roll;

	private int score;

	public Roll(int[] roll) {
		this(0, roll);
	}

	public Roll(int score, int[] roll) {
		this.score = score;
		this.roll = roll;
	}

	public int getScore() {
		if (score == 0) {
			for (int i = 0; i < roll.length; i++) {
				score += roll[i];
			}
		}
		return score;
	}
}

public class Section {

	Map<String, List<Roll>> playerScores = new HashMap<String, List<Roll>>();

	public void addRoll(String playerName, Roll roll) {
		List<Roll> rolls = getRollsForPlayer(playerName);
		rolls.add(roll);
		playerScores.put(playerName, rolls);
	}

	public int scoreForPlayer(String playerName) {
		List<Roll> rolls = getRollsForPlayer(playerName);
		int score = 0;
		for (Roll r : rolls) {
			score += r.getScore();
		}
		return score;
	}

	private List<Roll> getRollsForPlayer(String playerName) {
		List<Roll> rolls = new ArrayList<Roll>();
		if (playerScores.containsKey(playerName)) {
			rolls = playerScores.get(playerName);
		}
		return rolls;
	}
}

Some ‘big’ refactorings were made. The decision whether a Yatzy roll was made is moved out of the Game class. You are now free to put your own score to any roll with the optional constructor to Roll. Some of the design and my thoughts will be analyzed more in detail later.

I didn’t quite feel I could fit all that in this post. But as you might see, the responsbilities of each class are minimized. The real responsibilities of each class is much clearer this way, in my opinion. Anyway, I’ll share more of my thoughts later in a more analytical blog post about my feelings on BDD, TDD and design later this week. Until then.

Take care!

Advertisements

About Ola Ellnestam

Agile Coach, Systems developer, Agile Practitioner and father of three.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s