A Better(?) Idea

I think I see something that would improve the InvaderGroup logic. Let’s find out.

Let me begin by trying to explain my idea, as if you were here to nod and frown and ask me questions. I’ll speak to one of my rubber ducks, in your absence.

InvaderGroup code that handles hitting the bumpers needs to be more clear. Let’s think about just what it needs to do.

The Group’s update_next method updates one invader at a time, to get a visual effect. Its update_next method is called 60 times a second, and it moves one invader at a time. When all the invaders have moved, the InvaderFleet, which calls the update_next method, needs to know that the cycle is over and what to do. So the Group returns one of three return values:

CONTINUE: No special action, just call back next time, we’re still moving individuals;
NEW_CYCLE: We have moved the last invader, take normal action, which is to step the invaders’ origin;
REVERSE: We have hit the bumper and have moved the last invader, step down and reverse motion.

So most of the time, the Group does not care about the bumpers. However, it is sent interact_with_bumper on every cycle, because that’s how the game’s clocking works. I think it would make sense if it worked like this:

If we’re in the middle of moving invaders, ignore interact_with_bumper;
If we have just moved the last invader, pay attention to interact_with_bumper;
When we process interact_with_bumper, ask each invader if it is entering the bumper;
If any invader is entering the bumper, return REVERSE on the next call to update_next, otherwise NEW_CYCLE.

One thing to note is that we already return the NEW_CYCLE or REVERSE value on a call to update_next that is after the call that moved the last invader: there is a separate call that does no moving, just returns the information Fleet needs. I think that’ll make our job easier.

Part of this plan is to remove the callback that we currently get during the interact_with_bumper. Instead of processing the callback, I propose that we’ll just ask the invaders if they’re entering (not leaving) the bumper. (Entering means moving toward it, not away. You’ll see how we handle that as we do it.)

This may break some tests, because our tests for this are a bit more dependent on the implementation than we might like. We’ll see, and we’ll see if we can improve the tests.

Sound reasonable? Let’s go for it.

Improving InvaderGroup

The first thing I propose to do is to pass begin_interactions to the group. I think we’ll find it useful in the process. I’m sorry that I don’t have a better explanation than that.

class InvaderFleet:
    def begin_interactions(self, fleets):
        self.invader_group.begin_interactions(fleets)

class InvaderGroup:
    def begin_interactions(self, fleets):
        pass

Green. Harmless. Commit: add begin_interactions.

Now I want to try something that I think will work: to condition the interact_with_bumper to only be called if we are finished moving all the invaders.

class InvaderGroup:
    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        if self.should_reverse:
            return
        for invader in self.invaders:
            if self.should_reverse:
                return
            invader.interact_with_bumper(bumper, self)

A test fails, but I want to see if the game still works: I think it will. It does. The test:

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

Right, we haven’t cycled through all the invaders. I’ll just jam it.

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group._next_invader = len(group.invaders)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

Test passes, at the cost of touching the Group where it would rather not be touched. We’ll allow that for now. No, we won’t. We’ll fix that right here and now, mister!

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.testing_set_to_end()
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

class InvaderGroup:
    def testing_set_to_end(self):
        self._next_invader = len(self.invaders)

Tests are green. Commit: only check bumpers after all invaders have moved.

Now let’s initialize should_reverse in the begin_interactions, and remove the other inits for it. That will break some tests I think.

    def begin_interactions(self, fleets):
        self.should_reverse = False

One test breaks. It’ll want a call to begin_interactions, I reckon:

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.testing_set_to_end()
        group.begin_interactions(None)
        group.interact_with_bumper(bumper, None)
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE

This does not fix my test! I am surprised. I am also dismayed, because this means that there’s something going on that I do not understand. Understanding is kind of key to my role in this programming thing.

I add an assert to the test and that assert passes but the final one fails:

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.testing_set_to_end()
        group.begin_interactions(None)
        group.interact_with_bumper(bumper, None)
        assert group.should_reverse
        result = group.end_cycle()
        assert CycleStatus.REVERSE == result

Oh. I have confused myself with the blank line that I inserted into the test. LOL.

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.testing_set_to_end()
        group.begin_interactions(None)
        group.interact_with_bumper(bumper, None)
        assert group.should_reverse
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE
        # continuing the story ...
        origin = (0, 0)
        result = CycleStatus.CONTINUE
        while result == CycleStatus.CONTINUE:
            invader.position = (u.BUMPER_RIGHT, pos_y)
            result = group.update_next(origin, -1)
        assert result == CycleStatus.NEW_CYCLE

I added the comment there so that next time I won’t stop reading at the blank line. Little learning there about long tests and blank lines in Python.

We need to be sure that the flag got reset before we do this loop.

    def test_reversal_on_entry(self):
        group = InvaderGroup()
        bumper = Bumper(u.BUMPER_RIGHT, +1)
        invader = group.invaders[0]
        _pos_x, pos_y = invader.position
        invader.position = (u.BUMPER_RIGHT, pos_y)
        group.testing_set_to_end()
        group.begin_interactions(None)
        group.interact_with_bumper(bumper, None)
        assert group.should_reverse
        result = group.end_cycle()
        assert result == CycleStatus.REVERSE
        # continuing the story ...
        origin = (0, 0)
        result = CycleStatus.CONTINUE
        group.begin_interactions(None)
        while result == CycleStatus.CONTINUE:
            invader.position = (u.BUMPER_RIGHT, pos_y)
            result = group.update_next(origin, -1)
        assert result == CycleStatus.NEW_CYCLE

Green. Check the game, since I’ve been confused. Works fine. Commit: init should_reverse only in begin_interactions.

Now I want to change how the interactions go with the invaders. I want to just ask them all if they are entering the bumper and use that to set the flag. I think this is what I want:

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        colliders = (invader for invader in self.invaders if invader.is_entering(bumper, self.current_direction))
        self.should_reverse = any(colliders)

And of course the Invaders do not know from is_entering, so:

    def is_entering(self, bumper, direction):
        return bumper.intersecting(self.rect) and bumper.incoming_direction == direction

Some pretty serious feature envy there, but the tests are green. I test the game and it reverses at the right side but walks right off the left side. Bizarre. Revert, make sure it was working before this change.

OK, it’s working now. Why did that change not work?

Rolled back, we have this:

class InvaderGroup:
    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        if self.should_reverse:
            return
        for invader in self.invaders:
            if self.should_reverse:
                return
            invader.interact_with_bumper(bumper, self)

    def at_edge(self, bumper_direction):
        if self.current_direction == bumper_direction:
            self.should_reverse = True

class Invader:
    def interact_with_bumper(self, bumper, invader_group):
        if bumper.intersecting(self.rect):
            invader_group.at_edge(bumper.incoming_direction)

Let’s go incrementally here. First let’s do the is_entering method on Invader and use it. First, pass in the current direction:

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        for invader in self.invaders:
            invader.interact_with_bumper(bumper, self, self.current_direction)

And I removed the optimization lines, as they were cluttering things. We’re green. Game’s good. Commit: new parameter.

Now new method … no, this is simpler …

    def interact_with_bumper(self, bumper, invader_group, current_direction):
        if bumper.intersecting(self.rect) and bumper.incoming_direction == current_direction:
            invader_group.at_edge(bumper.incoming_direction)

Green and game is good. Commit: check current direction in interact_with_bumper.

Extract method:

    def interact_with_bumper(self, bumper, invader_group, current_direction):
        if self.is_entering(bumper, current_direction):
            invader_group.at_edge(bumper.incoming_direction)

    def is_entering(self, bumper, current_direction):
        return bumper.intersecting(self.rect) and bumper.incoming_direction == current_direction

Green. Commit: extract method.

Call the method longhand.

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        for invader in self.invaders:
            if invader.is_entering(bumper, self.current_direction):
                self.should_reverse = True

Green and good. Use is_entering.

Should be able to remove at_edge now, and interact_with_bumper. But isn’t there a test calling that? Will it fail? Nothing fails. Test in game. Good. Commit: remove unused methods.

I think we’re where we were except for my use of any.

I’ll not look at what I did and instead do it right this time.

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        colliding = (invader.is_entering(bumper, self.current_direction) for invader in self.invaders)
        self.should_reverse = any(colliding)

And that fails. I think I don’t know how to do any. I wonder whether current_direction is set once and for all in the comprehension.

Ah. I see it. The issue is that there are two bumpers, not one. So, the way I have used the comprehension, if the second bumper is far away from the invaders, and is checked second, it will return false and we’ll set the flag back.

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
        self.should_reverse |= any(colliding)

Green and game is good. Commit: Improved bumper checking.

One more thing, that Feature Envy:

class Invader:
    def is_entering(self, bumper, current_direction):
        return bumper.intersecting(self.rect) and bumper.incoming_direction == current_direction

We refer to bumper twice. Let’s let him help us.

class Invader:
    def is_entering(self, bumper, current_direction):
        return bumper.am_i_entering(self.rect, current_direction)

class Bumper:
    def am_i_entering(self, rect, direction):
        return self.intersecting(rect) and direction == self.incoming_direction

Commit: remove feature envy.

But now, would we do better to use that method in the group?

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
        self.should_reverse |= any(colliding)

No, we’d have to fetch out the invader’s rectangle. Let’s let it ride, this is much improved over where we started.

What about removing the guard clause in favor of an if?

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader == len(self.invaders):
            colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
            self.should_reverse |= any(colliding)

And let’s give that a name:

    def interact_with_bumper(self, bumper, _fleet):
        if self.invaders_have_all_moved():
            colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
            self.should_reverse |= any(colliding)

    def invaders_have_all_moved(self):
        return self._next_invader == len(self.invaders)

What else? I think you could make a case that the |= is a bit obscure. We could write it out longhand, but I don’t think that would be much better:

    self.should_reverse = self.should_reverse | any(colliding)

If anything, it seems to me, that makes the “or” less visible. Still, the need for the “or” is a bit obscure, because there are those two bumpers. I don’t have a better idea right now, unless we made them different classes with different interact_with methods? That spreads out the logic even worse. We’ll stop for now.

Summary

I’ll include all the relevant code after the summary for those who are particularly masochistic today.

Amusing learning: don’t put blank lines in Python code: it looks too much like the end of whatever it is.

From a comfort viewpoint, this went rather well, except for that one moment when the final any logic failed. I didn’t see that it was because the any code was running twice and clearing the flag. That confusion tied me up for a while.

Other than that, the changes went smoothly. I think there was one other surprise where a test broke but the cause was immediately clear.

I think this is better. If you disagree, let me know, and tell me why, we’ll talk about it.

From the viewpoint of the larger picture, I don’t think we’ve noted any major “code signs”. We have made InvaderGroup a bit more “standard” in that it uses begin_interactions and interact_with more like we’d expect. And I think that asking the Invader if it is entering the bumper was a nice touch. But the only larger lesson I see here might be:

Never Give Up: If the code seems to need work and you have time, give it a look, maybe two or three. Quite often, you’ll find a solution that will make things better.

Now, in your “real world”, you may not have much time to clean things up. I believe that will bite you, making the code base harder and harder to work with. What I would do would be to try to work always to clean up whatever area we were working in, rather than just bang in the code. Over time, that will pay off, and it will tend to generate a more peaceful pace as well. I might find it hard to do at first, and might have to ease into it.

What you should do, of course, is whatever seems best to you. You’ll probably find value in experimenting a bit, not pushing boundaries too far, just finding out whether the krill are a bit more tasty in a slightly different part of the ocean.

See you next time!

(Partial Listings)

class InvaderFleet(InvadersFlyer):
    def begin_interactions(self, fleets):
        self.invader_group.begin_interactions(fleets)

    def interact_with_bumper(self, bumper, _fleets):
        self.invader_group.interact_with_bumper(bumper, self)

class InvaderGroup:
    def update_next(self, origin, current_direction):
        self.handle_direction_change(current_direction)
        return self.perform_update_step(origin)

    def handle_direction_change(self, current_direction):
        if self.current_direction != current_direction:
            self.current_direction = current_direction

    def perform_update_step(self, origin):
        if self._next_invader < len(self.invaders):
            self.move_one_invader(origin)
            return CycleStatus.CONTINUE
        else:
            return self.end_cycle()

    def move_one_invader(self, origin):
        invader = self.next_invader()
        invader.position = origin
        self._next_invader += 1

    def end_cycle(self):
        self._next_invader = 0
        return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE

    def next_invader(self):
        return self.invaders[self._next_invader]

    def begin_interactions(self, fleets):
        self.should_reverse = False

    def interact_with_bumper(self, bumper, _fleet):
        if self._next_invader < len(self.invaders):
            return
        colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
        self.should_reverse |= any(colliding)

class Invader:
    def is_entering(self, bumper, current_direction):
        return bumper.am_i_entering(self.rect, current_direction)

class Bumper:
    def am_i_entering(self, rect, direction):
        return self.intersecting(rect) and direction == self.incoming_direction