Python Asteroids+Invaders on GitHub

Dude, did you ever look at your hand? No, I mean really look at it?

No, I am not stoned. The strongest non-prescription drug I consume is the caffeine in my iced chai, and I haven’t had a sip of that yet. But today I want to look at duplication, I mean really look at it.

Kent Beck’s “Rules of Simple Design” are:

The code is simple enough when, in priority order:1

  1. It passes all the tests;
  2. It contains no duplication;
  3. It expresses all our design ideas about the code;
  4. It minimizes programming entities.

The “no duplication” rule stands out as the most “negative” of the rules. Is duplication bad? Not exactly, but if the same or sufficiently similar code appears more than once, it suggests that there is a common idea whose commonality is not yet expressed. By rule #3, we know, or should know, that all our code expresses some idea, and even in the duplicated form, there are surely ideas being expressed.

Looking at two chunks of same-or-similar code, we might say that one expresses finding all the x coordinates that are in open areas and the other expresses finding all the invaders who are in a given column. But there is a third, common idea lurking there as well: find all the objects that meet some criterion.

The third rule would say that we must express that idea, so perhaps we would create an object finder object, or we might create a high-level searching function, or we might just use an existing language feature, like a Python list comprehension, which, at least to the experienced pythonista, expresses finding all the things that meet some criterion.

By any of those means, we would reduce the duplication in the program, and simultaneously improve its expressiveness a bit.

Now I confess that, having worked with those rules for decades now, I have mostly thought of duplication of code as “bad”. After all, if we’re supposed to get rid of duplication, duplication must be bad.

But yesterday I found myself creating duplication, making the three classes we were working with more and more alike. Was I somehow making the code worse, in the hope of making it better? That seems odd.

I finally realized that I wasn’t trying to make the code worse, I was trying to isolate and clarify the ideas in the code. And even yesterday, as common bits of code would arise, we often found ways to eliminate them by using an additional object (such as Sprite), or by calling a common method, or by creating constants in our constant pool. And each of those moves expressed an idea.

This object displays itself like a Sprite does: use a Sprite. These objects do the same calculation on the width of a Sprite: make the sprite do that calculation itself. These calculations are all constant: move them to the pool and reference them.

So therefore …

  1. When we see code that is duplicated, it is almost certainly expressing an idea. We might profit from identifying that idea and expressing it more directly.
  2. When we see code that is similar, we might profit from making it the same, and applying rule 1.

Now let me have a sip of my chai, and let’s go make some duplication.

The Players

We’re in the process of dealing with an observation that I’ve made several times, including just a few days back: the InvaderPlayer, ReservePlayer, and RobotPlayer objects are quite similar. We’re looking at them to see if there is a simpler, smaller, more expressive way to do what they do.

So far, we have really only deal with their __init__ methods, although the changes we have made have occasionally rippled down into other methods, just replacing references to members with constants and similar trivial implied changes. Today, we’ll start again with init, and almost certainly move beyond it.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)

        self.free_to_fire = True
        self.fire_request_allowed = True
        self.shot_count = 0

class RobotPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)

        self.count = 0
        self.invader_x_values = []
        self._can_shoot = True

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)

        self.reserve_number = reserve_number

We see similarity in the first two. Something about firing. And what is count in RobotPlayer? Is it like shot_count, or something different?

A command-B on count tells me that it is not used at all. Remove it. Commit: remove unused count.

What about _can_shoot, versus free_to_fire and fire_request_allowed? Is there similarity there that we should exploit? Let’s check them out.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)

        self.free_to_fire = True
        self.fire_request_allowed = True
        self.shot_count = 0

    def begin_interactions(self, fleets):
        self.free_to_fire = True

    def interact_with_playershot(self, bumper, fleets):
        self.free_to_fire = False

    def trigger_pulled(self, fleets):
        if self.fire_request_allowed:
            self.attempt_firing(fleets)
        self.fire_request_allowed = False

    def trigger_released(self):
        self.fire_request_allowed = True

    def attempt_firing(self, fleets):
        if self.free_to_fire:
            self.fire(fleets)

    def fire(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("shoot", frac)
        self.shot_count += 1
        fleets.append(PlayerShot(self._sprite.center))

    def update(self, _delta_time, fleets):
        if not pygame.get_init():
            return
        keys = pygame.key.get_pressed()
        self.check_motion(keys)
        self.check_firing(fleets, keys)

    def check_firing(self, fleets, keys):
        if keys[pygame.K_k]:
            self.trigger_pulled(fleets)
        else:
            self.trigger_released()

class RobotPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)

        self.invader_x_values = []
        self._can_shoot = True

    def begin_interactions(self, fleets):
        self.invader_x_values = []
        self._can_shoot = True

    def interact_with_playershot(self, shot, fleets):
        self._can_shoot = False

    def update(self, delta_time, fleets):
        self.move_toward_target()
        self.fire_when_ready(fleets)

    def fire_when_ready(self, fleets):
        if self._can_shoot:
            fleets.append(PlayerShot(self._sprite.center))

There’s a bit to look at there, but it seems very clear that _can_shoot and free_to_fire are the same idea: we are free to fire if there is no PlayerShot on the screen.

We will give these the same name, and let’s try to be a bit more explicit about private members. I think _free_to_fire is the better name.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)
        self._free_to_fire = True

        self.fire_request_allowed = True
        self.shot_count = 0

class RobotPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.position = Vector2(u.INVADER_PLAYER_LEFT, u.INVADER_PLAYER_Y)
        self._free_to_fire = True

        self.invader_x_values = []

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)

Let me make the first two a bit more like the third, to emphasize the similarity.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT
        self.position = Vector2(x, u.INVADER_PLAYER_Y)
        self._free_to_fire = True

        self.fire_request_allowed = True
        self.shot_count = 0

class RobotPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT
        self.position = Vector2(x, u.INVADER_PLAYER_Y)
        self._free_to_fire = True

        self.invader_x_values = []

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)

        self.reserve_number = reserve_number

Commit: refactoring removing differences.

Hm. If we were to pass reserve_number zero to InvaderPlayer and RobotPlayer, we could have the very same expression for position in all three.

Reflection

At this point I want to question what I’m doing. It’s becoming more and more clear that we have three different firing strategies, no firing, firing whenever there’s no shot on the screen, and user-triggered firing. We haven’t really looked at motion, but we know there are three strategies for moving: don’t move, move directed by the robot, move directed by the user.

The moving strategy and firing strategy are always in lockstep: none, automated, or user-controlled. So it seems very likely that we “should” have separate strategy objects used by a single Player class. Whether we should best do that with classes or some other means isn’t quite clear to me.

But if we were sitting around the coffee table talking about this, we’d surely start looking at solutions, and we’d surely be tempted to start creating those strategy things and plugging them in.

We’re itching to just get down to it and do it. But let’s hold off, and keep making them alike until we run out of ways to do that, just to see what happens. All these changes so far are trivial, requiring little or no thought.

We’ll do this: we’ll pass in reserve_number, use it to set position, and … shall we save it as a member in all of them? Yes, I think we shall. I was tempted to “save space” by storing the member only sometimes. Silly: there is never more than one of InvaderPlayer or RobotPlayer in use, so we’d save one integer.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.INVADER_PLAYER_Y)
        self.reserve_number = reserve_number

        self._free_to_fire = True
        self.fire_request_allowed = True
        self.shot_count = 0

class RobotPlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.INVADER_PLAYER_Y)
        self.reserve_number = reserve_number

        self._free_to_fire = True
        self.invader_x_values = []

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)
        self.reserve_number = reserve_number

I moved _free_to_fire back down to the unique area. Commit: adding duplication.

Now I’m really feeling the need to deal with the behavior. Let’s review the ReservePlayer first, because it has almost none.

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)
        self.reserve_number = reserve_number

    def __gt__(self, other):
        return self.reserve_number > other.reserve_number

    def rightmost_of(self, another_reserve_player):
        return max(self, another_reserve_player)

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

    def interact_with_destructor(self, destructor, fleets):
        fleets.remove(self)
        fleets.append(PlayerExplosion(self.position))

We have those two interesting methods __gt__ and rightmost_of, which are used by whatever logic converts a reserve player to a real one, always taking the rightmost reserve token off the list at the bottom of the screen. Given that we have reserve_number in all three classes now, it could be reasonable to add these two methods as well. The only argument against is the additional slots in the class, and those will be represented only once for all instances anyway.

What about interact_with_destructor?

class InvaderPlayer
    def interact_with_destructor(self, destructor, fleets):
        self.hit_by_something(fleets)

    def hit_by_something(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

class RobotPlayer
    def interact_with_destructor(self, destructor, fleets):
        self.explode(fleets)

    def explode(self, fleets):
        fleets.remove(self)
        fleets.append(PlayerExplosion(self.position))

class ReservePlayer
    def interact_with_destructor(self, destructor, fleets):
        fleets.remove(self)
        fleets.append(PlayerExplosion(self.position))

We should probably make these all the same, so that they all make the sound when they explode. That will make the horrific events when the invaders get to the bottom all the more horrific.

Let’s rename the RobotPlayer’s explode to hit_by_something and make it the same as InvaderPlayer. In fact we’ll do them all the same.

class InvaderPlayer
    def interact_with_destructor(self, destructor, fleets):
        self.hit_by_something(fleets)

    def hit_by_something(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

    def hit_invader(self, invader, fleets):
        self.hit_by_something(fleets)

    def x_fraction(self):
        x = self.position.x - u.INVADER_PLAYER_LEFT
        total_width = u.INVADER_PLAYER_RIGHT - u.INVADER_PLAYER_LEFT
        return x / total_width

class RobotPlayer
    def interact_with_destructor(self, destructor, fleets):
        self.hit_by_something(fleets)

    def hit_by_something(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

    def x_fraction(self):
        x = self.position.x - u.INVADER_PLAYER_LEFT
        total_width = u.INVADER_PLAYER_RIGHT - u.INVADER_PLAYER_LEFT
        return x / total_width

class ReservePlayer
    def interact_with_destructor(self, destructor, fleets):
        self.hit_by_something(fleets)

    def hit_by_something(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

    def x_fraction(self):
        x = self.position.x - u.INVADER_PLAYER_LEFT
        total_width = u.INVADER_PLAYER_RIGHT - u.INVADER_PLAYER_LEFT
        return x / total_width

Reflection

We’re still just following our nose here, making more and more duplication. One very easy thing to do at this point would be to create a superclass containing all the duplicated material and inherit it. None of these objects would have the need to override any of the concrete methods in the superclass. But my betters often avoid concrete methods in superclasses. I personally have found them useful and not troublesome. I’m not really clear on what makes them so bad. But I do lean away from them. I do allow concrete methods that ignore the game’s events, so that all the objects only have to implement methods they wish to see. But here, it’s a bit different.

I am in no hurry here. If I were, I might be inclined to do something a bit radical, like actually implement a strategy or something but let’s not rush. Let’s move everything that’s the same up to the top and everything that’s different down below. No, let’s reverse that. Common stuff will be moved below a line in each class.

That’s done. I’ll spare you the listing. Commit: move COMMON ELEMENTS to end of classes.

I think I’ll move the greater than and rightmost items to the common elements. It’s harmless and makes the classes more alike.

Commit: __gt__ and _rightmost_of added as COMMON ELEMENTS

I think that ReservePlayer is pretty much done, unless we were to extend it with methods it would never use.

class ReservePlayer(Spritely, InvadersFlyer):
    def __init__(self, reserve_number=0):
        self._sprite = Sprite.player()
        x = u.INVADER_PLAYER_LEFT + reserve_number*(5*self.rect.width//4)
        self.position = Vector2(x, u.RESERVE_PLAYER_Y)
        self.reserve_number = reserve_number

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

# COMMON ELEMENTS

    def __gt__(self, other):
        return self.reserve_number > other.reserve_number

    def rightmost_of(self, another_reserve_player):
        return max(self, another_reserve_player)

    def interact_with_destructor(self, destructor, fleets):
        self.hit_by_something(fleets)

    def hit_by_something(self, fleets):
        frac = self.x_fraction()
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

    def x_fraction(self):
        x = self.position.x - u.INVADER_PLAYER_LEFT
        total_width = u.INVADER_PLAYER_RIGHT - u.INVADER_PLAYER_LEFT
        return x / total_width

The two methods __init__ and interact_with are necessary and the latter at least is required to be unique for each class … at least up until now. And we will want that to continue to be the case, I think. Certainly a shot wants to know that it has hit the InvaderPlayer or RobotPlayer, but does not want to interact with a reservePlayer … it should right on through them. (I think they really stop at the BottomLine but in any case they don’t want to interact with ReservePlayer.)

I think it’s time to decide what we’re going to do here. We can probably create a bit more duplication, and we’ll look at that as we proceed, but let’s put a stake in the ground and stop for the day.

We will provide a strategy helper object to each of our three classes. The one for ReservePlayer will do nothing. The ones for InvaderPlayer and RobotPlayer will take over motion and firing logic for the classes they help.

We’ll evolve these helpers locally, probably starting with the RobotPlayer, but we’ll give them the same protocol so that we’ll subsequently be able to collapse things.

I expect the flow of work to be something like this:

  1. Evolve a helper object in each class, handling motion and firing;
  2. Ensure that these classes have the same protocol;
  3. When the helpers are complete, all three classes should have identical code, other than creation of the helper;
  4. Give one of the three class methods so that it can create any of the three types of helpers;
  5. Use that one for all three cases of InvaderPlayer, RobotPlayer, or ReservePlayer;
  6. Remove the others.
  7. Give the remaining one a more generic name.

For now, we’ve made our classes even more alike, and have better isolated the commonalities and differences. Small sensible steps.

I think we’ll leave that for next year. I wish a happy new year for you and yours, and for me and mine.

See you next year!


  1. In Beck’s own writing, he has written numbers 2 and 3 in both possible orders, with duplication as #2 or as #3. I prefer it as #2 because it is such a strong signal, as we discuss here today. ↩