Musings on life, the universe and software development

Last time I'd just implemented the code for handling spares, and was looking to handle strikes next. First, however, there's the issue of these private members in Roll. All 3 of them are derived entirely rom GameState, so they don't seem to belong here. Just to refresh our memories, the members in question are:

    static const bool thisRollCounts=GameState::ballsLeftThisFrame || 
        !IsLastFrame<GameState>::value;
    static const bool thisIsFirstBallInFrame=thisRollCounts && GameState::ballsLeftThisFrame==0;
    static const bool thisIsLastBallInFrame=GameState::ballsLeftThisFrame==1;

So, before we move to handling strikes, I'd like to move these out of Roll. thisRollCounts comes first, so we'll start there, and apply Extract Method:

template<GameState>
struct ThisRollCounts
{
    static const bool value=GameState::ballsLeftThisFrame || 
        !IsLastFrame<GameState>::value;
};

The name no longer fits, so we'll rename it to NextRollCounts. So far so good. Next on the list is thisIsLastBallInFrame; I'm feeling confident, so we'll extract it, and rename in one go:

template<typename GameState>
struct NextBallIsStartOfFrame
{
    static const bool value=NextRollCounts<GameState>::value && GameState::ballsLeftThisFrame==0;
};

The tests still pass, so we can continue to the last of the three: thisIsLastBallInFrame. Again, I'll extract it and rename it in one hit:

template<typename GameState>
struct NextBallIsLastInFrame
{
    static const bool value=GameState::ballsLeftThisFrame==1;
};

Strike handling

This refactoring out of the way, we can move on to the next test:

AssertEqual<Roll<Roll<Roll<Roll<Roll<
    NullGame,10>,4>,5>,3>,0>::totalScore,31> strike;

In order for this to pass, we need some way of identifying that a strike has two bonus balls, whereas a spare has only one. Just defining bonusRolls as two for a strike and one for a spare isn't enough, though. If there are bonus balls remaining from the previous frame, we want to keep a tally of them, and finally we need to modify NeedsBonus so it uses bonusRolls:

struct NullGame
{
    static const unsigned bonusRolls=0;
...
};

template<typename GameState,unsigned roll>
struct Roll
{
    static const bool isStrike=NextBallIsStartOfFrame<GameState>::value &&
         roll==10;
    static const unsigned bonusRolls=(scoreThisFrame==10?(isStrike?2:1):0)+
        (GameState::bonusRolls?GameState::bonusRolls-1:0);
...
};

template<typename GameState>
struct NeedsBonus
{
    static const bool value=GameState::bonusRolls!=0;
};

Two consecutive strikes is still enough to flummox this code, though; some rolls need to be scored as a bonus in two frames, as the next test demonstrates:

AssertEqual<Roll<Roll<Roll<Roll<
    NullGame,10>,10>,4>,5>::totalScore,24+19+9> doubleStrike;

Now we need to take account of two previous rolls: if the previous frame was a spare, count this roll for a bonus; if the previous two frames were both strikes, count this roll for two bonuses. Just adding this we can see that we need some way to get to the PreviousGameState, and that NullGame is going to need isStrike:

template<typename GameState,unsigned roll>
struct Roll
{
    static const unsigned bonusMultiplier=(GameState::scoreThisFrame==10) + 
        GameState::PreviousGameState::isStrike;
    static const unsigned totalScore=GameState::totalScore+
        bonusMultiplier*roll+(NextRollCounts<GameState>::value?roll:0);
...
};

We can get the PreviousGameState with a typedef, so adding that we get:

struct NullGame
{
    typedef NullGame PreviousGameState;
    static const bool isStrike=false;
...
};

template<typename GameState,unsigned roll>
struct Roll
{
    typedef GameState PreviousGameState;
...
};

The test still fails: we're scoring 43 rather than 52. The light comes on if this ball is a strike, we're not marking this as the end of the frame. Easily fixed:

template<typename GameState,unsigned roll>
struct Roll
{
    static const unsigned ballsLeftThisFrame=(NextBallIsLastInFrame<GameState>::value || isStrike)?0:1;
...
};

All tests pass. Looking at the code, we can see that we no longer need bonusRolls, or even NeedsBonus, so we can remove those. The tests still pass.

bonusMultiplier now seems out of place; it's another implementation detail, so it doesn't deserve to be a public member, but not only that, it's solely a function of GameState. Extracting it is easy:

template<typename GameState>
struct BonusMultiplier
{
    static const unsigned value=(GameState::scoreThisFrame==10) + 
        GameState::PreviousGameState::isStrike;
};

The next two tests just pass:

AssertEqual<Roll<Roll<Roll<Roll<Roll<NullGame,10>,10>,10>,4>,5>::totalScore,30+24+19+9> tripleStrike;
AssertEqual<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<
    NullGame,10>,4>,6>,10>,4>,6>,10>,4>,6>,10>,4>,6>,10>,4>,6>,10>::totalScore,200> alternating;

A not so perfect game

But the final test, for a perfect game, fails:

AssertEqual<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<
    NullGame,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>::totalScore,300> perfect;

The compiler says:

ct-bowling.cpp: In instantiation of `Assert<false>':
ct-bowling.cpp:80:   instantiated from `AssertEqual<320, 300>'
ct-bowling.cpp:80:   instantiated from here
ct-bowling.cpp:4: error: creating array with size zero (`false')

320 rather than 300? That must mean that we're scoring the last two rolls, which must mean there's something wrong in NextRollCounts or in the frame counting code.

A new test, for the frame count, shows that it is indeed the frame counting code which is wrong:

AssertEqual<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<
    NullGame,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>::frameCount,10> perfectFrameCount;

The compiler complains that the frame count is 11 rather than 10. Looking at the code I can see that the frame count is always incremented if there are no more balls from the previous frame, irrespective of whether or not it was the last frame. This is actually an incorrect implementation of NextBallIsStartOfFrame, so we can just replace it with that. This test passes, but the perfect game test still fails.

We've got two tests that check properties of the perfect game, so we can extract that into a type of its own:

typedef Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<Roll<
    NullGame,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10>,10> PerfectGame;

The last roll should be scored as a bonus only once, so we can check for that:

AssertEqual<BonusMultiplier<PerfectGame::PreviousGameState>::value,1> perfectFrameFinalBonus;

This test fails: the bonus multiplier is two rather than one. That's because the bonus multiplier counts one if the roll two rolls ago was a strike, and one if the total score for the frame with the previous roll is 10 (which covers a strike and a spare). In the final frame, if we scored a strike, then there are two bonus balls, but the "score this frame" is still 10 on the second, so it gets counted twice.

We only care if the previous roll was a spare or a strike if it counted, so we make that change:

template<typename GameState>
struct BonusMultiplier
{
    typedef typename GameState::PreviousGameState PreviousGameState;
    static const unsigned value=(NextRollCounts<PreviousGameState>::value &&
                                 (GameState::scoreThisFrame==10)) + 
        PreviousGameState::isStrike;
};

The perfectFrameFinalBonus test now passes, but the perfect frame score still fails — now with only 310 rather than 320 we had eariler. Close, but no cigar.

Looking at the code, I get a hunch that we're also scoring the last frame as a normal roll. A quick test confirms that suspicion:

Assert<!NextRollCounts<PerfectGame::PreviousGameState>::value> perfectFrameFinalBallCounts;

Further thought leads me to believe this must be because the ballsLeftThisFrame is wrong. Another test confirms this:

AssertEqual<PerfectGame::PreviousGameState::ballsLeftThisFrame,0> perfectFrameFinalBallsLeft;

Thinking through the logic again, there is a ball left this frame, if it is the first ball in a frame, and not a strike. Writing this in the code gives:

    static const unsigned ballsLeftThisFrame=NextBallIsStartOfFrame<GameState>::value && !isStrike;

Which makes all the tests pass. Marvellous. ballsLeftThisFrame is clearly a bool rather than a number, so we change the code to take account of that.

Refactoring

Looking at the code, totalScore could do with simplifying as there are two references to roll, so we can extract RollMultiplier:

template<typename GameState>
struct RollMultiplier
{
    static const unsigned value=BonusMultiplier<GameState>::value+
        NextRollCounts<GameState>::value;
};

template<typename GameState,unsigned roll>
struct Roll
{
    static const unsigned totalScore=GameState::totalScore+
        RollMultiplier<GameState>::value*roll;
...
};

The tests still pass. scoreThisFrame feels wrong, since it is only used by BonusMultiplier, where it is used as shorthand for isStrike || isSpare. I'll rewrite it that way, which requires roll to be exposed; not too much of a problem:

template<typename GameState>
struct BonusMultiplier
{
    typedef typename GameState::PreviousGameState PreviousGameState;
    static const unsigned value=(NextRollCounts<PreviousGameState>::value &&
                                 (GameState::isStrike || GameState::isSpare)) + 
        PreviousGameState::isStrike;
};

template<typename GameState,unsigned roll>
struct Roll
{
    static const unsigned isSpare=GameState::ballsLeftThisFrame &&
        ((roll+GameState::roll)==10);
...
};

We also have to add isSpareto NullGame. The tests all pass, so we can remove all references to scoreThisFrame. The tests still pass, so we're done.

Wrapping up

There's more I'd like to do to improve this code — extracting the isStrike and isSpare code seems a good place to start. For now, we're done — the tests pass, and the code is better than it was.