Right On the Beat

We continue working on the game’s beat sound. I am faced with a dilemma.

Our tests so far check that we compute the correct delay between beats, based on the number of invaders left, and that we return the four sounds in the desired order, which is curiously 4, 1, 2, 3, …, because that’s how they’re named. It’s time to do the rest, which I think goes something like this:

On each update, the InvaderFleet will call the TonePlayer, something like play_tone(number_of_invaders);
TonePlayer updates its elapsed time since last update;
If the elapsed time is less than the correct delay, return;
Otherwise, play the correct tone, zero the elapsed time, and set up the next tone.

My dilemma is this: obviously we can code this and make it work. However, what I would prefer to do would be to have the entire object well-tested, including the conditional logic about playing or not playing the sound. But we do not want the tests to try to play sounds, since the sound player is not guaranteed to be set up during testing, and we don’t want the noise in any case.

It seems to me that to test this, either I need a fake sound object to call, or I need to monkey-patch the code for testing purposes. Of these two, the fake sound object offends me less.

Let’s try to write a test and see what it might look like.

    def test_sound_on_first_call(self):
        player = TonePlayer()
        player.play_tone(55)
        assert tone_played == "fastinvader4"
        player.play_tone(55)
        assert tone_played == None

In this test, somehow magically the value tone_played is set to whatever tone was played … and if it is set, we are certain that the sound player was sent that value.

OK, what if we were to pass in a fake sound player to TonePlayer creation.

Let’s code one up:

class FakeSoundPlayer:
    def play_stereo(self, sound, frac):
        return sound

Just returning the sound played seems reasonable. Update our test:

    def test_sound_on_first_call(self):
        player = TonePlayer(FakeSoundPlayer())
        tone_played = player.play_tone(55)
        assert tone_played == "fastinvader4"
        tone_played = player.play_tone(55)
        assert tone_played is None

And update the TonePlayer to make this pass.

Hm. I thought this would pass:

from Sound import player
class TonePlayer:
    def __init__(self, fake_player=None):
        self._tone_player = fake_player if fake_player else player
        self._tones = ("fastinvader4", "fastinvader1", "fastinvader2", "fastinvader3")
        self._tone_index = 3

    def play_tone(self, number_of_invaders):
        return player.play_stereo("fastinvader4", 0.5)

My test fails on the first assertion. I think I’ll remove the second part:

    def test_sound_on_first_call(self):
        player = TonePlayer(FakeSoundPlayer())
        tone_played = player.play_tone(55)
        assert tone_played == "fastinvader4"

Why? Well, folks will tell you that a test should only have one assertion, so that when it fails, you immediately know why. I don’t always follow that advice but right now, if I make the first assertion pass with that literal value, the second will fail and I can’t tell without looking whether the first one passed. Now, I can. And it isn’t passing.

Why not? A quick print in FakeTonePlayer tells me that it isn’t being called. Is my init too clever? No. I’m not using it. Should have:

    def play_tone(self, number_of_invaders):
        return self._tone_player.play_stereo("fastinvader4", 0.5)

First bit passes. New test:

    def test_no_sound_on_second_call(self):
        player = TonePlayer(FakeSoundPlayer())
        tone_played = player.play_tone(55)
        tone_played = player.play_tone(55)
        assert tone_played is None

Test fails as expected. We need to do the time logic to make it pass.

class TonePlayer:
    def __init__(self, fake_player=None):
        self._tone_player = fake_player if fake_player else player
        self._tones = ("fastinvader4", "fastinvader1", "fastinvader2", "fastinvader3")
        self._tone_index = 3
        self._elapsed_time = 999
        self._delay = self._delays[0]

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self._delay:
            return None
        self._delay = self.delay(number_of_invaders)
        self._elapsed_time = 0
        return self._tone_player.play_stereo("fastinvader4", 0.5)

The test passes. I don’t actually like the code much at all. I think we should check elapsed time dynamically against delays, like this:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        self._elapsed_time = 0
        return self._tone_player.play_stereo("fastinvader4", 0.5)

I don’t like this much either, because delay() is rather costly as written:

    def delay(self, current_invader_count):
        for invader_count_limit, delay in zip(self._invader_count_limits, self._delays):
            if invader_count_limit <= current_invader_count:
                return delay
        return 0x05

That zip operation will be done on every clock tick. I have no reason to think we can’t afford it, my MacNook is faster than a Cray-1, but it’s still visibly inefficient. We’ll leave it for now and work on through some more tests.

    def test_plays_in_correct_order(self):
        player = TonePlayer(FakeSoundPlayer())
        for correct in ["fastinvader4", "fastinvader1", "fastinvader2", "fastinvader3"]:
            assert player.play_tone(55) == correct
            player._elapsed_time += 99

This test fails on the second iteration, looking for "fastinvader1", which is what I expected. We have no code to update what is to be played.

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        self._elapsed_time = 0
        return self._tone_player.play_stereo(self.next_tone(), 0.5)

This passes. I am ready to try the sounds in the game:

    def update(self, delta_time, _fleets):
        self.tone_player.play_tone(self.invader_group.invader_count())
        result = self.invader_group.update_next(self.origin)
        self.process_result(result, _fleets)

The sounds come out as intended. They do go faster and faster as the invader count goes down. The feature works and we will commit. Then we’ll review the code to see whether we consider it to be done. Commit: Invader ominous beat works correctly.

Reflection and Code Review

I think we could have committed a few more times this morning, but I didn’t revive the habit and was too focused on getting the test working that checks that we actually play the right sound. That required two odd things:

First, it required that tiny fake object, which was a reasonable way to plug in some testing behavior. However, second, it requires the method play_tone to return the tone played, or None if none is played. That is a bit odd. We could add a comment:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        self._elapsed_time = 0
        return self._tone_player.play_stereo(self.next_tone(), 0.5)  # returned for testing

I don’t love comments, but it might help someone.

Let’s look at the whole TonePlayer class:

class TonePlayer:
    _invader_count_limits = [0x32, 0x2B, 0x24, 0x1C, 0x16, 0x11, 0x0D, 0x0A, 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01]
    _delays = [0x34, 0x2E, 0x27, 0x22, 0x1C, 0x18, 0x15, 0x13, 0x10, 0x0E, 0x0D, 0x0C, 0x0B, 0x09, 0x07, 0x05]

    def __init__(self, fake_player=None):
        self._tone_player = fake_player if fake_player else player
        self._tones = ("fastinvader4", "fastinvader1", "fastinvader2", "fastinvader3")
        self._tone_index = 3
        self._elapsed_time = 999

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        self._elapsed_time = 0
        return self._tone_player.play_stereo(self.next_tone(), 0.5)  # returned for testing

    def next_tone(self):
        self._tone_index = (self._tone_index + 1) % 4
        return self._tones[self._tone_index]

    def delay(self, current_invader_count):
        for invader_count_limit, delay in zip(self._invader_count_limits, self._delays):
            if invader_count_limit <= current_invader_count:
                return delay
        return 0x05

We should make the tones class variables:

class TonePlayer:
    _invader_count_limits = [0x32, 0x2B, 0x24, 0x1C, 0x16, 0x11, 0x0D, 0x0A, 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01]
    _delays = [0x34, 0x2E, 0x27, 0x22, 0x1C, 0x18, 0x15, 0x13, 0x10, 0x0E, 0x0D, 0x0C, 0x0B, 0x09, 0x07, 0x05]
    _tones = ("fastinvader4", "fastinvader1", "fastinvader2", "fastinvader3")

    def __init__(self, fake_player=None):
        self._tone_player = fake_player if fake_player else player
        self._tone_index = 3
        self._elapsed_time = 999

We have already observed the duplicated calculation (done every tick) here:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        self._elapsed_time = 0
        return self._tone_player.play_stereo(self.next_tone(), 0.5)  # returned for testing

Let’s improve that code. First, Extract Method:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        if self._elapsed_time < self.delay(number_of_invaders):
            return None
        return self.play_and_reset_time()

    def play_and_reset_time(self):
        self._elapsed_time = 0
        return self._tone_player.play_stereo(self.next_tone(), 0.5)  # returned for testing

Now what I want (what I really really want) is an if-else situation. I’d like to do it with machine refactorings, for two reasons: First, they are more reliable than my coding, and second, I want to practice thinking of the right sequence of machine refactorings, so that I can use them more often.

Extract variable:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        no_sound_needed = self._elapsed_time < self.delay(number_of_invaders)
        if no_sound_needed:
            return None
        return self.play_and_reset_time()

I give up and type the next step:

    def play_tone(self, number_of_invaders):
        self._elapsed_time += 1
        no_sound_needed = self._elapsed_time < self.delay(number_of_invaders)
        if no_sound_needed:
            return None
        else:
            return self.play_and_reset_time()

Tests are passing by the way. We could be committing. Hill might be. I prefer to wait until I’m happy with what I have, and am sure I won’t roll back and start over. Extract method:

    def play_tone(self, number_of_invaders):
        no_sound_needed = self.sound_is_not_needed(number_of_invaders)
        if no_sound_needed:
            return None
        else:
            return self.play_and_reset_time()

    def sound_is_not_needed(self, number_of_invaders):
        self._elapsed_time += 1
        no_sound_needed = self._elapsed_time < self.delay(number_of_invaders)
        return no_sound_needed

Inline:

    def play_tone(self, number_of_invaders):
        if self.sound_is_not_needed(number_of_invaders):
            return None
        else:
            return self.play_and_reset_time()

I would much prefer to have the method sound_is_needed or some other positive statement.

I don’t see it.Ah, maybe like this: Invert the if:

    def play_tone(self, number_of_invaders):
        if not self.sound_is_not_needed(number_of_invaders):
            return self.play_and_reset_time()
        else:
            return None

Extract Method:

    def play_tone(self, number_of_invaders):
        if self.sound_is_needed(number_of_invaders):
            return self.play_and_reset_time()
        else:
            return None

    def sound_is_needed(self, number_of_invaders):
        return not self.sound_is_not_needed(number_of_invaders)

    def sound_is_not_needed(self, number_of_invaders):
        self._elapsed_time += 1
        no_sound_needed = self._elapsed_time < self.delay(number_of_invaders)
        return no_sound_needed

Ha. Perfect. Inline:

    def sound_is_needed(self, number_of_invaders):
        self._elapsed_time += 1
        no_sound_needed = self._elapsed_time < self.delay(number_of_invaders)
        return not no_sound_needed

Inline:

    def sound_is_needed(self, number_of_invaders):
        self._elapsed_time += 1
        return not self._elapsed_time < self.delay(number_of_invaders)

And PyCharm offers to invert the comparison.

    def sound_is_needed(self, number_of_invaders):
        self._elapsed_time += 1
        return self._elapsed_time >= self.delay(number_of_invaders)

I prefer this code and freely grant that it is still not optimized. Commit: refactoring.

I do not like that we are always calling this:

    def delay(self, current_invader_count):
        for invader_count_limit, delay in zip(self._invader_count_limits, self._delays):
            if invader_count_limit <= current_invader_count:
                return delay
        return 0x05

This is doing that zip operation which I worry about. What is zip? If it is an iterator, I probably don’t care. A little research tells me that it is in fact a zip object, a specialized iterator. I’m going to stop worrying. Let’s sum up.

Summary

We have our beat sounds working. See and hear(!) video below. The TonePlayer object was test-driven and we can be confident that it works as intended. We could write some “sad path” tests, but my practice generally does not include testing internal objects for things that I’m confident are not done, and this one is pretty bullet-proof anyway, since the delay checker has been checked for invalid values.

I am pleased. The beat goes on.

See you next time!

Alt: Video showing Space Invaders game play with audio sounds including beat, shots, and saucer.