Python 135

Python Asteroids on GitHub

Last night, Hill observed something notable. We worked on it then, and I’ll do more now.

Apparently in a fit of boredom, Chet asked to see my Asteroids code, so I did a little walk-through of things. Along the way GeePaw Hill made an interesting observation. We were looking at this code in Saucer:

class Saucer(Flyer):
    def interact_with_missile(self, missile, fleets):
        if missile.is_saucer_missile:
            self.missile_tally += 1
        if missile.are_we_colliding(self.position, self._radius):
            fleets.add_flyer(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

I had mentioned that the Saucer needs to know how many missiles are in flight, because it can only have two in flight at once, and so this code counts saucer missiles. Part of the niceness of the decentralized design is that since an object sees all the other objects that are in flight, it can do things like count them, aim at them, whatever.

Hill pointed out, quite rightly, that the first if statement above is a type check, and wondered why there wasn’t a class SaucerMissile and an interact_with_saucermissile method in the interaction family.

We discussed it a bit and the closest I could come to a reason why I hadn’t done that came in two parts.

First, it is possible that I didn’t think of it. Missiles get created with slightly different parameters in the two class methods we use to create them:

    @classmethod
    def from_ship(cls, position, velocity):
        return cls(position, velocity, u.MISSILE_SCORE_LIST, u.SAUCER_SCORE_LIST)

    @classmethod
    def from_saucer(cls, position, velocity):
        return cls(position, velocity, [0, 0, 0], [0, 0])

So, given that I had those, I went ahead and put in the flag, more or less thoughtlessly.

But a second reason for avoiding it is that a SaucerMissile would probably be a subclass of Missile, since they have lots of code in common. And while I am not above inheriting implementation, I do lean toward doing it very sparingly in the current design. I think that the only implementation inheritance in this design is the interact_with_ methods in Flyer, which are all just pass.

Hill avowed that once I had sinned, I might as well go for it, so while the gang watched and commented, I put in SaucerMissile:

class SaucerMissile(Missile):
    def __init__(self, position, velocity, missile_score_list, saucer_score_list, _position=None, size=2):
        super().__init__(position, velocity, missile_score_list, saucer_score_list)

    def interact_with(self, attacker, fleets):
        attacker.interact_with_saucer_missile(self, fleets)

That allows Saucer to be avoid the explicit type check:

    def interact_with_missile(self, missile, fleets):
        if missile.is_saucer_missile:
            assert False
        if missile.are_we_colliding(self.position, self._radius):
            fleets.add_flyer(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

    def interact_with_saucer_missile(self, missile, fleets):
        self.missile_tally += 1
        if missile.are_we_colliding(self.position, self._radius):
            fleets.add_flyer(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

The code above is the code from last night: I don’t consider it “done”, but it works fine. We put in the assert False there to make sure no saucer missiles were creeping in as the wrong class.

N.B.

That may not be true. I do find a bug later in the article, so the repo is probably, as we say in the trade, borked. (Did you think I was going to say something else? I was, but thought better of it.)

We went around on implementation inheritance a bit, as one does, but Hill and I will never agree, because Hill insists on his position, whereas I insist on being correct, a far more reasonable position on my part, I’m sure you’ll agree.

I do agree in any case that this approach is certainly permitted in my design, since I freely permit reasonable inheritance of implementation, and I think it’s “better” because it avoids that is_saucer_missile check.

We had to change a number of tests to get to green, since they were creating Missiles where they should now be creating SaucerMissiles, and the like. None of them were interesting, just the usual cleanup that one has to do sometimes.

But there was one issue that troubles me. When I added the SaucerMissile subclass, I expected tests to fail. There is a test that is supposed to be checking all Flyer subclasses for properly implementing the interact protocol. And that test did not fail.

A smaller issue is that the convention for the interact name probably means that it should be interact_with_saucermissile, not interact_with_saucer_missile, because those tests just take the class name and covert it to lower case.

But why don’t they run? I hope to find out real soon now.

Here is the test in question:

    def test_all_interact_with_implemented_in_flyer(self):
        subclasses = Flyer.__subclasses__()
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(Flyer)
        pass_code = just_pass.__code__.co_code
        for klass in subclasses:
            name = klass.__name__.lower()
            required_method = "interact_with_" + name
            assert required_method in attributes
            interact_with_method = klass.interact_with
            interact_with_code = interact_with_method.__code__.co_code
            assert interact_with_code != pass_code, name + " has pass in interact_with"

My guess at the issue is that __subclasses__ only returns immediate subclasses, and we need sub-subclasses as well. Does Python have an __addsubclasses__? Apparently not, but Stack Overflow suggests that I do it this way:

def get_subclasses(klass):
    for subclass in klass.__subclasses__():
        yield from get_subclasses(subclass)
        yield subclass

So that’s nice. Test now fails looking for interact_with_saucermissile. Let me rename my more reasonably named substitute.

And the test passes. NOw I’ll remove my interact_with in the SaucerMissile and make sure the test detects that as well.

I do get a failure, but it’s on that assert False. I really want this test to work. Ah, but it cannot, because my superclass, Missile has the method. Can I change the test?

After more searching and fiddling:

class TestFlyer:
    def test_all_interact_with_implemented_in_flyer(self):
        subclasses = get_subclasses(Flyer)
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(Flyer)
        pass_code = just_pass.__code__.co_code
        for klass in subclasses:
            name = klass.__name__.lower()
            required_method = "interact_with_" + name
            assert required_method in attributes
            if "interact_with" in klass.__dict__:
                interact_with_method = klass.__dict__["interact_with"]
                interact_with_code = interact_with_method.__code__.co_code
                assert interact_with_code != pass_code, name + " has pass in interact_with"
            else:
                assert False, name + " does not implement `interact_with`"

I changed my method name to interact_withx and the test fails with no method present. Just for fun, I’ll name it correctly and set it to pass, though I think this is the better test: we are unlikely to implement it as pass in the class itself.

>               assert interact_with_code != pass_code, name + " has pass in interact_with"
E               AssertionError: saucermissile has pass in interact_with
E               assert b'\x97\x00d\x00S\x00' != b'\x97\x00d\x00S\x00'

So now we check that it is there and not implemented as pass. That’s a rule for our objects: they must announce themselves to others.

We are green. Commit: enhance test to find interact_with directly in the subclass and check it for not being pass.

PyCharm hates the word “saucermissile” and squiggles it for a spelling error. I save it to the dictionary, whatever that means.

This code was just cut and pasted last night, and it isn’t quite right:

    def interact_with_missile(self, missile, fleets):
        if missile.are_we_colliding(self.position, self._radius):
            fleets.add_flyer(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

    def interact_with_saucermissile(self, missile, fleets):
        self.missile_tally += 1
        if missile.are_we_colliding(self.position, self._radius):
            fleets.add_flyer(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

If a saucer missile hits the saucer (which I think could not really happen) we should not bother to accumulate the score. Remove that line. Commit: do not tally score when colliding with own missile. would have been zero anyway.

I have shipped a bug! In game play, saucer missiles do not kill the ship. Stop the presses!

    def interact_with_saucermissile(self, missile, fleets):
        self.explode_if_hit(fleets, missile)

That fixes the bug. Commit: ship killed by saucer missile.

There must be a test that needs changing. No, even worse! I can find no test that verifies that the ship can be killed by a missile.

    def test_saucer_missile_kills_ship(self):
        pos = Vector2(100, 100)
        ship = Ship(pos)
        missile = SaucerMissile.from_saucer(pos, Vector2(0, 0))
        fleets = Fleets()
        fleets.add_flyer(ship)
        fleets.add_flyer(missile)
        fi = FI(fleets)
        assert fi.saucer_missiles
        assert fi.ships
        fleets.perform_interactions()
        assert not fi.saucer_missiles
        assert not fi.ships
        assert fi.explosions

Now “obviously” this bug can never arise again because no damn fool is going to remove that method from Ship. But the test is easy to write and should have been there all along. Ideally, some will tell you, I should have written the test first, to show that the bug was what I thought it was, and only then fixed it. I can only say in my defense that I panicked and wanted the bug gone ASAP.

Commit: add test for saucer missile vs ship.

I think we’re nearly good here.

Review and Comment

Hill’s observation was good. I would rephrase it something like this:

The program has a type-identifying capability in the interact_with-x methods. but the check for is_saucer_missile is a type check that does not use that same mechanism. Shouldn’t the program use the standard scheme throughout?

I am a bit troubled by inheriting all that code from Missile into SaucerMissile. I don’t mind all those pass methods being inherited at all, but inheriting masses of code troubles me a bit. I could win a debate that if you’re going to allow inheritance of concrete methods at all, this is perfectly righteous, but it does trouble me nonetheless.

I think it is probably the second-best solution we could have. What might be better? It might be better to have a new object, say, GenericMissile, and for both Missile and SaucerMissile to delegate to that object to do all the work. Then again, it might not be better. We’d have to try it to be sure, and at this writing I don’t think I’d prefer it enough to induce me to try it.

There are now some methods that say saucermissile, which is really required by the conventions imposed in the test above that inspects the subclasses. And thee are some that say saucer_missile. Let’s root those out and fix them.

class Missile(Flyer):
    Saucer = None
    radius = 2

    def __init__(self, position, velocity, missile_score_list, saucer_score_list):
        self.score_list = missile_score_list
        if missile_score_list[0] == 0:
            self.is_ship_missile = False
            self.is_saucer_missile = True
        else:
            self.is_ship_missile = True
            self.is_saucer_missile = False
        self._timer = Timer(u.MISSILE_LIFETIME)
        self._saucer_score_list = saucer_score_list
        self._location = MovableLocation(position, velocity)

Can we eliminate the use of these two members? This test can certainly go.

    def test_missiles_know_type(self):
        ship_missile = Missile.from_ship(u.CENTER, Vector2(0, 0))
        assert ship_missile.is_ship_missile
        assert not ship_missile.is_saucer_missile
        saucer_missile = Missile.from_saucer(u.CENTER, Vector2(0, 0))
        assert not saucer_missile.is_ship_missile
        assert saucer_missile.is_saucer_missile

This

class Ship(Flyer):
    def interact_with_missile(self, missile, fleets):
        self.tally_ship_missiles(missile)
        self.explode_if_hit(fleets, missile)

    def tally_ship_missiles(self, missile):
        if missile.is_ship_missile:
            self._missile_tally += 1

This no longer needs to check the type.

    def interact_with_missile(self, missile, fleets):
        self._missile_tally += 1
        self.explode_if_hit(fleets, missile)

That seems to break a test. The test is wrong:

    def test_ship_counts_missiles(self):
        fleets = Fleets()
        ship = Ship(u.CENTER)
        ship.begin_interactions(fleets)
        m_ship = Missile.from_ship(Vector2(0, 0), Vector2(0, 0))
        m_saucer = SaucerMissile.from_saucer(Vector2(0, 0), Vector2(0, 0))
        ship.interact_with_missile(m_ship, fleets)
        ship.interact_with_missile(m_saucer, fleets)
        assert ship._missile_tally == 1

The saucer missile won’t go in that way.

    def test_ship_counts_missiles(self):
        fleets = Fleets()
        ship = Ship(u.CENTER)
        ship.begin_interactions(fleets)
        m_ship = Missile.from_ship(Vector2(0, 0), Vector2(0, 0))
        m_saucer = SaucerMissile.from_saucer(Vector2(0, 0), Vector2(0, 0))
        ship.interact_with_missile(m_ship, fleets)
        ship.interact_with_saucermissile(m_saucer, fleets)
        assert ship._missile_tally == 1

Test passes. Commit the corrected test. Now see about removing those flags.

    def __init__(self, position, velocity, missile_score_list, saucer_score_list):
        self.score_list = missile_score_list
        self._timer = Timer(u.MISSILE_LIFETIME)
        self._saucer_score_list = saucer_score_list
        self._location = MovableLocation(position, velocity)

Green. Commit: remove is_ flags from Missile.

I am troubled by these class methods:

    @classmethod
    def from_ship(cls, position, velocity):
        return cls(position, velocity, u.MISSILE_SCORE_LIST, u.SAUCER_SCORE_LIST)

    @classmethod
    def from_saucer(cls, position, velocity):
        return cls(position, velocity, [0, 0, 0], [0, 0])

Ah, I know why. I should move the second one down to SaucerMissile, so that you cannot say Missile.from_saucer at all. I find this test failing:

    def test_missile_v_missile(self):
        pos = Vector2(100, 100)
        zero_vel = Vector2(0, 0)
        m1 = Missile.from_ship(pos, zero_vel)
        m2 = SaucerMissile.from_saucer(pos, zero_vel)
        fleets = Fleets()
        fi = FI(fleets)
        fleets.add_flyer(m1)
        fleets.add_flyer(m2)
        interactor = Interactor(fleets)
        interactor.perform_interactions()
        assert not fi.missiles
        assert not fi.saucer_missiles

Even after I change the missile type. We need both interactions in Missile, all deadly.

class Missile(Flyer):
    def interact_with_missile(self, missile, fleets):
        if missile.are_we_colliding(self.position, self.radius):
            self.die(fleets)

    def interact_with_saucermissile(self, missile, fleets):
        if missile.are_we_colliding(self.position, self.radius):
            self.die(fleets)

That makes the test pass. There is another:

    def test_missile_scoring(self):
        p = Vector2(12, 34)
        v = Vector2(56, 78)
        ship_missile = Missile.from_ship(p, v)
        assert ship_missile.score_list == u.MISSILE_SCORE_LIST
        saucer_missile = SaucerMissile.from_saucer(p, v)
        assert saucer_missile.score_list == [0, 0, 0]

That passes. Commit: fix two tests to properly use SaucerMissile.from_saucer.

OK, I am pretty satisfied for now.

Summary

Again, Hill’s observation was a good one. Implementing it was a bit intricate: it has taken ten commits to get everything sorted to my liking, and the change did involve shipping a buggy version for a few minutes.

I very much hate when that happens. My tests are quite solid, and I have come to rely on them, but when there’s a hole in the net, things can slip through. I think I’ve plugged a couple more of them. The lesson, of course, is what we have always known, no finite amount of testing can ever guarantee that a program works. We always have to be careful, to look in the crevices, to check and double check … and still, sometimes, we’ll fail.

But I do hate it. And I write about it because it’s the truth. We are not perfect. Well, I am not perfect and I know how I’d bet about you.

Furthermore, I suspect that there may be other cracks and crevices around this

Breaking saucer missiles out as a separate type is the right thing to do in this design, I believe, because the model is all these different kinds of things interacting, and once we have that model we should stick to it.

But I do have more discomfort inheriting real code rather than just the pass from the defaults in Flyer.

Of course, what we could do would be to implement saucer missile by duplicating Missile code and having SaucerMissile inherit from Flyer. Then we’d see if we wanted to do something about the duplication.

I kind of wish I hadn’t thought of that. It might be better.

But not today. Today has had more than enough in it to satisfy me for now.

Added in Post

Oh hell. Saucer missiles vs asteroids! There seem to be no tests for missiles destroying asteroids, of either kind.

I have to do that now, sorry. I’ll spare you the code.

Grrr. This easy change, “just adding a subclass” was harder than it looked. And it has increased code volume. I need to think about that. Next time.

See you next time!