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()
if self.amount:
else:
``````

Would this be better?

``````    def tick(self, delta_time, fleets):
fleets.clear()
``````

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()
``````

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.

``````    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.

``````    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!