Python Asteroids+Invaders on GitHub

Drilling down was useful, I think, but let’s take a look around for new code signs. We wind up working on names. Again.

I’m just going to browse code until something catches my eye.

Randomly:

    def x_fraction(self):
        x = self.rect.centerx - u.BUMPER_LEFT
        denom = u.BUMPER_RIGHT - u.BUMPER_LEFT
        return x / denom

PyCharm squiggles denom because it’s not a word, which draws my attention to it. It’s not a very good name and wouldn’t be much better if it were denominator, which is of course what I had in mind.

Let’s try these renamings:

    def x_fraction(self):
        x_distance = self.rect.centerx - u.BUMPER_LEFT
        total_distance = u.BUMPER_RIGHT - u.BUMPER_LEFT
        return x_distance / total_distance

Better? I think so. Commit: tidying.

As I browse, I notice a number of utility objects, such as the scorekeeper, with implementations of mask and rect as return None. Those methods are currently abstract, and are in fact the only remaining abstract methods in InvadersFlyer. They do kind of clutter things up, but changing from abstract to non-abstract seems like a bigger deal than I want to undertake right now. Intellectually, I don’t see the harm, but I’d want to look hard at what could go wrong if some object failed to implement those two methods. So I might put a note on the refactoring board, but I’d not do anything just now.

Here’s one that is tricky to understand: InvadersSaucer. There are two helper classes:

class Unready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, invader_fleet, fleets):
        self._saucer._die_if_lonely(invader_fleet, fleets)

    def finish_initializing(self, shot_count):
        self._saucer._finish_initializing(shot_count)

    def just_draw(self, screen):
        pass

    def move_or_die(self, fleets):
        pass


class Ready:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, _invader_fleet, _fleets):
        pass

    def finish_initializing(self, shot_count):
        pass

    # noinspection PyProtectedMember
    def just_draw(self, screen):
        self._saucer._just_draw(screen)

    # noinspection PyProtectedMember
    def move_or_die(self, fleets):
        self._saucer._move_or_die(fleets)

The saucer itself is initialized with an Unready:

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)

The Saucer implements four methods that call the _readiness member to decide what to do:

    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 update(self, delta_time, fleets):
        self._readiness.move_or_die(fleets)

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

And clearly we want to see this:

    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)

This is a bit obscure. Maybe more than a bit. I remember what’s going on, let me describe it and then let’s see if we can make the code tell the story better.

The saucer chooses its direction (and therefore its starting point) based on the number of shots the player has fired, even or odd. So the saucer cannot appear until after it has seen the player, so as to get that value. We see it getting that value here:

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

So once it has that value, it will call the _readiness object to finish initializing. If that object is an Unready, we see in the code that it will call the _finish_initializing code above, and otherwise, if Ready, it will do nothing. _finish_initializing sets the _readiness to Ready.

When Unready, we do not draw and we do not move: those are marked pass. We do implement finish_initializing as we’ve seen and also die_if_lonely. That’s called here:

    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)

We do not start the saucer if there are fewer than 8 invaders. Why? It makes it too easy to shoot the saucer, I suppose. We just do it because the original game did it.

Ready and Unready are essentially Strategy objects. They implement the overall behavior rules, which are something like:

  1. The saucer starts on the right if the player has fired an even number of shots, left otherwise;
  2. The saucer does not start if there are fewer than 8 invaders on the screen;
  3. We can’t draw the saucer until it is started;
  4. We can’t move the saucer until it is started.

This behavior used to be implemented with a bunch of if statements in the various methods, and the Ready/Unready were built in order to eliminate them, as described here.

I think that this works well, but what’s going on does not jump out at us. What might we do about this?

Comments

One solution that people jump to is comments. We could write up the story, much as we did in this article, and paste it into the file as comments. That’s not a bad solution, especially if the comments are at a high enough level.

Tests

Another approach is to use tests. What tests do we have now for this behavior?

Here are the test names from test_invaderssaucer:

    def test_saucer_moves(self):
    def test_does_not_run_with_7_invaders(self):
    def test_does_run_with_8_invaders(self):
    def test_returns_after_dying_on_right(self):
    def test_returns_after_dying_on_left(self):
    def test_missing_shot(self):
    def test_dies_if_hit(self):
    def test_first_score(self):
    def test_initialized(self):
    def test_start_on_right(self):
    def test_sets_up_ready(self):

Those are pretty good, and if the programmer would look at them, they’d help a lot.

Improved Code

Kent Beck used to say “A comment is the code’s way of asking too be made more clear”. He may still say it, for all I know: it’s a darn good saying. How might we make this code more clear?

I think better names could help. The methods are all quite small and I feel that their names make sense. What about the Ready, Unready, and _readiness names? Could they be more helpful?

What if we were to rename _readiness to _strategy? That would be a strong hint that we’re using Strategy pattern.

What about Ready and Unready. How about PostInitStrategy and PreInitStrategy?

Here are some of the interesting code bits with the new names:

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._strategy = PreInitStrategy(self)

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

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

    def _finish_initializing(self, shot_count):
        self._strategy = PostInitStrategy(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 update(self, delta_time, fleets):
        self._strategy.move_or_die(fleets)

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

I think in _finish_initializing, I’d prefer the strategy setting moved to the end. I think it’ll be easier to notice. Or better yet, let’s do that, plus this:

    def _finish_initializing(self, shot_count):
        self._set_start_and_speed(shot_count)
        self._strategy = PostInitStrategy(self)

    def _set_start_and_speed(self, shot_count):
        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)

In fact, let’s modify the __init__ similarly.

class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        self._initialize_what_we_can()
        self._strategy = PreInitStrategy(self)

    def _initialize_what_we_can(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]

I think I like that. Commit: renaming and tidying to clarify use of Strategy.

Let’s sum up. I’ll paste the whole file at the end in case you want a look.

Summary Reflections

I think that this renaming helps make the saucer more likely to be understood. It is inherently tricky, in that it cannot be fully initialized when it is created, at least not according to the way our game works at present. It needs to find out the game situation so as to init itself correctly.

The tests, which we didn’t review in detail, do tell a bit of the story, but I think I’d like them to tell the whole story and tell it better. And then I might put in a comment(!) saying “Be sure to see the tests, they tell the story”.

The size of the file (150 lines counting the imports and Strategy classes) makes me wonder whether there might be some way to split up the responsibilities into one more more helper classes. The init is long, but it’s almost all computing constants, so perhaps using the universe constants could simplify that a bit. There are only three if statements in the file, which isn’t bad. Zero would be better. The methods are all quite short, which I like.

One random idea comes to mind. It wouldn’t save much, but we could create a SaucerSound object, put it in the mix, and have it play the saucer sound and adjust the stereo.

This just came to me:

Oh! What if we had a SaucerCreator object? Instead of dropping a Saucer into the TimeCapsule, we’d put a SaucerCreator in there. It would run, collect the necessary info, and create the Saucer all in one go. That might be a really good idea!

It almost seems with these objects that drawing is an afterthought. That makes me wonder whether they should all have an auxiliary drawing object that gets used for that purpose.

Efficiency Concerns

We have seen elsewhere that the time to call a method in Python is about 4 nanoseconds, 13 clock ticks on my laptop. It turns out that the M1 executes 8 (RISC) instructions per cycle. Point is, it’s quite fast.

The Entity-Component-System architecture, used in high-performance gaming, addresses a separate efficiency issue, which is that memory access is much slower than CPU speed, and object-oriented code jumps around in memory quite a bit. ECS designs keep the code organized around faster memory access, which, I am told, makes a big difference for them.

If we had performance issues, and we could isolate them to memory access, we might care. Most of us, I think, do not have such issues, couldn’t do much about them if we had them, and therefore need not care. We do better, I think, to organize our code for our best understanding.

Now I prefer tiny methods with meaningful names. Other programmers prefer larger methods, which they believe lets them take in larger chunks, which they believe improves their understanding. I am not inside their heads, so I do not know. I know that I prefer tiny methods, and it’s my house, my rules, for my programs and articles.

Saucer

I think we’ve improved the saucer code a bit with better names. Naming seems to be a major component of my toolkit, when it comes to making the code more clear.

I find that interesting. It might be worth paying attention to this, because many of my favorite moves are really all about names: Rename (obviously), Extract Variable, and Extract Method. Helper classes like today’s Strategies benefit from better names.

We’ll watch that a bit, see what else comes up that isn’t naming. Is there anything? Come back and find out with me!


# noinspection PyProtectedMember
class PreInitStrategy:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, invader_fleet, fleets):
        self._saucer._die_if_lonely(invader_fleet, fleets)

    def finish_initializing(self, shot_count):
        self._saucer._finish_initializing(shot_count)

    def just_draw(self, screen):
        pass

    def move_or_die(self, fleets):
        pass


class PostInitStrategy:
    def __init__(self, saucer):
        self._saucer = saucer

    def die_if_lonely(self, _invader_fleet, _fleets):
        pass

    def finish_initializing(self, shot_count):
        pass

    # noinspection PyProtectedMember
    def just_draw(self, screen):
        self._saucer._just_draw(screen)

    # noinspection PyProtectedMember
    def move_or_die(self, fleets):
        self._saucer._move_or_die(fleets)


class InvadersSaucer(InvadersFlyer):
    def __init__(self):
        self._initialize_what_we_can()
        self._strategy = PreInitStrategy(self)

    def _initialize_what_we_can(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]

    @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._strategy.die_if_lonely(invader_fleet, fleets)

    def interact_with_invaderplayer(self, player, fleets):
        self._player_shot_count = player.shot_count
        self._strategy.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._strategy.move_or_die(fleets)

    def draw(self, screen):
        self._strategy.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._set_start_and_speed(shot_count)
        self._strategy = PostInitStrategy(self)

    def _set_start_and_speed(self, shot_count):
        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):
        self._move_along_x()
        self._adjust_stereo_position()
        self._die_if_done(fleets)

    def _move_along_x(self):
        self.position = (self.position.x + self._speed, self.position.y)

    def _adjust_stereo_position(self):
        frac = (self.position.x - self._left) / (self._right - self._left)
        player.play_stereo("ufo_lowpitch", frac, False)

    def _die_if_done(self, fleets):
        if self._going_off_screen():
            self._die(fleets)

    def _going_off_screen(self):
        return not self._left <= self.position.x <= self._right

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