Python 179 - The Code Speaks

Python Asteroids on GitHub

Today I think it might be fun to extract a SafetyChecker class from ShipMaker by refactoring. First I get a better idea, and then the code tells me what it really wants. Fascinating!

ShipMaker has a few responsibilities which we might delineate, call out, or describe thusly:

• Observe when there is no ship;
• Wait a discreet interval;
• Check for safe emergence;
• If the player has ships left, create one;
• If there are no ships left, create a GameOver.

As it happens, it does those things in about that order, which means that when there are no ships remaining, it takes a while before GameOver comes up. That’s OK, but it does come to mind, and I like to record my thinking, since programming is often best when it involves thinking.

Today we plan to extract a SafetyChecker from ShipMaker, via refactoring. The other day, we created two new objects, ShipProviders, and then plugged them in. That’s one way, and one of its drawbacks is that, almost invariably, the object we create almost fits, but not quite. A bit like buying a suit off the rack, I suppose.

When we create a new class by refactoring, we are more likely to get a good fit, especially if we keep things running all the time. However, we are probably a bit more likely to wind up with a special purpose object, fit for the user class from which we extracted it, but perhaps less applicable to other usage. Often, additional usage requires more refactoring.

That will not concern us here: we will only ever have one use of SafetyChecker. Its sole responsibility will be to offload some complexity from ShipMaker.

Let’s speculate a bit about what the SafetyChecker might be like. Ideas include:

• Could be created once and for all when ShipMaker is created;
• Should be reset at every begin_interactions cycle;
• Needs to know whether Saucer or Missile objects appear in the cycle;
• Needs to interact with Asteroids, so that it can analyze their positions and paths;
• Can answer to the ShipMaker whether or not it is safe to create the Ship.

It seems likely to me that we will “just” forward the necessary interact_with_* message from ShipMaker to SafetyChecker.

It also occurs to me that really only one of its functions is complicated, the asteroid analysis. That brings up some options that we hadn’t thought of:

1. Perhaps we only need an AsteroidAnalyzer object, not a SafetyChecker. ShipMaker maintains a _safe_to_emerge flag, and we could just send each asteroid to the analyzer and use its result.
2. Perhaps SafetyChecker would like to have an AsteroidAnalyzer helper.

Enough speculation. It is time to let the code participate in our design session¹. Here’s the code that ShipMaker uses to decide whether it’s safe to rez the ship:

class ShipMaker(Flyer):
    def begin_interactions(self, fleets):
        self._game_over = False
        self._need_ship = True
        self._safe_to_emerge = True

Above, in each cycle, we init some flags. The main one of interest here is _safe_to_emerge but the other two are relevant.

    def interact_with_missile(self, missile, fleets):
        self._safe_to_emerge = False

    def interact_with_saucer(self, saucer, fleets):
        self._safe_to_emerge = False

    def interact_with_ship(self, ship, fleets):
        self._need_ship = False

The three interactions above just note that it is not safe to emerge. It does seem a bit silly to forward anything to a SafetyChecker object if it’s all that simple. An up-tick for the AsteroidAnalyzer idea?

    def interact_with_asteroid(self, asteroid, fleets):
        distance = asteroid.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + asteroid.radius:
            self._safe_to_emerge = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = asteroid._location
            if not ml.moving_away_from(u.CENTER):
                self._safe_to_emerge = False

The method above is the big one, and it has been changing just recently. AsteroidAnalyzer, if we choose to do it, would offload just this function, and SafetyChecker, if we chose to do that, would include this capability one way or another.

Then when we need the flag, and after the timer has elapsed, we do this:

    def create_ship(self, fleets):
        if not self._safe_to_emerge:
            return False
        self.rez_available_ship(fleets)
        return True

The method above is the one that the timer runs. It is called when the timer has expired, and on every tick thereafter, until it finally returns True, signifying that it is satisfied.

Well. Much as I thought a SafetyChecker was the right idea when I walked in here, I now see that most of the complexity is in the single method interact_with_asteroid. Therefore most of the benefit will come from an object for analyzing asteroids, not from a general SafetyChecker. Therefore …

We’ll extract an AsteroidAnalyzer and see if anything further is needed after that.

Important

This is why we let the code participate in our design. The code tells us what it really wants, not what our fallible recollection of the code leads us want, and not what our vivid imagination of some potentially useful object leads us to want.

Furthermore

We discover a bit further down that the code has more to say. It has guided us to pull out a tiny object … and then guides us to replace that object with a single method on another class. Watch closely. Listen to the code.

Let’s begin by extracting a method from here:

    def interact_with_asteroid(self, asteroid, fleets):
        distance = asteroid.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + asteroid.radius:
            self._safe_to_emerge = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = asteroid._location
            if not ml.moving_away_from(u.CENTER):
                self._safe_to_emerge = False

I want the method to return True if the asteroid is safe, and False if it is not. Let’s change it so that you can see why I want that:

    def interact_with_asteroid(self, asteroid, fleets):
        safe = True
        distance = asteroid.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + asteroid.radius:
            safe = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = asteroid._location
            if not ml.moving_away_from(u.CENTER):
                safe = False
        self._safe_to_emerge = self._safe_to_emerge and safe

In this form, the code before the final line just determines whether this specific asteroid is safe, and whether the situation is safe depends on all the asteroids being safe. So _safe_to_emerge is the “and” of all the safe conditions of all the asteroids we check.

I hope that’s clear. If not, help me express it better and I’ll revise.

We can commit: prepare for asteroid_is_safe function.

One step closer. We are committed. I pushed the code to the repo in the sky. Now extract a method:

    def interact_with_asteroid(self, asteroid, fleets):
        safe = self.asteroid_is_safe(asteroid)
        self._safe_to_emerge = self._safe_to_emerge and safe

    def asteroid_is_safe(self, asteroid):
        safe = True
        distance = asteroid.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + asteroid.radius:
            safe = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = asteroid._location
            if not ml.moving_away_from(u.CENTER):
                safe = False
        return safe

PyCharm did the heavy lifting there, of course. I want to inline the temp in the first method:

    def interact_with_asteroid(self, asteroid, fleets):
        self._safe_to_emerge = self._safe_to_emerge and self.asteroid_is_safe(asteroid)

Again PyCharm does the work. Commit: inline variable.

I have mixed feelings about the larger asteroid_is_safe method. Let’s leave it for now, and begin to extract the AsteroidAnalyzer. We’ll use that name for now.

I’ll try “Strangler”. My plan is to create the Analyzer, passing self (ShipMaker) and the asteroid, and to ask it whether it is safe. We’ll begin by having it call us back, which is the first step in Strangler. I am now a bit sorry that I inlined that temp, but I’ll work from here:

    def interact_with_asteroid(self, asteroid, fleets):
        analyzer = AsteroidAnalyzer(self)
        self._safe_to_emerge = self._safe_to_emerge and analyzer.is_safe(asteroid)

This won’t compile, so we create the class:

class AsteroidAnalyzer:
    def __init__(self, shipmaker):
        self._shipmaker = shipmaker

    def is_safe(self, asteroid):
        return self._shipmaker.asteroid_is_safe(asteroid)

This is Strangler². We create a small trivial object, passing it whatever it needs, and implementing its functions just by calling back to the legacy code. In this case, that’s ShipMaker.asteroid_is_safe.

Then, bit by bit, we move the code over. Of course in our case, we only have the one method to move. But we can commit now! AsteroidAnalyzer calls back to ShipMaker.

Now move the method. PyCharm seems not to know how to do that. I am mistaken. It can move it for me, but it moves it to the top level: We have this:

class AsteroidAnalyzer:
    def __init__(self, shipmaker):
        self._shipmaker = shipmaker

    def is_safe(self, asteroid):
        return asteroid_is_safe(asteroid)


def asteroid_is_safe(asteroid):
    safe = True
    distance = asteroid.position.distance_to(u.CENTER)
    ship_radius = 25
    if distance < ship_radius + asteroid.radius:
        safe = False
    elif distance < u.SAFE_EMERGENCE_DISTANCE:
        ml = asteroid._location
        if not ml.moving_away_from(u.CENTER):
            safe = False
    return safe

Strictly speaking, that’s OK. We are green. We commit: Move asteroid_is_safe to top level of AsteroidAnalyzer.

I don’t like that positioning, however, but we are going to learn something in a moment, something that I did not anticipate. I’ll tab that function in and call it with self:

class AsteroidAnalyzer:
    def __init__(self, shipmaker):
        self._shipmaker = shipmaker

    def is_safe(self, asteroid):
        return self.asteroid_is_safe(asteroid)

    def asteroid_is_safe(self, asteroid):
        safe = True
        distance = asteroid.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + asteroid.radius:
            safe = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = asteroid._location
            if not ml.moving_away_from(u.CENTER):
                safe = False
        return safe

Green. Commit: make asteroid_is_safe a method.

What have we learned? Well, PyCharm has squiggled asteroid_is_safe, pointing out the obvious, that it could be a static method. It has absolutely no interest in self.

The code has spoken! This wants to be a method on Asteroid!

Fascinating! Should I be angry that I didn’t see that? Should I be sorry that I wasted all this refactoring? No! I am delighted that my refactoring has shown me this important fact. Let’s see if we can move the code again, over to Asteroid.

PyCharm does this for me:

from asteroid import asteroid_is_safe


class AsteroidAnalyzer:
    def __init__(self, shipmaker):
        self._shipmaker = shipmaker

    def is_safe(self, asteroid):
        return asteroid_is_safe(asteroid)

Again it made asteroid_is_safe a top-level function. Not quite what I really want. I want a method. PyCharm isn’t quite up to that. I’ll go over and tab it in, changing to this:

class Asteroid(Flyer):
    def asteroid_is_safe(self):
        safe = True
        distance = self.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + self.radius:
            safe = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = self._location
            if not ml.moving_away_from(u.CENTER):
                safe = False
        return safe

And:

class AsteroidAnalyzer:
    def __init__(self, shipmaker):
        self._shipmaker = shipmaker

    def is_safe(self, asteroid):
        return asteroid.asteroid_is_safe()

Green. Commit: AsteroidAnalyzer forwards to asteroid.

Of course, ShipMaker has a better alternative now. Not this:

    def interact_with_asteroid(self, asteroid, fleets):
        analyzer = AsteroidAnalyzer(self)
        self._safe_to_emerge = self._safe_to_emerge and analyzer.is_safe(asteroid)

But this:

    def interact_with_asteroid(self, asteroid, fleets):
        self._safe_to_emerge = self._safe_to_emerge and asteroid.asteroid_is_safe()

Green. Commit: ShipMaker uses asteroid.asteroid_is_safe, not analyzer.

Rename that method:

    def interact_with_asteroid(self, asteroid, fleets):
        self._safe_to_emerge = self._safe_to_emerge and asteroid.is_safe_for_emergence()

Commit: rename method to is_safe_for_emergence.

Now there are no references to AsteroidAnalyzer. It has served its purpose and we remove it entirely. Commit: Thanking AsteroidAnalyzer for its brief service and removing it.

Fascinating!

So that was interesting, wasn’t it? I had what seemed like a good idea, extracting all the safety checking from ShipMaker, off to some new object. When I let the code participate, the code suggested that we would get the most benefit from a simpler object, AsteroidAnalyzer.

I might have seen then that we really just had a method that wanted to be on Asteroid, but in fact I did not see that. I am a simple man.

The original method was not marked static, since it referred to members in ShipMaker. The temporarily extracted one may have been, but even if it was, I was by then in the process of creating the Analyzer and was focused there.

A few simple moves later, the Analyzer, which really only ever had one interesting method in it, whispered HEY THIS METHOD DOESN’T BELONG HERE!, and we moved it to Asteroid. Calling the method directly from ShipMaker allowed us to remove the Analyzer stepping stone.

Most of the process was managed by PyCharm, and the tests were running green all the time. I am quite confident that the new code is correct, which is to say that it does the same thing that it was doing before. We should review it soon, but it’s too fresh to do more than take a quick final look now:

    def is_safe_for_emergence(self):
        safe = True
        distance = self.position.distance_to(u.CENTER)
        ship_radius = 25
        if distance < ship_radius + self.radius:
            safe = False
        elif distance < u.SAFE_EMERGENCE_DISTANCE:
            ml = self._location
            if not ml.moving_away_from(u.CENTER):
                safe = False
        return safe

The worst bit there is the ship_radius, which we should either provide or look up. But it’s too fresh in our mind to go refining. We’ll let our memory of how great this all is fade a bit and look at it later. Or not: things are surely far better than they were.

We did not take a straight path from ShipMaker to an Asteroid method. Instead:

1. We considered a step to extract SafetyChecker.
2. It was too much, so we chose a step to extract AsteroidAnalyzer
3. We extracted a trivial Strangler version of AsteroidAnalyzer.
4. We moved the ShipMaker method to top level in AsteroidAnalyzer. That’s because that’s what PyCharm can do. On another day we might do that by hand.
5. We moved the top level function into an AsteroidAnalyzer method.
6. We saw that the method wanted to be on Asteroid.
7. We moved it to top level in Asteroid.
8. We moved it to a method on Asteroid.
9. We called it directly from ShipMaker.
10. We removed AsteroidAnalyzer.

And we committed and pushed working code every time. Well, nine of those ten times. I skipped one somewhere where I could have committed.

Each of these steps was exceeding small, and most of them were machine refactorings. The few that were not amounted to tabbing in and typing self somewhere.

There was no juggling of ideas. I had at most one ball in the air at a time, and in fact often it was as simple as thinking “extract” and having PyCharm do it and at no time did the ball leave my hand.

Yes it was ten steps. The whole process took less than an hour, including writing this article. It was simple, every step was to a slightly better place, and it involved no difficult thinking and no risky moves.

It’s like playing The Floor is Lava. You need to find safe steps from one place to another. And it’s really best if you don’t jump on the couch. Toss down lots of pillows and step on them.

And what’s best is that we got a better solution than either of the two I actually considered, SafetyChecker and AsteroidAnalyzer. The code told me what was better.

I love when that happens.

See you next time!