Python 133

Python Asteroids on GitHub

Let’s go after Fleets and Fleet. There’s too much there for what we need. And, perhaps, too little. Discovery: I like it!

In an early version, our game had the Game object owning a number of instances of Fleet, each containing one kind of Flyer, such as asteroids, or missiles. Later, we moved those individual collections into a Fleets object, containing the multiple instances of Fleet. In our current design, Fleets only contains one Fleet instance, because from the viewpoint of the game, all our Flyers are alike. It’s only in their individual interactions that the behavior we see as the Asteroids Game¹ emerges.

As such, it seems to me that we have a lot more mechanism than we need in the Fleets and Fleet class. I do very much favor covering native collections, but a cover on a cover seems like more than we need. So let’s take a look.

We’ll start with a quick look at the Fleets class’s use of Fleet, then dive into Fleet to see if we can get rid of it, then back to Fleets. Of course, what we find during this inspection may change what we do. I do think the plan is to replace Fleet with a vanilla collection, but we’ll see what we see.

My overall objective here is “less code”. I am sure we could find something more efficient than what we have, but the game efficiency seems good enough, so smaller, clear code seems to me to be the priority.

Fleets creates just one Fleet, and stuffs everything into it:

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = Fleet([])
        for asteroid in asteroids:
            self.add_flyer(asteroid)
        for missile in missiles:
            self.add_flyer(missile)
        for saucer in saucers:
            self.add_flyer(saucer)
        for saucer_missile in saucer_missiles:
            self.add_flyer(saucer_missile)
        for ship in ships:

    def add_flyer(self, flyer):
        self.flyers.append(flyer)

    def remove_flyer(self, flyer):
        self.flyers.remove(flyer)

There’s more, but that gives us the drift of things. I also notice these two methods:

    @staticmethod
    def beat1():
        player.play("beat1")

    @staticmethod
    def beat2():
        player.play("beat2")

We have the Thumper object now. These are redundant, unnecessary, and unused². Delete them. Commit: remove unused beats.

Now let’s take a first look at Fleet.

class Fleet:
    def __init__(self, flyers):
        self.flyers = flyers

    def __bool__(self):
        return bool(self.flyers)

    def __iter__(self):
        return self.flyers.copy().__iter__()

    def __len__(self):
        return len(self.flyers)

    def __getitem__(self, item):
        return self.flyers[item]

    def append(self, flyer):
        self.flyers.append(flyer)

    def clear(self):
        self.flyers.clear()

    def extend(self, list_of_flyers):
        self.flyers.extend(list_of_flyers)

    def remove(self, flyer):
        if flyer in self.flyers:
            self.flyers.remove(flyer)

    def draw(self, screen):
        for flyer in self:
            flyer.draw(screen)

    def move(self, delta_time, fleets):
        for flyer in self:
            flyer.update(delta_time, fleets)

    def tick(self, delta_time, fleets):
        for flyer in self:
            flyer.tick(delta_time, fleets)

That’s the whole thing. The most interesting bits, I think, are the dunder methods:

    def __bool__(self):
        return bool(self.flyers)

This dunder method allows us to say, to a given Fleet instance,

    if this_fleet:

And this_fleet will return True if if self.flyers: would return true, otherwise False. And since flyers is a list, __bool__ makes a Fleet return True if it has contents, False otherwise. I don’t think we use this facility any more.

The __iter__ dunder method makes Fleet act like a list with regard to use as an iterator for flyer in this_fleet:. __len__ lets it respond to the len call like a list would. And __getitem__ lets us subscript into a Fleet as we would a list.

As far as these methods are concerned, it seems to me that if we replaced our single Fleet instance with a plain list, it would work the same as it does now, with one less level of indirection.

Of the other methods, append, clear, extend, and remove are just methods we would have on a list, so those are fine. The draw, move, and tick methods are special, and Fleets does use them:

class Fleets:
    def draw(self, screen):
        self.flyers.draw(screen)

    def move(self, delta_time):
        self.flyers.move(delta_time, self)

    def tick(self, delta_time):
        self.flyers.tick(delta_time, self)

I do not believe that Fleet is carrying enough weight to justify its existence. Let’s see about converting Fleets to just hold on to a simple list.

Fleets already has a property that we’ll find useful here:

class Fleets:
    @property
    def all_objects(self):
        return self.flyers

That just returns our single Fleet instance, but all the uses of it seem to treat it like a simple iterable list, either using for or in on it.

Let’s try changing that property to return the inner list rather than the Fleet instance.

    @property
    def all_objects(self):
        return self.flyers.flyers

That code exposes the list inside a Fleet. My tests all run. Now let’s look at the calls to those special methods, draw, move, and tick:

    def draw(self, screen):
        self.flyers.draw(screen)

    def move(self, delta_time):
        self.flyers.move(delta_time, self)

    def tick(self, delta_time):
        self.flyers.tick(delta_time, self)

If we just iterate here, we can remove those:

    def draw(self, screen):
        for flyer in self.all_objects:
            flyer.draw(screen)

    def move(self, delta_time):
        for flyer in self.all_objects:
            flyer.move(delta_time, self)

    def tick(self, delta_time):
        for flyer in self.all_objects:
            flyer.tick(delta_time, self)

Tests are green. I will try removing those methods from Fleet. Test pass. Try the game just to be sure.

Hmm, interesting. I get this error:

  File "/Users/ron/PycharmProjects/firstGo/game.py", line 64, in asteroids_tick
    self.fleets.move(delta_time)
  File "/Users/ron/PycharmProjects/firstGo/fleets.py", line 52, in move
    flyer.move(delta_time, self)
    ^^^^^^^^^^
AttributeError: 'Coin' object has no attribute 'move'

I’ll bet that was there before I removed those methods from Fleet. Indeed. Roll back, do again.

I think this may have been caused when I changed this method, so I’ll do it again:

    @property
    def all_objects(self):
        return self.flyers.flyers

Try the game. It works. So that is OK.

Oh. The move in Fleets calls update in Fleet. I didn’t inline it correctly. But update is the word we really want, I think.

Game calls move and it should call update. We’ll ³ to that in a moment.

First make the change we made before, only correctly.

    def draw(self, screen):
        for flyer in self.all_objects:
            flyer.draw(screen)

    def move(self, delta_time):
        for flyer in self.all_objects:
            flyer.update(delta_time, self)  # <=== weird

    def tick(self, delta_time):
        for flyer in self.all_objects:
            flyer.tick(delta_time, self)

Green. Test game. We’re good. Remove the unused methods in Fleet. Still good. Commit: remove draw, move and tick.

Why are we committing these changes to Fleet? We know we’re going to remove it. Aren’t we just wasting valuable time and Git space?

On a real product-oriented team. we might only have time for this much improvement as we pass through Fleets and Fleet on some other mission. This commit was possibly too large: after all, we could have done each of draw, move, and tick separately.

It really only takes moments to do the commit, and it allows us to use available time and ideas efficiently, making small changes as we see them. It would allow us to spread the removal of Fleet over days or weeks, if we needed to, improving it, carving away at it, over time.

Small steps. Personally, I can recall no time when I wished I had taken a larger step, and many many times when I can recall taking too large a step. A few paragraphs above, for example.

We are green. I want to just try replacing our Fleet instance with a list, as soon as possible, so that my tests and, yes, trying the game, can lead me to the necessary changes.

Since Fleet now only has the dunder methods, plus append, clear, extend, and remove, I think we will find that this works:

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = []
        ...

So much for my thinking: 40 tests break. I chuckle. What are they? I hope they are tests for Fleet. When I change this:

    @property
    def all_objects(self):
        return self.flyers

(Recall that that used to say self.flyers.flyers) 39 tests suddenly start running again. One fails. Ah yes … it’s a remove of a thing that isn’t there. The Fleet object does this:

    def remove(self, flyer):
        if flyer in self.flyers:
            self.flyers.remove(flyer)

Why, oh why, does Python insist that removing something that isn’t there is an error? Oh, wait, I just thought of something.

This is in Fleet:

class Fleet:
    def __iter__(self):
        return self.flyers.copy().__iter__()

We are allowed to remove things from our lists during iteration. Will putting the copy call in fix this test?

    @property
    def all_objects(self):
        return self.flyers.copy()

It does not, but we need that. And we do not have a test for it. Let’s write a naive one. Remove the copy so that our new test will fail.

    def test_copies_all_objects(self):
        fleets = Fleets()
        assert fleets.all_objects is not fleets.flyers

Just a test to force me to put the copy() back. With it back, that test runs.

We still need a safe remove.

    def remove_flyer(self, flyer):
        if flyer in self.flyers:
            self.flyers.remove(flyer)

We’re green. Test in game. Good. Commit: Fleets no longer uses Fleet.

Now let’s see about removing Fleet. Are there references? Only the import in Fleets. We remove that. Then the module. Test again. Commit: Remove Fleet class.

We’re at our usual finishing length, maybe we should stop. Let’s quickly review Fleets to see what we can see.

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = []
        for asteroid in asteroids:
            self.add_flyer(asteroid)
        for missile in missiles:
            self.add_flyer(missile)
        for saucer in saucers:
            self.add_flyer(saucer)
        for saucer_missile in saucer_missiles:
            self.add_flyer(saucer_missile)
        for ship in ships:
            self.add_flyer(ship)

    @property
    def all_objects(self):
        return self.flyers.copy()

    @property
    def _asteroids(self):
        return self.select(lambda a: isinstance(a, Asteroid))

    @property
    def _ships(self):
        return self.select(lambda s: isinstance(s, Ship))

    # adds and removes

    def add_flyer(self, flyer):
        self.flyers.append(flyer)

    def remove_flyer(self, flyer):
        if flyer in self.flyers:
            self.flyers.remove(flyer)

    def clear(self):
        self.flyers.clear()

    def draw(self, screen):
        for flyer in self.all_objects:
            flyer.draw(screen)

    def move(self, delta_time):
        for flyer in self.all_objects:
            flyer.update(delta_time, self)

    def select(self, condition):
        return [flyer for flyer in self.all_objects if condition(flyer)]

    def tick(self, delta_time):
        for flyer in self.all_objects:
            flyer.tick(delta_time, self)

    def begin_interactions(self):
        for flyer in self.all_objects:
            flyer.begin_interactions(self)

    def end_interactions(self):
        for flyer in self.all_objects:
            flyer.end_interactions(self)

Oh, yes, things to see here. We can stop iterating in __init__. The first way is like this

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = []
        self.flyers.extend(asteroids)
        self.flyers.extend(missiles)
        self.flyers.extend(saucers)
        self.flyers.extend(saucer_missiles)
        self.flyers.extend(ships)

But there is another way.

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = list(chain(asteroids, missiles, saucers, saucer_missiles, ships))

The itertools.chain is an iterable that spans all the input collections. Of course in actual game use, we are adding nothing. We only accept those collections to simplify tests. Let’s check those tests. I’d like to be rid of this oddity.

Here’s one:

    def test_len_etc(self):
        saucer_missiles = []
        fleets = Fleets([], [], [], saucer_missiles, [])
        fi = FI(fleets)
        assert len(fi.saucer_missiles) == 0
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(0, 0)))
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(20, 20)))
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(30, 30)))
        assert len(fi.saucer_missiles) == 3
        assert fi.saucer_missiles[1]._location.velocity.x == 20

Let’s just not do that. We’re just adding an empty collection. We did that because an earlier version of the test needed access to the inner collection. Now we use FI for that sort of thing:

    def test_len_etc(self):
        fleets = Fleets()
        fi = FI(fleets)
        assert len(fi.saucer_missiles) == 0
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(0, 0)))
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(20, 20)))
        fleets.add_flyer(Missile.from_saucer(Vector2(0, 0), Vector2(30, 30)))
        assert len(fi.saucer_missiles) == 3
        assert fi.saucer_missiles[1]._location.velocity.x == 20

I’ll look at the tests that are like that one and show you interesting ones. I expect none.

After using search for a while, I think of a simpler way to find the offenders. I’ll change the __init__ and see who doesn’t work any more. More efficient.

I do this:

class Fleets:
    def __init__(self, asteroids=(), missiles=(), saucers=(), saucer_missiles=(), ships=()):
        self.flyers = list()

All the tests run. Did I in fact find them all already? No. Some tests are not robust enough to fail under these circumstances. Still none of the tests were interesting, they all are the same as the one above, where I just create a vanilla Fleets and then add_flyer as needed. I think all creators of Fleets are now passing no parameters.

class Fleets:
    def __init__(self):
        self.flyers = list()

Green as my Captain Midnight Secret Decoder Ring. Test game. Not that we need to, we know that Game doesn’t pass any collections. Commit: Fleets no longer accepts starting collections.

What about those private properties:

    @property
    def _asteroids(self):
        return self.select(lambda a: isinstance(a, Asteroid))

    @property
    def _ships(self):
        return self.select(lambda s: isinstance(s, Ship))

Any takers? Nope. Remove them. Commit: remove private properties. Fleets is pretty lean and mean now. But there’s one more thing that seems odd:

class Fleets:
    def begin_interactions(self):
        for flyer in self.all_objects:
            flyer.begin_interactions(self)

    def end_interactions(self):
        for flyer in self.all_objects:
            flyer.end_interactions(self)

We have these two, but we do not have the thing in the middle. We have the class Interactor, that looks like this:

class Interactor:
    def __init__(self, fleets):
        self.fleets = fleets

    def perform_interactions(self):
        self.fleets.begin_interactions()
        self.interact_all_pairs()
        self.fleets.end_interactions()

    def interact_all_pairs(self):
        for target, attacker in self.all_pairs():
            self.interact_one_pair(target, attacker)

    def all_pairs(self):
        return itertools.combinations(self.fleets.all_objects, 2)

    def interact_one_pair(self, target, attacker):
        attacker.interact_with(target, self.fleets)
        target.interact_with(attacker, self.fleets)

I think we should probably merge Interactor and Fleets. I also think that’s a bit more than I want to undertake this morning. I intended to be done 200 lines ago.

Interactor has about 25 tests, so unwinding it might be tricky. But it’ll be worth it, I think. Right now the Game knows about Fleets and about Interactor:

class Game:
    def perform_interactions(self):
        Interactor(self.fleets).perform_interactions()

You know what? We could fix that by giving Fleets a perform_interactions and that would hide Interactions from the Game. That’s easy, we’ll do that now:

class Fleets:
    def perform_interactions(self):
        Interactor(self).perform_interactions()

And:

class Game:
    def perform_interactions(self):
        self.fleets.perform_interactions()

Good to go. Commit: Move Interactor creation inside Fleets. Game no longer knows Interactor.

That will do for this morning. We have more good results than anticipated. Let’s sum up.

Summary

We have removed one entire class, Fleet, converting Fleets from covering a Fleet that covered a list, to just covering the list itself. We’ve removed unneeded methods from Fleets, and by adding one method to it, removed all knowledge of Interactor from Game.

Game, I believe, now only knows about Fleets and Coin from our code. This may seem like a small change. After all, Game only ever sent one message to Interactor. Yes, but Interactor is a major player in how things work and that means that, in principle, Game knew more than it needed to about how things work. Now it knows less, and that’s a good thing.

As happens to me Every Single Time, I start out with a simple mission like “do something about Fleets and Fleet”, and in doing what needs to be done, I discover opportunities that I wasn’t aware of, resulting, almost always, in a better result than I set out to attain, accomplished differently from how I thought I would do it.

I love that feeling of discovery.

Decades ago, full of all the reading I’ve done on programming and design, I think I believed that with enough design, we could know just what needed to be done and how to do it. I no longer believe that. I think that with a good enough design we can know roughly what needs to be done, and roughly how we should do it, and that the code is larger than even our brains the size of planets can encompass, and that we discover most of what really needs to be done.

What that implies is that the code needs to support discovery. It needs to be all those buzzwords, modular, cohesive, SOLID, blah blah. It needs to consist of small pieces, each doing a thing, able to be arranged and extended as we need—as we discover that we need.

This program is easy to change and getting easier. That’s a very good thing. Most programs in my experience are hard to change and getting harder.

I am pleased. I’m almost sorry the game is so close to done-done, because it is so ready and willing to be modified. Yum.

See you next time!