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.
An individual on
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 along1.
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.
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 # 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 defer2 that for now.
The next test is more problematical:
def test_fleet_motion(self): fleet = InvaderFleet() fleet.step = Vector2(30, 0) pos = fleet.invaders.position fleet.update(1.0, None) new_pos = fleet.invaders.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
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
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.
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!
I suppose these days, ChatGPT might be making it up as it goes along but ChatGPT lies and I do not let it program for me. ↩
Should we defer it? Perhaps not. The more magic numbers we get in our code, the more brittle it will be. Nonetheless, I’m working another problem right now and decide to let this slide. I can’t do everything. ↩