Python 140

Python Asteroids on GitHub

SaucerMissile must go: it is not bearing enough weight for the trouble it causes.

In my morning fretting before getting up, I was first thinking about this general decentralized design and it ability to be easily understood, in terms of Beck’s dictum that the code should express all our ideas about it. I am still not sure about that.

Then, perhaps after a short interlude of additional sleep, I began to think about SaucerMissile, whose installation made sense at the time. The issue began during last week’s Zoom meeting, when Hill noticed some code that said:

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)

We all agreed that this was a type check, and that the design we have manages types by a double dispatch, and that if there were two missile classes, one for ship and one for saucer, then this code need not do type checking.

This is a valid observation, in that the equivalent of that code now says this:

    def interact_with_missile(self, missile, fleets):
        if missile.are_we_colliding(self.position, self._radius):
            fleets.append(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):
            self.explode(fleets)
        pass

Now the type checking is already done by the double dispatch and we do not have the if statement. There is similar code in Ship, done the other way, because both Ship and Saucer count their missiles so as not to fire too many at once.

Now what happened was that as we chatted, I just typed in the code to provide the new class and we sort of bashed it into place. It seemed to work, although that was not really true, as subsequent changes have shown us.

Time: 0713

I am now going to check all pairs of implementations of these two methods, to see how many are actually different. There are about 16 such implementations.

Time: 0715

That didn’t take long. There are two classes that treat the two calls differently, Saucer and Ship. Each has the count in the right place to count its own missiles, and the Saucer includes code to score points if hit by a ship’s missile.

The cost of this change was quite large. To begin with, 14 classes had to implement the new interact_with_saucermissile method to be exactly what they already had for interact_with_missile. And even the two that do use it have code that is duplicated in time or space, as you can see in the example above.

In addition, the change was implicated in the release of at least three defects into GitHub, although of course the root cause was the failures of the exploitative capitalist system and the rising tide of electric vehicles, and, I suppose, partly due to my having fewer tests than I might have had and, just guessing here, I might have been careless.

I conclude, this morning, that SaucerMissile is not bearing its weight. I agree that a type check is undesirable, but I do not agree that the best response to the apparent type checking in Saucer and Ship justified changing 14 other classes and releasing three defects.

Ergo SaucerMissile Delenda Est¹

We cannot just roll back to last Tuesday. We have added some tests, and for all I know, useful features. We must go forward. We shall do so judiciously, calmly, and with extreme prejudice.

We are fortunate in that there is only one use of SaucerMissile class in the game, though there are others in tests. Here is SaucerMissile:

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

    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_saucermissile(self, fleets)

The sole use of this class in the game is this:

class Gunner:
    def missile_at_angle(self, position, desired_angle, velocity_adjustment):
        missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(desired_angle) + velocity_adjustment
        offset = Vector2(2 * self._radius, 0).rotate(desired_angle)
        return SaucerMissile.from_saucer(position + offset, missile_velocity)

So if we move the from_saucer class method back up into Missile, and change that return to refer to Missile, we’ll no longer be using the class in the game. Let’s try it.

I am happy to see that five tests fail. Let’s see what they are.

    def test_fire_on_time(self):
        delta_time = u.SAUCER_MISSILE_DELAY
        ship_position = Vector2(1, 1)
        ship = Ship(ship_position)
        fleets = Fleets()
        Gunner().fire(delta_time, Saucer(), ship, fleets)
        assert FI(fleets).saucer_missiles

This test should be checking for missiles now. Doubtless there are others. Changing this one to check FI.missiles makes it go. Three similar changes get us down to one failure. The final failure is a little more interesting:

    def test_can_only_fire_two(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        saucer.fire_if_possible(delta_time=0.1, fleets=fleets)
        assert not fi.saucer_missiles
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.saucer_missiles) == 1
        saucer.begin_interactions(fleets)
        for m in fi.saucer_missiles:
            saucer.interact_with_saucermissile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.saucer_missiles) == 2
        saucer.begin_interactions(fleets)
        for m in fi.saucer_missiles:
            saucer.interact_with_saucermissile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.saucer_missiles) == 2

This one is sending interact_with_saucermissile, which of course we will be removing. We can certainly change all the saucer stuff out but let’s think a bit harder here. After we make this change, all missiles will be alike. So if we were to add in an interaction with a missile from the ship, which should not interfere with the count, that might be a better test. First let’s make it run.

    def test_can_only_fire_two(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        saucer.fire_if_possible(delta_time=0.1, fleets=fleets)
        assert not fi.missiles
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 1
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_saucermissile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_saucermissile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2

At this point, sending interact_with_saucer_missile, the test runs. But the game will no longer have SaucerMissile instances, so these calls should be changed to interact_with_missile. That will break. And it does. Let’s modify saucer so that it passes, but first, let’s make it harder.

    def test_can_only_fire_two(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        ship_missile = Missile.from_ship(Vector2(0, 0), Vector2(0, 0))
        saucer.fire_if_possible(delta_time=0.1, fleets=fleets)
        assert not fi.missiles
        fleets.append(ship_missile)  # <===
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 1
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_missile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_missile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2

Perfect, the test fails. Let’s look into saucer and fix it.

N.B.

Not quite perfect, as we’ll see below. But a decent step nonetheless.

    def interact_with_missile(self, missile, fleets):
        if missile.are_we_colliding(self.position, self._radius):
            fleets.append(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):
            self.explode(fleets)
        pass

Here we need to tally correctly in interact_with_missile. I’m not sure about the score but I think it should continue to work. I code this:

    def interact_with_missile(self, missile, fleets):
        self.missile_tally += missile.saucer_tally
        if missile.are_we_colliding(self.position, self._radius):
            fleets.append(Score(self.score_for_hitting(missile)))
            self.explode(fleets)

Where did the type check go? I plan to get rid of it by having two data fields in Missile, saucer_tally and ship_tally, like this:

class Missile(Flyer):
    @classmethod
    def from_saucer(cls, position, velocity):
        return cls(position, velocity, [0, 0, 0], [0, 0], False)

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

    def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
        if from_ship:
            self.saucer_tally = 0
            self.ship_tally = 1
        else:
            self.saucer_tally = 1
            self.ship_tally = 0
        self.score_list = missile_score_list
        self._timer = Timer(u.MISSILE_LIFETIME)
        self._saucer_score_list = saucer_score_list
        self._location = MovableLocation(position, velocity)

I’ve added a parameter T/F to the init, which inits the two tally values to 0, 1 or 1, 0 as needed. There are now some tests failing, which disappoints me. Oh. They are referring to SaucerMissile and should not do that. Most of the changes are fixed by changing to Missile and checking missiles. This one continues to fail:

Oh, and it should. I added a missile but didn’t change the numbers.

N.B.

See? Here is where I finally get the test right. But the change is easy.

    def test_can_only_fire_two(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        ship_missile = Missile.from_ship(Vector2(0, 0), Vector2(0, 0))
        saucer.fire_if_possible(delta_time=0.1, fleets=fleets)
        assert not fi.missiles
        fleets.append(ship_missile)  # add extra ship missile
        extra = 1
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 1 + extra
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_missile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2 + extra
        saucer.begin_interactions(fleets)
        for m in fi.missiles:
            saucer.interact_with_missile(m, fleets)
        saucer.fire_if_possible(u.SAUCER_MISSILE_DELAY, fleets=fleets)
        assert len(fi.missiles) == 2 + extra

We are green. I do not believe things are working. In particular, I think that the ship’s missile tally is not correct. Let’s look at Ship. But where are my tests?

Let’s make sure first that no one is using SaucerMissile class or the counting method in FI.

A brief delay ensues.

I think I’m there. I’ve removed the saucer_missiles property from FleetsInspector, and there are no references to the class SaucerMissile, and the tests are green. I delete the class definition and tests stay green. But I am certain there is a bug. The bug is that the ship will not be able to fire all four missiles while the saucer missiles are flying. I want to test this in game but we will write a test for it as well.

Let’s do the right thing and write the test first. It’ll be like the one above.

Here’s the code with the defect in it:

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

Both saucer and ship missiles come through interact_with_missile now, so they’ll both count against our tally. This code needs to refer to the missile’s ship_tally. We won’t change it yet, let’s get the test.

To my everlasting shame, we do not even have a test_ship.py at all. There are very few tests for Ship and they are elsewhere. Most are in test_asteroids, which refers to the game, not to the object.

I write this new test:

    def test_can_fire_four_missiles_even_with_saucer_firing(self):
        ship = Ship(Vector2(100, 100))
        fleets = Fleets()
        fleets.append(ship)
        fi = FI(fleets)
        for x in range(2):
            fleets.append(Missile.from_saucer(Vector2(200 + 10*x, 200), Vector2(0, 0)))
        for x in range(3):
            fleets.append(Missile.from_ship(Vector2(300+ 10*x, 200), Vector2(0, 0)))
        assert len(fi.missiles) == 5
        fleets.perform_interactions()
        ship.fire_if_possible(fleets)
        assert len(fi.missiles) == 6

I am fairly happy with this test. It fakes in 2 saucer missiles and 3 ship missiles, runs the interactions so that the ship (mis)counts the missiles, then tries to fire one more. If the ship counted correctly, the test would run.

The test is red. Now we fix the code:

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

The test is green. SaucerMissile class is gone, but its traces are not. Before I go after the traces, I want to try the game. I am not absolutely confident. I’m about 90 or 95.

Super! The game seems quite solid. Now let’s find all those implementations of interact_with_saucermissile and rip them out.

When I remove all those, including the abstract definition, some tests fail. Let’s see why. One, test_flyer_protocol was explicitly looking for that method among others. Remove it from the list.

Two tests were still calling the interact_with_saucermissile method. I must have missed them when I searched them out. Change them to call interact_with_missile. They go green.

It is 0841 and I started the actual work on this about 0715. So, including writing and such, 90 minutes to remove the class. And 16 objects no longer include three or more useless lines of code, no longer include interact_with_saucermissile at all, no longer have to think about saucer missiles. Ship and Saucer think about them. The missiles are distinguished from each other by what they contain, not what class they are:

class Missile(Flyer):
    @classmethod
    def from_saucer(cls, position, velocity):
        return cls(position, velocity, [0, 0, 0], [0, 0], False)

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

    def __init__(self, position, velocity, missile_score_list, saucer_score_list, from_ship):
        if from_ship:
            self.saucer_tally = 0
            self.ship_tally = 1
        else:
            self.saucer_tally = 1
            self.ship_tally = 0
        self.score_list = missile_score_list
        self._timer = Timer(u.MISSILE_LIFETIME)
        self._saucer_score_list = saucer_score_list
        self._location = MovableLocation(position, velocity)

The missiles are distinguished by their score tables: the ship missiles get non-zero scores, and by their ship_tally, saucer_tally values: 1 for whichever object fired them and zero for the other.

Is that still an implicit type check? I would argue that it’s a bit interesting, a bit odd, but that if they had not seen the is_saucer_missile in the prior code, most folks would not call it a type check, and few if any would call for a special subclass to get rid of the 1,0 things.

If they did, we now know enough to be inclined to decline. Adding that class cost us a few days of work, three or four nearly-shipped defects, and changes to 16 different classes, all to avoid two if statements. Even in the objects that cared, all two of them, the change made for more code than we originally had.

The good news? We may have learned some things.

Results

The code is smaller and simpler now than with the added class. That’s a net gain. We could “regret” putting the class in at all, but in fact we have learned some interesting things, which are related but worth identifying.

Insufficient testing

It looks to me as if I built fewer tests than I needed toward the beginning of this effort, or had fewer already in place than I needed with this scheme, and then began to do better testing as time passed. The result was that there were not sufficient tests to find all the defects that adding the new class triggered. Of course, I never have enough tests, and never have just the right ones, and I suspect that it’s always a bit of a struggle for most of us to be sure to have what we need.

A truly obsessive person might be able to live by the rule “Never write a line of code that isn’t required by a failing test”, but I’m not that person, and I think most of the people I know are not that person. So I expect always to want to bear down on testing. However …

Higher-level testing

This scheme has the property that quite often, to write a decent test involves creating a Fleets instance, populating it, and then executing either a series of specific interact_with calls, or fleets.perform_interactions. This is necessary because the state of the game is distributed, decentralized. The Game state is the state of all the objects in the game, and the Fleets instance is where all that is retained.

It is possible that a London-school tester — I am Detroit² school all the way — would find these objects easier to test using test doubles, which I generally eschew³ whenever possible

Increasing cost of change

Perhaps the most important learning, which I would say we knew but didn’t really know⁴, is that the cost of adding a class with a required abstract method is increasingly high every time we add such a class. This may drive us to split the hierarchy along the lines of Actors and Audience, Flyers and Observers, or something, so that most of the invisible classes can optionally implement methods instead of being required to.

Need to think harder

We are learning some important drawbacks of this “decentralized” design. It has a certain elegance, but it also has what seems like additional complexity because we have to think about all the interactions of all the objects. Now, the truth is, each of those interactions is actually a game requirement, so we always have to think about them, but perhaps some of them would be easier to think about if we didn’t create a special object to handle it.

Tendency to over-use the capability

Perhaps the simplest example of over-use is Explosion. We create an Explosion object, toss it into the mix, and on the next clock tick, the Explosion adds a bunch of Fragment instances and destroys itself. Wherever we do that, we have access to the Fleets object, so why not just add the darn Fragments and not have Explosion.

I think I did Explosion because it was fun to do it that way, and had a certain kind of, I don’t know, character, that appealed to me. But was it a good idea? Quite possibly not.

Expression of ideas (Beck’s rules)

It seems to me that what seems like a simple requirement, “ship can have four missiles running, saucer can have two” does not show up readily in this code. It’s not awful, but I am coming to question whether this scheme may influence me toward code that is less clear than I might produce with another approach.

Summary

Since our work in programming is all about changing code, any work we do from which we learn has value. So this exercise, from articles 135 through 140, has been of significant value.

I am curious to find out what I do next. If you are as well, please join me here next time!