Python Asteroids+Invaders on GitHub

I put in a new method yesterday. I’m not sure if it was a good idea, potentially good idea, or sheer waste. I love changing code.

The Spritely mixin, which we’ll look at in a moment, used to depend on there being a member in any Spritely class, _sprite. Your mission as a user of Spritely was to have a Sprite in that member variable.

Yesterday I made an accessor sort of thing in Spritely, and now it looks like this:

class Spritely:
    @property
    def sprite(self):
        return self._sprite

    @property
    def mask(self):
        return self.sprite.mask

    @property
    def rect(self):
        return self.sprite.rectangle

    @property
    def position(self):
        return self.sprite.position

    @position.setter
    def position(self, vector):
        self.sprite.position = vector

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

    def draw(self, screen):
        self.sprite.draw(screen)

I added the sprite method, an accessor which fetches the member _sprite. Users still have to define _sprite, or they could override sprite in their class and return a Sprite there. Or, they could even have a fixed attribute / member, sprite containing a Sprite.

When I did this, I really just did it because I didn’t like all the methods referring to the private _sprite, even though as a mixin, it’s almost legit to do that.

I am new to mixins, but I think that they generally are not expected to have an __init__ and create attributes in the classes that use them. I could be wrong. I would expect that, unless they are all static functions, mixin members would be expected to refer to attributes of the user class.

I’ve found a lot of posts on how to do mixins but they were mostly about the mechanics. I guess I’ll have to do some more research to see whether there are acknowledged good practices. (Pace GeePaw, who would probably say that not using them would be the ideal practice.)

Perhaps my concern about referencing a private is not appropriate to a mixin. So maybe it was OK as it stood. This scheme seems more flexible, because you could override sprite, but the truth is you could override _sprite just as easily and exactly the same way.

Worth it? Premature? Just plain bad idea? What do you think and why?

Some “Work”

I was on a quest to remove the need for the rect and/or mask methods in Spritely. Let’s see if we can see how they’re used and whether we can eliminate them.

I slam an assert False into rect to see what shows up. 18 tests break. One is in Bumper:

class Invader(Spritely):
    def is_entering(self, bumper, current_direction):
        return bumper.am_i_entering(self.rect, current_direction)

class Bumper(InvadersFlyer):
    def am_i_entering(self, rect, direction):
        return self.intersecting(rect) and direction == self.incoming_direction

    def intersecting(self, rect: Rect):
        return self.beyond_on_right(rect) if self.incoming_direction > 0 else self.beyond_on_left(rect)

    def beyond_on_left(self, rect):
        return rect.bottomleft[0] <= self.x

    def beyond_on_right(self, rect):
        return rect.bottomright[0] >= self.x

The Bumper essentially considers itself to be a vertical line (exceedingly thin rectangle?) at some x position. And it just checks the x coordinate of the provided rectangle against its own x.

If the bumper had a rectangle, we could use rectangle intersection. That wouldn’t be better. If the bumper had a Sprite, we could use Sprite collision. That might eliminate the need for the rectangle, allowing us to pass in the Sprite instead.

Rather than commit to doing this just yet, let’s look at some of the other users of the rect property. If we can’t get rid of it, this may not be worth doing.

class InvaderPlayer(InvadersFlyer):
class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.step = 4
        half_width = self.rect.width / 2
        self.left = 64 + half_width
        self.right = 960 - half_width
        self.rect.center = Vector2(self.left, u.INVADER_PLAYER_Y)
        self.free_to_fire = True
        self.fire_request_allowed = True
        self.shot_count = 0

This needs improvement anyway, because it is setting position via rect.center. We’ll improve this.

class InvaderPlayer(Spritely, InvadersFlyer):
    def __init__(self):
        self._sprite = Sprite.player()
        self.step = 4
        half_width = self._sprite.width / 2
        self.left = 64 + half_width
        self.right = 960 - half_width
        self.position = Vector2(self.left, u.INVADER_PLAYER_Y)
        self.free_to_fire = True
        self.fire_request_allowed = True
        self.shot_count = 0

    def move(self, amount):
        centerx = max(self.left, min(self.rect.centerx + amount, self.right))
        self.position = Vector2(centerx, self.position.y)

class Sprite:
    @property
    def width(self):
        return self.rectangle.width

That fixes a bunch of tests, only 13 failing now. Best commit: remove use of rect from invader player.

I find and remove a few more uses of rect from invader player and commit again with the same message. Nothing interesting to see here, just added new properties to Sprite to cover rectangle.

We should talk about why that’s better. I’m not sure that it is. It’s not quite like deferring to the Sprite to do something, is it? I do think that it makes sense for a Sprite to have rectangle-like properties however.

We’ll let this thread run a bit and see what we get. We only have four tests failing because of access to rect.

A few tests were referring to rect and I found yet another one in InvaderPlayer to fix. Committed. Now we are just left with the Bumpers.

I change the Bumpers to expect a Sprite and pass in the Sprite. I change them to refer to topleft and topright and make sure that those are implemented on Sprite.

No one is calling rect on Sprite. I can remove it. Whoa! With an assert False in rect, all my tests pass. When I remove rect 40 tests fail. Oh dear. rect is an abstract method in InvadersFlyer, so we have to implement it, or change the rules.

Almost all the implementations of rect are returning None, which means they would all cause crashes if anyone ever asked for their rectangle. Actual rectangle implementors are InvaderExplosion, PlayerExplosion, ReservePlayer, and ShotExplosion. All of those could be converted to use Sprites. And they probably should be.

I change Spritely.rect to return None, but I am not happy.

Summary: Not Happy

I’m not exactly sad, but what I see here is troubling, at least from the angle of an OO fanatic, which I mostly am or would like to be.

Changing Sprite to implement rectangle properties is better, by my lights, than allowing people to pull the rectangle out of the Sprite. But the fact that they want those properties … that’s not so good. Each one of those accesses is a tiny little invasion of Sprite’s privacy. Each one is a violation of the Law of Demeter. Each one violates encapsulation.

Each one is a tiny thorn in my paw.

Therefore, we “should” look at each one of those situations and determine what really needs to be done, and how the Sprite can help with that. If we were to do that, I think we would wind up with a better design than the one we have here. But the one we have here works, you see, and it is rather well tested, and we don’t really need to change any of these objects and there is really no reason to make it better.

Except that there is. We’re doing this exercise as an exercise, to learn what we consider to be good code, by creating and changing code.

Let’s look at one, briefly, to see what might be better. Here is the only access to Sprite.topright:

class Bumper:
    def beyond_on_left(self, sprite):
        return sprite.topleft[0] <= self.x

    def beyond_on_right(self, sprite):
        return sprite.topright[0] >= self.x

I included the topleft reference because it will help give us a clue what to do.

Sprite understands rectangles_collide. Could Bumper not use that instead? I think it could, and that would be more abstract than sucking the corners out of the Sprite. If we were truly fanatical, we might rename that method to extents_overlap or something of that nature.

But there’s more. The Bumper should probably really be interacting with the actual object in question, which is an Invader. So we probably should be sending messages between Invader and Bumper, which they might locally defer to their own Sprite as needed.

So there is a small design issue here, one of avoiding a violation of Sprite’s privacy, but also a larger one, which may be that someone has invaded the privacy of Bumper or Invader as well.

I think part of what we’re feeling is simply that Sprite is capable of bearing more of the load of interaction, and that we just haven’t found that balance yet. But another part is the objects like Bumper were pretty much just banged together out of old orange crates and nails we found on the basement shelf, and they could use a bit of design and polishing.

Summary Summary

It’s endless, it truly is. And I think that’s the marvel of programming, if you enjoy it. And, if you don’t enjoy it, I might seriously advise you to consider another way of spending all those hours every day. I say “might” only because I try never to give advice. But I am seriously tempted in the case of programmers who do not enjoy programming. Life’s too short to do something unpleasant, even if the pay is good.

But for those of us who do enjoy it, I think it is just marvelous that try as we might to do things perfectly, we will always find these little treasure troves of code that would like to be improved. I admit that sometimes when I look at one of them, my first reaction is OMG WHO WROTE THIS ****, but after that initial reaction, there’s a delightful puzzle left there for me to play with, code to be improved and made more attractive.

I do love that. And we’ll do more next time! See you then!