Python Asteroids on GitHub

I am trying to do little things in a house full of banging. I found something possibly worth thinking about. And maybe I go off the rails. I’m not sure.

I found this, in Coin:

class Coin(FLyer):
    def tick(self, delta_time, fleets):
        fleets.clear()
        fleets.add_flyer(SaucerMaker())
        fleets.add_flyer(ScoreKeeper())
        fleets.add_flyer(Thumper())
        fleets.add_flyer(WaveMaker())
        if self.amount:
            fleets.add_flyer(ShipMaker())
        else:
            fleets.add_flyer(GameOver())

Would this be better?

    def tick(self, delta_time, fleets):
        fleets.clear()
        fleets.add_flyer(SaucerMaker())
        fleets.add_flyer(ScoreKeeper())
        fleets.add_flyer(Thumper())
        fleets.add_flyer(WaveMaker())
        fleets.add_flyer(ShipMaker() if self.amount else GameOver())

What if we did this?

class Coin(Flyer):
    @classmethod
    def quarter(cls):
        return cls(True)

    @classmethod
    def slug(cls):
        return cls(False)

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

    def tick(self, delta_time, fleets):
        fleets.clear()
        fleets.add_flyer(SaucerMaker())
        fleets.add_flyer(ScoreKeeper())
        fleets.add_flyer(Thumper())
        fleets.add_flyer(WaveMaker())
        fleets.add_flyer(ShipMaker() if self.is_quarter else GameOver())

I like that better still. You could argue that having two subclasses of Coin, Slug and Quarter might be better. But I’ll settle for this. Commit: Refactor Coin for a tiny bit of clarity.

Unclear Code

I found this less than clear:

class MovableLocation:
    def move(self, delta_time):
        position = self.position + self.velocity*delta_time
        old_x = position.x
        old_y = position.y
        position.x = position.x % self.size
        position.y = position.y % self.size
        self.position = position
        return position.x != old_x, position.y != old_y

This function returns two booleans, True if the location has wrapped around in x or y. I do not find that obvious.

Let’s see if we can make it better. I’m not sure just how.

How about this?

    def move(self, delta_time):
        position = self.position + self.velocity*delta_time
        unwrapped = position.copy()
        position.update(position.x % self.size, position.y % self.size)
        self.position = position
        return position.x != unwrapped.x, position.y != unwrapped.y

I’m not impressed. This is done a billion times a second so we don’t really want to be all that fancy.

    def move(self, delta_time):
        new_x = self.position.x + self.velocity.x*delta_time
        new_y = self.position.y + self.velocity.y*delta_time
        wrapped_x = new_x % self.size
        wrapped_y = new_y % self.size
        self.position = Vector2(wrapped_x, wrapped_y)
        return new_x != wrapped_x, new_y != wrapped_y

Possibly better names:

    def move(self, delta_time):
        raw_x = self.position.x + self.velocity.x*delta_time
        raw_y = self.position.y + self.velocity.y*delta_time
        fixed_x = raw_x % self.size
        fixed_y = raw_y % self.size
        self.position = Vector2(fixed_x, fixed_y)
        return raw_x != fixed_x, raw_y != fixed_y

I don’t love that we know that our position is a Vector2. We used to just know that it had x and y.

How about this?

    def move(self, delta_time):
        x, x_changed = self.move_coordinate(self.position.x, delta_time*self.velocity.x, self.size)
        y, y_changed = self.move_coordinate(self.position.y, delta_time*self.velocity.y, self.size)
        self.position = Vector2(x, y)
        return x_changed, y_changed

    def move_coordinate(self, pos, delta_v, size):
        raw_value = pos + delta_v
        fixed_value = raw_value % size
        return fixed_value, fixed_value != raw_value

At least part of the issue is that the function is weird. It updates location and returns an indication of whether it had to wrap.

Try again.

    def move(self, delta_time):
        position = self.position + self.velocity*delta_time
        self.position = Vector2(position.x % self.size, position.y % self.size)
        return position.x != self.position.x, position.y != self.position.y

Once more, rename:

    def move(self, delta_time):
        raw_position = self.position + self.velocity*delta_time
        self.position = Vector2(raw_position.x % self.size, raw_position.y % self.size)
        return raw_position.x != self.position.x, raw_position.y != self.position.y

I sure wish Vector2 understood %. I bet we could implement that.

Let’s pretend we have %.

    def move(self, delta_time):
        raw_position = self.position + self.velocity*delta_time
        self.position = self.wrap_around(raw_position)
        return raw_position.x != self.position.x, raw_position.y != self.position.y

    def wrap_around(self, raw_position):
        return Vector2(raw_position.x % self.size, raw_position.y % self.size)

Commit: refactor move, intending improved clarity.

I think that’s as good as it gets. I welcome comments on which you prefer, or even better ideas.

Summary

Were these things “worth doing”? I think the Coin change certainly was, it just took a moment. The move change? Hard to say, but I do think that when we encounter it a year from now, it’ll be more clear what it does than it would have been.

Compare old and new:

Old:

    def move(self, delta_time):
        position = self.position + self.velocity*delta_time
        old_x = position.x
        old_y = position.y
        position.x = position.x % self.size
        position.y = position.y % self.size
        self.position = position
        return position.x != old_x, position.y != old_y

New:

    def move(self, delta_time):
        raw_position = self.position + self.velocity*delta_time
        self.position = self.wrap_around(raw_position)
        return \
            raw_position.x != self.position.x, \
            raw_position.y != self.position.y

    def wrap_around(self, raw_position):
        return Vector2(
        	raw_position.x % self.size, 
        	raw_position.y % self.size)

What do you think? Me, I think the roofers have driven me mad.

See you next time!