Python Asteroids+Invaders on GitHub

I have an idea for how to get rid of the conditionals in the saucer code. It is a nasty idea. I must try it.

The InvadersSaucer object can only be fully initialized after it sees an InvaderPlayer and the InvadersFleet objects, because its behavior is conditioned on how many invaders exist, and how many shots have been fired. In the current implementation, we do as much initializing as possible in __init__, and set a flag initialized to False. Then various methods check that flag before proceeding. The code looks like this:

class InvadersSaucer(InvadersFlyer):
    def __init__(self, direction=1):
        self.direction = direction
        maker = BitmapMaker.instance()
        self._map = maker.saucer
        self._mask = pygame.mask.from_surface(self._map)
        self._rect = self._map.get_rect()
        half_width = self._rect.width // 2
        self._left = u.BUMPER_LEFT + half_width
        self._right = u.BUMPER_RIGHT - half_width
        self._speed = 0
        self._player = None
        self._score_list = [100, 50, 50, 100, 150, 100, 100, 50, 300, 100, 100, 100, 50, 150, 100]
        self.initialized = False

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if not self.initialized and invader_fleet.invader_count() < 8:
            self.die(fleets)

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.init_motion(self._player.shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

    def update(self, delta_time, fleets):
        if self.initialized:
            self.move_or_die(fleets)

    def draw(self, screen):
        if self.initialized:
            screen.blit(self._map, self.rect)

It turns out that I made a few mistakes when I wrote this: I forgot to check the flag before drawing and before checking the invader count. The former mistake could have caused a spurious saucer to appear briefly on screen, and the latter would cause the saucer to vanish if you shot the 8th remaining invader while the saucer was on screen. Both of those mistakes were defects that would have occurred only rarely, but they both could happen.

I had had hopes that some kind of simple helper object could have made this code more simple, but I’ve not yet found such a thing. This morning, I had an idea for something that I want to try. The pattern may or may not be called Pluggable Behavior. I can’t look it up just now, because I have no Internet this morning. Perhaps later today.

If I am not mistaken—and I certainly could be—Python methods are stored in a dictionary associated with the class, now the instance. But, if I am not mistaken—vide ultra—the instance’s dictionary is checked first, and then the class dictionary. If that’s true, we could override a method in the instance. Dynamically.

Nota Bene:
I’m not saying this is a good idea: I’m saying it’s an idea and I think it is nasty. But I think we should try it, to learn a bit and see how it looks. My plan is not to commit this until it’s done, and perhaps never to commit it.

I’ll start with a test class and test.

class _Pluggable:
    def __init__(self):
        self.p1 = 0
        self.p2 = 0


class TestPluggable:
    def test_pluggable(self):
        p = _Pluggable()
        assert p.p1 == 0
        assert p.p2 == 1

That fails as intended and when I check p2 for zero, it passes. No surprises yet.

I want the ability to turn a method off. So I need a method that does something, I need to test it, I need to turn it off, and test it again. Like this:


class _Pluggable:
    def __init__(self):
        self.p1 = 0
        self.p2 = 0

    def increment_p1(self):
        self.p1 += 1

    def turn_off_increment(self):
        pass


class TestPluggable:
    def test_pluggable(self):
        p = _Pluggable()
        assert p.p1 == 0
        assert p.p2 == 0
        p.increment_p1()
        assert p.p1 == 1
        p.turn_off_increment()
        p.increment_p1()
        assert p.p1 == 1

This fails at the end with 2 instead of one. I need to implement turn_off_increment:


class _Pluggable:
    def __init__(self):
        self.p1 = 0
        self.p2 = 0

    def increment_p1(self):
        self.p1 += 1

    def turn_off_increment(self):
        self.increment_p1 = self.do_nothing

    def do_nothing(self, *args, **kwargs):
        pass


class TestPluggable:
    def test_pluggable(self):
        p = _Pluggable()
        assert p.p1 == 0
        assert p.p2 == 0
        p.increment_p1()
        assert p.p1 == 1
        p.turn_off_increment()
        p.increment_p1()
        assert p.p1 == 1

Test passes. You may be wondering why I defined do_nothihg like that. The reason is that I’d like to to be able to override any method, no matter what its parameters may be, so do_nothing must accept any number of args and any number of keyword args. That’s how you say that in Python.

I need two more tests. First, we need to be able to undo the override, and second, we should make sure that the override doesn’t affect other instances (although it clearly cannot).

class _Pluggable:
    def __init__(self):
        self.p1 = 0
        self.p2 = 0

    def increment_p1(self):
        self.p1 += 1

    def turn_off_increment(self):
        self.increment_p1 = self.do_nothing

    def turn_on_increment(self):
        if self.__dict__.get("increment_p1"):
            self.__delattr__("increment_p1")

    def do_nothing(self, *args, **kwargs):
        pass


class TestPluggable:
    def test_pluggable(self):
        p = _Pluggable()
        assert p.p1 == 0
        assert p.p2 == 0
        p.increment_p1()
        assert p.p1 == 1
        p.turn_off_increment()
        p.increment_p1()
        assert p.p1 == 1
        p.turn_on_increment()
        p.increment_p1()
        assert p.p1 == 2
        p.turn_on_increment()
        p.increment_p1()
        assert p.p1 == 3

In turn_on_increment, I’m checking to see if the method is in the local dictionary before removing it. I think this is righteous, but without the Internet I can’t check.

Let’s do a two-instance test. In so doing, I decide that I need better names, so the new situation is this:

class _Pluggable:
    def __init__(self):
        self.count = 0

    def increment(self):
        self.count += 1

    def turn_off_increment(self):
        self.increment = self.do_nothing

    def turn_on_increment(self):
        if self.__dict__.get("increment"):
            self.__delattr__("increment")

    def do_nothing(self, *args, **kwargs):
        pass


class TestPluggable:
    def test_pluggable(self):
        p = _Pluggable()
        assert p.count == 0
        p.increment()
        assert p.count == 1
        p.turn_off_increment()
        p.increment()
        assert p.count == 1
        p.turn_on_increment()
        p.increment()
        assert p.count == 2
        p.turn_on_increment()
        p.increment()
        assert p.count == 3

    def test_two_instances(self):
        p1 = _Pluggable()
        p2 = _Pluggable()
        assert p1.count == 0
        assert p2.count == 0
        p1.turn_off_increment()
        p1.increment()
        p2.increment()
        assert p1.count == 0
        assert p2.count == 1

        p1.turn_on_increment()
        p2.turn_off_increment()
        p1.increment()
        p2.increment()
        assert p1.count == 1
        assert p2.count == 1

We are green. Commit: TestPluggable.

Nasty Plan

We now know how to turn a method off and turn it on again. We could use this ability in the saucer code to turn off the methods that should not be called before initialization, and, after initialization, we could turn those on and turn off the ones that no longer need to run.

We could. My colleague Dr Ian Malcolm advises me to ask whether we should do it, and I am pretty sure he means that we shouldn’t.

We could do it with all the patching code right in the saucer class, or we could build a little Patcher object to do the job for us. The latter would be a bit more official. In fact, it seems to me that this is tricky enough that we should go ahead and build the Patcher and use it even if we then throw it all away.

So be it. Let’s extend our existing tests to use a new class, Patcher.

class _Pluggable:
    def __init__(self):
        self.count = 0

    def increment(self):
        self.count += 1
    def turn_off_increment(self):
        Patcher.turn_off(self, "increment")

    def turn_on_increment(self):
        Patcher.turn_on(self, "increment")

That breaks everything, of course, since there is no Patcher. Note that I am just using the class, not an instance: there’s not much point to instance methods, at least not as I see it now. (I freely grant some trepidation here: I have rarely used class-only objects. There’s just something not quite right about it. We’ll see.)

class Patcher:

    @classmethod
    def nothing(cls, *args, **kwargs):
        pass

    @classmethod
    def turn_off(cls, obj, name):
        obj.__setattr__(name, cls.nothing)

    @classmethod
    def turn_on(cls, obj, name):
        if obj.__dict__.get(name):
            obj.__delattr__(name)

My tests are green. Patcher works. Commit this much, no harm no foul. Commit: Patcher class tested.

Now we can use it. First I’ll use it to turn draw off and on.

class InvadersSaucer(InvadersFlyer):
    def __init__(self, direction=1):
        self.direction = direction
        maker = BitmapMaker.instance()
        self._map = maker.saucer
        self._mask = pygame.mask.from_surface(self._map)
        self._rect = self._map.get_rect()
        half_width = self._rect.width // 2
        self._left = u.BUMPER_LEFT + half_width
        self._right = u.BUMPER_RIGHT - half_width
        self._speed = 0
        self._player = None
        self._score_list = [100, 50, 50, 100, 150, 100, 100, 50, 300, 100, 100, 100, 50, 150, 100]
        self.initialized = False
        Patcher.turn_off(self, "draw")

    def draw(self, screen):
        screen.blit(self._map, self.rect)

Huh. I can’t really test that. Let’s turn off update instead.

        Patcher.turn_off(self, "update")

    def update(self, delta_time, fleets):
        self.move_or_die(fleets)

At this point, the saucer should appear but should not move. That’s what happens. Makes it really easy to shoot, too.

Now in init_motion, turn it back on:

    def init_motion(self, shot_count):
        self.initialized = True
        Patcher.turn_on(self, "update")
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Now it should move again. And it does. This is working.

Turn off draw and back on again. same way. Still just fine.

Now we have this method:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if not self.initialized and invader_fleet.invader_count() < 8:
            self.die(fleets)

We only want this to execute when we are not initialized, so we can turn it off in init_motion:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:
            self.die(fleets)

    def init_motion(self, shot_count):
        self.initialized = True
        Patcher.turn_on(self, "update")
        Patcher.turn_on(self, "draw")
        Patcher.turn_off(self, "interact_with_invaderfleet")
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

And the final reference to the flag is this:

    def end_interactions(self, fleets):
        if not self.initialized and self._player:
            self.init_motion(self._player.shot_count)

Now, at last, my conscience begins to prick me. It’s really a bit risky to turn off the interaction methods interact_with_invaderfleet and end_interactions. Even worse, if we carry out this plan, the code will look like this:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:

    ...

    def end_interactions(self, fleets):
        if self._player:
            self.init_motion(self._player.shot_count)

    def init_motion(self, shot_count):
        self.initialized = True
        Patcher.turn_on(self, "update")
        Patcher.turn_on(self, "draw")
        Patcher.turn_off(self, "interact_with_invaderfleet")
        Patcher.turn_off(self, "end_interactions")
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

    def update(self, delta_time, fleets):
        self.move_or_die(fleets)

    def draw(self, screen):
        screen.blit(self._map, self.rect)

There is no indication in those key methods that would tell us that sometimes they exist and sometimes they do not.

This just will not do: the code looks clear but in fact, owing to the action of Patcher, the code is not doing what it says. Sometimes it does what it says and sometimes it doesn’t.

This is not good. This is NASTY, just as I predicted.

I roll it back. Goodbye, nasty code.

Reflection

Well, I knew it would be nasty. So I’m glad to know how to do it and I’ve saved the Patcher in case of some dire emergency, but the code still has all those conditionals in it.

There is another way. What made me throw this code away wasn’t the behavior changing, it was the fact that the code did not reflect the fact that the behavior changed, in the places it actually changed.

What it, for example, update looked like this:

    def update(self, delta_time, fleets):
        self.update_behavior(fleets)

    def initialized_update_behavior(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self.die(fleets)
        else:
            self.position = (x, self.position.y)

    def uninitialized_update_behavior(self, fleets):
        pass

And in init:

self.update_behavior = self.uninitialized_update_behavior

And in init_motion:

self.update_behavior = self.initialized_update_behavior

Well, that does express the fact that the behavior is being changed. But honestly, is the code above really better than what follows, using the flag?

    def update(self, delta_time, fleets):
        if self.initialized:
            self.move_or_die(fleets)

    def move_or_die(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self.die(fleets)
        else:
            self.position = (x, self.position.y)

If the indirect call is better, it’s not enough better.

I don’t like the if statements , but I like the ways I’ve tried to remove them even less. I haven’t thought of a good way to defer the decision to another object, and I’ve looked at two ways of plugging different behavior in at run time, and those are both too strange, weird, and nasty.

For now, we’ve tried climbing the walls, we’ve tried digging in two different places, and we haven’t been able to get rid of this flag and its associated conditionals.

I’m not giving up: if we do find a way that we can like, it will probably be an idea worth remembering. But for now, we’ve learned a lot about patching and have not found a way to make this conditional code better.

I remember, back in my youth, Lo! these many years ago, I used to enjoy doing clever things like this. I have come to think that while it’s good to know how to do them, it’s not good to do them unless they are really necessary. I’m not sure what it would take to make it necessary, but I feel sure that this situation does not rise to that level.

I’ll keep thinking and if you have an idea, drop me a toot. See you next time!