Python Asteroids on GitHub

I’ll just have a look in and see what might need a little tidying, then, shall I?

Perhaps we’ll find something more interesting than mere tidying, but tidying is its own reward quite often.

Here’s Coin, early in the alphabet:

class Coin:

    @classmethod
    def quarter(cls, fleets=None):
        coin = cls(True, True)
        if fleets:
            coin.populate(fleets)
        return coin

    @classmethod
    def slug(cls, fleets=None):
        coin = cls(False, True)
        if fleets:
            coin.populate(fleets)
        return coin

    @classmethod
    def no_asteroids(cls, fleets=None):
        coin = cls(True, False)
        if fleets:
            coin.populate(fleets)
        return coin

    def __init__(self, is_quarter=True, want_asteroids=True):
        self.is_quarter = is_quarter
        self.want_asteroids = want_asteroids

    def populate(self, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        if self.want_asteroids:
            fleets.append(WaveMaker())
        fleets.append(ShipMaker() if self.is_quarter else GameOver())

Those True / False parameters aren’t helpful, especially with one of them with a reasonable name want_asteroids and the other is_quarter, not really helpful.

We always use this class the same way, always passing it a Fleets instance. Let’s remove the None stuff.

    @classmethod
    def quarter(cls, fleets):
        coin = cls(True, True)
        coin.populate(fleets)
        return coin

    @classmethod
    def slug(cls, fleets):
        coin = cls(False, True)
        coin.populate(fleets)
        return coin

    @classmethod
    def no_asteroids(cls, fleets):
        coin = cls(True, False)
        coin.populate(fleets)
        return coin

No one uses the return value, which is useless in any case. I’m not sure what to do about that. We could rename the class methods to something like insert_quarter and not return the coin, just do the thing.

Let’s commit this, then go after the T / F stuff. Commit: tidying.

Now what to do about the T / F? Let’s write it out longhand, then see what we see.

First, I just inline populate and remove the method:

class Coin:

    @classmethod
    def quarter(cls, fleets):
        coin = cls(True, True)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        if coin.want_asteroids:
            fleets.append(WaveMaker())
        fleets.append(ShipMaker() if coin.is_quarter else GameOver())
        return coin

    @classmethod
    def slug(cls, fleets):
        coin = cls(False, True)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        if coin.want_asteroids:
            fleets.append(WaveMaker())
        fleets.append(ShipMaker() if coin.is_quarter else GameOver())
        return coin

    @classmethod
    def no_asteroids(cls, fleets):
        coin = cls(True, False)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        if coin.want_asteroids:
            fleets.append(WaveMaker())
        fleets.append(ShipMaker() if coin.is_quarter else GameOver())
        return coin

    def __init__(self, is_quarter=True, want_asteroids=True):
        pass

At this point I don’t see much point to creating the instance at all. I’m not sure what the pythonic thing to do about that might be. For now, we’ll remove the conditionals throughout, based on the flags that are still shown.

class Coin:

    @classmethod
    def quarter(cls, fleets):
        coin = cls(True, True)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        coin = cls(False, True)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        coin = cls(True, False)
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(ShipMaker())

I stopped returning the coin. Now don’t create it:

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(ShipMaker())

I should be committing all this but I’m going to wait until I’m a bit more happy. Extract common lines:

class Coin:

    @classmethod
    def quarter(cls, fleets):
        cls.append_common_elements(fleets)
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        cls.append_common_elements(fleets)
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        cls.append_common_elements(fleets)
        fleets.append(ShipMaker())

    @classmethod
    def append_common_elements(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

Let’s rename that method:

class Coin:

    @classmethod
    def quarter(cls, fleets):
        cls.create_common_elements(fleets)
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        cls.create_common_elements(fleets)
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        cls.create_common_elements(fleets)
        fleets.append(ShipMaker())

    @classmethod
    def create_common_elements(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

Yes, that’s better, I think. Commit: tidying.

Feature Envy

When we look at this class now, we see some pretty serious Feature Envy. These methods only refer to the Fleets instance. They clearly don’t belong here. Where do they belong? Well, Fleets seems likely, doesn’t it?

Here’s how we use Coin in the Game:

class Game:
    def accept_coins(self):
        keys = pygame.key.get_pressed()
        if keys[pygame.K_q]:
            Coin.quarter(self.fleets)
        elif keys[pygame.K_a]:
            Coin.no_asteroids(self.fleets)

There’s no notable support for moving methods to a new class, but these really want to be methods on Fleets, it seems quite clear. And I think the name Fleets is getting questionable as well.

Let’s put a copy of quarter over on Fleets, with a new name insert_quarter:


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

    def insert_quarter(self):
        self.create_common_elements()
        self.append(WaveMaker())
        self.append(ShipMaker())

    def create_common_elements(self):
        self.clear()
        self.append(SaucerMaker())
        self.append(ScoreKeeper())
        self.append(Thumper())

And we see why we don’t want to do that. We don’t want Fleets to know how to create Asteroids. Fleets is a repository of objects, and it doesn’t know what kind of objects they are. Belay that change. Roll back.

Reflection

I suppose someone more clever than I am would have seen that before spending two minutes copying a method over, but we only have me. I saw it soon enough: Coin is the only object that knows how to create a game, where we define a game as an initial load of Fleets with all the objects necessary to play some game, which happens to be Asteroids but could be something else.

I want to add a comment to Coin to that effect, because it’s notable.

class Coin:
    # This is the only class that knows what objects
    # make up a game, in the current case, Asteroids.
    # Want another game? Create another kind of coin.

Commit: add comment.

One more thing: I think WaveMaker might want to be renamed to AsteroidMaker. Or perhaps AsteroidWaveMaker. Why? Because when I read the coin, I don’t immediately see what the difference is.

Maybe the whole thing is too clever. Maybe the name Coin is too clever, maybe the names coin and slug are too clever.

And maybe we should wait until we have another game before we decide what to call these things, though I am tempted toward Coin -> AsteroidsGame and quarter -> play_game, slug -> attract_mode, and no_asteroids -> play_without_asteroids.

We’ll hold off on that.

We have simplified the Coin class substantially, but I think removing that duplication actually makes it harder to understand. Inline the method back:

class Coin:
    # This is the only class that knows what objects
    # make up a game, in the current case, Asteroids.
    # Want another game? Create another kind of coin.

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(ShipMaker())

Yes. I like that better. But let’s try one other thing. Roll that back. Try this:

class Coin:
    # This is the only class that knows what objects
    # make up a game, in the current case, Asteroids.
    # Want another game? Create another kind of coin.

    @classmethod
    def quarter(cls, fleets):
        cls.create_common_elements(fleets)
        cls.add_game_elements(fleets)

    @classmethod
    def add_game_elements(cls, fleets):
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        cls.create_common_elements(fleets)
        cls.add_attract_mode_elements(fleets)

    @classmethod
    def add_attract_mode_elements(cls, fleets):
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        cls.create_common_elements(fleets)
        fleets.append(ShipMaker())

    @classmethod
    def create_common_elements(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

I’m not sure I like that best of all, but I like it well enough to keep it for now.

Commit: explaining method names, possibly improved.

Summary

I find it fascinating that in spite of how incredibly good-looking I am, there seems to be something to improve everywhere I look in my code. What is perhaps even more fascinating is that it’s easy to improve and rarely do improvements ever break anything. This is in part due to PyCharm’s excellent refactoring capability, and in part due to the fact that I have fairly decent tests.

We’ll keep looking for things to improve. If they all seem this small, we may have to move on to something different. Ideas welcome, but it’s not easy finding something that fits my narrow limitations.

See you next time!

No, Wait!

I don’t like that after all. One more idea: what if we did the special bits first and then the others?

Inline everything again:

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(WaveMaker())
        fleets.append(GameOver())

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())
        fleets.append(ShipMaker())

Now move things up.

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(GameOver())
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(ShipMaker())
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

And now:

    @classmethod
    def quarter(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(ShipMaker())
        cls.append_common_elements(fleets)

    @classmethod
    def append_common_elements(cls, fleets):
        fleets.append(SaucerMaker())
        fleets.append(ScoreKeeper())
        fleets.append(Thumper())

    @classmethod
    def slug(cls, fleets):
        fleets.clear()
        fleets.append(WaveMaker())
        fleets.append(GameOver())
        cls.append_common_elements(fleets)

    @classmethod
    def no_asteroids(cls, fleets):
        fleets.clear()
        fleets.append(ShipMaker())
        cls.append_common_elements(fleets)

OK, I promise, that’s the last one. For now. But you know what might be better? What if we created an instance and then called it? We could get rid of those class methods.

Wait. Can’t we just make them top level and still use Coin? I’ll look into that. Later. Maybe never. Stop me, this is scrabbling for crumbs of betterness. There must be something more significant out there.

See you next time!