Continuing the FrameStatus Implementation
Time for a New Test
We have our strike tests working, so it’s time for a new test. I’m inclined to try open frames next, because they might just work. If they don’t, it’ll be interesting to find out why. Here goes …
[Test] public void OpenFrame() {
frame.RollConsumed(4);
frame.RollConsumed(5);
frameStatus = frame.FrameStatus();
Assert.AreEqual("-", frameStatus.Symbol);
Assert.AreEqual("9", frameStatus.Score);
Assert.AreEqual("4", frameStatus.FirstRoll);
Assert.AreEqual("5", frameStatus.SecondRoll);
Assert.IsTrue(frameStatus.Complete);
}
OK, it’s clear now that this test won’t be green. We don’t have the dash for open implemented, and we don’t have the second roll saved. We also noticed that when we run this test, it’ll go red, but we won’t know about the subsequent tests, some of which should pass. This argues for the one assert per test practice, so we’ll try it. Instead of that test, we’ll create a new fixture for open frames:
[TestFixture] public class OpenFrameDisplayTestFixture {
Frame frame;
FrameStatus frameStatus;
[SetUp] public void SetUp() {
frame = new Frame();
frame.RollConsumed(4);
frame.RollConsumed(5);
frameStatus = frame.FrameStatus();
}
[Test] public void SymbolIsDash() {
Assert.AreEqual("-", frameStatus.Symbol);
}
[Test] public void ScoreIsNine() {
Assert.AreEqual("9", frameStatus.Score);
}
[Test] public void FirstRollIsFour() {
Assert.AreEqual("4", frameStatus.FirstRoll);
}
[Test] public void SecondRollIsFive() {
Assert.AreEqual("5", frameStatus.SecondRoll);
}
[Test] public void StatusIsComplete() {
Assert.IsTrue(frameStatus.Complete);
}
}
This is a little tedious, but we get information right away. The total score and complete work, but the other three do not. We expected the first roll to work, but we’re betting now that there’s still a “fake it” implementation in there or something like that.
We’re already liking this practice better than we expected to: it breaks our work down into smaller chunks and we should be able to get a little jolt of green-bar adrenaline sooner. Let’s look at the first roll implementation first. Sure enough, FrameStatus fakes that value:
public class FrameStatus {
...
public String FirstRoll {
get { return "10"; }
}
...
}
We need to change the FirstRollKicker to return the first roll – when it gets it! Our cool trick of yesterday, of calling the Kicker as it is installed, won’t work for this, because the FirstRollKicker (a) isn’t installed with SetKicker, and (b) wouldn’t know what the first roll was going to be anyway. So, after some discussion, we override AddRoll in FirstRollKicker, and implement the implied methods down to FrameStatus:
public class FirstRollKicker : ConsumingRollAddingKicker {
protected override IKicker NextKicker(int pins) {
if ( IsStrike(pins) )
return new TwoBonusKicker();
else
return new SecondRollKicker(pins);
}
protected override void AddRoll(Frame frame, int pins) {
frame.SetFirstRoll(pins);
base.AddRoll(frame, pins);
}
private bool IsStrike(int pins) {
return pins == 10;
}
}
public class Frame {
private int score;
protected IKicker kicker;
protected FrameStatus frameStatus; //Frame Status (sarcasm)
...
public void SetFirstRoll(int pins) {
frameStatus.SetFirstRoll(pins);
}
}
public class FrameStatus {
private string score;
private string symbol;
private string firstRoll;
...
public String FirstRoll {
get { return firstRoll; }
}
...
}
Let’s go after second roll and symbol at the same time, since they both belong in SecondRollKicker – we think. Should be the same as in FirstRollKicker, with new methods for second roll and symbol:
public class SecondRollKicker : ConsumingRollAddingKicker {
int firstRoll;
public SecondRollKicker(int pins) {
firstRoll = pins;
}
protected override void AddRoll(Frame frame, int pins) {
frame.SetSecondRoll(pins);
SetSymbol(frame, pins);
base.AddRoll (frame, pins);
}
private void SetSymbol(Frame frame, int secondRoll) {
if (IsSpare(secondRoll))
frame.SetSpare();
else
frame.SetOpen();
}
protected override IKicker NextKicker(int secondRoll) {
if (IsSpare(secondRoll))
return new OneBonusKicker();
else
return new IgnoreKicker();
}
private bool IsSpare(int secondRoll) {
return firstRoll + secondRoll == 10;
}
}
public class Frame {
private int score;
protected IKicker kicker;
protected FrameStatus frameStatus; //Frame Status (sarcasm)
...
public void SetSecondRoll(int pins) {
frameStatus.SetSecondRoll(pins);
}
public void SetSpare() {
frameStatus.SetSpare();
}
public void SetOpen() {
frameStatus.SetOpen();
}
}
public class FrameStatus {
private string score;
private string symbol;
private string firstRoll;
private string secondRoll;
public FrameStatus() {
score = null;
symbol = " ";
}
public String Symbol {
get { return symbol; }
}
public String Score {
get { return score; }
set { score = value; }
}
public String FirstRoll {
get { return firstRoll; }
}
public bool Complete {
get { return score != null; }
}
public String SecondRoll {
get { return secondRoll; }
}
public void SetStrike() {
symbol = "X";
}
public void SetFirstRoll(int pins) {
firstRoll = pins.ToString();
}
public void SetSecondRoll(int pins) {
secondRoll = pins.ToString();
}
public void SetSpare() {
symbol = "/";
}
public void SetOpen() {
symbol = "-";
}
}
Interim Assessment
This is a lot of little things, spread all over. Each one is different, but they’re still all over. This is what it feels like when you don’t have the right abstraction – you have to make changes all over. Does this mean that our Kicker idea is the wrong abstraction? It might. Another nail in its coffin? Perhaps.
The good news, we hope, is that we think we’re done. The spare feature should work, because we took the occasion to implement it – without a complete test, mind you, but with a compiler error, while we did the open frame. Let’s try a test for spare, in the existing mondo test class:
[Test] public void Spare() {
frame.RollConsumed(6);
frame.RollConsumed(4);
frame.RollConsumed(3);
frameStatus = frame.FrameStatus();
Assert.AreEqual("/", frameStatus.Symbol);
Assert.AreEqual("13", frameStatus.Score);
Assert.AreEqual("6", frameStatus.FirstRoll);
Assert.AreEqual("4", frameStatus.SecondRoll);
Assert.IsTrue(frameStatus.Complete);
}
Heart in mouth, I ran this – and it goes green. I do think we’re done. Chet is prowling the bookstore, we’ll see what he thinks when he gets back.
Chet agrees: We’re done for our current purposes.
Retrospective
One important feature that isn’t yet implemented in our FrameStatus is the cumulative game score. We have the individual frame score, but not the sum of the scores. This is an easy calculation, but working, as we are, at the individual frame level, and summing in the Game, the only place we can put that feature is probalby in the Game. Easy enough to do … but one question is just when it should be done. We can imagine that in a full-game display, we want to display the cumulative score where it’s available and not otherwise. We’ll decide tomorrow whether to code this up, or whether it’s more interesting to explore one of the other implementations.
We wound up only having to modify two of our five Kickers today. Three of them are modified now, and two are not: the bonus Kickers didn’t need any changes, and the change we made yesterday to IgnoreKicker was good enough for today.
Our scheme of calling the kickers at install didn’t really pay off much, but it did work for Ignore, and is probably the best way of doing that, because the frame is complete when Ignore is set, not when it is executed later, if it is.
But when we changed the Kickers, they had to call to the Frame to set status, and the frame had to call to the FrameStatus. We could have written less code if the Kickers had the FrameStatus, saving the methods in the Frame, but there’d be code to set the FrameStatus into the Kickers and pass it around. Not much better, we think.
This has taken longer than we expected intuitively, probably twice as long. It feels to me as if the Kickers and the Frame are now more tightly coupled, and of course the Frame and FrameStatus are tightly coupled as well. We could remove the Frame connection, as mentioned, by giving the status to the Kickers, which would remove a bunch of pass-thru methods from Frame.
What has happened is that the Kicker implementation removed almost all knowledge of the game state from Frame: it knows the score, but it doesn’t know how it got there, nor does it know whether it was a strike or spare or much of anything. All that information is in the Kickers … and worse yet, it’s not really saved there. The state of the Kicker state machine encodes the state but doesn’t save it. So we were essentially putting state back into the Frame (an FrameStatus) with this implementation.
At this point, the myth appears to be busted: the Kicker change, though cool, made the code much larger, and made the implementation of this story put back functionality that the first design change got rid of. Our tentative assessment is that the Kicker implementation was not a good idea.
We’ll have to see what happens with installing frame status into other implementations, but for now: BUSTED.
The Current Code
We’re up to 360 lines of code now, in the Kicker solution. The structure is still arguably reasonable, but there’s this big channel of interconnection between Kickers, Frame, and FrameStatus. and each of these changes seems to require changes to three classes. Not a good sign.
Now we still do not know how hard it’s going to be to do these changes in other versions, but it’s going to be hard to have it be much worse. Still, this isn’t an argument against a really good OO solution to bowling: it’s just evidence that this might not be a really good OO solution.
In addition, there are code improvements needed that we haven’t been paying much attention to, since our focus is on learning about the impact of various approaches to the problem on future flexibility. There are methods that should be protected or private, and probably awkwardnesses that we haven’t even noticed. But still, the code doesn’t have much duplication, and functionality seems pretty well separated into classes with single functions.
[TestFixture] public class FrameTestFixture { Frame frame; public FrameTestFixture() { } [SetUp] public void SetUp() { frame = new Frame(); } [Test] public void OpenFrame() { Assert.IsTrue(frame.RollConsumed(3)); Assert.IsTrue(frame.RollConsumed(4)); Assert.IsFalse(frame.RollConsumed(2)); Assert.AreEqual(7, frame.Score()); } [Test] public void SpareFrame() { Assert.IsTrue(frame.RollConsumed(6), "Done too soon, roll 1"); Assert.IsTrue(frame.RollConsumed(4), "Done too soon, roll 2"); Assert.IsFalse(frame.RollConsumed(2), "Not done soon enough, roll 3"); Assert.AreEqual(12, frame.Score()); } [Test] public void SpareFrameWithZeroBonus() { Assert.IsTrue(frame.RollConsumed(6)); Assert.IsTrue(frame.RollConsumed(4)); Assert.IsFalse(frame.RollConsumed(0)); Assert.IsFalse(frame.RollConsumed(2)); Assert.AreEqual(10, frame.Score()); } [Test] public void Strike() { Assert.IsTrue(frame.RollConsumed(10), "Strike"); Assert.IsFalse(frame.RollConsumed(4), "First Bonus"); Assert.IsFalse(frame.RollConsumed(5), "Second Bonus"); Assert.IsFalse(frame.RollConsumed(2), "Ignored next roll"); Assert.AreEqual(19, frame.Score()); } [Test] public void TenthFrameConsumesEverything() { frame = new TenthFrame(); Assert.IsTrue(frame.RollConsumed(6), "first"); Assert.IsTrue(frame.RollConsumed(4), "second"); Assert.IsTrue(frame.RollConsumed(10), "third"); } } [TestFixture] public class BowlingTestFixture { Game game; public BowlingTestFixture() { } [SetUp] public void SetUp() { game = new Game(); } public void RollMany(int count, int pins) { for ( int roll = 0; roll < count; roll++ ) { game.Roll(pins); } } public void RollAll(int[] rolls) { foreach ( int pins in rolls ) { game.Roll(pins); } } [Test] public void GutterBalls() { RollMany(20,0); Assert.AreEqual(0, game.Score()); } [Test] public void OpenFrames() { RollAll( new int[] {1,2, 3,4, 1,2, 3,4, 1,2, 3,4, 1,2, 3,4, 1,2, 3,4 }); Assert.AreEqual(50, game.Score()); } [Test] public void Alternating() { RollAll( new int[] { 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10}); Assert.AreEqual(200, game.Score()); } [Test] public void AlternatingSpareFirst() { RollAll( new int[] { 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6,4, 10, 6, 4}); Assert.AreEqual(200, game.Score()); } } [TestFixture] public class FrameDisplayTestFixture { Frame frame; FrameStatus frameStatus; public FrameDisplayTestFixture() { } [SetUp] public void SetUp() { frame = new Frame(); } [Test] public void Strike() { frame.RollConsumed(10); frameStatus = frame.FrameStatus(); Assert.AreEqual("X", frameStatus.Symbol); Assert.IsNull(frameStatus.Score); Assert.AreEqual("10", frameStatus.FirstRoll); Assert.IsNull(frameStatus.SecondRoll); Assert.IsFalse(frameStatus.Complete); } [Test] public void StrikeSecondRoll() { frame.RollConsumed(10); frame.RollConsumed(5); frameStatus = frame.FrameStatus(); Assert.AreEqual("X", frameStatus.Symbol); Assert.IsNull(frameStatus.Score); Assert.AreEqual("10", frameStatus.FirstRoll); Assert.IsNull(frameStatus.SecondRoll); Assert.IsFalse(frameStatus.Complete); } [Test] public void StrikeThirdRoll() { frame.RollConsumed(10); frame.RollConsumed(5); frame.RollConsumed(4); frameStatus = frame.FrameStatus(); Assert.AreEqual("X", frameStatus.Symbol); Assert.AreEqual("19", frameStatus.Score); Assert.AreEqual("10", frameStatus.FirstRoll); Assert.IsNull(frameStatus.SecondRoll); Assert.IsTrue(frameStatus.Complete); } [Test] public void Spare() { frame.RollConsumed(6); frame.RollConsumed(4); frame.RollConsumed(3); frameStatus = frame.FrameStatus(); Assert.AreEqual("/", frameStatus.Symbol); Assert.AreEqual("13", frameStatus.Score); Assert.AreEqual("6", frameStatus.FirstRoll); Assert.AreEqual("4", frameStatus.SecondRoll); Assert.IsTrue(frameStatus.Complete); } } public class Game { Frame[] frames; public Game() { frames = new Frame[10]; for ( int i = 0; i < 9; i++ ) { frames[i] = new Frame(); } frames[9] = new TenthFrame(); } public void Roll(int pins) { int frameCounter = 0; while ( !frames[frameCounter].RollConsumed(pins)) frameCounter++; } public int Score() { int score = 0; foreach ( Frame frame in frames ) { score += frame.Score(); } return score; } } public class Frame { private int score; protected IKicker kicker; protected FrameStatus frameStatus; //Frame Status (sarcasm) public Frame() { score = 0; kicker = new FirstRollKicker(); frameStatus = new FrameStatus(); } public void SetKicker(IKicker aKicker) { kicker = aKicker; kicker.SetStatus(this); } public void AddRoll(int pins) { score += pins; } public virtual bool RollConsumed(int pins) { return kicker.RollConsumed(this, pins); } public int Score() { return score; } public FrameStatus FrameStatus() { return frameStatus; } public void SetScore() { frameStatus.Score = Score().ToString(); } public void SetStrike() { frameStatus.SetStrike(); } public void SetFirstRoll(int pins) { frameStatus.SetFirstRoll(pins); } public void SetSecondRoll(int pins) { frameStatus.SetSecondRoll(pins); } public void SetSpare() { frameStatus.SetSpare(); } public void SetOpen() { frameStatus.SetOpen(); } } public class TenthFrame : Frame { public override bool RollConsumed(int pins) { kicker.RollConsumed(this, pins); return true; } } public interface IKicker { bool RollConsumed(Frame frame, int pins); void SetStatus(Frame frame); } public class IgnoreKicker: IKicker { public bool RollConsumed(Frame frame, int pins) { return false; } public void SetStatus(Frame frame) { frame.SetScore(); } } public abstract class RollAddingKicker: IKicker { public bool RollConsumed(Frame frame, int pins) { AddRoll(frame, pins); SetKicker(frame, NextKicker(pins)); return BallConsumed(); } public virtual void SetStatus(Frame frame) { return; } private void SetKicker(Frame frame, IKicker kicker) { frame.SetKicker(kicker); } protected virtual void AddRoll(Frame frame, int pins) { frame.AddRoll(pins); } protected abstract IKicker NextKicker(int pins); protected abstract bool BallConsumed(); } public abstract class ConsumingRollAddingKicker: RollAddingKicker { protected override bool BallConsumed() { return true; } } public abstract class NonconsumingRollAddingKicker: RollAddingKicker { protected override bool BallConsumed() { return false; } } public class FirstRollKicker : ConsumingRollAddingKicker { protected override IKicker NextKicker(int pins) { if ( IsStrike(pins) ) return new TwoBonusKicker(); else return new SecondRollKicker(pins); } protected override void AddRoll(Frame frame, int pins) { frame.SetFirstRoll(pins); base.AddRoll(frame, pins); } private bool IsStrike(int pins) { return pins == 10; } } public class SecondRollKicker : ConsumingRollAddingKicker { int firstRoll; public SecondRollKicker(int pins) { firstRoll = pins; } protected override void AddRoll(Frame frame, int pins) { frame.SetSecondRoll(pins); SetSymbol(frame, pins); base.AddRoll (frame, pins); } private void SetSymbol(Frame frame, int secondRoll) { if (IsSpare(secondRoll)) frame.SetSpare(); else frame.SetOpen(); } protected override IKicker NextKicker(int secondRoll) { if (IsSpare(secondRoll)) return new OneBonusKicker(); else return new IgnoreKicker(); } private bool IsSpare(int secondRoll) { return firstRoll + secondRoll == 10; } } public class TwoBonusKicker: NonconsumingRollAddingKicker { protected override IKicker NextKicker(int ignoredPins) { return new OneBonusKicker(); } public override void SetStatus(Frame frame) { frame.SetStrike(); } } public class OneBonusKicker: NonconsumingRollAddingKicker { protected override IKicker NextKicker(int ignoredPins) { return new IgnoreKicker(); } }