This morning I thought we’d start partitioning the 3 Player classes. The situation suggests otherwise. Some simple refactoring. It’s all good.

Happy new year!

Last year, yesterday, I was thinking about using the common code in the players as the base, and helpers to deal with the differences. I think I got there via thinking about putting the common code in a superclass, which, while it would work, is the sort of thing that will make my friends look at me askance.

But if we’re not going to use a superclass for the common behavior, does it make more sense to continue to have our three separate Flyer classes, InvaderPlayer, RobotPlayer, and ReservePlayer, and to have a common helper class that they all use? In other words, extract the similarities to a new (helper) class. I think that may make more sense.

Let’s try it. I’ll just manually extract the common code to a new class, PlayerHelper. It looks like this:

class PlayerHelper(Spritely):
    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_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

Already I sense that there are issues. We probably don’t really want to defer interact_with_destructor, but we probably do want to remove the duplication of hit_by_something and x_fraction.

I’ll bet there is at least one other use of x_fraction or equivalent, with the invader explosion. I am correct: there is something very similar.

class Invader(...)
    def interact_with_group_and_playershot(self, shot, group, fleets):
        if self.colliding(shot):
            player.play_stereo("invaderkilled", self.x_fraction())
            shot.hit_invader(self, fleets)
            group.kill(self)
            fleets.append(InvaderScore(self._score))
            fleets.append(InvaderExplosion(self.position))

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

There are differences in its x_fraction but if it was the same it would sound the same. And certainly those u. numbers are the same or similar anyway:

BUMPER_LEFT = 64
BUMPER_RIGHT = 960
INVADER_HALF_WIDTH = 32
INVADER_PLAYER_HALF_WIDTH = Sprite.player().half_width
INVADER_PLAYER_LEFT = 64 + INVADER_PLAYER_HALF_WIDTH
INVADER_PLAYER_RIGHT = 960 - INVADER_PLAYER_HALF_WIDTH

Yes, we could make all our x_fraction methods the same and it would sound the same.

Wow. New year, new issues left over from last year.

Where should x_fraction really go? Well, I think a case could be made that instead of using the pygame player directly, we might do better to have a sound player for Invaders, or perhaps one for all games, and that object would deal with computing stereo fractions. (That’s what x_fraction is all about, after all.)

We do not have a facility for utility functions or methods. We try—oh we try—not to put concrete code in our superclasses, but let’s face it, x_fraction applies to any object with a position.

Reflection

We’ve been cleaning up our three suspect classes, InvaderPlayer, RobotPlayer, and ReservePlayer, making them more and more similar, and simplifying them in the process. That’s good.

But I—I won’t blame “we” for this—have been thinking in terms of doing some transformation to eliminate the duplication among these three classes. The duplication amounts to exactly the code up above in PlayerHelper.

That’s only about 21 non-blank lines. Some of them are so short that it will take as much code to call them as it does to just do the thing.

The exceptions are two: x_fraction is a handy calculation that is needed by four classes, including our three. And hit_by_something encompasses about four lines that our objects all do, namely explode.

We’re on the wrong track. I’m not sure of the right track, but a delegate or superclass or player-specific helper class does not seem to be in the cards.

We need a better plan.

Roll back

We’re back with our three classes including a bit of common code. This is duplication. We would like to reduce it. Let’s look first at the __init__ methods:

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

OK, the idea of giving everyone a reserve_number didn’t pan out, as it was part of making them more similar. Perhaps it should have been clear that making two classes more … oh hell, don’t go looking for blame … the code’s not how we want it. Make it like we want it. If, later, it isn’t how we want it. make it how we want it then.

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=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

Now only ReservePlayer needs (or can have) those comparison methods. Remove from InvaderPlayer and RobotPlayer:

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

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

Green and good. Commit: Removing duplicate code only applicable to ReservePlayer.

The common code between the objects is now down to just this:

# COMMON ELEMENTS

    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

Pfff. That’s not worth really worrying about. We could certainly put a utility function in u.py for the screen fraction. Shall we try it? It should take a position as a parameter, not an object.

Let’s extract a variable in one of the classes:

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

Now extract a function:

    def x_fraction(self):
        position_x = self.position.x
        return self.screen_fraction(position_x)

    def screen_fraction(self, position_x):
        x = position_x - u.INVADER_PLAYER_LEFT
        total_width = u.INVADER_PLAYER_RIGHT - u.INVADER_PLAYER_LEFT
        return x / total_width

Now move that function over to u.py:

def screen_fraction(position_x):
    x = position_x - INVADER_PLAYER_LEFT
    total_width = INVADER_PLAYER_RIGHT - INVADER_PLAYER_LEFT
    return x / total_width

Change the caller to use it:

    def x_fraction(self):
        position_x = self.position.x
        return u.screen_fraction(position_x)

Inline variable:

    def x_fraction(self):
        return u.screen_fraction(self.position.x)

Inline the function itself:

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

And it’s gone from that class, which happens to be RobotPlayer. We change InvaderPlayer and ReservePlayer to be the same, not going through all the rigmarole, just pasting in the fraction line and removing x_fraction.

Curiously, when I change InvaderPlayer, some tests break. They don’t want me to remove x_fraction, how odd.

Oh. The fire method also uses x_fraction. Fix:

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

Green. Good. Commit: move screen fraction computation to u.py utility function.

There was another implementor of the old x_fraction method. Find and change. Green. Commit: use screen_fraction, remove x_fraction.

Pause to Reflect

We’re left with nothing in common except those few lines in the init, setting up the sprite and position, the response to the destructor, and this method:

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

That says why, it doesn’t say what. It should be named explode. Rename it in all those places. Commit: rename method to explode.

I just noticed that the InvaderShot does not die when it hits a robot player. Fix and commit: invader shot dies on hitting robot.

We should have a test for that. Slow down, Ron.

Add the test, much like a couple of others:

    def test_dies_on_robot(self):
        fleets = Fleets()
        fi = FI(fleets)
        player = RobotPlayer()
        player.position = Vector2(100, 100)
        shot = InvaderShot(Vector2(100, 100), Sprite.rollers())
        assert shot.colliding(player)
        fleets.append(shot)
        assert fi.invader_shots
        shot.interact_with_invaderplayer(player, fleets)
        fleets.end_interactions()
        assert not fi.invader_shots

Commit: verify invader shot dies on robot.

Reflection

Where are we? Not where we thought we’d be. We have one four line method that about four objects have approximately in common, the explode method. The other remaining similarities among our xPlayer classes are more coincidental than indicative of a missing abstraction.

Do we need an InvadersExploder object, so that we’d say something like

        ...
        Exploder.explode(PlayerExplosion, "explosion", self.position, fleets)
        ...

Instead of

        ...
        self.explode(fleets)
        ...

    def explode(self, fleets):
        frac = u.screen_fraction(self.position.x)
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

Possibly. I’m not loving the idea right now. For now, it’s time to sum up.

Summary

It’s interesting how I’ll be going along doing generally useful things, making code more similar and then simplifying it, lots of little steps that more or less make sense on their own. but with a longer-range plan in the back of my mind. In this case it was the idea of breaking out separate classes for the various players, and then morphed into making one helper with the common elements.

That idea just kind of evaporated, as the common code got smaller and smaller. One bit got pushed to u.py, a new concept for us, a utility function. (I think there may be other free-standing functions in here somewhere, either local to some file or just stuffed somewhere that seemed reasonable. Putting them in u.py might be better, if we see them.)

Let’s do one thing: Let’s change the screen_fraction function to expect position, not position.x and change the callers. That will remove a tiny bit of duplication (.x four times) and reduce our need to think by a tiny margin. Trivial change, small but non-zero value.

def screen_fraction(position):
    x = position.x - INVADER_PLAYER_LEFT
    total_width = INVADER_PLAYER_RIGHT - INVADER_PLAYER_LEFT
    return x / total_width

The callers all change something like this:

    def explode(self, fleets):
        frac = u.screen_fraction(self.position)  # formerly position.x
        player.play_stereo("explosion", frac)
        fleets.append(PlayerExplosion(self.position))
        fleets.remove(self)

I think that’s better. Besides, we might get four-channel audio someday. Commit: screen_fraction takes position, not position.x.

How do you feel about this, Ron?

Each day that I’ve worked on this, I’ve felt relaxed and generally good, feeling that I was improving the code just a bit, in very small steps. I thought I knew where I was headed, and never felt like I was off track or confused. When I got to the end of the small changes, I was not where I thought I would be. I thought I would see a clever breakout of my three classes into three much simpler ones and a fourth helper, or one consolidated one and three different little classes that isolated, well, differences.

When I got there, that was not what I saw. No big deal: we’re exploring, not following a map. We get to the top of a hill, imagining that there might be a stream on the other side, but instead there is a pond. OK, fine. Don’t ford stream, walk around pond.

There is still that very tiny bit of duplication in the explode method: at least four objects have similar code for exploding.

If we wanted to, we could create a concrete superclass Exploder and inherit it. That would give Hill the creeps, with concrete inherited behavior, but it may well be that that’s the best way to do delegation in Python … or at least the best way that I know.

Must think about that. Delegation in Python: Good Ways to Do It. Might lead somewhere.

For now, we are in a good place. We did not go today where yesterday we predicted we’d go. No big deal. Yesterday I thought I’d take Kress Road. Maybe today I’ll stay on M-36. We decide where to go next as we move along the road.

See you down the road!