Having used the BitmapMaker in the game code, I envision a better way of using it.
Caching a BitmapMaker
The BitmapMaker creates all the game’s bitmaps in one go when an instance is created. That’s probably just fine, except that this is how I use it in the single case implemented so far:
class InvaderGroup: def create_invader_bitmaps(self): maker = BitmapMaker() aliens = maker.aliens alien_table = (aliens[0:2], aliens[0:2], aliens[2:4], aliens[2:4], aliens[4:]) return alien_table
As a user of the class, I want to grab it and then fetch out only the bitmaps that I need. If that pattern persists, and I feel sure that it will, we’ll create a number of instances, they’ll create and save all the bitmaps, and then we’ll fetch the few that we actually want. That’s inefficient. I don’t care much about efficiency but creating the thing multiple times just isn’t nice.
Let’s do a simple “singleton” trick, giving BitmapMaker a class method
instance, and just create one. We’ll create it lazily, because the class isn’t even used when the game is Asteroids.
I don’t think I’ll TDD this. I may regret that, but this is going to be pretty simple.
Let’s change our call first.
def create_invader_bitmaps(self): maker = BitmapMaker.instance() aliens = maker.aliens alien_table = (aliens[0:2], aliens[0:2], aliens[2:4], aliens[2:4], aliens[4:]) return alien_table
PyCharm informs me that there is no such critter, and offers to help. It’s not a lot of help, but it saves a bit of typing:
class BitmapMaker: @classmethod def instance(cls): pass
Now if we have an instance we want to return it and if not, create it and return it. I finish up the thing this way:
class BitmapMaker: _instance = None @classmethod def instance(cls): if not cls._instance: cls._instance = cls() return cls._instance
That does the job. And because I’m not really doing a full Singleton pattern, anyone who wants a BitmapMaker of their very own can still create one. We could fix that if we wanted. I think you do it by overriding
__new__, but I don’t expect ever to want to know that.
We’re green and the invaders march. Commit: provide instance class method on BitmapMaker to avoid repeated effort.
I did a little work yesterday when you weren’t looking. Let me show you now. I was thinking about what to draw next in the game, and that led me to thinking about the shots from the invaders, and that reminded me that when the game decides to shoot, the shot always emits from the bottom-most invader in whatever column fires. Otherwise the shot would run over some invaders before it started dropping toward the player. So I was thinking about how to find the bottom-most invader and wrote this test.
def test_bottom_of_column(self): group = InvaderGroup() invader = group.bottom_of_column(5) assert invader.column == 5 for i in range(5): invader = group.bottom_of_column(5) group.kill(invader) invader = group.bottom_of_column(5) assert not invader
The test posits a method
bottom_of_column in InvaderGroup, and a new property,
column on the invader. I think that when we look at what I did yesterday, we’ll see a better way. But here’s what I did yesterday:
class InvaderGroup: def bottom_of_column(self, column): matching = [invader for invader in self.invaders if invader.column == column] if matching: return matching else: return None def kill(self, invader): self.invaders.remove(invader) class Invader: def __init__(self, x, y, bitmaps): self.bitmaps = bitmaps self.relative_position = Vector2(INVADER_SPACING * x, -INVADER_SPACING * y) self.rect = pygame.Rect(0, 0, 64, 32) self.image = 0 @property def column(self): return self.relative_position.x // INVADER_SPACING
This works. But it seems a bit naff to take the column info and scale it up in
__init__ and then back down in
column. We have loads of memory. Let’s just save the information. For now, we’ll just save the column, though the row may be interesting at some future time. If it is, we’ll worry about it then.
I think we’ll rename the input parameters while we’re at it. I decide just to remove the property and let folks access the column if they want to. I trust myself not to do anything evil. Probably.
class Invader: def __init__(self, column, row, bitmaps): self.bitmaps = bitmaps self.column = column self.relative_position = Vector2(INVADER_SPACING * column, -INVADER_SPACING * row) self.rect = pygame.Rect(0, 0, 64, 32) self.image = 0
This works fine so far. Commit: create
column member on invader.
kill method will not long stand:
class InvaderGroup: def kill(self, invader): self.invaders.remove(invader)
“Why not?”, you wonder. The InvaderGroup keeps track of which invader is next to move, since we move them one at a time, every 60th of a second. It gives them that scary ripple effect that they have. And the invaders are kept in a list. If the
next_invader index is less than the index of the invader we kill, no harm done. But if it is greater than or equal to the index of the moribund invader, all the invaders will slide up one in the list and we’ll skip one in the move cycle.
I haven’t decided how to deal with this subject, but it seems to me that it would be prudent to make it work correctly, even if this is not the implementation we wind up with.
Let’s write a test. To orient myself, I first look at the update:
class InvaderGroup: def update_next(self, origin): invader = self.invaders[self.next_invader] invader.set_position(origin) self.next_invader += 1 if self.next_invader < len(self.invaders): return "ok" else: self.next_invader = 0 return "end"
The return values are used in InvaderFleet, so that it will know when to adjust its origin, which is changed every time an invader update cycle is complete.
I think I’d like my test to work in terms of invaders, not just indexes, because I really don’t know whether I’ll stick with the list, or perhaps have an alive/dead flag in the invaders. So let’s extract a tiny method for our test to use. I want to call that method
next_invader, so first let’s mark the member integer private. Rename
def update_next(self, origin): invader = self.invaders[self._next_invader] invader.set_position(origin) self._next_invader += 1 if self._next_invader < len(self.invaders): return "ok" else: self._next_invader = 0 return "end"
And Extract Method:
def update_next(self, origin): invader = self.next_invader() invader.set_position(origin) self._next_invader += 1 if self._next_invader < len(self.invaders): return "ok" else: self._next_invader = 0 return "end" def next_invader(self): return self.invaders[self._next_invader]
And commit: provide and use
Now, if you’ll excuse me for a moment, I haven’t yet made my morning chai, nor fetched my morning banana and granola bar.
I’m back. Now about the test. What is our concern? I can think of several ways of expressing what I’m worried about.
When an invader is removed:
- we should not try to update it;
- we should still update all the others;
- if we are currently updating past that invader, we should not miss any;
- we should never try to update past the end of the list.
As I think about this, I realize that the list data structure isn’t really that great for deleting, as it will move, on the average, half the list on every removal. Now, I freely grant that that’s only once or twice every second at worst, so probably we shouldn’t worry about it.
Let’s go ahead and write the test and worry about the implementation later. How about these test ideas:
- Set _next_invader ahead of the one to be removed. Ensure that we get the same next guy after the remove.
- Set it on the one to be removed. Ensure we don’t get the removed on, but the one beyond.
- Set it after the one to be removed. Ensure we get the same one after the removal.
That seems to me to cover the cases. The first test runs:
def test_remove_ahead_of_cursor(self): group = InvaderGroup() to_remove = group.invaders next = group.next_invader() group.kill(to_remove) assert group.next_invader() == next
I think I like its form. Let’s try the second one.
def test_remove_at_cursor(self): group = InvaderGroup() to_remove = group.invaders group._next_invader = 23 should_update = group.invaders assert group.next_invader() == to_remove next = group.next_invader() group.kill(to_remove) assert group.next_invader() == should_update
That’s not as clear as it might be. Improve it thus:
def test_remove_at_cursor(self): group = InvaderGroup() to_remove = group.invaders group._next_invader = 23 should_update = group.invaders next_up = group.next_invader() assert next_up == to_remove group.kill(to_remove) assert group.next_invader() == should_update
This one passes. Why? Because if we’re looking right at the one removed, we’ll be looking at the correct next one after the remove. The third test, I think, will fail.
def test_remove_after_cursor(self): group = InvaderGroup() to_remove = group.invaders group._next_invader = 30 should_update = group.invaders group.kill(to_remove) assert group.next_invader() == should_update
This fails, asexpected. And I think we can fix it thus:
def kill(self, invader): index = self.invaders.index(invader) del self.invaders[index] if self._next_invader > index: self._next_invader -= 1
I am told by the Internet that
del, as used above, removes the item from the list without the search. I can’t think how it does that but I suppose the compiler can do anything it wants. Frankly, I don’t like it. The list method
pop also takes an index. Let’s compromise and do that:
def kill(self, invader): index = self.invaders.index(invader) self.invaders.pop(index) if self._next_invader > index: self._next_invader -= 1
Tests are green. I’m going to commit but I see an issue: what if it’s the last invader?
Commit: InvaderGroup.kill and next_invader play nicely together.
But wait! What if the one we’re on is the last invader? If it is, then it is invader and
_next_invader is also zero. And now there are no more invaders left and
update_next should return “end” without actually updating anything.
I think I can write a test that will throw an exception.
def test_remove_last_invader(self): group = InvaderGroup() for count in range(55): group.kill(group.invaders) result = group.update_next(Vector2(0, 0)) assert result == "end"
I get the expected exception:
def next_invader(self): > return self.invaders[self._next_invader] E IndexError: list index out of range
I think we have no choice but to check at the beginning of the
update_next. But we can probably arrange things so that we check only at the beginning. Let’s try it.
def update_next(self, origin): if self._next_invader > len(self.invaders): self._next_invader = 0 return "end" invader = self.next_invader() invader.set_position(origin) self._next_invader += 1 return "ok"
With this code, we’ll get an extra call to update and return “end” after that. I think the game will work, but there are two tests failing. I want to see if the game works, because if it doesn’t I’ll know this idea is more wrong than I think it is.
The game does explode and I see that the code should say
def update_next(self, origin): if self._next_invader >= len(self.invaders): self._next_invader = 0 return "end" invader = self.next_invader() invader.set_position(origin) self._next_invader += 1 return "ok"
But a test fails. What is it? I am pleased to see that it is this one:
def test_update_next(self): group = InvaderGroup() origin = Vector2(100, 100) for i in range(54): result = group.update_next(origin) assert result == "ok" result = group.update_next(origin) assert result == "end"
That’s failing because now we return “ok” on the last invader and only return “end” when we try to update beyond the end. Change the 54 to 55.
def test_update_next(self): group = InvaderGroup() origin = Vector2(100, 100) for i in range(55): result = group.update_next(origin) assert result == "ok" result = group.update_next(origin) assert result == "end"
Green. Commit: change update_next to return “end” on first call past end of the rack. Users shouldn’t care.
I see we’re nearly two hours in and over 350 lines of input to the article. Let’s reflect on what we’ve just done, then sum up.
Yesterday’s work gave us a
kill method that I knew was unsustainable because it didn’t deal with keeping the next invader concept current. So we wrote some tests to ensure that we always got the right invader back.
Somehow, I saw that things would go badly if we were removing the last invader. That was a good catch, because killing all the invaders is difficult and as a terrible game player, I might not have found the problem for a long time. So we wrote a simple test that threw an exception. It was clear by the nature of
kill that we would leave the
next_update doomed to fetch a non-existent invader, since it always fetched and there were none.
The update to
next_invader seemed to me to be clear, although it wasn’t a refactoring, it was a fix. We could have just added the new check and left the others in place, but somehow it seemed to me that it would be OK if we always returned “ok” on an update that updated, and returned “end” when we tried an update and discovered we were past the end.
The test ran with the new change, which gave me confidence, even though another failed. I was pretty sure it was the one that checked for “end” after 54 updates. But until I tried the game, I really thought my change was correct. When the game threw an exception, the fix was clear.
Had I not run the game, I think my test would still have caught the issue. A quick check, done just now, confirms that. So, counting the new ones, the test around
kill are pretty robust.
The seemingly simple change to
update_next is not really quite that simple. It changes what we might call an invariant. It used to be that we returned “end” upon updating the last invader, and now we return “end” on the update after the last invader. So there is always a call to update that doesn’t update, just returns end.
I’m not saying that’s wrong. It’s a bit like returning EOF on the first call that doesn’t get any data, as opposed to returning it on the last call that does return data. It might even be a better way to do than what we had. But it is different, and as such, I wish it had been more obvious that it was different.
I’ll think about that, and the notion of invariant. Maybe something will come to me.
For now, it works in its new form and no other object had to change, so I like it.
Let’s sum up.
I’m glad I took the time to reflect a bit more deeply about the next invader logic. I generally find it useful to take a step back and think. Frequent readers may have noticed how often, during Reflection sections or Summary (what rhymes with “summary”?) sections, I think of something and change it right there. The step back has great value while things are still fresh in my mind, because it invites me to look at the bigger picture. The change in perspective often turns something up that the detailed thinking did not.
Sometimes it’s just a poor variable name or a possible extraction. Sometimes it’s something bigger. It pays off often enough that I try always to take that little step back and think for a bit.
I also reported a bit of work around finding all the invaders in a column, and we improved the code a bit this morning. It’s not terribly efficient, but I really don’t think it matters much. It does search all the invaders checking column, but we’ll never notice the time it takes. If it did seem too slow we could do something else: all the invader storage logic is inside InvaderGroup, where we can adjust it if needed … as we did today with
Today we also made a simple improvement to the BitmapMaker, providing a class method
instance() that returns a configured maker without redoing all the work that it does. It’s a simple change that avoids recreating all the bitmaps for every user.
We could imagine, instead of that trick, having the maker make only the maps requested. We could, for example, provide a property for each map or map collection that it returns, and build just the desired maps at that time. That would add more code to the maker and while the changes would be “trivial”, I think there’s a decent chance that I’d make a mistake moving things around. It would certainly be more work.
I grant that we could have done nothing, and just let the thing be created and do extra work on each use, but that just didn’t sit well with me. The
instance() notion was easy and I feel better about it.
I like to feel better about things. When I see a little something that can make me feel better, I do it. I wish I had that same attitude with exercise.
A good morning, and a bit of improvement to the game. Next time, I think we need to add something visible. Player? Invader shots? I’m not sure.
See you next time, and we’ll all find out together!