Dungeon 100

I decide to redo the music playing logic with TDD. Results are mixed.

Last night at the Zoom Ensemble, I showed MusicPlayer to the group. My main question was something like “I didn’t TDD this. Years ago I’d have said to TDD everything. I don’t see the value of TDD here. Whazzup???”

Here’s the code for your review:

MusicPlayer = class()

function MusicPlayer:init(runner, dungeon,battle)
    self.runner = runner
    assert(runner~=nil, "runner nil")
    self.dungeon = dungeon or asset.downloaded.A_Hero_s_Quest.Dungeon
    self.battle = battle or asset.downloaded.A_Hero_s_Quest.Battle
    self.playing = nil
    self.locked = false
    tween.delay(1, self.decide,self)
end

function MusicPlayer:battleMusic()
    if self.locked or self.playing == self.battle then return end
    self:play(self.battle)
end

function MusicPlayer:decide()
    if self:hasMonsterNearPlayer() then
        self:battleMusic()
    else
        self:dungeonMusic()
    end
    tween.delay(1,self.decide,self)
end

function MusicPlayer:dungeonMusic()
    if self.locked or self.playing == self.dungeon then return end
    self:play(self.dungeon)
end

function MusicPlayer:hasMonsterNearPlayer()
    local range = 5
    return self.runner:hasMonsterNearPlayer(range)
end

function MusicPlayer:play(tune)
    music(tune,true)
    self.playing = tune
    self.locked = true
    tween.delay(3,self.unlock, self)
end

function MusicPlayer:unlock()
    self.locked = false
end

Now, it’s odd. This code is pretty simple on the surface, and in fact I wrote it without tests and without difficulty. It’s more complex than it appears, however, because of the tween.delay calls which make things happen “later”. The check to see if there are monsters close by is done once per second, and once a tune is started, it is “locked” for 3 seconds and then unlocked. This ensures that we don’t just stutter back and forth between tunes in too irritating a fashion.

So the flow here is more complicated than it may appear, and it would certainly be possible to get it wrong. This is a good argument for TDD during its creation, or testing after the fact.

And yet, I, one of nature’s greatest proponents of TDD, didn’t use TDD to create it. Whazzup???

In the Zoom Ensemble, I wondered whether there are code chunks that are too small to TDD, and we all agreed, for example, that we wouldn’t TDD a getter or setter. (Most of us would wind up with the getter/setter covered by another test.) GeePaw Hill focuses his TDD on “branching logic” and the only visible real branching here is in the decide method, which is at least as hard to test as it is to write:

function MusicPlayer:decide()
    if self:hasMonsterNearPlayer() then
        self:battleMusic()
    else
        self:dungeonMusic()
    end
    tween.delay(1,self.decide,self)
end

Even if we were to adjust the code for easier testing, we still have nothing but an if that’s harder to test than to write.

I believe it was GeePaw who made another point, which was that what we see in the world isn’t people writing methods that are so small and simple they don’t need testing. We can’t get them to write small simple methods at all. So perhaps my “too small” theory is moot. But the fact remains:

I, a proponent of TDD, didn’t TDD this, and have skipped TDDing for many other things here in the dungeon and other programs, and WHAZZUP???

Again, from GeePaw, we have what he calls “the money principle”, the fact that we do TDD because it is the fastest way we know to get working code. Why doesn’t he call it “the speed principle”? I have no explanation. But I do agree entirely that, by and large, TDD is the best way I know to get quickly to code that is working, easily seen to be working, and protected against future edits breaking it.

Now, I’m not here to tell you what to do, or even to advise you, but if I will say that if someone were to get as good at TDD as GeePaw, or Bill Wake, or even me, and then there were to decide not to TDD something, I’d be OK with it. If someone barely could TDD at all decided not to TDD something, I’d be less sanguine about it.

Now at various times, both Chet and Bryan suggested that if I had started with TDD for my music player, it might have turned out to be just as good or better, and it certainly would have been testable. We didn’t try that last night, but just for fun, let’s try it this morning.

TDDing a MusicPlayer

I’m going to do this in a separate Codea project, but I’ll be assuming that it’s really part of Dungeon and can use Dungeon objects and must adhere to the general structure.

The requirements are:

• Usually the tune “dungeon” plays as the game proceeds.
• If monsters are near the player, the tune “battle” plays instead.
• No tune plays for less than three seconds, to avoid bouncing back and forth erratically.

What would be a reasonable first test? Maybe something like this:

        _:test("Initial song is dungeon", function()
            local mp = MusicPlayer()
            _:expect(mp:nowPlaying()).is("dungeon")
        end)

That forces this:

MusicPlayer = class()

function MusicPlayer:init()
    
end

function MusicPlayer:nowPlaying()
    return "dungeon"
end

Test is green. Now we need to decide a bit of interface. Let’s assume a method, monsterIsNear, sent when there’s a nearby monster.

        _:test("Song is battle when monster is near", function()
            local mp = MusicPlayer()
            mp:monsterIsNear()
            _:expect(mp:nowPlaying()).is("battle")
        end)

This fails, and drives me to code this:

function MusicPlayer:init()
    self.playing = "dungeon"
end

function MusicPlayer:monsterIsNear()
    self.playing = "battle"
end

function MusicPlayer:nowPlaying()
    return self.playing
end

I guess, in for a penny, we should have a method monsterIsFar:

        _:test("Song goes back to dungeon when monster is far", function()
            local mp = MusicPlayer()
            mp:monsterIsNear()
            mp:monsterIsFar()
            _:expect(mp:nowPlaying()).is("dungeon")
        end)

This fails and requires:

function MusicPlayer:monsterIsFar()
    self.playing = "dungeon"
end

Tests are all nice and green. Of course no music is playing, and we’ve not done anything about the timers. Nor do we have any idea how monsterIsNear and monsterIsFar work. Even a quick review of the Sesame Street bit on “near” and “far” is no help.

But, I have to say, there is a kind of confidence building up in me, that this thing is shaping up as I’d like it to. That said, it does feel like it’s taking a lot of typing and a lot of time.

Go slow to go fast and all that.

Now there’s the notion of the three-second delay before the player switches tunes. What if we were to say that that’s not MusicPlayer’s problem. If you ask it to switch tunes every millisecond, it’ll do it. Timing is someone else’s problem.

We do want to have it play the actual tunes. Now my original design was that the MusicPlayer knows the “playlist”, i.e. the two tunes it can currently play. But we can test that as well.

        _:test("MP knows the default tunes", function()
            local mp = MusicPlayer()
            _:expect(mp:tuneFor("dungeon")).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
            _:expect(mp:tuneFor("battle")).is(asset.downloaded.A_Hero_s_Quest.Battle)
        end)

That requires me to code something like this:

function MusicPlayer:init()
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self.playing = "dungeon"
end

function MusicPlayer:tuneFor(tuneName)
    return self.tunes[tuneName]
end

Tests still green. Now this still won’t play anything. Works great except for that detail and a few others, like the timing.

Now we know that we want to play the song, looping, when we play it. And we know that we do that this way:

    music(asset,true)

Where asset is whatever song we want to play. The music function is built in to Codea. But we could pass in the function to call in the init:

        _:test("MP actually plays the tune", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(tunePlaying).is(mp:tuneFor("dungeon"))
        end)

Now I need to define my fakeMusic function:

function fakeMusic(tune,loop)
    tunePlaying = tune
end

The test doesn’t run yet, providing the excellent message:

5: MP actually plays the tune  -- Actual: nil, Expected: Asset Key: Dungeon.m4a (path: "/private/var/mobile/Containers/Data/Application/7AE638E9-80DA-41C0-93BC-01F4D3CF1781/Library/AssetPackDownloads/A Hero's Quest.assets/Dungeon.m4a")

We need to make the player play.

function MusicPlayer:init(musicFunction)
    self.music = musicFunction or music
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self.playing = "dungeon"
end

Now we have the function or the fake one saved … so we need to call it …

function MusicPlayer:play()
    self.music(self.playing, true)
end

And we need to call that:

function MusicPlayer:monsterIsFar()
    self.playing = "dungeon"
    self:play()
end

function MusicPlayer:monsterIsNear()
    self.playing = "battle"
    self:play()
end

I expect the test to run or to provide an interesting error.

2: Song is battle when monster is near -- Tests:70: attempt to call a nil value (field 'music')
5: MP actually plays the tune  -- Actual: nil, Expected: Asset Key: Dungeon.m4a (path: "/private/var/mobile/Containers/Data/Application/7AE638E9-80DA-41C0-93BC-01F4D3CF1781/Library/AssetPackDownloads/A Hero's Quest.assets/Dungeon.m4a")

Hm. What have I done wrong? If I were a T&C|R person, I’d revert and do over. I’m not, so I’ll look.

Line 70 is this:

function MusicPlayer:play()
    self.music(self.playing, true)
end

So music is nil. That seems odd, since:

function MusicPlayer:init(musicFunction)
    self.music = musicFunction or music
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self.playing = "dungeon"
end

Ah. False alarm that test said

    local mp = MusicPlayer

And should have said:

    local mp = MusicPlayer()

Now for the problem that’s more like I expected.

5: MP actually plays the tune  -- Actual: nil, Expected: Asset Key: Dungeon.m4a (path: "/private/var/mobile/Containers/Data/Application/7AE638E9-80DA-41C0-93BC-01F4D3CF1781/Library/AssetPackDownloads/A Hero's Quest.assets/Dungeon.m4a")

Well, we didn’t really init the class correctly, since we just set the playing variable but we don’t play it.

function MusicPlayer:init(musicFunction)
    self.music = musicFunction or music
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self.playing = "dungeon"
    self:play()
end

That gives me a new and informative error:

5: MP actually plays the tune  -- Actual: dungeon, Expected: Asset Key: Dungeon.m4a (path: "/private/var/mobile/Containers/Data/Application/7AE638E9-80DA-41C0-93BC-01F4D3CF1781/Library/AssetPackDownloads/A Hero's Quest.assets/Dungeon.m4a")

I’m not sending the tune to music, I’m sending the thing the tune is called. So:

function MusicPlayer:play()
    self.music(self:tuneFor(self.playing), true)
end

Now the test runs. Let’s extend it:

        _:test("MP actually plays the tune", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(tunePlaying).is(mp:tuneFor("dungeon"))
            mp:monsterIsNear()
            _:expect(tunePlaying).is(mp:tuneFor("battle"))
        end)

And the test still runs.

Let’s review the class we have so far:

MusicPlayer = class()

function MusicPlayer:init(musicFunction)
    self.music = musicFunction or music
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self.playing = "dungeon"
    self:play()
end

function MusicPlayer:monsterIsFar()
    self.playing = "dungeon"
    self:play()
end

function MusicPlayer:monsterIsNear()
    self.playing = "battle"
    self:play()
end

function MusicPlayer:nowPlaying()
    return self.playing
end

function MusicPlayer:play()
    self.music(self:tuneFor(self.playing), true)
end

function MusicPlayer:tuneFor(tuneName)
    return self.tunes[tuneName]
end

Pretty simple so far. No real logic.

What might we object to? Well, a pure music player wouldn’t know about monsters being near or far, it would just know to play certain tunes if told to. So let’s refactor first:

function MusicPlayer:init(musicFunction)
    self.music = musicFunction or music
    self.tunes = {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self:play("dungeon")
end

function MusicPlayer:monsterIsFar()
    self:play("dungeon")
end

function MusicPlayer:monsterIsNear()
    
    self:play("battle")
end

function MusicPlayer:play(tuneName)
    self.playing = tuneName
    self.music(self:tuneFor(self.playing), true)
end

I notice that I’m getting sound, because some of my MPs are called without the fakeMusic function, and they all should be using it. So I’ll make that change to the tests.

Now, we’re trying to untangle “monster is near” from the decisions made in the player, which should just have to do with playing whatever tune whose name we provide:

So let’s change the tests just to request the tune they want. Maybe that would have been a better design all along.

This makes the tests look a bit silly:

        _:test("Initial song is dungeon", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(mp:nowPlaying()).is("dungeon")
        end)
        
        _:test("Song is battle when monster is near", function()
            local mp = MusicPlayer(fakeMusic)
            mp:play("battle")
            _:expect(mp:nowPlaying()).is("battle")
        end)
        
        _:test("Song goes back to dungeon when monster is far", function()
            local mp = MusicPlayer(fakeMusic)
            mp:play("battle")
            mp:play("dungeon")
            _:expect(mp:nowPlaying()).is("dungeon")
        end)
        
        _:test("MP knows the default tunes", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(mp:tuneFor("dungeon")).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
            _:expect(mp:tuneFor("battle")).is(asset.downloaded.A_Hero_s_Quest.Battle)
        end)
        
        _:test("MP actually plays the tune", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(tunePlaying).is(mp:tuneFor("dungeon"))
            mp:play("battle")
            _:expect(tunePlaying).is(mp:tuneFor("battle"))
        end)

The names don’t match the code, because we’ve discovered that we don’t want our music player ideas tangled up with the notion of close and distant monsters.

So we are faced with a design question, because our code told us we had things tangled up.

So let’s remove the game-oriented methods from MusicPlayer:

function MusicPlayer:monsterIsFar()
    self:play("dungeon")
end

function MusicPlayer:monsterIsNear()    
    self:play("battle")
end

Now for the object in hand, MusicPlayer, we have more tests than we need, and they have bad names. I think we’ll push this thing to its limit, so let’s simplify the tests down to something minimal but complete.

        _:test("Initial song is dungeon", function()
            local mp = MusicPlayer(fakeMusic)
            _:expect(mp:nowPlaying()).is("dungeon")
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
        end)
        
        _:test("Song is battle when requested", function()
            local mp = MusicPlayer(fakeMusic)
            mp:play("battle")
            _:expect(mp:nowPlaying()).is("battle")
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Battle)
        end)
        
        _:test("Song goes back to dungeon when requested", function()
            local mp = MusicPlayer(fakeMusic)
            mp:play("battle")
            mp:play("dungeon")
            _:expect(mp:nowPlaying()).is("dungeon")
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
        end)

This packet tells us that the MP knows the name of the tune that it is playing, and that it is playing the right tune for that name. We might want to deal with a name that isn’t in our table.

Let’s have it ignore tunes it doesn’t know.

        _:test("Player plays nothing if tune is unknown", function()
            local mp = MusicPlayer(fakeMusic)
            tunePlaying = "never set"
            mp:play("unknown")
            _:expect(tunePlaying).is("never set")
        end)

That fails:

4: Player plays default tune if necessary  -- Actual: nil, Expected: never set

And we change play:

function MusicPlayer:play(tuneName)
    local tune = self:tuneFor(tuneName)
    if tune then
        self.playing = tuneName
        self.music(tune, true)
    end
end

Tests all run. So, here we are at 1015 with a fully tested MusicPlayer that knows two tunes and can play them. Clearly what I must have had in mind with that dictionary in init is that we could replace the dictionary with some other playlist.

We’re TDDing here, so we might as well test that.

I realize right away that I want the paylist to be the first parameter, since otherwise everyone would have to pass in music and we really have in mind that no one has to pass anything. So the test starts like this:

        _:test("Player accepts alternative playlist", function()
            local mp = MusicPlayer(playList, fakeMusic)
        end)

And the others have to change:

        _:test("Player plays nothing if tune is unknown", function()
            local mp = MusicPlayer(nil,fakeMusic)
            tunePlaying = "never set"
            mp:play("unknown")
            _:expect(tunePlaying).is("never set")
        end)

And so on … so I have to change MP:

function MusicPlayer:init(playList, musicFunction)
    self.music = musicFunction or music
    self.tunes = playList or {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self:play("dungeon")
end

Tests are good again, so let’s continue with our new one … and I get this far:

        _:test("Player accepts alternative playlist", function()
            local playList = {a=asset.downloaded.A_Hero_s_Quest.Arrow_Shoot_1, b=asset.downloaded.A_Hero_s_Quest.Arrow_Shoot_2 }
            local mp = MusicPlayer(playList, fakeMusic)
            _:expect(...
        end)

When I realize that I have no way of knowing which tune is first. The MP is still assuming that it knows. Either I have to remove the initial default call, or I have to provide the name of the default tune.

This is irritating. I’ve had to change the protocol of this object already, and it’s way beyond the original requirement, which was just to play “dungeon” most of the time and “battle” when monsters were near.

Yes, this object will be much more robust, much more usable, much more reusable, than the one yesterday. But it still can’t do what I actually need in the game, and it’s getting more and more general than I need.

Therefore YAGNI. We’re not gonna need it. But we’re so close. If I’d accept playing nothing until requested, unless you happen to have a tune named “dungeon”, I’d be done now, with the player part anyway.

No. Rip it out. No. Leave it in, we’re so close. No, you’re in this far, complete it, make it general. OK, new rule. If there is a tune named “default”, we’ll play it at the beginning. We’ll never play it again unless asked to. And we’ll default it, in our table, to the same as “dungeon”.

        _:test("Player accepts alternative playlist", function()
            local playList = {a=asset.downloaded.A_Hero_s_Quest.Arrow_Shoot_1, b=asset.downloaded.A_Hero_s_Quest.Arrow_Shoot_2, default=asset.downloaded.A_Hero_s_Quest.Curse }
            local mp = MusicPlayer(playList, fakeMusic)
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Curse)
        end)

This will fail because it’ll play nothing because it doesn’t have a “dungeon” tune.

5: Player accepts alternative playlist  -- Actual: never set, Expected: Asset Key: Curse.caf (path: "/private/var/mobile/Containers/Data/Application/7AE638E9-80DA-41C0-93BC-01F4D3CF1781/Library/AssetPackDownloads/A Hero's Quest.assets/Curse.caf")

Right. Now the fix:

function MusicPlayer:init(playList, musicFunction)
    self.music = musicFunction or music
    self.tunes = playList or {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon, default=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self:play("default")
end

First test fails now:

1: Initial song is dungeon  -- Actual: default, Expected: dungeon

Test is:

        _:test("Initial song is dungeon", function()
            local mp = MusicPlayer(nil,fakeMusic)
            _:expect(mp:nowPlaying()).is("dungeon")
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
        end)

Should be:

        _:test("Initial song is default", function()
            local mp = MusicPlayer(nil,fakeMusic)
            _:expect(mp:nowPlaying()).is("default")
            _:expect(tunePlaying).is(asset.downloaded.A_Hero_s_Quest.Dungeon)
        end)

OK, tests are green and MP is working and a bit more general than we really need.

And we still don’t have anything happening about whether monsters are close or distant, nor the locking of a tune for at least three seconds.

Our MusicPlayer, by design, will just switch tunes on a dime. If we want that other functionality, we could jam it in here, or put it elsewhere.

And at this point we see one decent reason why what we’re doing this morning might be better than what we did yesterday.

Yesterday’s MusicPlayer combines at least these ideas:

• play selected tunes
• play one when monsters are far, another when near
• never switch tunes faster than every three seconds

That’s not very cohesive. If we’re going for small, single-purpose objects, yesterday’s code isn’t that.

Let’s see if we can deal with the notion of not switching until three seconds have elapsed. That leaves a question open:

When we are given a new tune to play, and the three seconds aren’t up, should we ignore the new tune, or wait until the time is up and then play it?

I favor the second answer. Now, I’m sure that in our situation, another call to battle will come along, but we don’t need to require that, so let’s not.

I’m going to create a new object: SmoothMusicPlayer. At least that’s the name I have in mind for now, the idea of “smooth” being that it doesn’t jitter back and forth.

Now we do have a serious issue here, which is how do we do timing in a TDD test.

I suspect we’re going to need another test double, this one for timing. And because I like tweens, I’m going to use them. So we’ll probably have to fake a tween.

Let’s test:

        _:test("Smooth player doesn't accept new tune until time elapsed", function()
            local sp = SmoothPlayer()
            sp:play("battle")
            _:expect(sp:nowPlaying()).is("default")
            sp:timeElapsed()
            _:expect(sp:nowPlaying()).is("battle")
        end)

Now, honestly, I’m getting bored with this and I’m going to build out a lot of this object here and now. I may regret that.

SmoothPlayer = class()

function SmoothPlayer:init(delay, playList, musicFunction)
    self.delay = delay or 0
    self.mp = MusicPlayer(playList, musicFunction)
    self.request = self.mp:nowPlaying()
    self:setTimer()
end

function SmoothPlayer:play(tune)
    self.request = tune
    self:setTimer()
end

function SmoothPlayer:setTimer()
    if self.delay > 0 then tween.delay(self.delay, self.timeElapsed,self) end
end
    
function SmoothPlayer:timeElapsed()
    if self.play ~= self.mp:nowPlaying() then
        self.mp:play(self.request)
    end
end

That’s a fairly big batch, but I feel pretty good about it. Note that if I create my SP with no parms, it doesn’t set its timer, but it is still sensitive to calls to timeElapsed. The test might actually work, but I’m not that optimistic.

6: Smooth player doesn't accept new tune until time elapsed -- Tests:52: attempt to call a nil value (method 'nowPlaying')

Hm, probably I should implement nowPlaying on SmoothPlayer if I plan to call it:

function SmoothPlayer:nowPlaying()
    return self.mp:nowPlaying()
end

Test is green but plays music. I need to call SP correctly:

        _:test("Smooth player doesn't accept new tune until time elapsed", function()
            local sp = SmoothPlayer(0,nil,fakeMusic)
            sp:play("battle")
            _:expect(sp:nowPlaying()).is("default")
            sp:timeElapsed()
            _:expect(sp:nowPlaying()).is("battle")
        end)

Test is green and silent. I am confident, though, of course, I’ve not actually tested the tween itself. If I were less confident than I am in the code, I’d probably have to create a test double for the tween, and while a purist might do that, I’m here to do good work, not to be a purist.

So that’s done, SmoothPlayer works like MusicPlayer but requires three seconds between tunes, and will then play whatever is on deck if it’s not already playing.

Maybe the term “on deck” is a good one. Let’s refactor:

function SmoothPlayer:init(delay, playList, musicFunction)
    self.delay = delay or 0
    self.mp = MusicPlayer(playList, musicFunction)
    self.onDeck = self.mp:nowPlaying()
    self:setTimer()
end

function SmoothPlayer:nowPlaying()
    return self.mp:nowPlaying()
end

function SmoothPlayer:play(tune)
    self.onDeck = tune
    self:setTimer()
end

function SmoothPlayer:setTimer()
    if self.delay > 0 then tween.delay(self.delay, self.timeElapsed,self) end
end
    
function SmoothPlayer:timeElapsed()
    if self.onDeck ~= self.mp:nowPlaying() then
        self.mp:play(self.onDeck)
    end
end

Now as I refactored that to use the onDeck name, I noticed something: when you call our play function, we set the timer then. That means that before your new tune comes up, three more seconds must elapse before it runs, even if they already have.

Furthermore, If there’s already a timer running, we’ll start another, not having stopped the first one. That’s not good.

SmoothTimer is flaky. Typing it all in did not go as well as I thought it would.

Nothing for it but to revert and move in smaller steps. I carve it down to here:

function SmoothPlayer:init(delay, playList, musicFunction)
    self.delay = delay or 0
    self.mp = MusicPlayer(playList, musicFunction)
    self.onDeck = self.mp:nowPlaying()
    self:setTimer()
end

function SmoothPlayer:nowPlaying()
    return self.mp:nowPlaying()
end

And will build it up again.

The test:

        _:test("Smooth player doesn't accept new tune until time elapsed", function()
            local sp = SmoothPlayer(0,nil,fakeMusic)
            sp:play("battle")
            _:expect(sp:nowPlaying()).is("default")
            sp:timeElapsed()
            _:expect(sp:nowPlaying()).is("battle")
        end)

I think this much is nearly good. I’ll implement a null ‘smoothTimer and timeElapsed` and get the error on “battle”.

6: Smooth player doesn't accept new tune until time elapsed -- Tests:51: attempt to call a nil value (method 'play')

OK, play.

function SmoothPlayer:play(tune)
    self.onDeck = tune
end

This stills seems OK. The test now says:

6: Smooth player doesn't accept new tune until time elapsed  -- Actual: default, Expected: battle

OK, that’s as expected. We could also invasively check SP’s onDeck variable, but it’s none of our business. If he manages his variables correctly, our tests will run.

Let’s posit that the SP has a locked variable that it locks when it plays something, and unlocks after the time is elapsed.

I come up with this, again, a big bite:

function SmoothPlayer:init(delay, playList, musicFunction)
    self.delay = delay or 0
    self.mp = MusicPlayer(playList, musicFunction)
    self.onDeck = self.mp:nowPlaying()
    self:setTimer()
end

function SmoothPlayer:nowPlaying()
    return self.mp:nowPlaying()
end

function SmoothPlayer:play(tune)
    self.onDeck = tune
end

function SmoothPlayer:playOnDeckTune()
    if self:nowPlaying() == self.onDeck then return end
    self.mp:play(self.onDeck)
    self:setTimer()
end

function SmoothPlayer:setTimer()
    if self.delay == 0 then return end
    self.locked = true
    tween.delay(self.delay, self.unlock, self)
end

function SmoothPlayer:unlock()
    self.locked = false
    self:playOnDeckTune()
end

My test works. Does the code work? I wonder. I can, however test that “manually”. I put this in Main:

    local sp = SmoothPlayer(10)
    sp:play("battle")

The dungeon tune plays for a while and then the battle tune. So I believe this class works.

We now need some kind of an object that plays the dungeon tune most of the time, and plays battle music when monsters are near.

I hope you’ll forgive me if I just code this:

MonsterPlayer = class()

function MonsterPlayer:init(runner)
    self.runner = runner
    self.sp = SmoothPlayer(3)
    tween.delay(1, self.everySoOften, self)
end

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    if close then sp:play("battle") else sp:play("dungeon") end
end

function MonsterPlayer:everySoOften()
    self:checkForDanger()
    tween.delay(1, self.everySoOften, self)
end

I can’t readily test this here in my separate project. Instead, I’m going to go nuts and replace my existing MusicPlayer tab with the complete contents of this one, tests, classes, and all.

Now I need to find where I created the MusicPlayer and create a MonsterPlayer.

function GameRunner:init()
    self.tileSize = 64
    self.tileCountX = 85 -- if these change, zoomed-out scale 
    self.tileCountY = 64 -- may also need to be changed.
    self.tiles = {}
    for x = 1,self.tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,self.tileCountY+1 do
            local tile = Tile:edge(x,y, self)
            self:setTile(tile)
        end
    end
    self.cofloater = Floater(self, 50,25,4)
    self.musicPlayer = MusicPlayer(self)
end

This last line becomes, of course:

    self.musicPlayer = MonsterPlayer(self)

Now I’m just going to run the game and see what happens.

What happens is this:

MusicPlayer:70: attempt to index a nil value (global 'sp')
stack traceback:
	MusicPlayer:70: in method 'checkForDanger'
	MusicPlayer:74: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

:158: in upvalue 'finishTween'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

:589: in function <...in pairs(tweens) do
    c = c + 1
  end
  return c
end

:582>

Because I wrote this untested line:

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    if close then sp:play("battle") else sp:play("dungeon") end
end

This might be better:

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    if close then self.sp:play("battle") else self.sp:play("dungeon") end
end

Run again:

Well, the battle music starts, but then it never goes back to dungeon. Darn. I was really hoping this would just plug in and go. Let’s hope the problem is in my untested MonsterPlayer, but it sure looks pretty simple.

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    print("checking ", close)
    if close then self.sp:play("battle") else self.sp:play("dungeon") end
end

This really ought to print checking and true or false, every second. The prints seem correct. But this time the music stayed in dungeon mode even when monsters were present.

Ah, I see the bug:

function SmoothPlayer:playOnDeckTune()
    if self:nowPlaying() == self.onDeck then return end
    self.mp:play(self.onDeck)
    self:setTimer()
end

function SmoothPlayer:setTimer()
    if self.delay == 0 then return end
    self.locked = true
    tween.delay(self.delay, self.unlock, self)
end

function SmoothPlayer:unlock()
    self.locked = false
    self:playOnDeckTune()
end

If the timer ever expires while the right tune is playing, we don’t reset the timer. That is not OK. I think we either need to keep that timer running, or start it when called to play something different. Keeping it running seems better. And the locked flag is unused. I’ll remove it.

function SmoothPlayer:playOnDeckTune()
    if self:nowPlaying() ~= self.onDeck then
        self.mp:play(self.onDeck)
    end
end

function SmoothPlayer:setTimer()
    if self.delay == 0 then return end
    tween.delay(self.delay, self.unlock, self)
end

function SmoothPlayer:unlock()
    self:playOnDeckTune()
    self:setTimer()
end

Now I expect this to work. And it does work as advertised. There is always a three second delay between escaping a monster or killing it, and the music going back to “dungeon”. That’s because I chose to keep the timer running and to remove the locked flag. I think for now we’ll leave this be.

Now it’s time to look at what we have and assess which approach was better. It is 1200, and it was surely no later than 0900 when I started, probably earlier, more like 0830. So we have three or four hours in this version and two rather simple defects, one in untested code and one in tested code.

Retro

But let’s look at the classes and see what we see:

MonsterPlayer = class()

function MonsterPlayer:init(runner)
    self.runner = runner
    self.sp = SmoothPlayer(3)
    tween.delay(1, self.everySoOften, self)
end

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    if close then self.sp:play("battle") else self.sp:play("dungeon") end
end

function MonsterPlayer:everySoOften()
    self:checkForDanger()
    tween.delay(1, self.everySoOften, self)
end

This is the main thing we’ll encounter, since it’s the only one called outside this class. It seems pretty clear to me, it gets a SmoothPlayer with a 3 second cycle, and it checks every so often (1 second) for danger. If it sees danger, it plays the battle tune otherwise plays the dungeon tune.

That seems quite cohesive and it’s only three methods. What about SmoothPlayer?

SmoothPlayer = class()

function SmoothPlayer:init(delay, playList, musicFunction)
    self.delay = delay or 0
    self.mp = MusicPlayer(playList, musicFunction)
    self.onDeck = self.mp:nowPlaying()
    self:setTimer()
end

function SmoothPlayer:nowPlaying()
    return self.mp:nowPlaying()
end

function SmoothPlayer:play(tune)
    self.onDeck = tune
end

function SmoothPlayer:playOnDeckTune()
    if self:nowPlaying() ~= self.onDeck then
        self.mp:play(self.onDeck)
    end
end

function SmoothPlayer:setTimer()
    if self.delay == 0 then return end
    tween.delay(self.delay, self.unlock, self)
end

function SmoothPlayer:unlock()
    self:playOnDeckTune()
    self:setTimer()
end

This is a bit more complex, and I almost suspect it could be simpler. Its job seems simple enough: play whatever tune you were most recently asked to play, but be sure to play all tunes for at least self.delay seconds.

This code was built with TDD, and I did the best job I was capable of at the time, and it still had a defect in that it never played a new tune if the timer had once expired.

Finally, we have our bottom-level MusicPlayer:

MusicPlayer = class()

function MusicPlayer:init(playList, musicFunction)
    self.music = musicFunction or music
    self.tunes = playList or {battle=asset.downloaded.A_Hero_s_Quest.Battle, dungeon=asset.downloaded.A_Hero_s_Quest.Dungeon, default=asset.downloaded.A_Hero_s_Quest.Dungeon}
    self:play("default")
end

function MusicPlayer:nowPlaying()
    return self.playing
end

function MusicPlayer:play(tuneName)
    local tune = self:tuneFor(tuneName)
    if tune then
        self.playing = tuneName
        self.music(tune, true)
    end
end

function MusicPlayer:tuneFor(tuneName)
    return self.tunes[tuneName]
end

This, again, is quite simple. It just plays whatever you ask it to play, then and there. It has a default playlist, but accepts a different one, and it plays nothing if you ask it to play a song it doesn’t know. (That will leave whatever is already playing running, if there is anything.)

Am I going to accept this new version? The diff says -30+120 lines so we have net added 90 lines. The tab is now 139 lines of test and code. It used to be 49.

This new version has taken me four hours for sure, counting writing this article. It’s better tested but it has grown the solution to this problem from about 50 lines to 140.

I’m going to paste this fixed version back into the separate project where I wrote it, and then I think I’ll revert.

Yes. Revert. Let’s talk about whether and why.

Whether to Revert, and Why

Now that I’ve done it, I’m wondering. There are 50 lines of tests in today’s version, so the code is only 40 lines longer than yesterday’s. That’s still nearly double, as yesterday’s was only 49 lines.

Maybe the separation of concerns is worth it. And maybe we can improve that SmoothPlayer code a bit. And we do have some testing, though it’s not perfect by any means.

I’ve certainly not cracked the code on testing timed objects with tweens, and that’s probably why I left a defect in the tested code. So I have things to learn.

All that said, though, I think having the tests in place, even still a bit weak, and having the concerns separated, is probably better. I’m so tempted to use the new one but there are things not to like.

One is that, because of the default vs dungeon thing, It plays the default tune for 3 seconds and then the dungeon tune, i.e. starting over. Second, the forced three second delay after monsters get close won’t do.

I’m sticking with the decision to revert, but it’s a near call. I think we got somewhat better code, but it’s not quite working to spec, so it needs further testing. And I need to think a bit more about how I might test timing.

I could imagine coming back to this spike and pushing it further and I can imagine that it might work well enough to install it into the game.

We’ll see. For now, we’ll stick with the cards we have.

See you next time!

D2.zip