Python 136
Yesterday’s experience causes me to question a central design notion.
As reported yesterday, geePaw Hill rightly pointed out that my code, checking a missile to see if it was from the saucer, was inconsistent with the overall design, where each kind of game object has its own class.
We started breaking out a SaucerMissile class Tuesday night, and I completed the job yesterday. At least I am pretty sure it’s complete.
But there were some common mistakes that I made, due to common changes needed, and that is causing me to question a key aspect of the current design. The changes needed, which I missed more than once, were these:
The key “idea” of the current “decentralized” design is that the Game offers each pair of objects a chance to interact. A double dispatch is used, so that each object implements a unique method for each type of objects it wants to deal with, methods like interact_with_ship or interact_with_missile. And each type of object, for example the new saucer missile is required, to implement interact_with(other) by sending other.interact_with_saucermissile(self).
Now I have implemented all the detailed interact_with_xyz methods in the Flyer superclass of all such objects, as pass. So, this is quite nice, because it means that the individual classes only have to implement interact_with_xyz for the specific classes they care about. It’s a bit like a publish-subscribe, done at compile time.
Now, many of my objects interact with missiles, the ship and saucer and asteroids at least, perhaps others. And so when I implemented the SaucerMissile, saucer missiles stopped interacting via interact_with_missile and started calling interact_with_saucermissile.
And that method is explicitly ignored unless the object implements it, i.e. unless the programmer, i.e. yours truly, remembers to implement the new method where it’s needed.
And I didn’t remember them all. I spent most of my article yesterday discovering another case that I had forgotten, and I was relying too much on my tests. They did help me at first but after a while, things were broken but no tests existed to show the problem.
And why would they? I had created the new class without tests, thoughtlessly imagining that the existing tests would find any issues and that, if not, a bit of game play would. As a result, what we did Tuesday night didn’t quite work, and I committed at least two defects to github yesterday.
A Clear Design Issue
When similar mistakes are common in our work with our code, we are wise to look for a common cause, and that common cause is typically some kind of design issue in the code. It might be a large issue, it might be a small one, but there’s almost certainly some common cause that needs to be addressed.
Now, part of the problem was that I had implemented a subclass of a subclass of Flyer, and my code for checking that all the right methods were implemented didn’t handle that case. So among yesterday’s efforts, I beefed up that test. But that was not enough.
like SaucerMissile, the tests require that I implement interact_with in that class, and they require that Flyer implements interact_with_saucermissile as a pass.
The good news is that using that scheme, no object is required to implement interact_with_saucermissile unless we want it to. And for a long time, that was exactly what was needed. Most of the new Flyer subclasses are specialized objects like Score and ScoreKeeper, which interact with each other but with nothing else. That’s an aspect of the design that I really like, the way that the specialized object manage things like the score, and game over state and such.
However, if you add a new front-line game object that should interact visibly with the other visible objects, that default bites us, because there are a lot of objects who need to implement that new method and nothing requires them to do so.
Summary of Issue
When a new object of class Foo is implemented, the game’s tests require that the Foo object must implement:
class Foo(Flyer):
def interact_with(other, fleets):
other.interact_with_foo(self, fleets)
And the tests also demand that the Flyer superclass must implement:
class Flyer:
def interact_with_foo(other, fleets):
pass
Then, any class that needs to interact with Foo, also implements interact_with_foo, and all is well.
But because of that pass in the superclass, it is far too easy to forget some object that actually needs the new method, because it generally fails silently.
Tentative Conclusion
The automatic inheritance interact_with_foo is seriously error prone, in some cases. In cases where the object is new and specialized, we probably don’t want to have to explicitly visit every class and implement interact_with_foo, and we would prefer not to litter all our classes with interact_with_ methods for every kind of Flyer subclass. That would cause most of the classes to implement about a dozen or fourteen interact_with_ methods, most of which really should be pass.
But the alternative is covert errors.
So we have to change the rules. The question is, I think, how far do we go?
The easy answer is to go all the way: every Flyer must explicitly implement every interact_with_, even if it only implements it with pass. It’s easy, and it means that whenever we implement the nth Flyer subclass, we must add all those methods to it, and its method to n-1 other classes.
In the present scheme, a new object generally only implements a couple of interact_with_ methods and only one other class generally has to add the new method. Very convenient. If we do the easy answer, we’ll have to update over a dozen classes instead of just a couple.
I am minded to try to save myself this work. This is almost certainly a bad idea, since the work is easy enough, just look at the class, think a moment to be sure you know what it should do about the new object, and, typically, paste in the pass method. Still too easy? We might get on a roll and paste one where we shouldn’t have?
But this problem was special. A new visible game entity was added, the SaucerMissile, and the other visible game entities clearly need to interact with it, and the others clearly do not. So we could split the hierarchy and have ActiveFlyer and SpecialFlyer or something and let the special guys default and the active guys not default.
Right, except that I believe there is still a bug in the game right now. Let’s look.
Here is ShipMaker:
class ShipMaker(Flyer):
def __init__(self):
self.ships_remaining = u.SHIPS_PER_QUARTER
self._timer = Timer(u.SHIP_EMERGENCE_TIME)
self._game_over = False
self._need_ship = True
self._safe_to_emerge = False
def begin_interactions(self, fleets):
self._game_over = False
self._need_ship = True
self._safe_to_emerge = True
def interact_with_asteroid(self, asteroid, fleets):
if asteroid.position.distance_to(u.CENTER) < u.SAFE_EMERGENCE_DISTANCE:
self._safe_to_emerge = False
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
def tick(self, delta_time, fleets):
if self._need_ship and not self._game_over:
self._timer.tick(delta_time, self.create_ship, fleets)
def create_ship(self, fleets):
if not self._safe_to_emerge:
return False
if self.ships_remaining > 0:
self.ships_remaining -= 1
fleets.add_flyer(Ship(u.CENTER))
else:
fleets.add_flyer(GameOver())
self._game_over = True
return True
def interact_with(self, other, fleets):
other.interact_with_shipmaker(self, fleets)
def draw(self, screen):
pass
Notice that we have that _safe_to_emerge flag and that it is set to False if we see a saucer, a missile, or an asteroid that is too close.
Notice anything missing? That’s right: I forgot to check for saucer_missile here, and so the ship can emerge even while saucer missiles are on the screen.
This really slams the door on something clever, because I would surely not have put ShipMaker into my special subclasses that have to implement everything.
The Plan
I think there is no safe alternative but to require all the objects to implement all the interact_with_ methods. This is easily done, because Flyer looks like this:
(Just scan it for the pattern of abstract vs not.)
class Flyer(ABC):
@abstractmethod
def interact_with(self, other, fleets):
pass
@abstractmethod
def draw(self, screen):
pass
@abstractmethod
def tick(self, delta_time, fleets):
pass
# concrete methods, inheritable
# so sue me
def begin_interactions(self, fleets):
pass
def end_interactions(self, fleets):
pass
def interact_with_asteroid(self, asteroid, fleets):
pass
def interact_with_coin(self, missile, fleets):
pass
def interact_with_explosion(self, explosion, fleets):
pass
def interact_with_fragment(self, fragment, fleets):
pass
def interact_with_gameover(self, game_over, fleets):
pass
def interact_with_missile(self, missile, fleets):
pass
def interact_with_saucermissile(self, missile, fleets):
pass
def interact_with_score(self, score, fleets):
pass
def interact_with_scorekeeper(self, scorekeeper, fleets):
pass
def interact_with_saucer(self, saucer, fleets):
pass
def interact_with_saucermaker(self, saucermaker, fleets):
pass
def interact_with_ship(self, ship, fleets):
pass
def interact_with_shipmaker(self, shipmaker, fleets):
pass
def interact_with_thumper(self, thumper, fleets):
pass
def interact_with_wavemaker(self, wavemaker, fleets):
pass
def update(self, delta_time, fleets):
pass
I apologize for the length, but I’m sure you got the pattern and skipped down to here.
Note the snarky comment in there:
# concrete methods, inheritable
# so sue me
Never write snarky comments, you’ll eat them someday.
Anyway, all I have to do is mark those methods as @abstractmethod and PyCharm will make me implement them everywhere. At least I think it will do that. Let’s try just one.
@abstractmethod
def interact_with_saucermissile(self, missile, fleets):
pass
44 tests fail when I do that. The messages are all similar:
def test_frag(self):
> frag = Fragment(position=u.CENTER, fragments=["ignored"])
E TypeError: Can't instantiate abstract class Fragment with abstract method interact_with_saucermissile
I think that PyCharm’s inspect will also help me find these. Yes, I get a heading for just this topic.
Now I still do have the ability to sort of compromise. I can move some of these methods into abstract and some not. I can trust myself to make the correct judgment on which ones to do which way.
I believe this to be an inferior decision, because I am clearly not to be trusted, but I am going to do it. I’m going to look at each method and decide whether everyone should have to decide about it or not.
Here goes. Here’s what I do:
class Flyer(ABC):
@abstractmethod
def interact_with(self, other, fleets):
pass
@abstractmethod
def draw(self, screen):
pass
@abstractmethod
def tick(self, delta_time, fleets):
pass
@abstractmethod
def interact_with_asteroid(self, asteroid, fleets):
pass
@abstractmethod
def interact_with_explosion(self, explosion, fleets):
pass
@abstractmethod
def interact_with_fragment(self, fragment, fleets):
pass
@abstractmethod
def interact_with_missile(self, missile, fleets):
pass
@abstractmethod
def interact_with_saucermissile(self, missile, fleets):
pass
@abstractmethod
def interact_with_saucer(self, saucer, fleets):
pass
@abstractmethod
def interact_with_ship(self, ship, fleets):
pass
# concrete methods, inheritable
# so sue me
def begin_interactions(self, fleets):
pass
def end_interactions(self, fleets):
pass
def interact_with_coin(self, missile, fleets):
pass
def interact_with_gameover(self, game_over, fleets):
pass
def interact_with_score(self, score, fleets):
pass
def interact_with_scorekeeper(self, scorekeeper, fleets):
pass
def interact_with_saucermaker(self, saucermaker, fleets):
pass
def interact_with_shipmaker(self, shipmaker, fleets):
pass
def interact_with_thumper(self, thumper, fleets):
pass
def interact_with_wavemaker(self, wavemaker, fleets):
pass
def update(self, delta_time, fleets):
pass
Now I get to find all the cases where these are not implemented and implement them. This is a very irritating thing to have to do. And while the PyCharm Inspect does identify the classes with the problem, it does not identify the particular methods.
And I can’t write a test to find them because Python won’t compile these classes, so the tests basically can’t run. There is nothing for it but to go through them all.
Wait? I think PyCharm can implement abstract methods for me. Yes, it will. And it puts in pass. This will make it less awful.
After only a century or so, I have managed, with PyCharm’s most excellent help, to put concrete implementations of all those abstract methods into all the classes that inherit from Flyer. Game works, and I am quite sure that adding a bunch of redundant pass methods couldn’t break anything so we’ll commit: implemented many but not all of the interact_with_xyz methods as abstract, requiring all Flyers to implement them explicitly.
That was a sixteen file commit. Whew.
Summary
As frequent readers know, GeePaw Hill and I have circled the issue of implementation inheritance frequently. I think we agree in part: neither of us thinks it’s the sort of thing that most programmers should use frequently, if at all. Where we differ is on the edges. He might never use it, where I will in fact use it where it seems reasonable.
And it seemed reasonable here, and it bit me quite hard. Three or four shipped defects, all due to my easy-going handling of the interact_with_xyz methods, which left holes for defects to creep in when I implemented a new subclass of Flyer. The new subclass needed lots of interaction with other objects, most of which I forgot.
What I’ve done now is still a compromise. I have left several methods non-abstract with inherited null behavior:
class Flyer:
def begin_interactions(self, fleets):
pass
def end_interactions(self, fleets):
pass
def interact_with_coin(self, missile, fleets):
pass
def interact_with_gameover(self, game_over, fleets):
pass
def interact_with_score(self, score, fleets):
pass
def interact_with_scorekeeper(self, scorekeeper, fleets):
pass
def interact_with_saucermaker(self, saucermaker, fleets):
pass
def interact_with_shipmaker(self, shipmaker, fleets):
pass
def interact_with_thumper(self, thumper, fleets):
pass
def interact_with_wavemaker(self, wavemaker, fleets):
pass
def update(self, delta_time, fleets):
pass
Even worse, when I look at that list I wonder about update. I think that if some object did need to implement it, it would be seriously broken, rather than create a covert difficult to spot error. But still, it should be looked into.
Now, if I had just made all the methods abstract to begin with, I would still have had to edit all the objects every time I added a new Flyer, but the change would just be to decide on a single method.
Is this compromise a good one? Given that I basically just had PyCharm implement the methods for me, and then I just arranged them better … I might just as well have done them all.
Bottom Line
I may still be looking at trouble. Quite possibly I should have followed GeePaw’s style on this. In my zeal for easy and simple code, I may have gone too far.
And it has only taken me 136 articles to notice. And I probably should have gone all the way. I am probably still wrong.
Updated
I did the ship-saucermissile change wrong. Safe to emerge was still wrong.
I gotta think about this.
See you next time, I hope!