Python 58 - Add More Simplicity

Python Asteroids on GitHub

There isn’t enough simplicity in here yet. Let’s add some more.

So far, we’ve added simplicity to Game by giving it a new semi-smart object, SpaceObjects, that contains all the individual groups of space objects, asteroids, missiles, and all that. The main saving so far was just that we removed five member collections from the Game class. Let’s start applying some leverage.

I think there’s an easy one to begin with. We have this code:

class Game:
    def draw_everything(self):
        screen = self.screen
        screen.fill("midnightblue")
        for saucer in self.saucers:
            saucer.draw(screen)
        for missile in self.saucer_missiles:
            missile.draw(screen)
        for ship in self.ships:
            ship.draw(screen)
        for asteroid in self.asteroids:
            asteroid.draw(screen)
        for missile in self.missiles:
            missile.draw(screen)
        self.draw_score()
        self.draw_available_ships()

We see some duplication here, about five for loops. We can reduce that by asking our SpaceObjects instance to draw itself:

    def draw_everything(self):
        screen = self.screen
        screen.fill("midnightblue")
        self.space_objects.draw(screen)
        self.draw_score()
        self.draw_available_ships()

Of course, SpaceObjects doesn’t know how to do that, so we teach it:

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

The tests don’t know about drawing, so they are happy. And the game works perfectly, no surprise there. Let’s commit: Game defers space object drawing to its space_objects collection.

You’ll have noticed that I used two new words in the code above, “fleet” and “flyer”. I was thinking this morning about what I might do today, and it was clear to me that if the current lists of space objects was a smarter object, I could defer draw down to that object rather than have this nested loop. And having used “space_objects” already, I was trying to think of a good word for the collection of a specific kind of space object. The word “fleet” came to me. And, just now, writing the loop, I used that word and then, on the fly, as it were, decided to call the things in the fleet “flyers”.

Let’s do a little renaming in SpaceObjects class.

class SpaceObjects:
    def __init__(self, asteroids, missiles, saucers, saucer_missiles, ships):
        self.fleets = (asteroids, missiles, saucers, saucer_missiles, ships)

PyCharm has changed a lot of things for me here, but the tests are good and a quick check confirms that the game works. I think I’d like to rename the class to Fleets. PyCharm does that with a quick keystroke and even renames the file for me. I like these JetBrains people.

Commit: rename SpaceObjects to Fleets, member variable is fleets.

Let’s reflect and plan.

ReflectoPlan

We see immediately why covering collection classes pays off so often. We’ve reduced ten lines of code to about four, and we’ve made it less necessary for Game to even know how many different fleets we have. Good stuff.

So we have at least two ways we might go next:

1. We could continue to find places in Game to make use of the Fleets object. I’m sure we can at least improve the move_everything code, although that will require us to refactor how the saucer moves, because it currently has enough information to aim at the ship and to destroy itself, all during move. I think it’ll be easy enough, just a bit of work.

2. We could create another smart collection, Fleet, and defer draw down to it. We can foresee that we’ll surely defer move as well, and that we’ll probably defer some of the collision logic at least down to Fleets.

I think we should do #2 first. If we do, we’ll have the Fleet object in place and doing draw, and it should help us think about how best to handle move and other operations. I’m pretty sure I can get the refactoring of move right in Saucer, but with Fleet and Fleets both in place, I’ll have a bit more insight, which can’t hurt.

Fleet Object

So we want a class Fleet that holds onto one collection of asteroids, missiles, whatever. I’m wondering who knows about the Fleet. Right now, we have code that treats the individual collections with collection code, like for loops and append.

It is possible to subclass from list. That is not my usual practice: I am more inclined to cover the class, so that it only has the features I want to give it. We’ll stick with my way, but we might have to learn how to do a Python iterator along the way.

Tests?

We only have a couple of tests for the Fleets class:

class TestFleets:
    def test_creation(self):
        asteroids = ["asteroid"]
        missiles = ["missile"]
        saucers = ["saucer"]
        saucer_missiles = ["saucer_missile"]
        ships = ["ship"]
        fleets = Fleets(asteroids, missiles, saucers, saucer_missiles, ships)

    def test_access(self):
        asteroids = ["asteroid"]
        missiles = ["missile"]
        saucers = ["saucer"]
        saucer_missiles = ["saucer_missile"]
        ships = ["ship"]
        fleets = Fleets(asteroids, missiles, saucers, saucer_missiles, ships)
        assert fleets.asteroids == asteroids
        assert fleets.missiles == missiles
        assert fleets.saucers == saucers
        assert fleets.saucer_missiles == saucer_missiles
        assert fleets.ships == ships

The Fleets class has no other testable behavior as yet and I have every hope of getting rid of the accessors. The Fleet class is similar, in that in its first incarnation it’ll only know draw. Nonetheless, let’s at least honor it with a creation test.

    def test_fleet_creation(self):
        asteroids = ["asteroid"]
        fleet = Fleet(asteroids)
        assert fleet

You strict typing nerds will be concerned about my just passing a collection of strings into these tests. Now in a language like Kotlin, I’d have to at least have an interface for what the collection has in it, but the good and bad of Python is that the Fleet object doesn’t care what class its member collections contain. It is going to send them the messages that we send, and they’d better respond.

I find this a blessing. Some find it a curse. Whichever it is, this test wants me to define a Fleet class.

For some reason, PyCharm doesn’t offer to create a new class in a new module, but I don’t care, I can do it manually.

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

Not much to it. The test runs green. Commit: initial Fleet class.

Now we can change our draw in Fleets:

class Fleets:
    def draw(self, screen):
        for fleet in self.fleets:
            fleet.draw(screen)

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

Tests are green of course. But we need to use the Fleet class in the Game. I almost forgot. In fact, let’s face it, I did forget.

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

    @property
    def asteroids(self):
        return self.fleets[0].flyers

And the same for the other properties. I have three tests failing.

Ah. I’ve broken check_collisions:

class Collider:
    def check_collisions(self):
        for pair in itertools.combinations(self.space_objects.fleets, 2):
            self.check_individual_collisions(pair[0], pair[1])
        return self.score

    def check_individual_collisions(self, attackers, targets):
        for target in targets.copy():
            for attacker in attackers.copy():
                if self.mutual_destruction(target, targets, attacker, attackers):
                    break

The easiest fix here is to accept that I have fleets in the first method, and unwrap them in the second:

    def check_individual_collisions(self, attackers, targets):
        for target in targets.flyers.copy():
            for attacker in attackers.flyers.copy():
                if self.mutual_destruction(target, targets, attacker, attackers):
                    break

That fixes some things but some are still failing:

>       if self not in asteroids: return # already dead
E       TypeError: argument of type 'Fleet' is not iterable

There are two places where I use in. The “easy” fix is to make Fleet iterable. I resolve that issue by returning the iterator for self.flyers. The tests then demand two more methods on Fleet, append and remove. The resulting class is this:

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

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

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

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

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

I’m green and I expect the game to work. It does. Commit: Fleet object handles draw, append, remove, and is iterable.

Let’s reflect.

Reflection

On the bright side, we’ve removed some of Game’s knowledge about the details of our Fleets, but not yet all. We have deferred drawing to Fleets and from there to Fleet. That’s quite nice:

class Fleets:
    def draw(self, screen):
        for fleet in self.fleets:
            fleet.draw(screen)

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

We could actually change Fleet.draw to say for flyer in self, since Fleet is now iterable. Shall we do that? Yes, let’s. Works, of course Commit: Fleet.draw iterates self.

We have been bullied into making Fleet iterable. An alternative would have been to remove the checks for in and just implement append and remove but I was interested in learning how to make it iterable.

You’ll note that I finessed the entire problem of building an iterator object, because I’m just covering one collection, so it suffices to return the iterator for that collection. Think of all the brain cells I have saved for another day by not yet needing to build a “real” iterator.

This is my usual practice with collection covers. If I do need to iterate them, I’ll expose the iterator on the inner collection where that’s possible. It reduces the complexity of my code and relies on an iterator that is surely at least as robust as the one I’d create.

Now let’s see what we can do about using Fleets and Fleet to improve Game.

I’m interesting in what code is using the Fleets accessors, asteroids, missiles, and so on.

Game has its own accessors for those. We’ll see what it does with them. It seems to do a lot.

Those are all interesting. The Game knows that there need to be asteroids, and it checks for them, creates a wave timer for them, checks for safety (using other collections). It clears the asteroids when the game starts. We might be able to defer that down easily in due time. More immediately, Game does this:

    def move_everything(self, delta_time):
        for the_saucer in self.saucers.copy():
            the_saucer.move(delta_time, self.saucers, self.saucer_missiles, self.ships)
        for missile in self.saucer_missiles:
            missile.move(delta_time)
        for the_ship in self.ships:
            the_ship.move(delta_time)
        for asteroid in self.asteroids:
            asteroid.move(delta_time)
        for missile in self.missiles:
            missile.move(delta_time)

We’ve talked about this and have it in our sights to improve. If the saucer were kind enough to accept the same calling sequence as the others, we could defer this down to Fleets.

Fact is, we can defer it down there anyway. Fleets knows all the collections by name. There are no tests for move_everything. I kind of wish there were but this change is easy anyway.

Scarf up this code and replace it:

class Game:
    def move_everything(self, delta_time):
        self.space_objects.move_everything(delta_time)

class Fleets:
    def move_everything(self, delta_time):
        for the_saucer in self.saucers.copy():
            the_saucer.move(delta_time, self.saucers, self.saucer_missiles, self.ships)
        for missile in self.saucer_missiles:
            missile.move(delta_time)
        for the_ship in self.ships:
            the_ship.move(delta_time)
        for asteroid in self.asteroids:
            asteroid.move(delta_time)
        for missile in self.missiles:
            missile.move(delta_time)

I think this is problematical in the copy. self.saucers is a Fleet and copying it won’t copy the underlying collection. But we don’t need to worry about that too much: we’re going to change how this works.

We’re green and working. Commit: defer move_everything to Fleets.

Now we have to extract the special stuff from the saucer and deal with it. Let’s review Saucer.move:

class Saucer:
    def move(self, delta_time, saucers, saucer_missiles, ships):
        self.fire_if_possible(delta_time, saucer_missiles, ships)
        self.check_zigzag(delta_time)
        self.position += delta_time * self.velocity
        x = self.position.x
        if x < 0 or x > u.SCREEN_SIZE:
            if self in saucers:
                saucers.remove(self)

I want to argue that getting rid of itself is something that an object might want to do during motion, so that passing the object’s own Fleet in to it makes sense. And so does zig-zagging make sense. It’s just the firing that is an issue.

So my plan is that all the flyers will receive a second parameter in move, namely their home collection. And, for now, we’ll just disable the saucer’s firing. That’s won’t even break the firing tests, because they call fire_if_possible directly, but the game won’t fire until we fix it.

class Fleets:
    def move_everything(self, delta_time):
        for fleet in self.fleets:
            fleet.move(delta_time)

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

class Saucer:
    def move(self, delta_time, saucers):
        # self.fire_if_possible(delta_time, saucer_missiles, ships)
        self.check_zigzag(delta_time)
        self.position += delta_time * self.velocity
        x = self.position.x
        if x < 0 or x > u.SCREEN_SIZE:
            if self in saucers:
                saucers.remove(self)

class Asteroid:
    def move(self, delta_time, _asteroids):
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position

class Missile:
    def move(self, deltaTime, _missiles):
        position = self.position + self.velocity * deltaTime
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position

class Ship
    def move(self, delta_time, _ships):
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position

I expect the game to work, with broken tests. It does. Find the tests:

    def test_ship_move(self):
        ship = Ship(Vector2(50, 60))
        ship.velocity = Vector2(10, 16)
        ship.move(0.5, [ship])
        assert ship.position == Vector2(55, 68)

Just needed to add the collection. It’s ignored but I filled it in anyway.

    def test_move(self):
        saucer = Saucer()
        saucer.ready()
        starting = saucer.position
        saucer.move(delta_time=0.1, saucers=[])
        assert saucer.position.x == u.SAUCER_VELOCITY.x*0.1
        saucer.move(delta_time=0.1, saucers=[])
        assert saucer.position.x == 2*u.SAUCER_VELOCITY.x*0.1

Here I had to remove the extra parameters to move.

    def test_vanish_at_edge(self):
        saucer = Saucer()
        saucers = [saucer]
        saucer.ready()
        saucer.move(1, saucers)
        assert saucers
        while saucer.position.x < u.SCREEN_SIZE:
            assert saucers
            saucer.move(delta_time=0.1, saucers=saucers)
        assert not saucers

The tests are green. Commit: move_everything moved to Fleets. Saucer is not firing missiles.

We’ll want to fix missile firing but let’s first see what other references to the individual collections we can remove from Game.

With a little browsing I come across this:

    def check_missile_timeout(self, delta_time):
        for missile in self.missiles.copy():
            missile.update(self.missiles, delta_time)
        for missile in self.saucer_missiles.copy():
            missile.update(self.saucer_missiles, delta_time)

Let’s see whether Missile couldn’t deal with this directly.

    def update(self, missiles, delta_time):
        self.timer.tick(delta_time, missiles)

We can call this from move. I hate that I have the collection first, so I’ll fix that in a separate step. There are four senders of this function, the two above and two tests.

We remove the check_missile_timeout method and the call to it.

Then this:

    def move(self, delta_time, missiles):
        self.update(missiles, delta_time)
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position

Missiles still time out, as one would expect. Now let’s change the signature on that method to match our convention of putting the collections last and delta_time first.

    def move(self, delta_time, missiles):
        self.update(delta_time, missiles)
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position

    def update(self, delta_time, missiles):
        self.timer.tick(delta_time, missiles)

PyCharm fixed up the tests for me. Excellent. Commit: missile checks timeout during move, no need to check from Game.

I’ve been at this for just two hours, might be time to wrap up. A quick scan for references to the Game accessors for individual collections.

saucer_missiles isn’t accessed. Remove it. Remove it from Fleets and from the tests. Commit: saucer_missiles accessor removed throughout.

The other accessors are used more than once, many of them involved in spawning waves or a new ship. Those will take a bit more effort than I’m prepared to expend this late in the morning. Let’s sum up.

We now have two “smart” collections, Fleet, which is a collection of Fleet instances, and Fleet, which is a collection of flyers, i.e. a coherent collection of asteroids, missiles, saucer, or ship. We are now deferring draw and move from Game to Fleets and from there to Fleet, and from there to the individual flyer instance. Very nice.

We’re not firing missiles from the saucer. Let’s fix that before we wrap. We’ll put it here:

class Game:
    def asteroids_tick(self, delta_time):
        self.check_saucer_spawn(self.saucer, self.saucers, delta_time)
        self.check_ship_spawn(self.ship, self.ships, delta_time)
        self.check_next_wave(delta_time)
        self.control_ship(self.ship, delta_time)
        self.move_everything(delta_time)
        self.process_collisions()
        self.draw_everything()
        if self.game_over: self.draw_game_over()

I’ll just add another trivial method. Oh my: that’s the method that wants saucer_missiles. I’ve got to put those back in.

Done and:

    def asteroids_tick(self, delta_time):
        self.check_saucer_spawn(self.saucer, self.saucers, delta_time)
        self.check_ship_spawn(self.ship, self.ships, delta_time)
        self.check_saucer_firing(delta_time, self. saucers, self.saucer_missiles, self.ships)
        self.check_next_wave(delta_time)
        self.control_ship(self.ship, delta_time)
        self.move_everything(delta_time)
        self.process_collisions()
        self.draw_everything()
        if self.game_over: self.draw_game_over()

    def check_saucer_firing(self, delta_time, saucers, saucer_missiles, ships):
        for saucer in saucers:
            saucer.fire_if_possible(delta_time, saucer_missiles, ships)

This could be, and perhaps should be pushed down into Fleet, but we’re just here to get our feature back. Deeper thinking is beyond me now.

Summary

We’ve added Fleets and Fleet and are deferring drawing and moving down to them, and modified Missile so that it does its necessary timing as part of its move operation. We pulled Saucer missile firing out and are calling that directly, but we are winnowing down the calls to the individual fleet objects bit by bit.

If there is a lesson here for me, it is one that I’ve “learned” many times: always wrap native collections with meaningful objects. Had I done that early on, I’d have had different solutions available to things like firing saucer missiles, and some of these changes might have been done the first time.

But that’s OK. We are in the business of changing code and we never get it perfect, we only ever get it better.

And it’s better. Game class is already much simpler and Fleets and Fleet are quite simple given how much leverage they’re providing.

We probably have at least two or three more sessions before we have everything pushed into Fleets and Fleet that belong there, and some of the issues will be interesting. For example:

The fire_if_possible method only exists on Saucer, so we can’t really push it down into Fleet at all, and pushing it into Fleets would be odd, because we’d prefer that Fleets not know much of anything about its individual fleets.

We could, however, add fire_if_possible to the required methods to be a flyer, and implement it as pass for most of the classes. That adds an odd requirement to the flyer classes, so it’s questionable, but it would simplify some code. We’ll think about that in due time.

For now, we’ve added some very nice simplicity, reducing the details known to game substantially. We’ll get much further, I expect.

This is the nature of changing code. We can make it worse, or make it better. Making it better has very nice impact and has it right away. I’m pleased, and hope that you are as well.

Example Results

We went from here:

class Game:
    def move_everything(self, delta_time):
        for the_saucer in self.saucers.copy():
            the_saucer.move(delta_time, self.saucers, self.saucer_missiles, self.ships)
        for missile in self.saucer_missiles:
            missile.move(delta_time)
        for the_ship in self.ships:
            the_ship.move(delta_time)
        for asteroid in self.asteroids:
            asteroid.move(delta_time)
        for missile in self.missiles:
            missile.move(delta_time)

To here:

class Game:
    def move_everything(self, delta_time):
        self.space_objects.move_everything(delta_time)

class Fleets:
    def move_everything(self, delta_time):
        for fleet in self.fleets:
            fleet.move(delta_time)

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

I think that’s quite fine. Do you agree? Let me know either way!

See you next time!

EEEK!

As I was reviewing the article I thought of something. We need to be careful about not removing objects from our collections while we are iterating them. I think that in the case of moving the missile timeout into missile.move, we may have done that. I’ve made an urgent note to look into that.

I wonder how I could ensure that we do this when it’s needed and not when it isn’t? We could, of course, just do it always:

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

That surely fixes the potential bugs, at the cost of a lot of unnecessary copying. I’ll commit it: Fleet iter always copies the collection before iterating.

Whew!