Python 67 - Looking Around
We do a bit of review of the code today, and we find some nice improvements.
We have moved a lot of “functionality” out of Game class and down into Fleets, Fleet, and flyer classes. Some of those changes went smoothly, and some were a bit rough, although none of them were disasters. But now that the capabilities are moved and working, I want to look around and see whether they code is “right”. I think the basic responsibilities are in the right place, or nearly so, but since mostly I stopped working when the code ran, it seems likely that the code could be better.
Why do I do this? Well, in a toy asteroids game done at home, I do it because I like things to be right, because I like pushing code, and because I get to write about what happens. But in “real life”, whatever that may be, it’s a good bet that any code we write will be revisited, often soon, sometimes later, and if it’s good clear code, we’ll be able to make necessary changes rapidly, and if it’s not so good, it will slow us down.
I think it’s worth one more brief sanding pass over this, and even if we visit it later, I think it pays to clean up the things we see. But taking the time now, before tearing up the card, may be best, because our minds are still relatively clear about what has been done.
Anyway, you do you. I’m going to look into these classes and see what I see.
The basic “trick” that we applied in moving these responsibilities was to create instances of Timer, in a suitable object, and tick those timers. Each timer, when it times out, triggers an action, whether to attempt to fire a missile, to create a new saucer, whatever. So let’s see all the timers.
There are 18 references to Timer, of which six are actual uses in the Game:
Some of these are not the results of our recent refactoring, but we’ll look at them all anyway.
Missile
class Missile:
self.timer = Timer(u.MISSILE_LIFETIME, self.timeout)
def update(self, delta_time, missiles):
self.timer.tick(delta_time, missiles)
def timeout(self, missiles):
missiles.remove(self)
Wouldn’t you just know that the one you were sure was just fine would have an issue? This works well, but what is this update method? Why isn’t the timer ticked in the Missile’s tick method:
class Missile:
def tick(self, delta_time, fleet, _fleets):
self.move(delta_time, fleet)
return True
I vaguely recall that there is a method somewhere in game that calls update, only on missiles, predating our general use of tick. Find senders.
Ah, it’s just us, in move:
class Missile:
def move(self, delta_time, missiles):
self.update(delta_time, missiles)
position = self.position + self.velocity * delta_time
position.x = position.x % u.SCREEN_SIZE
position.y = position.y % u.SCREEN_SIZE
self.position = position
I think we should move that into tick and rename it to time_out. No, that’s a terrible name, we already have a method called timeout. Rename it to tick_timer. Then move it to tick:
def tick(self, delta_time, fleet, _fleets):
self.tick_timer(delta_time, fleet)
self.move(delta_time, fleet)
return True
But now move doesn’t need the missiles parm.
class Missile:
def move(self, delta_time):
position = self.position + self.velocity * delta_time
position.x = position.x % u.SCREEN_SIZE
position.y = position.y % u.SCREEN_SIZE
self.position = position
def tick_timer(self, delta_time, missiles):
self.timer.tick(delta_time, missiles)
def tick(self, delta_time, fleet, _fleets):
self.tick_timer(delta_time, fleet)
self.move(delta_time)
return True
def timeout(self, missiles):
missiles.remove(self)
Tests are green, game works, commit: refactor missile timeout for alleged clarity.
So that was interesting. A small improvement, but definitely an improvement. Saucer has two timers, let’s review them as long as we’re here.
Saucer
class Saucer:
# noinspection PyAttributeOutsideInit
def set_firing_timer(self):
self.missile_timer = Timer(u.SAUCER_MISSILE_DELAY, self.fire_if_missile_available)
def set_zig_timer(self):
# noinspection PyAttributeOutsideInit
self.zig_timer = Timer(u.SAUCER_ZIG_TIME, self.zig_zag_action)
def zig_zag_action(self):
self.velocity = self.new_direction()
def tick(self, delta_time, fleet, fleets):
saucer_missiles = fleets.saucer_missiles
ships = fleets.ships
self.fire_if_possible(delta_time, saucer_missiles, ships)
self.move(delta_time, fleet)
return True
def fire_if_possible(self, delta_time, saucer_missiles, ships):
self.missile_timer.tick(delta_time, saucer_missiles, ships)
I want to track down into the firing logic, because I have a p-baked idea about it but where is the code for checking zig-zag? Is that in move again?
def move(self, delta_time, saucers):
# self.fire_if_possible(delta_time, saucer_missiles, ships)
self.check_zigzag(delta_time)
self.position += delta_time * self.velocity
x = self.position.x
if x < 0 or x > u.SCREEN_SIZE:
if self in saucers:
saucers.remove(self)
We see that we (well, I, you’re smarter than this) moved the one method but not the other. I’ll move check_zigzag to tick. Green. Commit: move check_zigzag to tick.
def move(self, delta_time, saucers):
self.position += delta_time * self.velocity
x = self.position.x
if x < 0 or x > u.SCREEN_SIZE:
if self in saucers:
saucers.remove(self)
def tick(self, delta_time, fleet, fleets):
saucer_missiles = fleets.saucer_missiles
ships = fleets.ships
self.fire_if_possible(delta_time, saucer_missiles, ships)
self.check_zigzag(delta_time)
self.move(delta_time, fleet)
return True
Looking at move reminds me of a defect. When the saucer passes outside the sides of the screen, it removes itself. But there is no check for going off the top or bottom and in fact sometimes it disappears off the top or bottom and never comes back. It should wrap around. The fix is this:
def move(self, delta_time, saucers):
self.position += delta_time * self.velocity
self.position.y = self.position.y % u.SCREEN_SIZE
x = self.position.x
if x < 0 or x > u.SCREEN_SIZE:
if self in saucers:
saucers.remove(self)
I do not have a test for this. I should have. OK.
def test_off_low(self):
saucer = Saucer()
saucer.position = Vector2(100, 3)
saucer.velocity = Vector2(100, -100)
saucer.move(0.1, [])
assert saucer.position.y > 1000
def test_off_high(self):
saucer = Saucer()
saucer.position = Vector2(100, 1021)
saucer.velocity = Vector2(100, 100)
saucer.move(0.1, [])
assert saucer.position.y < 50
OK, happy now? Commit: fix bug where saucer goes off top or bottom and doesn’t wrap around.
OK, I’m glad that came up. It was bothering me, because I knew it was happening but the customer never noticed. No, actually, I did. Anyway, fixed now.
I was going to check out the firing logic.
class Saucer:
def fire_if_missile_available(self, saucer_missiles, ships):
if self.a_missile_is_available(saucer_missiles):
self.fire_a_missile(saucer_missiles, ships)
return True
else:
return False
@staticmethod
def a_missile_is_available(saucer_missiles):
return len(saucer_missiles) < u.SAUCER_MISSILE_LIMIT
def fire_a_missile(self, saucer_missiles, ships):
saucer_missiles.append(self.create_missile(ships))
Well, that’s not too bad. The True / False return is there to keep the timer running. Timer actions return True if they completed, False if they want another change next time. True resets the timer, False keeps triggering.
I was thinking that maybe the Fleet (saucer_missiles) should be more help, but checking the len to get the number of missiles in use is pretty straightforward.
Let’s speculate.
Suppose there was a MissileFleet. (Currently there is no such class.) The MissileFleet could have a limit, two for Saucer, four for Ship, part of its __init__. It could have a method, fire_if_you_can or something, where the Ship or Saucer would pass it a missile and MissileFleet would append it or not, depending on how many missiles were out there. It would return True/False depending on whether it fired.
Ah, but that would have us creating missiles to try to fire, whether they were going to fire or not.
We could give the MissileFleet method a callback, so that if there is room, it would call us back and we’d give it the missile to fire, and then it would return True/False so we’d know whether to reset. That way the missile would be created only if it was needed.
Meh. That would be cool, but it would be a lot of mechanism to replace a check for len.
Enough speculation. I think Saucer is good. Moving right along …
SaucerFleet
class SaucerFleet:
class SaucerFleet(Fleet):
def __init__(self, flyers):
super().__init__(flyers)
self.timer = Timer(u.SAUCER_EMERGENCE_TIME, self.bring_in_saucer)
def bring_in_saucer(self, fleets):
self.flyers.append(Saucer())
return True
def tick(self, delta_time, fleets):
super().tick(delta_time, fleets)
if not self.flyers:
self.timer.tick(delta_time, fleets)
return True
Hard to find much fault with that. I would like to rename flyers, the generic term for Fleet contents, to saucers, but the term is defined in the generic class, Fleet, so we’re stuck with the name, I guess.
Good enough. Moving right along …
ShipFleet
ShipFleet is a bit more complicated because of the ship emergence logic. Before I even start to trek through the code, I notice this:
class ShipFleet:
def spawn_ship_when_ready(self, fleets):
if not self.ships_remaining:
ShipFleet.game_over = True
return True
if self.safe_to_emerge(fleets):
ships = fleets.ships
ships.append(Ship(u.CENTER))
ShipFleet.ships_remaining -= 1
return True
else:
return False
Why am I fetching ships from fleets? I am the ships fleet.
def spawn_ship_when_ready(self, fleets):
if not self.ships_remaining:
ShipFleet.game_over = True
return True
if self.safe_to_emerge(fleets):
self.append(Ship(u.CENTER))
ShipFleet.ships_remaining -= 1
return True
else:
return False
Green. Commit: remove redundant fetch of fleets.ships in ShipFleet.
Tracing forward, how does safe_to_emerge work?
def safe_to_emerge(self, fleets):
if len(fleets.missiles) > 0:
return False
if len(fleets.saucer_missiles) > 0:
return False
return self.asteroids_far_enough_away(fleets.asteroids)
def asteroids_far_enough_away(self, asteroids):
for asteroid in asteroids:
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
return False
return True
What we see here is pretty serious feature envy. The second method doesn’t refer to self at all, and the first one wouldn’t, except that it calls the second one.
The first one refers to fleets three times and to no other object. This is a very subtle clue that Fleets might be a better place for this logic.
To kind of double-check that thinking, Fleets is basically the condition of the universe. It has all the objects that exist. And the ship wants to know about the universe: is it safe for a ship to come out? We can think of Fleets as the ship’s sensor array or something.
Do we have tests for safe_to_emerge? I think we have. Oh yes, lots:
def test_ship_rez(self):
ShipFleet.rez_from_fleet = True
ships = []
fleets = Fleets([], [], [], [], ships)
ship_fleet = fleets.ships
assert not ships
ship_fleet.tick(0.1, fleets)
assert not ships
ship_fleet.tick(u.SHIP_EMERGENCE_TIME, fleets)
assert ships
def test_unsafe_because_missile(self):
ShipFleet.rez_from_fleet = True
ships = []
missile = Missile(u.CENTER, Vector2(0,0), [0,0,0], [0,0,0])
missiles = [missile]
fleets = Fleets([], missiles, [], [], ships)
ship_fleet = fleets.ships
assert not ships
ship_fleet.tick(u.SHIP_EMERGENCE_TIME, fleets)
assert not ships
missiles.clear()
ship_fleet.tick(0.001, fleets)
assert ships
def test_unsafe_because_saucer_missile(self):
ships = []
missile = Missile(u.CENTER, Vector2(0,0), [0,0,0], [0,0,0])
saucer_missiles = [missile]
fleets = Fleets([], [], [], saucer_missiles, ships)
ship_fleet = fleets.ships
assert not ships
ship_fleet.tick(u.SHIP_EMERGENCE_TIME, fleets)
assert not ships
saucer_missiles.clear()
ship_fleet.tick(0.001, fleets)
assert ships
def test_unsafe_because_asteroid(self):
ShipFleet.rez_from_fleet = True
ships = []
asteroid = Asteroid()
asteroid.position = u.CENTER + Vector2(u.SAFE_EMERGENCE_DISTANCE - 0.1, 0)
asteroids = [asteroid]
fleets = Fleets(asteroids, [], [], [], ships)
ship_fleet = fleets.ships
assert not ships
ship_fleet.tick(u.SHIP_EMERGENCE_TIME, fleets)
assert not ships
asteroids.clear()
ship_fleet.tick(0.001, fleets)
assert ships
I think that if I were to just defer from ShipFleet up to Fleets, these tests would start breaking and we could then “just” make them run again.
class ShipFleet:
def safe_to_emerge(self, fleets):
return fleets.safe_to_emerge()
# if len(fleets.missiles) > 0:
# return False
# if len(fleets.saucer_missiles) > 0:
# return False
# return self.asteroids_far_enough_away(fleets.asteroids)
I commented out the checks so that I don’t have to remember so much. Five tests are failing.
class Fleets:
def safe_to_emerge(self):
return False
That doesn’t help much. I’ll do better, I promise.
def safe_to_emerge(self):
if len(self.missiles) > 0:
return False
if len(self.saucer_missiles) > 0:
return False
return True
Now only one test is failing. I don’t have to look to know it’s the asteroids in range test. Pop that in:
class Fleets:
def safe_to_emerge(self):
if len(self.missiles) > 0:
return False
if len(self.saucer_missiles) > 0:
return False
return self.all_asteroids_are_away_from_center()
def all_asteroids_are_away_from_center(self):
for asteroid in self.asteroids:
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
return False
return True
Tests are green. Remove stuff from ShipFleet.
class Fleet:
def spawn_ship_when_ready(self, fleets):
if not self.ships_remaining:
ShipFleet.game_over = True
return True
if self.safe_to_emerge(fleets):
self.append(Ship(u.CENTER))
ShipFleet.ships_remaining -= 1
return True
else:
return False
def safe_to_emerge(self, fleets):
return fleets.safe_to_emerge()
That could benefit from an inline, I think:
def spawn_ship_when_ready(self, fleets):
if not self.ships_remaining:
ShipFleet.game_over = True
return True
if fleets.safe_to_emerge():
self.append(Ship(u.CENTER))
ShipFleet.ships_remaining -= 1
return True
else:
return False
Commit: Move safe_to_emerge from ShipFleet to Fleet.
So. Let’s reflect.
Reflection
ShipFleet was pulling all kind of data out of Fleets, picking its pockets, violating the Law of Demeter and flagging its methods as “could be static”, PyCharm’s charming way of saying “uh you don’t even references self here are you sure about this?”
Moving the question over to Fleets removed two questionable methods from ShipFleet, replacing them with two methods that make more sense in the Fleets context.
Is there more to wonder about? Well, there’s ShipFleet.game_over. That’s fairly deep in the nest of objects to be storing such a game-critical value, but the fact is that it’s determined down here. It’s ShipFleet that righteously knows whether we are out of ships.
I am tempted, so tempted, to do something differently about this, but my best ideas are a callback up to Game or to raise an exception1, but I really don’t like those options.
I think we’ll let this ride. We’ve improved things and we’re nigh on to 500 lines of article.
Summary
We found things to improve, everywhere we looked. Honestly, I think that’s good: we don’t want perfect code, we want better code. So if we see a few things and clean them up, the whole code space will tend to be tidy enough to let us work with ease. Working with ease is the fastest way to work.
Do I, personally, spend too much time improving code, if I were in a product situation? Yes, quite possibly. On the other hand, the changes today were each in just one or two methods, and writing them up takes far more time than doing them. So, quite possibly, even being as fastidious as I am about going back over the code would still pay off.
But the central point isn’t how often I do it, it is how often a small improvement is possible and easy to do, making the code more amenable to future change. A place for everything, and everything in its place. Who said that? I have no idea.
And the tests! Today’s final change, moving the safe-to-emerge concept to a different class was made almost trivial by the tests. They stopped running when I created an empty method, and ran one after another as I added the clauses to that method. So nice to have them. I never regret having tests and often regret not having them. And sometimes, like today, I’m just delighted to have them.
We’ll glance around further in upcoming sessions, and we also have some yellow sticky Jira notes to deal with.
A good morning. What time is the Derby?
See you next time!
-
Nobody wants an exception. – Chet Hendrickson ↩