Python Asteroids+Invaders on GitHub

I have in mind a few small things that would like to have attention. Are they worth it? I think so.

As I was making my morning iced chai, I was thinking about small changes. I found myself observing that we ourselves are made up of tiny bits, which are themselves made up of tiny bits, and since we matter, small things matter. Of course it’s not quite that easy. When is a small code change worth making?

There are of course quite a few ways in which a change can improve things. If it fixes or prevents a problem, or if it makes the program more efficient in a valuable way, then it’s probably worth doing. But we’re more concerned with changes that “just” improve the code.

A change that improves the code probably really only pays off when subsequent changes are easier or more reliable than they would have been otherwise—and when the savings then is more than the investment now. But we do not know the future very well, although we can guess. Maybe it makes sense to leave cruft where we are unlikely ever to tread again, and clean up areas where we are sure we’ll be back.

That’s correct, but it’s not quite my answer. My answer is that when we run across code that can be improved with a small investment, anywhere, we should improve it. Why? Two reasons at least:

First, the fact that we are in this code suggests that it will be visited again. I’m not sure why that happens, but there are always places we seem to visit often and places where we seldom go. The fact that we are here now makes it likely that this is a place we visit more often. So improving it will pay off. It’s a decent bet.

Second, and more important to me, is that this is my work we’re looking at, and I want to do good work. So whenever my work is imperfect—that is, always—I choose to improve it, because I want to confirm and strengthen my ability to do good work. I learn to do good work by doing it. No matter where I take the opportunity to improve myself, my self-improvement will pay off in the important work that I subsequently do.

Third—as always, I was wrong about the two, but at least this time I fuzzed my estimate—I do it for the tiny jolt of joy that I get, right then and there, from making something better.

Net net net, small changes are good for the business, making things go better, and good for me, making me more productive and more happy. In a real work situation, I would try to create a reasonable balance between polishing and building new capability, and the nature of the work would probably keep my attention mostly on the code that will provide the most benefit. But sometimes, when the pressure was low, I might still wander over to that patch of code that had been nagging at me, and give it a quick spiffing up.

Today, we’re going to do a little of that.

Spritely, mask, and rect

The new Spritely mixin implements mask and rect methods that access our Sprite. It seems to me that there should be no reason for other objects to be grabbing onto those elements of our Sprite. I propose to find out what’s going on and see whether we can reduce or eliminate access to those elements.

Tossing an assert False into mask breaks a test and breaks the ImageMasher. I fix the test, which is just checking some masking, to fetch the masks it wants from the object’s sprite. Invasive but will save me a method.

Pause
I am troubled by this. Why? I’m not sure, but I’ll make a note of it. Something about if a test needs that information, and has to reach inside the object to get it, something is out of whack somewhere.

Let’s look at ImageMasher. That’s used in RoadFurniture:

class RoadFurniture:
    def mash_image(self, shot):
        masher = ImageMasher.from_flyers(self, shot)
        new_mask, new_surface = masher.update(self.mask, self.surface)
        self._sprite._masks = (new_mask,)
        self._sprite._surfaces = (new_surface,)

Oh, nice! I wanted to find this and work on it anyway. The way this object reaches inside the sprite and mashes its member variables is not the mashing we should have in mind. We should at least give the Sprite methods to update those values. But more to the point, why not do the mashing in the sprite itself. Let’s look at update, which is doing the work.

class ImageMasher:
    def update(self, mask, surface):
        self.determine_damage()
        self.apply_damage_to_surface(surface)
        return self.get_new_mask(), surface

The first thing we notice is that the mask parameter here is unused. Change signature. Whoa! Big issue there, PyCharm changed all the update methods in the whole program. Not what I had in mind. Do it by hand.

I was thinking we should move image mashing into Sprite, but in fact the Masher is rather complicated:

class ImageMasher:
    def __init__(self, target_masker, shot_masker, explosion_masker):
        self.target_masker = target_masker
        self.shot_masker = shot_masker
        self.explosion_masker = explosion_masker

    def update(self, surface):
        self.determine_damage()
        self.apply_damage_to_surface(surface)
        return self.get_new_mask(), surface

    def determine_damage(self):
        self.erase_shot()
        self.erase_explosion()

    def erase_shot(self):
        self.target_masker.erase(self.shot_masker)

    def erase_explosion(self):
        self.target_masker.erase(self.explosion_masker)

    def get_new_mask(self):
        return self.target_masker.mask

    def apply_damage_to_surface(self, surface):
        new_mask = self.get_new_mask()
        for x in range(new_mask.get_rect().width):
            for y in range(new_mask.get_rect().height):
                bit = new_mask.get_at((x, y))
                if not bit:
                    surface.set_at((x, y), (0, 0, 0))

I think we want that capability to remain in a separate class. Note that its whole job is to return a new mask and surface from update. Other than testing, it’s only used in RoadFurniture, as we saw:

class RoadFurniture:
    def mash_image(self, shot):
        masher = ImageMasher.from_flyers(self, shot)
        new_mask, new_surface = masher.update(self.surface)
        self._sprite._masks = (new_mask,)
        self._sprite._surfaces = (new_surface,)

What if we moved the capability to Sprite, but not the implementation? Let’s do that by intention.

class RoadFurniture:
    def mash_image(self, shot):
        self._sprite.mash_from(shot)

“By intention” means “I wish this worked”, followed by “make it so”. Make it so:

class Sprite:
    def mash_from(self, shot):
        masher = ImageMasher.from_flyers(self, shot)
        new_mask, new_surface = masher.update(self.surface)
        self._masks = (new_mask,)
        self._surfaces = (new_surface,)

This turns out to work, thanks to some Python polymorphism, because Sprite responds correctly to mask and rect. So much for getting rid of them.

This method does assume that the object being mashed has no animation, and thus has only a single element in its _masks and _surfaces members. I think that’s a bit flaky. We could check and complain. We could do something more elaborate. Or we could leave it this way, just a bit better. And that’s what we will do, at least for now.

Now the only non-test use of ImageMasher is in Sprite. Commit that very message.

What about Masker, which is used in the ImageMasher? It’s also used in Collider, but only sometimes:

class Collider:
    def __init__(self, left, right):
        try:
            self.left_collider = left._sprite
            self.right_collider = right._sprite
        except AttributeError:
            self.left_collider = Masker(left.mask, left.position)
            self.right_collider = Masker(right.mask, right.position)

    def colliding(self):
        return self.left_collider.colliding(self.right_collider)

I believe the except never happens. Where is Collider used?

picture showing objects using Collider

All those objects have Sprites. But there are tests that rely on Collider. Dealing with that is more effort than we should expend in tidying.

But what about changing how the individual objects do their collision, avoiding Collider, to see where that leads?

I’ll do this by intention also, starting in Invader:

class Invader(Spritely):
    def colliding(self, invaders_flyer):
        collider = Collider(self, invaders_flyer)
        return collider.colliding()

I want to pass this to my sprite and ask it about collision with the other object, which is a flyer, not a sprite at this point:

    def colliding(self, invaders_flyer):
        return self._sprite.colliding_with_flyer(invaders_flyer)

I wish that worked. Make it so:

class Sprite:
    def colliding_with_flyer(self, flyer):
        return self.colliding(flyer._sprite)

That works with a complaint from PyCharm that I’m accessing a private member. I could hush it. I think I’ll leave it. I am starting to think that the sprite should be public or have a public property. We’ll let that perk.

I think we can make this same change to all the spritely flyers. Or, we can put colliding into Spritely and remove it from the others.

As it happens, some of the flyers do implement colliding but some of them are checking a collision without such a method, For example:

    def interact_with_invadershot(self, shot, fleets):
        collider = Collider(self, shot)
        if collider.colliding():
            self.hit_by_shot(fleets)

That would be better, by my lights, with a call to self.colliding:

    def interact_with_invadershot(self, shot, fleets):
        if self.colliding(shot):
            self.hit_by_shot(fleets)

    def colliding(self, flyer):
        collider = Collider(self, flyer)
        return collider.colliding()

Now, of course, I can do that the new way:

    def colliding(self, flyer):
        return self._sprite.colliding_with_flyer(flyer)

I’ve been having so much fun I’ve forgotten to commit. Commit: changing collision logic to use Sprite.

Now if Spritely were to implement colliding(flyer), we could inherit that capability. Is that a good thing? Some would disagree but I propose to try it and see how it feels.

class Spritely:
    def colliding(self, flyer):
        return self._sprite.colliding_with_flyer(flyer)

Now I can remove that method from my Spritely classes that have it. Now who else uses Collider?

InvadersSaucer and RoadFurniture.

class InvadersSaucer(Spritely, InvadersFlyer):
    def interact_with_playershot(self, shot, fleets):
        if Collider(self, shot).colliding():
            self.explode_scream_and_die(fleets)

That becomes:

    def interact_with_playershot(self, shot, fleets):
        if self.colliding(shot):
            self.explode_scream_and_die(fleets)

And a similar change to RoadFurniture:

class RoadFurniture(Spritely, InvadersFlyer):
    def process_shot_collision(self, shot):
        if self.colliding(shot):
            self._tasks.remind_me(lambda: self.mash_image(shot))

I think we have no actual users of Collider. If that’s the case we can remove the class and the tests. After fiddling with removing imports, which somehow PyCharm didn’t help me with, that’s done.

Commit: class Collider and its tests removed.

Let’s sum up.

Summary

Our initial focus point was the mask and rect methods in Spritely. We moved away from those quite quickly. I suspect we’ll do well to look at them again in another section, to see where they lead. Ideally, it would be good if they were not needed at all. Note made for another day.

We began with a simple change, moving the creation of ImageMasher into Sprite. We wanted to retain the object, because its operation is fairly complicated and quite specialized. Moving it into Sprite makes it feel more “private” to me, and allows the Sprite to manipulate its own members rather than have them invaded from outside.

We then started to go after Masker, but instead our attention was drawn to Collider, a major user of Masker. With a few simple moves, we caused Collision users to refer instead to their Sprite’s collision capability.

Then, and this is certainly subject to question, we added the colliding method to the Spritely mixin, which provides the method automatically to all the Spritely users. We removed the method from the individual classes where it existed, and used colliding in all the places where Collider was used. That unhooked Collider and we were able to remove the class and its tests, and about a zillion import statements. Or anyway six or seven.

Was it righteous to add colliding to Spritely? It already contains one “standard” method, namely draw. Arguably, position and its setter are standard methods also. So adding one more is pretty legitimate in my view, although colliding does seem like a bigger deal than just an accessor or a simple drawing method.

Honestly, I think it’s open to debate. So long as no one needs to override a method that is defaulted in a superclass or mixin, I consider the usage pretty legitimate, though some of my esteemed colleagues would not do the same. If we started overriding those methods, that would be more of a concern.

Following our nose, we have removed a class, and knowledge of that class, from at least six or eight files, replacing it with a default approach to colliding. I do suspect we are a bit light on actual tests for the “check rectangle check mask” logic that is used in collision checking in Sprite, and I’ve made a note of that. We were already light on tests, but the same logic was checked in Collision’s tests. Doesn’t really protect Sprite, but did show that the logic worked. We’ve moved from not very satisfactory testing to slightly less satisfactory testing. Perhaps we’ll improve things later. The code works just fine, since it was test-driven. It’s just that its tests have withered and died.

The code is smaller. Fewer objects concern themselves with how collisions are done and just ask for the information.

I think we’re in a better place, and we got there with small steps and no hassle of any note at all.

What do you think? See you next time!