Python Asteroids on GitHub

As usual, what happens today isn’t what I planned. I recognize a design issue, implement a very interesting test, then generalize what happened to consideration of real teams. Watch how my thoughts evolve.

So I had this idea last evening. Yesterday we extended the ShipMaker to have a second flag, _game_over, which is set if there is a GameOver instance in the mix. That seems unnecessary, because it is ShipMaker that creates the GameOver, so it could set the flag its own darn self. But I wasn’t thinking that, I was thinking “when the ShipMaker sees a GameOver, it could just delete itself”. And that got me thinking about whether it’s really safe for objects to delete themselves during interactions. I’ve convinced myself of that in the past, with some actual tests, but I continue to have a bit of concern about that.

Probably the worst that could happen would be that some object would be skipped on that interaction cycle. That could still be bad, since we are relying on all three cycles to get objects properly initialized to detect conditions, to properly inform them of all the objects on screen, and to properly give them a chance to finish up. (I think we do not as yet use the finish_interactions call for anything, but I’m not certain of that.)

Nota Bene1
This was my plan for the morning right here. Quick mod to ShipMaker, then improve Fleets.

So that thinking leads me to want to simplify the Fleets object so that we can be certain that it manages deletions correctly.

But first, now that I’ve realized the ShipMaker-GameOver relationship can be simplified, let’s fix that.

We’ll change this:

class ShipMaker(Flyer):
    def interact_with_game_over(self, game_over, fleets):
        self._game_over = True

    def tick(self, delta_time, fleet, fleets):
        if self._need_ship and not self._game_over:
            self._timer.tick(delta_time, fleets)

    def create_ship(self, fleets):
        if not self._safe_to_emerge:
            return False
        if fleets.ships_remaining > 0:
            fleets.ships_remaining -= 1
            fleets.add_ship(Ship(u.CENTER))
        else:
            fleets.add_flyer(GameOver())
        return True

We’ll change it so that when we add the GameOver, we set our own flag, and therefore we will remove the interact_with.

    def create_ship(self, fleets):
        if not self._safe_to_emerge:
            return False
        if fleets.ships_remaining > 0:
            fleets.ships_remaining -= 1
            fleets.add_ship(Ship(u.CENTER))
        else:
            fleets.add_flyer(GameOver())
            self._game_over = True
        return True

That done, I think there are no implementors of interact_with_game_over, and we can remove its only call from GameOver:

class GameOver(Flyer):
    def interact_with(self, other, fleets):
        other.interact_with_game_over(self, fleets)

We change that back to pass. And since it is never sent, we can remove the method from Flyer. All tests continue to run Green.

Nota Bene
Here, having made that simple change, I think about whether it was a good idea, or not so good. I come up with a surprise for myself: a new design invariant.

A Design Question

We might ask ourselves, selves, would it be a better design if, for each Flyer subclass, there was an interact_with_that_subclass method in Flyer? Would it be better if every subclass of Flyer invariably implemented interact_with to call interact_with_my_subclass? Should it be up to a given Flyer to say “no one hears from me”, or should every subclass issue interaction calls unconditionally?

I’m almost sorry that I asked myself that, because it seems to me that we can never be sure that a given object should never interact with any other. The fundamental notion of this design is that the objects all interact with each other, and that an object can “subscribe” to interactions with any other.

Therefore … darn it … yes, our design should require every object to issue a suitable interact_with_ call.

Nota Bene
And now we are completely off the old plan, ignoring the Fleets issue for now, and instead moving the design to what I believe is a better place.

OK, having thought the thought, we must make it so. Must we make it so right here and now? We should at least not make it worse. That said, I think I’ll change the name of the method from interact_with_game_over to interact_with_gameover, because I have fantasies of writing a test to ensure that this new rule is followed, at least to the extent that I can.

class Flyer:
    def interact_with_gameover(self, game_over, fleets):
        pass

Now to call it, back in GameOver:

class GameOver(Flyer):
    def interact_with(self, other, fleets):
        other.interact_with_gameover(self, fleets)

No one subscribes to that method, so the tests are green. Should we do two commits here, one for changing ShipMaker and the other for the new method name? I suppose we should.

Commit: ShipMaker does not interact with GameOver, sets its own game_over flag.

Commit: GameOver implements interact_with_gameover, added to Flyer.

Nota Bene
See how I’m sort of drifting in the direction of doing the right thing? I do one small thing, then think of another. That’s the power of small steps: they make big things seem easy … and generally make them truly easier than when they are large.

Could we write a test to see if all the methods are implemented? Two questions: first, how do we get all subclasses of a class, and second, how do we get all methods of Flyer, and third2, how do we determine that each subclass at least has an explicit implementation of interact_with?

I am told by the Internet that all classes have a method __subclasses__. Let’s start our test.

    def test_all_interact_with_implemented_in_flyer(self):
        sc = Flyer.__subclasses__()
        attrs = dir(Flyer)
        for klass in sc:
            name = klass.__name__.lower()
            method = "interact_with_" + name
            assert method in attrs

This begins to fail with interact_with_fragment being missing. I’ll add it. Then it finds explosion. Then shipmaker. Then saucermaker. Then wavemaker. Then, a cute one, beginchecker, which is a testing class. I could remove it from the list, and I think I will.

    def test_all_interact_with_implemented_in_flyer(self):
        sc = Flyer.__subclasses__()
        ignores = ["BeginChecker"]
        sc = [klass for klass in sc if klass.__name__ not in ignores]
        attrs = dir(Flyer)
        for klass in sc:
            name = klass.__name__.lower()
            method = "interact_with_" + name
            assert method in attrs

Now it finds EndChecker, so I add that to the ignores. And the test passes.

This is significant!

Now we are certain that Flyer has the default pass methods for all subclasses of Flyer, and we know that this test will fail as soon as we add another class, until we give Flyer its method.

Nota Bene
And here, now that we have this new place to stand, we see that another small improvement is possible. Step by step …

What we do NOT know is whether each subclass of Flyer correctly implements interact_with, calling interact_with_classname. We do know that they all implement it, since it is abstract in Flyer and therefore Python requires us to implement it.

There is intricate code available on the Internet to determine whether a given method is pass or not.

Let’s push this further.

_RETURN_NONE = (lambda: None).__code__.co_code

    def test_all_interact_with_implemented_in_flyer(self):
        sc = Flyer.__subclasses__()
        ignores = ["BeginChecker", "EndChecker"]
        sc = [klass for klass in sc if klass.__name__ not in ignores]
        attrs = dir(Flyer)
        for klass in sc:
            name = klass.__name__.lower()
            method = "interact_with_" + name
            assert method in attrs
            iw = klass.interact_with
            iw_code = iw.__code__.co_code
            assert iw_code != _RETURN_NONE, name + " has pass in interact_with"

Ripped from the pages of the Internet, _RETURN_NONE is the code for return None, which is also the code for pass. The test fails, first with this:

>           assert iw_code != _RETURN_NONE, name + " has pass in interact_with"
E           AssertionError: fragment has pass in interact_with
E           assert b'\x97\x00d\x00S\x00' != b'\x97\x00d\x00S\x00'

Go fix Fragment:

class Fragment(Flyer):
    def interact_with(self, attacker, fleets):
        attacker.interact_with_fragment(self, fleets)

The test finds explosion. Fix:

    def interact_with(self, other, fleets):
        other.interact_with_explosion(self, fleets)

Find shipmaker and fix; saucermaker; wavemaker. And we pass.

OK, I like this. Let’s commit, then reflect. Commit: tests verify that all subclasses of Flyer correctly implement interact_with_ and all such functions are defined in Flyer to pass.

Reflection

First of all, congratulations to us to thinking a bit harder about whether it might be best to require Flyer to provide pass functions for all possible interact_with_something methods, and then whether we could actually test that each subclass actually calls that function.

And congratulations to the Internet for showing us how to do it, and to Python for making it rather easy. The final testing bit, with _RETURN_NONE, actually checks the compiled byte code of each method. That’s pretty deep in anyone’s bag of tricks, but it’s just two quick steps away. The __code__ attribute is the function’s actual code object and co_code converts that to a string. The trickiest bit was knowing that pass, which you can’t put in the lambda, compiles to return None, which we could put in the lambda.

Let’s try to make the test a bit more clear.

def just_pass():
    pass


class TestFlyer:
    def test_all_interact_with_implemented_in_flyer(self):
        sc = Flyer.__subclasses__()
        ignores = ["BeginChecker", "EndChecker"]
        sc = [klass for klass in sc if klass.__name__ not in ignores]
        attrs = dir(Flyer)
        pass_code = just_pass.__code__.co_code
        for klass in sc:
            name = klass.__name__.lower()
            method = "interact_with_" + name
            assert method in attrs
            iw = klass.interact_with
            iw_code = iw.__code__.co_code
            assert iw_code != pass_code, name + " has pass in interact_with"

I think that’s more clear. Would you agree? Now we have a pass function and we get its code and compare to the code of iw. We should use better names in the test.

class TestFlyer:
    def test_all_interact_with_implemented_in_flyer(self):
        subclasses = Flyer.__subclasses__()
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(Flyer)
        pass_code = just_pass.__code__.co_code
        for klass in subclasses:
            name = klass.__name__.lower()
            required_method = "interact_with_" + name
            assert required_method in attributes
            interact_with_method = klass.interact_with
            interact_with_code = interact_with_method.__code__.co_code
            assert interact_with_code != pass_code, name + " has pass in interact_with"

I think that’s even better. Commit: refactor for improved names.

So this is good. We have implemented an invariant, a rule that is not allowed to be broken, saying that for each subclass Foo of Flyer, the subclass must implement interact_with(other,fleets) as other.interact_with_foo(self,fleets), and that that method must be implemented in Flyer (we could check it for pass) and will not be implemented as pass in the subclass.

I think this makes for a better design, and the test teaches us some of the inner details of Python. We are mature enough, of course, not to do anything too strange with this powerful new knowledge.

I think this test could be made more clear. It might even be possible to check to see that the code is implemented correctly. We could certainly check for pass in the code in Flyer. I think that would be gold-plating. What we have here will detect some inevitable mistakes and keep the design up to snuff. We are forgetful, not malicious, and we won’t do some grotesque thing just to mess this up. Except maybe by accident. And if we do, we’ll improve the test.

Summary

I came in here planning to improve Fleets, after simplifying ShipMaker. we did simplify ShipMaker but that led me to recognize a design issue. Rather than leave it up to a given Flyer subclass to decide on its own whether to issue its interact_with_ method, we agreed (I feel sure you agreed) that the design would be better if each subclass was required to do the interact_with_ call, and with a bit of help from the entire Internet, we found a fairly decent way to test that the constraints we wanted were implemented.

So instead of improving Fleets, we’ve improved the overall design.

Real-World Teams?

Repeatedly, it seems almost daily, I start an article saying what I plan to do, and it seems that never is that what I actually wind up doing. What does this mean about my ability to plan? What does it tell us about real teams?

I find that this often happens, both here chez Ron3 and on real teams, especially the more effective teams. If I were to encounter a team where everyone said in the morning what they were going to do, and by end of day, they had all done exactly that, I would be almost certain that something was very wrong. And I’d bet that I would see decreasing speed of delivery, and that if I were to inspect the code, I would find it to be pretty terrible.

Why? Because no one on the team learned anything during the day. No one found an opportunity to improve something that wasn’t on their list.

Is it possible that a team could know their code so well, and plan so well that everything went as planned all day every day? In theory perhaps. But I know how I’d bet.

Here, Now, Today

I’m delighted to have discovered a way to improve the system’s design, and to have found a simple yet deep way to ensure that the design rules are followed. And I’m a bit proud of myself for being willing to divert from my plan to do something more important.

Of course, I have a very understanding management team, which does make that easier to do.

See you next time!


  1. He could say something in English. Is he some kind of polyglom thingie or what? ↩

  2. It seems to always happen that when I say I have two ideas, there’s at least one more. ↩

  3. There he goes again with that polyglorb stuff. ↩