Python 208 - Refactoring

Let’s continue to look at InvaderFleet and see whether some handy little objects might help us out.

InvaderFleet has all these member variables that change at different times. Here’s my list from the previous article, this time sorted to put things that change roughly together closer in the list, slow to fast.

step - Set up once, never changes.
down_step - Set up once, never changes.
invaders - Set up once. Might change when invaders are shot, and when a new cycle gets created.
reverse - Changes at edges, about every 24 cycles.
direction - Changes at edges, about every 24 cycles.
origin - Changes on every cycle, a full move of all the invaders.
next_invader - Changes on every update, 60 times a second.

I’ve sorted those members into that order in the code:

class InvaderFleet(Flyer):
    def __init__(self):
        self.step = Vector2(8, 0)
        self.down_step = Vector2(0, 32)
        self.invaders = [Invader(x%11, x//11) for x in range(55)]
        self.reverse = False
        self.direction = 1
        self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
        self.next_invader = len(self.invaders)
        self.position_all_invaders()

Can’t hurt, might help.

One thing leaps out at me. The invaders member is a plain vanilla list. I often use the vanilla collections, and I know that almost always, when I wrap a generic collection with a collection of my own, some benefit appears. I can’t often predict quite what it’ll be, but the collection provides a place to keep track of things.

Now we know something that isn’t done, but needs a bit of consideration. Invaders are going to be destroyed. We’re not exactly catering to that at the moment, but if we were to remove one from the list, things would almost work as intended. There would be an issue with the indexing, depending on whether the next_invader index was before or after the invader that was destroyed. We’ll need to keep all that in sync somehow, lest we skip over an invader and fail to move it.

So perhaps we should create an InvaderCollection for our InvaderFleet to use.

Let me space out those fields according to the time breakout. It might help me think.

class InvaderFleet(Flyer):
    def __init__(self):
        self.step = Vector2(8, 0)
        self.down_step = Vector2(0, 32)

        self.invaders = [Invader(x%11, x//11) for x in range(55)]

        self.reverse = False
        self.direction = 1

        self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)

        self.next_invader = len(self.invaders)
        
        self.position_all_invaders()

By the way, whenever we find ourselves wanting to space things out, it’s a clue that some separation is trying to show itself. Another clue that we have several objects here.

I think I’ll do the special collection, call it InvaderGroup. InvaderList? It is a list. Group is more generic.

class InvaderGroup():
    def __init__(self):
        self.invaders = ()
        self.create_invaders()

    def create_invaders(self):
        self.invaders = [Invader(x%11, x//11) for x in range(55)]


class InvaderFleet(Flyer):
    def __init__(self):
        self.step = Vector2(8, 0)
        self.down_step = Vector2(0, 32)

        self.invader_group = InvaderGroup()

        self.reverse = False
        self.direction = 1

        self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)

        self.next_invader = len(self.invaders)

        self.position_all_invaders()

    def position_all_invaders(self):
        for invader in self.invaders:
            invader.set_position(self.origin)

This has broken some code and tests. We’ll trek through the necessary changes. We’ll skip next_invader but I expect to push that at least down into InvaderGroup, perhaps further.

position_all_invaders needs an origin. Should we provide it, or should the Group know it? We should provide it, I think, as the Fleet. I’m not certain about that.

Now in fairness to the X Individual referred to in the preceding article, I am in fact now working this out as I go. Why not? I have to work it out sometime. Why not go as I work it out?

I push a lot of things down:

class InvaderGroup():
    def __init__(self):
        self.invaders = ()
        self.create_invaders()

    def create_invaders(self):
        self.invaders = [Invader(x%11, x//11) for x in range(55)]

    def position_all_invaders(self, origin):
        for invader in self.invaders:
            invader.set_position(origin)

    def draw(self, screen):
        for invader in self.invaders:
            invader.draw(screen)

    def interact_with_bumper(self, bumper, fleet):
        for invader in self.invaders:
            invader.interact_with_bumper(bumper, fleet)

    def set_invader_position(self, index, origin):
        self.invaders[index].set_position(origin)

And make the corresponding changes to the Fleet. I have tests failing and on the screen, the fleet is much lower down than it was, which is curious.

I look at the tests and add one property to InvaderFleet:

    def invaders(self):
        return self.invader_group.invaders

Let’s rename that to testing_only_invaders just to keep ourselves honest.

We’re green, but why are my invaders lower on the screen than they were?

I’m going to commit this, then pull back a previous version and see if I’ve lost my mind or what.

Commit: Creating InvaderGroup. Nearly done.

Ah. The change happened when I removed the row and column from Invader. That’s four commits back. Let’s find and fix the problem. I thought we had a test for this but if we do it’s not robust enough.

The bug is kind of interesting. It reminds me of that story about the federal tax being set to 25 times gross pay in one place and its accessor being fed_tax / 100. The relative positions of the invaders go forward, increasing y, and the previous way of positioning them was subtracting the y component. Compensating, if you will. The correct code:

class Invader:
    def __init__(self, row, column):
        self.relative_position = Vector2(INVADER_SPACING * row, -INVADER_SPACING * column)
        self.rect = pygame.Rect(0, 0, 64, 32)

row and column are simply the wrong names. Should be x and y. Changed.

There’s a test that needed to check for a negative y as well. I have one more concern: which invader is the lead one, the top left or bottom left. I’ve been thinking in terms of bottom left, not that it matters. It’s lower left. I did this:

    def draw(self, screen):
        if screen:
            circle_color = "green" if self.relative_position == Vector2(0, 0) else "red"
            pygame.draw.rect(screen, "yellow", self.rect)
            pygame.draw.circle(screen, circle_color, self.rect.center, 16)

And looked:

green guy

I’ll commit this. “Fix issue with upside-down drawing.”

I think we need a test for this. Let’s compare the first invader and the last invader and, lets see, which way does Y go? Increasing downward. I think that really means that I should key on the top left, but let’s do the test the way the code is and worry about that later. I think we’ll never care.

    def test_fleet_y_decreases_with_n(self):
        group = InvaderGroup()
        group.position_all_invaders(Vector2(0, 0))
        first: Invader = group.invaders[0]
        last: Invader = group.invaders[-1]
        assert first.position.y > last.position.y

Green. Commit: test y position of invader group.

Let’s have another break. Let’s reflect.

Reflection

We have broken out a “smart collection”, InvaderGroup, from InvaderFleet. The group handles creation, positioning, drawing and bumper checking for the Fleet. The individual moving isn’t quite what we’d like, I think.

Here are the changed bits in fleet:

class InvaderFleet(Flyer):
    def __init__(self):
        self.step = Vector2(8, 0)
        self.down_step = Vector2(0, 32)

        self.invader_group = InvaderGroup()
        self.origin = Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
        self.invader_group.position_all_invaders(self.origin)

        self.reverse = False
        self.direction = 1

        self.next_invader = len(self.invader_group.invaders)

    def update(self, delta_time, _fleets):
        self.check_end_cycle(delta_time)
        self.invader_group.set_invader_position(self.next_invader, self.origin)
        self.next_invader += 1

    def draw(self, screen):
        self.invader_group.draw(screen)

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

Mostly we just forward. We are still managing which invader to move in the Fleet, which needs improvement. And here’s the Group:

class InvaderGroup():
    def __init__(self):
        self.invaders = ()
        self.create_invaders()

    def create_invaders(self):
        self.invaders = [Invader(x%11, x//11) for x in range(55)]

    def position_all_invaders(self, origin):
        for invader in self.invaders:
            invader.set_position(origin)

    def draw(self, screen):
        for invader in self.invaders:
            invader.draw(screen)

    def interact_with_bumper(self, bumper, fleet):
        for invader in self.invaders:
            invader.interact_with_bumper(bumper, fleet)

    def set_invader_position(self, index, origin):
        self.invaders[index].set_position(origin)

Very simple so far.

I think this is better. Do you agree?

I think this was done thoughtfully, though I freely grant that it could have been done a bit more gracefully. Possibly a strangler approach would have been better. There’s an example of Strangler here.

We are improving the code incrementally. We aren’t doing a lot of what I’d call rework. Most of the changes are trivial. The tests have been helpful and easily placated with the added property in the InvaderFleet. Perhaps some of those tests should migrate to the Group.

We can look into that later … if it seems valuable.

See you next time!