Python 90 - More Decent
-ralization. Just a picture of the relationships of some of the objects to start. Then a question: a world record? Guinness, where are you? And then: progress!
Wednesday 1600:
Here’s a picture I drew the other day when thinking about how the objects interact. It really expresses what’s in the table from a previous article, in fact I used it to create the table, but I think it makes a nice bit of history for what will surely be a world record number of articles written about Asteroids by a single person.
I have written 305 asteroids articles so far, counting this one, and that’s assuming they are all correctly indexed.
I think the point of the diagram above, and for that matter, the point of my desktop yellow sticky note “Jira” is that although I program very incrementally, trying always to have my code supported by tests, and without much certainty about quite how it’s going to turn out, much less the exact steps for getting there, I am always analyzing and designing, not “just coding”.
Because I find a balance of analyzing, designing, testing, and coding, I’m able to provide visible features quite often, which tends to satisfy the business side of the house (that would be the east side) and keep the technical (west) side in good shape as well. I have seen the same approach work with larger teams in real situations, and if I were every condemned to software product development in a real corporation, this is how I would ask the team to work.
I am reminded of my article How to Impose Agile, which has this blurb:
There’s a lot of whinging going on about imposing ‘Agile’ methods and practices. These whiners just don’t know how to do imposition correctly. Me either, but here’s what I’d try.
Thursday 0700
This morning — a few lines ago it was around 1600 hours yesterday and now suddenly it’s morning today — I would like to modify how the asteroid and missile collide, moving toward the new scheme where each object receives interact_with other, and does other.interact_with_myclass(each), where myclass is whatever class each happens to be.
In our case here:
- an_asteroid gets
interact_with(an_object) - an_asteroid sends
an_object.interact_with_asteroid(an_asteroid) - an_object, whatever it is, now knows both classes and can act accordingly.
And it goes both ways, of course, with the other object, which happens to be a_missile, doing this:
- a_missile gets
interact_with(aSomething - a_missile sends
aSomething.interact_with_missile(a_missile) - aSomething, actually an_asteroid, now knows both classes and does the right thing.
Classic double dispatch. Why? Because we want the game not to know the classes of its objects, because we want the game to know nothing about the actual game of Asteroids or whatever game it may be. It just ticks all the objects, executes the interaction protocol, and draws them.
Deleting and Adding
Before we get too deep in the code here, I want to think about how we’ll manage the essential matters of deleting and adding objects to the mix. As things stand, the game is passing three parameters on attack_with, the attacker object, the collection from which the target came, and the Fleets object containing the entire mix. As implemented, interact_with_asteroid, say, only gets the attacker (missile) and Fleets. But the asteroid wants to destroy itself. The question is how does it do that. The options include:
- Ask fleets:
fleets.asteroidsto get its Fleet, because it knows it is an asteroid and then add itself to that Fleet; - Tell fleets:
fleets.addAsteroid(self), leaving it up to Fleets class to know how it wants to save things.
The latter is better. It is generally better, in my experience, to follow the famous “tell, don’t ask” advice, but in this case I think we already know the reason: before we’re done, the Fleets object will just be a single collection of all the space objects there are, including a bunch of new special purpose ones.
So I envision the flow of change as this:
- Stop talking to individual Fleet objects, saying
addandremove; - Instead implement special methods on Fleets class, saying e.g.
add_asteroidandremove_missile; - Implement those methods for now to put the objects into their separate collections, until we’re ready;
- At some future time, fold all the collections into a single one;
- And then at our leisure, replace calls like
add_steroidwith a simpleaddcall to the Fleets object.
Are we colliding?
Currently, the interact_with is only being called when objects are within range of each other, because Game is checking that. In the future, we will not likely do that: we’ll leave it up to the objects. So, as part of what we do this morning, or whenever we get there, we should probably check the range as part of the interaction.
I find that a bit troubling. If an_asteroid is within range of a_missile, then a_missile is within range of an_asteroid, and it seems wasteful to check it twice. I think we’d be wise to deal with that when we have the new structure in place, so having reported that I’m a bit troubled, we’ll move on to checking everywhere, at least for now.
Let’s do it.
We have interact_with_asteroid everywhere, doing a pass. We do not have interact_with_missile. And I wonder about tests. Do we have a decent test for asteroid vs missile? Ah, we do have some, going through mutual_destruction, the method that sends interact_with, formerly destroyed_by:
class Collider:
def mutual_destruction(self, target, targets, attacker, attackers):
if self.within_range(target, attacker):
self.score += target.score_for_hitting(attacker)
self.score += attacker.score_for_hitting(target)
attacker.interact_with(target, attackers, self.fleets)
target.interact_with(attacker, targets, self.fleets)
return True
else:
return False
There are things to worry about here, like the T/F return, which is used to break the outer loop. We’ll try not to worry. Let’s find tests for missile and asteroid:
def test_missile_asteroid_scores(self):
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
asteroids = [asteroid]
missile = Missile.from_ship(pos, Vector2(0, 0))
missiles = [missile]
collider = Collider(Fleets(asteroids, missiles, [], [], []))
collider.mutual_destruction(asteroid, asteroids, missile, missiles)
assert not missiles
assert collider.score == 20
assert len(asteroids) == 2
This should work for us. We’ll change the two interact_with calls to do the desired forwarding. Asteroid to missile should go first, because interact_with_asteroid is at least in there.
class Asteroid:
def interact_with(self, attacker, asteroids, fleets):
self.split_or_die(asteroids)
As soon as I change this, the test should break. I’ll comment the existing line because I expect to need it as a reminder.
def interact_with(self, attacker, asteroids, fleets):
attacker.interact_with_asteroid(self, fleets)
# self.split_or_die(asteroids)
Exactly one test has broken.
collider.mutual_destruction(asteroid, asteroids, missile, missiles)
assert not missiles
assert collider.score == 20
> assert len(asteroids) == 2
E assert 1 == 2
E + where 1 = len([<asteroid.Asteroid object at 0x1075af2d0>])
Pytest is pretty fine, I must admit. So we have not removed the existing asteroid, nor added in the two new ones. That’s because missile isn’t calling us back. If we make it do that, it will break the other part of the test … unless we do two things at once. Here’s missile now:
class Missile:
def interact_with(self, _attacker, missiles, _fleets):
if self in missiles:
missiles.remove(self)
def interact_with_asteroid(self, asteroid, fleets):
pass
I’m going to go hog wild here. No, on second thought, one thing at a time. First break the other part of the test as well:
def interact_with(self, attacker, _missiles, fleets):
attacker.interact_with_missile(self, fleets)
# if self in missiles:
# missiles.remove(self)
This is surely failing now because people don’t understand interact_with_missile. Seven tests fail. That’s irritating. Lets put interact_with_missile into our test that checks for implementation of the new methods and put it where it needs to go.
We could let the existing failures drive us, but I’d like to get all the checks put into that one test.
def test_double_dispatch_readiness(self):
classes = [Asteroid, Missile, Saucer, Ship, Fragment]
errors = []
for klass in classes:
attrs = dir(klass)
methods = ["interact_with", "interact_with_asteroid", "interact_with_missile"]
for method in methods:
if method not in attrs:
errors.append((klass.__name__, method))
assert not errors
Now to do it. I’ll put this into asteroid, missile, saucer, ship, and fragment:
def interact_with_missile(self, missile, fleets):
pass
That actually fixes two tests, which is interesting. Anyway now let’s look at our asteroid-missile test to see how it is failing now.
collider.mutual_destruction(asteroid, asteroids, missile, missiles)
> assert not missiles
E assert not [<missile.Missile object at 0x1076435d0>]
I think we expected that, because we commented out the removal code. We’ll put it where it belongs. I’ve checked Fleet and it does the if test before removal, so we won’t need it in Missile:
class Missile:
def interact_with_asteroid(self, asteroid, fleets):
fleets.missiles.remove(self)
Five tests are running. We’re back to the 2 vs 1 in our test, which we fix:
class Asteroid:
def interact_with_missile(self, missile, fleets):
self.split_or_die(fleets.asteroids)
Note that in both these cases, I’m still fetching the sub-collection. We’re trying to make minimal changes to get back to green.
It turns out that our current test is running, which does not surprise me. Four others have failed and while I made no prediction, I’m not surprised, because a number of other classes are now receiving interact_with_asteriod or interact_with_missile and don’t know what to do about it.
We’ll tick through the tests, happy to have them.
def test_missile_ship_does_not_score(self):
pos = Vector2(100, 100)
ship = Ship(pos)
ships = [ship]
missile = Missile.from_ship(pos, Vector2(0, 0))
missiles = [missile]
collider = Collider(Fleets([], missiles, [], [], ships))
collider.mutual_destruction(ship, ships, missile, missiles)
> assert not missiles
E assert not [<missile.Missile object at 0x10303fd90>]
test_collisions.py:70: AssertionError
Now we have an interesting question before us. Can we get to green without pushing this new protocol into all the other objects? If we can, can we do it without confusing YT by having protocols half finished?
All of these tests are about missile vs something. Ah. Let’s look at ship, I think I see what we can do:
def interact_with(self, attacker, ships, fleets):
self.explode(ships, fleets)
Yes. If we were to also send interact_with_ship back to the attacker, we could implement that on missile, same as with_asteriod and ship would work. But other classes would then get that message and fail, because no one expects interact_with_ship.
I think two things.
First, I think we’re going to want more tests of interactions before we’re done here.
Second, I’m starting to believe that we have to unwind the interact_with all the way before all our current tests run.
To that end, I’m going to require interact_with_ship and interact_with_saucer everywhere. Put them into that test.
methods = ["interact_with",
"interact_with_asteroid",
"interact_with_missile",
"interact_with_saucer",
"interact_with_ship"]
And I’ll put the pass methods in all those classes and Fragment.
That was easy, they all have the methods, all saying pass.
Now back to our tests. Pick this one:
def test_missile_ship_does_not_score(self):
pos = Vector2(100, 100)
ship = Ship(pos)
ships = [ship]
missile = Missile.from_ship(pos, Vector2(0, 0))
missiles = [missile]
collider = Collider(Fleets([], missiles, [], [], ships))
collider.mutual_destruction(ship, ships, missile, missiles)
> assert not missiles
E assert not [<missile.Missile object at 0x105090210>]
In Ship:
class Ship:
def interact_with(self, attacker, ships, fleets):
attacker.interact_with_ship(self, fleets)
self.explode(ships, fleets)
Should I move the other code or leave it here? Moving it makes more sense. I’m here now and we want our objects not to take action beyond forwarding.
def interact_with(self, attacker, ships, fleets):
attacker.interact_with_ship(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
self.explode(fleets.ships, fleets)
def interact_with_missile(self, missile, fleets):
self.explode(fleets.ships, fleets)
def interact_with_saucer(self, saucer, fleets):
self.explode(fleets.ships, fleets)
We see that this scheme will inevitably produce some short duplications. We’re resolved to pay that price to get our objects out of the game “god’” object.
More tests break. Is the ship-missile one fixed? Not yet, because in missile:
def interact_with(self, attacker, _missiles, fleets):
attacker.interact_with_missile(self, fleets)
# if self in missiles:
# missiles.remove(self)
def interact_with_asteroid(self, asteroid, fleets):
fleets.missiles.remove(self)
def interact_with_missile(self, missile, fleets):
pass
def interact_with_saucer(self, saucer, fleets):
fleets.missiles.remove(self)
def interact_with_ship(self, ship, fleets):
fleets.missiles.remove(self)
We could look up on the card above what should happen. Four tests breaking. The ship-missile one is working. I am inclined to treat these as indicators and just go into the individual objects and fix them up.
In saucer:
def interact_with(self, _attacker, saucers, _fleets):
player.play("bang_large", self._location)
player.play("bang_small", self._location)
if self in saucers: saucers.remove(self)
Let’s modify this a bit and then extract it.
def interact_with(self, _attacker, _saucers, fleets):
player.play("bang_large", self._location)
player.play("bang_small", self._location)
fleets.saucers.remove(self)
Extract:
def interact_with(self, _attacker, _saucers, fleets):
self.explode(fleets)
def explode(self, fleets):
player.play("bang_large", self._location)
player.play("bang_small", self._location)
fleets.saucers.remove(self)
Now do the dispatch and explode as needed:
def interact_with(self, attacker, _saucers, fleets):
attacker.interact_with_saucer(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
self.explode(fleets)
def interact_with_missile(self, missile, fleets):
self.explode(fleets)
def interact_with_saucer(self, saucer, fleets):
pass
def interact_with_ship(self, ship, fleets):
self.explode(fleets)
To my surprise, all the tests are green. I have my doubts about this, so I’ll try the game.
I get a crash, but I don’t think it relates to what we’re doing here:
File "/Users/ron/PycharmProjects/firstGo/fleet.py", line 47, in tick
flyer.tick(delta_time, self, fleets)
File "/Users/ron/PycharmProjects/firstGo/saucer.py", line 158, in tick
player.play("saucer_big", self._location, False)
File "/Users/ron/PycharmProjects/firstGo/sounds.py", line 34, in play
self.set_volume(chan, location)
File "/Users/ron/PycharmProjects/firstGo/sounds.py", line 43, in set_volume
chan.set_volume(1-frac_right, frac_right)
^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'set_volume'
def play(self, name, location=None, multi_channel=True):
if name in self.catalog:
sound = self.catalog[name]
count = sound.get_num_channels()
if multi_channel or count == 0:
chan = self.catalog[name].play()
self.set_volume(chan, location)
else:
print("missing sound", name)
It looks to me as if chan came back None. I didn’t know that was possible. The documents suggest that it isn’t. The error suggests that it is. Let’s fix that.
def play(self, name, location=None, multi_channel=True):
if name in self.catalog:
sound = self.catalog[name]
count = sound.get_num_channels()
if multi_channel or count == 0:
chan = self.catalog[name].play()
if chan:
self.set_volume(chan, location)
else:
print("channel came back None")
else:
print("missing sound", name)
Now let’s see if we can get that message reliably. My guess is that we cannot. So far, I can’t. I find on the Internet that despite the documents saying that it will force a channel, Sound.play can return None. OK, so be it.
We are green and the game appears to work perfectly. I will commit but then I want to convince myself by reading all the code and checking that we have enough tests. But I’ve seen all the collisions work on screen.
Commit: convert collisions to double dispatch. Green and game works, some double-checking yet to be done.
If there’s live code in anyone’s interact_with, we aren’t done. There is not. Now we’ll review the details for each.
class Asteroid:
def interact_with(self, attacker, asteroids, fleets):
attacker.interact_with_asteroid(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
pass
def interact_with_missile(self, missile, fleets):
self.split_or_die(fleets.asteroids)
def interact_with_saucer(self, saucer, fleets):
pass
def interact_with_ship(self, ship, fleets):
pass
This doesn’t look right to me. Asteroid should split on collision with saucer. Ha! It doesn’t work: the saucer exploded and the asteroid did not. Same for ship. It explodes, the asteroid does not split.
We need a test or two. I thought we might. Also we have shipped non-working code. Darn!
Should be a quick fix, I won’t revert the repo.
Here’s a test that is close but doesn’t check asteroids. We’ll enhance it.
def test_asteroid_saucer_does_not_score(self):
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
asteroids = [asteroid]
saucer = Saucer()
saucer.move_to(pos)
saucers = [saucer]
collider = Collider(Fleets(asteroids, [], saucers, [], []))
collider.mutual_destruction(asteroid, asteroids, saucer, saucers)
assert not saucers
assert collider.score == 0
Enhanced:
def test_asteroid_saucer_does_not_score(self):
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
asteroids = [asteroid]
saucer = Saucer()
saucer.move_to(pos)
saucers = [saucer]
collider = Collider(Fleets(asteroids, [], saucers, [], []))
collider.mutual_destruction(asteroid, asteroids, saucer, saucers)
assert not saucers
assert collider.score == 0
assert len(asteroids) == 2
Fails. Fix:
def interact_with_saucer(self, saucer, fleets):
self.split_or_die(fleets.asteroids)
Similarly enhance this test:
def test_asteroid_ship_does_not_score(self):
pos = Vector2(100, 100)
asteroid = Asteroid(2, pos)
asteroids = [asteroid]
ship = Ship(pos)
ships = [ship]
collider = Collider(Fleets(asteroids, [], [], [], ships))
collider.mutual_destruction(asteroid, asteroids, ship, ships)
assert not ships
assert collider.score == 0
assert len(asteroids) == 2
And fix:
def interact_with_ship(self, ship, fleets):
self.split_or_die(fleets.asteroids)
Green. Commit: asteroids now split when hit by ship or saucer. sorry.
The game was broken for about 12 minutes. Horrid!
Let’s continue our review.
class Missile:
def interact_with(self, attacker, _missiles, fleets):
attacker.interact_with_missile(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
fleets.missiles.remove(self)
def interact_with_missile(self, missile, fleets):
pass
def interact_with_saucer(self, saucer, fleets):
fleets.missiles.remove(self)
def interact_with_ship(self, ship, fleets):
fleets.missiles.remove(self)
That’s good unless we want missiles to be able to shoot down missiles. If we do want that, we should write a test. We’ll make a Jira, I kind of like the idea. But knowing I could implement it without a test, while tempting, isn’t on the table because of what just happened with saucers and asteroids.
class Saucer:
def interact_with(self, attacker, _saucers, fleets):
attacker.interact_with_saucer(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
self.explode(fleets)
def interact_with_missile(self, missile, fleets):
self.explode(fleets)
def interact_with_saucer(self, saucer, fleets):
pass
def interact_with_ship(self, ship, fleets):
self.explode(fleets)
Yep, that looks good.
class Ship:
def interact_with(self, attacker, ships, fleets):
attacker.interact_with_ship(self, fleets)
def interact_with_asteroid(self, asteroid, fleets):
self.explode(fleets.ships, fleets)
def interact_with_missile(self, missile, fleets):
self.explode(fleets.ships, fleets)
def interact_with_saucer(self, saucer, fleets):
self.explode(fleets.ships, fleets)
def interact_with_ship(self, ship, fleets):
pass
Right. We’re good as far as I can see. And we have a long article here so let’s sum up and assess. Or assess and sum up.
Summary
I had hoped to be able to convert one object at a time to the new double-dispatch scheme but that was not to be, as each new interact_with-X immediately forced everyone to deal with the new X, so it cascades until we close the loop. Didn’t really take long, and the tests mostly guided us.
EXCEPT for that saucer-ship vs asteroid bug. When I wrote those tests, the two that I enhanced above, I was thinking about scoring, and the old style of colliding was tested well enough, because we were sending destroyed_by and things were well and truly destroyed. With the double-dispatch, we needed a couple more tests and did not have them.
I’m surprised that I didn’t notice that when ship or saucer collided with an asteroid it didn’t split, but then a gorilla just walked through the room and I didn’t notice that either. What that tells us is that tests are important, multiple sets of eyes are important, double-checking is important, everything is important.
Where are we with this? Well, one thing we need to do is to stop checking range at the top and check it in the objects. Another is to fix up the code that is doing fleets.asteroids and replace with fleets.add_asteroid, and so on. Jira notes made.
We’ll review where we are next time, but we’re quite far along, with our double dispatch now decoding the specifics of what’s colliding with what. That will let us more closer to removing all that knowledge from the game. It might even work now, but I’m 2 1/2 hours in and 588 long long lines, so we’ll call it for now.
A good morning for me. And a good day to you as well. See you next time!