Python Asteroids+Invaders on GitHub

Seeking something to do, I’m back on my Invaders [DELETED] again. I do tidying. It comes down to joy.

Wordle has been an interesting diversion, but I feel that I’ve extracted all the value from the exercise that I reasonably can. So I’m turning back toward Space Invaders, to see if there is distraction to be mined there. And, for me, distraction is the point, although I do fondly hope that someone out there get a bit of value from what I do. Even if it’s only a rueful shake of the head.

I see that in the most recent Invaders article, almost a month ago, I embedded the Saucer’s conditional behavior into two tiny classes, Ready and Unready which do the two things needed in their two unique ways.

Hey!
When I opened the python-asteroids project just now, PyCharm popped up one of its little instructive hints, and it was one that I didn’t know: Surround With. If you select some code and do Surround With, PyCharm will offer you some things that it will, well, surround your code with. Like if or try or while. I must try to remember to try that, because it’s often just a bit of a pain to toss something under an if or something like that. I really do like PyCharm.

As I was saying before I so rudely interrupted myself, I did the Ready / Unready thing last time. And I couldn’t resist glancing at it. Since those little classes are in the same file as Saucer, let’s review Saucer just to get warmed up. Here’s the whole thing. We’ll give it a scan and see what we think.

class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        maker = BitmapMaker.instance()
        self._map = maker.saucer
        self._mask = pygame.mask.from_surface(self._map)
        self._rect = self._map.get_rect()
        half_width = self._rect.width // 2
        self._left = u.BUMPER_LEFT + half_width
        self._right = u.BUMPER_RIGHT - half_width
        self._speed = 0
        self._player = None
        self._score_list = [100, 50, 50, 100, 150, 100, 100, 50, 300, 100, 100, 100, 50, 150, 100]
        self._readiness = Unready(self)

    @property
    def mask(self):
        return self._mask

    @property
    def rect(self):
        return self._rect

    @property
    def position(self):
        return Vector2(self.rect.center)

    @position.setter
    def position(self, value):
        self.rect.center = value

    def interact_with(self, other, fleets):
        other.interact_with_invaderssaucer(self, fleets)

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        self._readiness.die_if_lonely(invader_fleet, fleets)

    def die_if_lonely(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:
            self.die(fleets)

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        self._readiness.finish_initializing(self._player.shot_count)

    def finish_initializing(self, shot_count):
        self._readiness = Ready(self)
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

    def interact_with_playershot(self, shot, fleets):
        if Collider(self, shot).colliding():
            explosion = InvadersExplosion.saucer_explosion(self.position, 0.5)
            fleets.append(explosion)
            fleets.append(InvaderScore(self.mystery_score()))
            self.die(fleets)

    def mystery_score(self):
        if not self._player:
            return 0
        score_index = self._player.shot_count % len(self._score_list)
        return self._score_list[score_index]

    def die(self, fleets):
        fleets.remove(self)
        fleets.append(TimeCapsule(10, InvadersSaucer()))

    def update(self, delta_time, fleets):
        self._readiness.move_or_die(fleets)

    def move_or_die(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self.die(fleets)
        else:
            self.position = (x, self.position.y)

    def draw(self, screen):
        self._readiness.just_draw(screen)

    def just_draw(self, screen):
        screen.blit(self._map, self.rect)

Some things that I notice:

  • The methods don’t seem to be in any particular order;
  • Might be useful to mark private methods with _, private and public seem intermixed;
  • The variables _left and _right should probably be called _left_starting_position or something;
  • The if not self._player: in mystery_score seems odd.

Let’s see about cleaning this up a bit. I always enjoy what Kent Beck calls “tidying”, and there’s usually a lesson for me in what needs it.

Let’s start with that if:

    def mystery_score(self):
        if not self._player:
            return 0
        score_index = self._player.shot_count % len(self._score_list)
        return self._score_list[score_index]

What’s this about? Well, we’re going to send a message to the player, to get their shot count, because we use that to look up the mystery score of the Saucer. So if somehow the player gets shot down before the Saucer gets hit, the player might not be there.

What else do we use the player for?

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        self._readiness.finish_initializing(self._player.shot_count)

That’s the only other use. Both uses want the shot count. Let’s save that when we see the player and not save the player.

It goes like this:

    def interact_with_invaderplayer(self, player, fleets):
        self._player = player
        self._player_shot_count = player.shot_count
        self._readiness.finish_initializing(self._player_shot_count)

My tests are not auto-running. One thing that PyCharm does not do perfectly is restore that state when you close and later reopen a project.

They do run, so commit: adding member _player_shot_count.

Now that we have it, we can use it where the if is:

    def mystery_score(self):
        score_index = self._player_shot_count % len(self._score_list)
        return self._score_list[score_index]

A test fails. I am not entirely surprised. What is it? It’s the test that tests scoring, of course. It looks like this:

    def test_first_score(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(saucer := InvadersSaucer())
        fleets.append(keeper := InvaderScoreKeeper())
        fleets.append(player := InvaderPlayer())
        saucer.interact_with_invaderplayer(player, fleets)
        shot = PlayerShot()
        shot.position = saucer.position

        def kill_saucer(expecting):
            saucer.interact_with_playershot(shot, fleets)
            score = fi.scores[-1]
            assert score.score == expecting
            player.fire(fleets)
        kill_saucer(100)
        kill_saucer(50)
        kill_saucer(50)
        kill_saucer(100)

        kill_saucer(150)
        kill_saucer(100)
        kill_saucer(100)
        kill_saucer(50)

        kill_saucer(300)
        kill_saucer(100)
        kill_saucer(100)
        kill_saucer(100)

        kill_saucer(50)
        kill_saucer(150)
        kill_saucer(100)
        kill_saucer(100)

The first check, for 100, passes, but the count is not incrementing. Why? Because in the kill_saucer function, we do not show the player to the saucer and it does not pick up the score. Fix the test.

        def kill_saucer(expecting):
            saucer.interact_with_playershot(shot, fleets)
            score = fi.scores[-1]
            assert score.score == expecting
            player.fire(fleets)
            saucer.interact_with_invaderplayer(player, fleets)

Test passes. Commit: Saucer now uses member _player_shot_count to calculate mystery score.

Now no one is using the _player and we can remove it. Green. Commit: No longer save _player.

Reflection

Worth it? I think so. We had an odd possible glitch in the game, the possibility that the Saucer would try to access the player when there was none, and we had to deal with it with a special check. We found that we really only wanted the shot count in the saucer, so arranged to save it instead of the player, avoiding any possibility of a timing issue.

The code is more direct, more reliable, less tricky, shorter, less vulnerable to a mistake. Small improvement? Sure. But easy and makes the world a little bit better place.

What else?

Let’s see about marking methods with underbars when they are private, and see if that suggests a better ordering of them.

Here’s an example:

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        self._readiness.die_if_lonely(invader_fleet, fleets)

    def _die_if_lonely(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:
            self.die(fleets)

Only the _readiness member calls this, in exactly one place. In favor of leaving it right there: it’ll be easy to find. Against: we might mistakenly assume that it’s always called; we probably don’t need to look at it, so it is clutter. Inclination: consider moving it down into a batch of private methods. Hold for now.

Commit: rename to private: _die_if_lonely.

Here’s another:

    def interact_with_invaderplayer(self, player, fleets):
        self._player_shot_count = player.shot_count
        self._readiness.finish_initializing(self._player_shot_count)

    def finish_initializing(self, shot_count):
        self._readiness = Ready(self)
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

Clearly private. I am also wondering about the names on Ready and Unready. Is finish_initializing the right name? Or is _readiness the right name? Would _option be a better name? Table that, but rename the finish method.

That change gives me a bit of concern, because there are now tests that call that method. Let’s review those a bit. They mostly look something like this:

    def test_does_run_with_8_invaders(self):
        fleets = Fleets()
        fi = FI(fleets)
        fleets.append(invader_fleet := InvaderFleet())
        invader_group = invader_fleet.invader_group
        assert invader_group.invader_count() == 55
        while invader_group.invader_count() > 8:
            invader_group.kill(invader_group.invaders[0])
        assert invader_group.invader_count() == 8
        fleets.append(saucer := InvadersSaucer())
        saucer._finish_initializing(0)
        assert fi.invader_saucers
        saucer.interact_with_invaderfleet(invader_fleet, fleets)
        saucer.update(1.0/60.0, fleets)
        assert fi.invader_saucers

I think these are intimate tests of saucer’s behavior. We could run all this through the interaction logic but the idea is to check this specific behavior. I decide to allow it. Tests can access privates if they want to, at least according to me.

Commit: Make _finish_initializing private.

Here again, I might want to move the private stuff to a separate section. Holding off on that.

This is clearly private:

    def mystery_score(self):
        score_index = self._player_shot_count % len(self._score_list)
        return self._score_list[score_index]

Make it so. Commit: Make _mystery_score private.

    def die(self, fleets):
        fleets.remove(self)
        fleets.append(TimeCapsule(10, InvadersSaucer()))

Clearly a private matter. Change it. Same for _move_or_die and _just_draw. Broke something. Don’t know what. Roll back. Try just die. Something breaks. I think that PyCharm may have guessed wrongly. Roll back again. Find and fix tests. Yes, PyCharm guessed wrongly and changed some calls to InvaderShot.die, which we were not renaming.

Commit: making methods private.

Changing move_or_die works. Commit: making methods private.

Same with just_draw. Commit: making methods private.

Now I do think I’ll move these methods to be together, public methods first, after the properties, and in logical order, that is the order of the game cycle. And the result is this:

class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        maker = BitmapMaker.instance()
        self._map = maker.saucer
        self._mask = pygame.mask.from_surface(self._map)
        self._rect = self._map.get_rect()
        half_width = self._rect.width // 2
        self._left = u.BUMPER_LEFT + half_width
        self._right = u.BUMPER_RIGHT - half_width
        self._speed = 0
        self._player_shot_count = 0
        self._score_list = [100, 50, 50, 100, 150, 100, 100, 50, 300, 100, 100, 100, 50, 150, 100]
        self._readiness = Unready(self)

    @property
    def mask(self):
        return self._mask

    @property
    def rect(self):
        return self._rect

    @property
    def position(self):
        return Vector2(self.rect.center)

    @position.setter
    def position(self, value):
        self.rect.center = value

    def interact_with(self, other, fleets):
        other.interact_with_invaderssaucer(self, fleets)

    def interact_with_invaderfleet(self, invader_fleet, fleets):
        self._readiness.die_if_lonely(invader_fleet, fleets)

    def interact_with_invaderplayer(self, player, fleets):
        self._player_shot_count = player.shot_count
        self._readiness.finish_initializing(self._player_shot_count)

    def interact_with_playershot(self, shot, fleets):
        if Collider(self, shot).colliding():
            explosion = InvadersExplosion.saucer_explosion(self.position, 0.5)
            fleets.append(explosion)
            fleets.append(InvaderScore(self._mystery_score()))
            self._die(fleets)

    def update(self, delta_time, fleets):
        self._readiness.move_or_die(fleets)

    def draw(self, screen):
        self._readiness.just_draw(screen)

    def _die(self, fleets):
        fleets.remove(self)
        fleets.append(TimeCapsule(10, InvadersSaucer()))

    def _die_if_lonely(self, invader_fleet, fleets):
        if invader_fleet.invader_count() < 8:
            self._die(fleets)

    def _finish_initializing(self, shot_count):
        self._readiness = Ready(self)
        speed = 8
        even_or_odd = shot_count % 2
        self._speed = (-speed, speed)[even_or_odd]
        left_or_right = (self._right, self._left)[even_or_odd]
        self.rect.center = Vector2(left_or_right, u.INVADER_SAUCER_Y)

    def _just_draw(self, screen):
        screen.blit(self._map, self.rect)

    def _move_or_die(self, fleets):
        x = self.position.x + self._speed
        if x > self._right or x < self._left:
            self._die(fleets)
        else:
            self.position = (x, self.position.y)

    def _mystery_score(self):
        score_index = self._player_shot_count % len(self._score_list)
        return self._score_list[score_index]

Commit: reorder methods.

Reflection

It looks to me as if I would benefit from being a bit more consistent about using underbar names for private methods and private members. This is the sort of thing one learns in a new language: detailed techniques that make things better. Would it be better if I had always done it? Sure, but I am not allowed to edit the past because it messes up the present in horrible ways. That’s why there is no time travel: there used to be but it got edited out.

Summary

I think this class is a bit nicer. When should we do such things, and when not?

One of my two sons, in both of whom I am well pleased, is a master auto technician. Every night, before he goes home, he puts all his tools into the tool chest, in the positions they belong in. If he leaves something out even as he works, it seems to me that he puts such things in a particular place, on a work table. I should interrogate him about his practice the next time we chat. But even without that, I can see that the result of the way he works is that he always knows exactly where the tool he needs is.

It’s the same thing here, with the exception that I do not do this regularly and I am not entirely consistent about where things go. But to the extent that I do it regularly, and to the extent that I am consistent, when I return to my code, I am more able to find things than when I am less careful.

We’ve seen a few different cases of that today.

We have the Ready and Unready classes from last time, which embody conditional behavior without if statements, keeping related behavior together.

We have the class saving the information it needs, the shot count, rather than asking the player for it if it has a player and defaulting in some odd way if it does not.

We have classified methods as to their use, public or private. And we have put all the public ones together in a logical order, and the private ones together as well. (But in alpha order, mostly because their logical order isn’t all that obvious.)

So, to repeat my question, when should we do things like this and when not?

Well, the real answer is that you have to decide for yourself. But here’s what I try to do:

I try to clean up right after I’ve made a set of changes. Leave my workspace clean and ready for next time.

However. By the time a set of changes is done, I am often tired, and my time at the keyboard is up. So, more often than I’d like, I leave things a bit messy.

I try, almost always, to review the code quite soon, ideally next session, to see what I see and what needs cleaning. Then, the workspace is more ready for next time.

However. Often I feel pressure to work on something else, and I imagine that for most of you, that pressure is even higher. My guess is that it’s best not to mark a story “done” before it has been cleaned up, but that will be very difficult in many shops. So, we may have to move on, leaving that file a bit messy.

The next best thing to do might be that when we have a story that takes us to a file that is not tidy, we spend a little time tidying up before we work on the story. If we didn’t wash the dishes last night, we curse the inevitable crust and wash them before we start cooking. (Or, of course, we can let them pile up. More cursing, later.)

Another decent practice is to notice small things as we work, and take a moment to improve them. That tends to make the code better, but it does leave us with somewhat ragged commits, especially if we do not commit after every green bar. My own take is that messy commits are better than messy code, but YMMV.

Finally, at some lower-pressure time, one might do as we did today, and just pick something and tidy it up. If you’re an early riser, getting to work early can be a good time for this. If you stay late to avoid traffic, maybe doing it after 5 makes sense.

But for many of you, it may seem that a low-pressure time never comes, and you’re too darn tired to do tidying after work hours. That’s too bad, but it’s not your fault, it’s management’s. Unfortunately, the stuff does run downhill, and it falls on you and on your code. For some of you, some of the time, there may be no way out.

In those cases, for my own sanity, I would do my best to tidy as I go, because tidying gives me relaxation and energy in the moment, and makes the job easier, typically in the very near future. But your sanity is not mine, and you have to find your own best available balance in work, and life, and code tidying.

I wish you great success in doing so. I hope you find your way to joy in your work, as I have in mine.

See you next time!