Python 207 - Refactoring

Python Asteroids+Invaders on GitHub

It seems to me that InvaderFleet may be asking for some refactoring. Let’s start with some thoughts on TDD, and a look at the tests and code.

TDD

An individual on Twitter X seemed to be objecting to TDD, with assertions like “TDD is about making it up as you go along” and “introduces so much rework that you end up with very little quality code”, and “running out of time to write tests”. I engaged them briefly. Don’t bother to look, we’ll make my points better here.

I use TDD for a number of reasons but one of the most important is that it lets me get to working, good quality code faster than if I didn’t use it. If it slowed me down or made my code worse, I’d be a fool to use it. Although I surely am a fool, I’m not quite that much of a fool. I can only conclude that the individual is referring to some set of behaviors that they call “TDD” that is not the same set of behaviors that I call “TDD”.

TDD gets me to working good quality code faster than if I didn’t use TDD. But that’s not the only reason why I use it.

I have found that no matter how much I design “on paper”, I cannot get to working, good quality code without making lots of mistakes. And since, “on paper”, I’m not going to design just a few lines and make them work, I write rather a large quantity of code without TDD, and then I have to debug it. That can take a long time. Often a very long time.

It is fair to count that time in “how long it takes to get to good quality code”, by the way, which the X Individual may or may not have been doing.

Now if you’ve been with me for more than a couple of articles, you also know that I still spend a lot of time thinking without coding, figuring out how things should be done. So I’m not quite just “making it up as you go along”, although I think in a debate, which I would win, by the way, I’d argue that even if we were to think until we could write the whole program down from memory, we’re always making it up as we go along¹.

I also find that even when I think about the code a lot, my thoughts never produce code the first time that I consider to be of high enough quality. I do not know the code of the X Individual, so perhaps they type in really pristine code that one can be proud of.

I should say “proud enough”. In these articles, I often go far beyond what I’d do in a production situation. I would refactor to improve things, always, but sometimes here I refactor just to show what happens, or to explore with you another way of doing things. I definitely gild the lily sometimes in these articles, and I would not recommend that on most real software efforts.

That said, I invite the X person, or anyone, to show me some code and see whether I can see things to improve in it. In most cases, I’d bet that I can.

Anyway, TDD makes me faster, helps me be confident that the code does what I intended, and helps me make it production quality. If it didn’t do those things, I’d try something else.

InvaderFleet

Partly because I’ve been finding it difficult to write really nice tests for InvaderFleet, and partly just because working with it makes me suspect this, I suspect that there are one or more objects trying to be born from InvaderFleet. Let’s review a bit and see what we can see. We’ll start with its tests.

The first few tests seem to me to be just fine. They drive out the fleet and check the status of the invaders it contains:

class TestInvaderFleet:
    def test_exists(self):
        fleet = InvaderFleet()

    def test_makes_invaders(self):
        fleet = InvaderFleet()
        invaders = fleet.invaders
        assert len(invaders) == 55

    def test_invaders_order(self):
        fleet = InvaderFleet()
        count = 0
        for y in range(5):
            for x in range(11):
                invader = fleet.invaders[count]
                assert invader.row == x
                assert invader.column == y
                count += 1

    def test_fleet_origin_is_centered(self):
        fleet = InvaderFleet()
        assert fleet.origin == Vector2(u.SCREEN_SIZE / 2 - 5*64, 512)
        invader = fleet.invaders[5]  # bottom row middle column
        assert invader.position.x == 512

I just renamed that last one. It used to be just “test_fleet_origin”. It does contain some magic numbers. Let’s note that that probably means we need to create some new manifest constants. But we’ll defer² that for now.

The next test is more problematical:

    def test_fleet_motion(self):
        fleet = InvaderFleet()
        fleet.step = Vector2(30, 0)
        pos = fleet.invaders[0].position
        fleet.update(1.0, None)
        new_pos = fleet.invaders[0].position
        assert fleet.direction == +1
        assert new_pos - pos == fleet.step
        fleet.at_edge(+1)
        assert fleet.reverse
        fleet.next_invader = len(fleet.invaders)
        fleet.update(1.0, None)
        assert fleet.direction == -1

Why? Well, what is it doing? It’s checking that the fleet starts out moving to the right, except that “right” is represented as +1. And it calls fleet.update to ensure that the fleet moves in the direction we expect, by the amount we expect. Is that obvious from the test? It wasn’t obvious to me just now, and I wrote it. Then the test goes on to do another thing. It reports that we’re at the edge, and calls update again to show that the direction has reversed.

Now that’s not wrong, exactly, because the fact is, we report at edge during the drawing cycle, and we use the fact later, in a moderately complicated sequence:

class InvaderFleet(Flyer):
    def at_edge(self, bumper_incoming_direction):
        self.reverse = bumper_incoming_direction == self.direction

    def update(self, delta_time, _fleets):
        self.check_end_cycle(delta_time)
        self.invaders[self.next_invader].move_relative(self.origin)
        self.next_invader += 1

    def check_end_cycle(self, delta_time):
        if self.next_invader >= len(self.invaders):
            self.reverse_or_continue(delta_time)

    def reverse_or_continue(self, delta_time):
        # we use +, not += because += modifies in place.
        if self.reverse:
            self.reverse = False
            self.direction = -self.direction
            self.origin = self.origin + self.direction * self.step + self.down_step
        else:
            self.origin = self.origin + self.direction * self.step
        self.next_invader = 0

When I review this code now, I notice that it is thinking kind of procedurally, wondering to itself whether it should reverse or continue. And it doesn’t actually just reverse, it steps down and reverses. Maybe some object could help us here

There’s also another clue about the need for another object. Let’s look at all the member variables, since we’re currently looking at reverse, direction, step, and origin.

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

Let’s think about how often each one of these members changes:

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

And the for clause? That needs at least to be pulled out and given a name. It is giving each invader its current screen position:

class Invader:
    def move_relative(self, origin):
        pos = Vector2(origin.x + 64*self.row, origin.y - 64*self.column)
        self.rect.center = pos

That’s not a move so much as a set position. And it’s not setting a relative position, it is setting an absolute position based on its relative relationship to the origin point.

Let’s extract the loop and call it position_all_invaders. And let’s rename move_relative to set_position:

    def set_position(self, origin):
        pos = Vector2(origin.x + 64*self.row, origin.y - 64*self.column)
        self.rect.center = pos

And let’s make it more clear what it does.

    def set_position(self, origin):
        my_relative_position = Vector2(64*self.row, 64*self.column)
        pos = origin + my_relative_position
        self.rect.center = pos

How often does my_relative_position change? Never. Make it a member. This is Introduce Field, alt-command-F on my machine. PyCharm helps but not perfectly:

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

    def set_position(self, origin):
        pos = origin + self.relative_position
        self.rect.center = pos

The line really needed to be below the definition of self.row and column. Except that now we don’t need them.

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

A test fails, checking row and column which no longer exist.

    def test_invaders_order(self):
        fleet = InvaderFleet()
        count = 0
        for y in range(5):
            for x in range(11):
                invader = fleet.invaders[count]
                assert invader.row == x
                assert invader.column == y
                count += 1

Change to:

    def test_invaders_order(self):
        fleet = InvaderFleet()
        count = 0
        for y in range(5):
            for x in range(11):
                invader = fleet.invaders[count]
                assert invader.relative_position.x == x*64
                assert invader.relative_position.y == y*64
                count += 1

I dislike that 64. Time for some constants. What is the spacing between objects called? There’s some cool word for it. We’ll use invader_spacing for now.

invader.py
INVADER_SPACING = 64


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)

    def test_invaders_order(self):
        fleet = InvaderFleet()
        count = 0
        for y in range(5):
            for x in range(11):
                invader = fleet.invaders[count]
                assert invader.relative_position.x == x * INVADER_SPACING
                assert invader.relative_position.y == y * INVADER_SPACING
                count += 1

OK, that’s better if not great. I’ll surely want to move that constant somewhere. Let’s commit: Improve invader with manifest constants and make her relative position invariant.

Here, for reflection, is Invader now:

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)

    @property
    def position(self):
        return Vector2(self.rect.center)

    def set_position(self, origin):
        self.rect.center = origin + self.relative_position

    def draw(self, screen):
        if screen:
            pygame.draw.rect(screen, "yellow", self.rect)
            pygame.draw.circle(screen, "red", self.rect.center, 16)

    def interact_with_bumper(self, bumper, invader_fleet):
        if bumper.intersecting(self.rect):
            invader_fleet.at_edge(bumper.incoming_direction)

I inlined the set_position temp just now.

Reflection

Now maybe you, and the X Individual, are such good programmers that you would have coded this right out of your coding pencil, but I can tell you that I’m not that good. But this is faster than what I had before, shorter, and in my view, substantially more clear. And my TDD tests are checking it (and yes, did need a small change).

This is what TDD and realizing that my initial code can use improvement brings me.

It also brings me to 300 lines of article, and my lines are generally more than one display line long.

So it’s fair, X Individual, to say that I have taken an hour to change a few lines of code. But that hour was spent showing and telling, not doing. The doing was the matter of moments.

Let’s close this article here and start another one, still aimed at InvaderFleet and its myriad members. I’ll try to be more strategic next time, but the truth is, tactical refactoring tends to give me good strategic outcomes.

See you shortly!