Python Asteroids on GitHub

We’ll start with some philosophizing, and then we’ll do a review of the saucer’s missile-firing logic. Fun is actually the point.

How much “quality” is too much? How much is too little? How much focus should we have on new or interesting technology and how much on “the business”? I was recently engaged in a short bit of conversation about that, on a Slack I belong to.

I have made some serious mistakes in product developments, that could be described as picking the wrong technology to use. I’ve had products get scrapped, because they didn’t deliver in time to avoid the axe, where we were using Forth, or Smalltalk, or Extended Set Theory (as we understood it). Would they not have been scrapped, would we have delivered, if we had been instead using C++ or Plain Old Relational Thinking?

There’s no way to know. My guess would be that the issue wasn’t the language or theory, but that we were following a pattern of development that focused on getting everything done before we could release anything. If I were condemned to live my development life over, and could only take one thing back to the past with me, I would want to take my understanding of the ability to deliver continuously instead of just “at the end”.

On the Slack, folks brought up some interesting issues. Perhaps the “good developers” who write the product pick some powerful language (Haskell was the example chosen), and maybe they even do a good job, but then the “poorly funded high turnover team in Elbonia” takes maintenance and cannot do the job.

I asserted at that point that the real bug is having a “poorly funded high turnover maintenance team in Elbonia” and that if we all programmed in Haskell all the time, someone would have to fix that bug or route around it. But there was a countervailing view that developers often become so enamored of the technology that they build an overly complicated mess that is so hard to understand that it has to be thrown away. The notion seemed to be that a fascination with technology leads to a mess.

I have seen some messes in my time, and been a part of creating them. And they did often involve some fascinating technology. However … I do not think it was the fascination with tech that caused the problem. It was, I believe, more down to the unwillingness to rework the code. In those days, that was in large part due to the notion that you wrote the thing and then it was done except for “maintenance”. That was the style at the time1.

Today, the style, at least the style that I would choose to follow and while I do not give advice, I think that my over six decades in this business suggests that you might at least want to consider what I say, the style that I would choose to follow today would be to move as quickly as possible to some tiny subset of the product that could be “shipped”, at least made visible to the company and ideally to real users, and to keep that subset growing and alive from that day forward.

I think that if I were to do that, some key things would (need to) happen:

  • We would have to learn to work incrementally;
  • We would need to learn to refactor;
  • We would need to have automated tests to support us;
  • We would have concrete information for management;
  • Management would see more clearly how to guide the product;
  • Management would want to ship it “now”;
  • Customers would start using it;
  • We’d get better feedback;

And the product would be in the hands of paying users almost before anyone realized it, and it’s much harder to cancel a product that is earning revenue and keeping customers happy.

My teams in the past were excellent developers. I say that now, with many years of learning what “excellent” means. We did not have all the skills that excellent developers need to have today: the whole incremental thing just wasn’t on the table. But with those things on the table, my teams would have seen them, been excited by them, learned them, and used them.

Why? Because we were fascinated by the technology and loved using it to do what we thought we were supposed to do. So, faced with what happens when you try to ship early and often, we would have dug into all the sources to figure out how to do it. I’m sure of it. I was there, and I know how we worked.

So therefore … I want programmer and teams to be profoundly engaged with the technology, and I trust them to use the technology to do what they are called upon to do. So I would argue that if some high-tech team, including some of mine, does go down some technical rat hole, it’s not because they are bad, or because they are technical, but because we didn’t ask them to show us working improved versions of the program every couple of weeks. (See How to Impose Agile for further thoughts.)

What does that mean for us right here?

I think that it means that a fascination with the details of our craft is highly desirable. We do not have to be fanatics, but surely the more attention we pay to doing our job well, the better job we will do. And, in my view, the more fun we’ll have doing it.

Now, let’s have a look at some code.

Saucer Firing Logic

I am interested in whether the various stages and concerns in firing missiles are well-reflected in our objects. We would like each object to have a rather cohesive set of concerns, and to be as independent of the other levels as it can be.

We’ll browse the saucer’s firing logic and see what we it tells us. It begins, as one might suspect, in Saucer:

Saucer

class Saucer(Flyer):
    def update(self, delta_time, fleets):
        player.play(self._sound, self._location, False)
        self.fire_if_possible(delta_time, fleets)
        self.check_zigzag(delta_time)
        self._move(delta_time, fleets)

    def fire_if_possible(self, delta_time, fleets):
        self._gunner.fire(delta_time, self, self._ship, fleets)

All Saucer does is, on every update (the top of the game cycle), it asks the gunner to fire. One simple firing concern: always ask the gunner to fire.

Gunner

class Gunner:
    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_if_missile_available, saucer, ship_or_none, fleets)

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if did_we_fire := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.fire_available_missile(saucer, ship_or_none, fleets)
        return did_we_fire

    def fire_available_missile(self, saucer, ship_or_none, fleets):
        if ship_or_none and self.should_target():
            solution = ShotOptimizer(saucer, ship_or_none).targeted_solution
        else:
            solution = ShotOptimizer(saucer, ship_or_none).random_solution
        fleets.append(solution.saucer_missile())

    def should_target(self):
        return self._always_target or random.random() < u.SAUCER_TARGETING_FRACTION

Well, the Gunner ticks its timer and if the timer has elapsed, does fire_if_missile_available.

fire_if_missile_available makes sure that there are fewer missiles on the screen than the saucer’s limit (2), and if there’s room for one more, it calls fire_available_missile. It returns whether or not it fired, because the timer will keep triggering until we finally get to fire one.

We have two kinds of firing, targeted or random. We check to see if there is a ship on screen to shoot at, and if we should target it. If so, we ask the ShotOptimizer for a targeted solution, otherwise a random one. We “should target” if we _always_target or if the dice are not in your favor. The _always_target flag is on for the small saucer and off for the large one.

You could make a case that there’s too much going on here. We manage the timer and we decide what kind of missile to fire. We consider the existence of missiles, of the ship, the size of the saucer, and the roll of the dice. Is this object cohesive enough, or not? It’s mostly about specifics of ship and saucer (and the player’s bad luck). It’s not about details like position or velocity. So it might be OK.

Still, while this code is fairly clear, looking at it from the viewpoint of separate things going on does make me wonder whether we might benefit from improving Gunner. For now we’ll move on.

ShotOptimizer

class ShotOptimizer:
    def __init__(self, saucer, ship):
        self.saucer = saucer
        self.ship = ship

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        safe_distance = self.saucer.missile_head_start
        target_position = self.lead_the_target(best_target_position, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance)

    @property
    def random_solution(self):
        return FiringSolution(self.random_position(), self.saucer.position, self.saucer.missile_head_start)

Off the bat, we see that ShotOptimizer knows the ship and the saucer. Makes sense: we need both of those in order to target a missile. However, its concern is almost entirely the positions and velocities of those objects, which it extracts immediately. This seemed better to me than requiring the caller to provide those details. Here, the ShotOptimizer knows the details it needs and the caller just gives it the ship and saucer. I’m good with that, I think.

We see that both methods called from Gunner return a FiringSolution. We’ll look at that shortly. We can see from here that a FiringSolution has two positions and a safe distance. We know from history that safe distance is the little offset that the missile gets so as not to explode its source, the saucer. We nod and move on.

The interesting method is of course the targeted_solution. We double check that there is a ship. I think this should never happen, but if there isn’t one, we can’t target it and things would go bad if we tried. Belt and suspenders, perhaps. (No onions, however.)

Let’s focus in on targeted_solution:

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        safe_distance = self.saucer.missile_head_start
        target_position = self.lead_the_target(best_target_position, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance)

What’s going on here?

We push down a level and improve some code

I don’t like the order of these calls. The reason is that one tricky bit is best_target_position and we skip a line before we use it.

Let’s try this:

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        safe_distance = self.saucer.missile_head_start
        best_target_position = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        target_position = self.lead_the_target(best_target_position, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance)

Better? Probably. I don’t like the name best because it isn’t best, it’s our initial choice.

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        safe_distance = self.saucer.missile_head_start
        initial_aiming_point = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        target_position = self.lead_the_target(initial_aiming_point, safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance)

The other issue is that the first two lines here are Explaining Variable Name, simply expressing what those calls do in the final call to FiringSolution. So is the last one. But the one in the middle is preparing an argument, not for FiringSolution, but for lead_the_target. Let’s see if we can extract those last two lines into a meaningful method.

    @property
    def targeted_solution(self):
        if not self.ship:
            return self.random_solution
        shooter_position = self.saucer.position
        safe_distance = self.saucer.missile_head_start
        target_position = self.choose_aiming_point(safe_distance, shooter_position)
        return FiringSolution(target_position, shooter_position, safe_distance)

    def choose_aiming_point(self, safe_distance, shooter_position):
        initial_aiming_point = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        target_position = self.lead_the_target(initial_aiming_point, safe_distance, shooter_position)
        return target_position

Better. But in the second method there we can do better still:

    def choose_aiming_point(self, safe_distance, shooter_position):
        initial_aiming_point = self.closest_aiming_point(shooter_position, self.ship.position, u.SCREEN_SIZE)
        improved_aiming_point = self.lead_the_target(initial_aiming_point, safe_distance, shooter_position)
        return improved_aiming_point

I think I like it. We could look at closest... but I think lead... is more interesting.

    def lead_the_target(self, target_position, safe_distance, shooter_position):
        aim_improver = AimImprover(target_position, self.ship.velocity, shooter_position, u.MISSILE_SPEED, safe_distance)
        for _ in range(3):
            target_position = aim_improver.improved_aiming_point(target_position)
        return target_position

Would a better name for improved_aiming_point be helpful? Something like better_aiming_point, or improve, or get_improved_aiming_point? How about adjust_aiming_point? refine_aiming_point?

Let’s try that last one.

    def lead_the_target(self, target_position, safe_distance, shooter_position):
        aim_improver = AimImprover(target_position, self.ship.velocity, shooter_position, u.MISSILE_SPEED, safe_distance)
        for _ in range(3):
            target_position = aim_improver.refine_aiming_point(target_position)
        return target_position

I think I like that better. Commit: tidying: extract method, some renaming.

We pop back up and look again at cohesion

But we are here for the big picture, not just small refinements. How cohesive is ShotOptimizer? Does it have more concerns than it should?

Well, arguably it has two, deliver a targeted FiringSolution or a random one, since it actually has two methods. We could, I suppose, have a TargetedShotOptimizer and RandomShotOptimizer class and decide at the Gunner level, but since the targeted one can possibly devolve to random, this is OK.

Its job is, given ship and saucer, deliver a FiringSolution. Pretty cohesive, I think.

Firing Solution

Let’s glance at FiringSolution:

class FiringSolution:
    def __init__(self, target_position, shooter_position, safe_distance):
        direction_to_target = (target_position - shooter_position).normalize()
        safety_offset = direction_to_target * safe_distance
        self.velocity = direction_to_target * u.MISSILE_SPEED
        self.start = shooter_position + safety_offset

    def saucer_missile(self):
        return Missile("saucer", self.start, self.velocity)

Given a target position, shooter position, and safe distance, provide a missile suitable for firing.

One job, one object. Highly cohesive. Small, compact. Nice.

AimImprover

How about AimImprover? I think it clearly has but one concern: Given the targeting parameters and an initial aiming point, provide a better aiming point. It does take a bit of code to accomplish that:

class AimImprover:
    def __init__(self, best_target_position, target_velocity, shooter_position, missile_speed, safe_distance):
        self.original_target_position = best_target_position
        self.target_velocity = target_velocity
        self.shooter_position = shooter_position
        self.missile_speed = missile_speed
        self.safe_distance = safe_distance

    def refine_aiming_point(self, initial_aiming_point):
        missile_travel_time = self.missile_travel_time_to(initial_aiming_point)
        anticipated_target_position = self.target_position_at_time(missile_travel_time)
        return anticipated_target_position

The top level is just that much. We pack away all kinds of useful info, we compute the travel time for the missile to the initial aiming point (which is our current guess at where the target (ship) will be), and then we calculate where the target will actually be at that time, and return that as our result.

How do we know the travel time? Easy, it’s travel distance over missile speed:

    def missile_travel_time_to(self, initial_aiming_point):
        missile_travel_distance = self.shooter_position.distance_to(initial_aiming_point) - self.safe_distance
        missile_travel_time = missile_travel_distance / self.missile_speed
        return missile_travel_time

And where will the target be? It’ll be where it started plus its motion, namely its velocity times the time it travels:

    def target_position_at_time(self, missile_travel_time):
        target_motion_vector = missile_travel_time * self.target_velocity
        anticipated_target_position = self.original_target_position + target_motion_vector
        return anticipated_target_position

We’ve polished this with about 10,000 grit by now, although we might still come up with a better name here and there. But the main issue I wanted to look at is how many concerns the object has and there is really just one: improve a targeting solution given all the info one needs in order to do that.

And therefore?

We’re looking at how many concerns these objects have—how cohesive they are—and we get something like this:

Saucer
Ask gunner to fire
Gunner
Check timer; check missile count; decide what kind of missile to fire; ask for a firing solution; fire the solution’s missile.
ShotOptimizer
Select random or targeted; select initial aiming point; improve it; create firing solution.
AimImprover
Given all required info, improve a proposed target position to account for missile travel time.

We might be concerned about creating new, more cohesive, smaller objects for Gunner and ShotOptimizer to use. Let’s speculate a bit about what those objects might be.

Gunner could tick the timer and ask the MissileStore to fire if possible. That object would have to return the True / False that we need in the timer.

MissileStore could check missile count and conditionally ask a MissileSelector to select and fire a missile (is that two concerns or just one?)

MissileSelector could check the existence of a ship and the dice, and ask ShotOptimizer for a FiringSolution.

If we really wanted to know what that design would look like, we’d have to code it, or at least lay it out on “paper”, but it seems that we’d pass down a bunch of arguments to create the thing and then send it one message, and that method would be implemented much like the corresponding method in the current Gunner.

Measures like cohesiveness have always seemed to me to be fairly vague guidelines, not strict metrics. And I’d freely grant that Gunner and ShotOptimizer seem to have a bit more going on than they might. But they’re also pretty simple, especially ShotOptimizer, which basically just has a sequence of operations that it goes through.

I wouldn’t deny that a better design for Gunner might exist, using a couple more objects of some kind. I just don’t see what those might be. It might be fun to take a look. I’m sure we’d learn something.

But if we were to do that, that would be another time. For now:

Summary

I am inclined to give the overall firing logic a decent grade, perhaps a B+ or A-. Perhaps even an A. I would not give it an A+, because Gunner seems capable of being improved. The individual objects seem fairly well positioned to deal with separable topics, and the interfaces seem pretty simple to me.

Could it be better? Of course. Is it good enough? Absolutely, and has been for quite some time. We’re considering it not because it’s bad, but because out on the edge of pretty good, there’s learning to be had.

See you next time! Bring onions!


  1. We also tied onions to our belts, which was the style at the time. ↩