Python Asteroids+Invaders on GitHub

I think first I’ll have the invader and player shots destroy each other if they collide. I had hoped for more. This caused enough trouble. Mostly resolved, I think.

For the shots to mutually destroy each other, we just need a couple of methods: that’s one nice thing about this design.

I don’t even feel the need to test this, but I’ll reconsider after I “just do it”.

Here’s how an invader does it:

    def interact_with_playershot(self, shot, group):
        if self.colliding(shot):
            shot.hit_invader()
            group.kill(self)

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

That’s a bit weird because invaders aren’t flyers, so the playershot doesn’t see the invaders, just the fleet. But this will tell us how we’ll do this new thing.

class InvaderShot(InvadersFlyer):
    def interact_with_playershot(self, shot, fleets):
        if self.colliding(shot):
            fleets.remove(self)

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

class PlayerShot(INvadersFlyer):
    def interact_with_invadershot(self, shot, fleets):
        if self.colliding(shot):
            fleets.remove(self)

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

I also had to make sure that these guys properly implement interact_with.

I run into lots of trouble. Because I have not made those methods abstract, Every time I run I get one error, telling me about the next class that has to have that method.

I need to do something better when I create a new object to interact with.

My next problem is that the InvaderShot does not have a mask yet. I’ll replicate what’s in Invader.

class InvaderShot(InvadersFlyer):
    def __init__(self, position, maps):
        self.maps = maps
        self.masks = [pygame.mask.from_surface(bitmap) for bitmap in self.maps]
        self.map = maps[0]
        self.map_index = 0
        self.rect = self.map.get_rect()
        self.rect.center = position
        self.count = 00

    @property
    def mask(self):
        return self.masks[self.map_index]

Now will it work? It almost does. The invader shot is destroyed, but the player shot continues on to the top and then explodes. And then I get an error:

AttributeError: 'ShotExplosion' object has no attribute 'mask'

Someone is checking to see if they are colliding with the explosion at the top of the screen. The mask is easily provided. But we need some way to require that via abstract method or something.

Reflection

Now what did I forget that made the player shot continue?

I hesitate to admit all this.

What I have done, for “convenience”, in at least two cases, is to keep two related classes in the same module file. One such case is that I built ShotExplosion in with PlayerShot.

Then, when searching out where to put the code for PlayerShot’s interaction with InvaderShot, I pasted it, not into PlayerShot, but into ShotExplosion.

Naturally, that didn’t work. It was also wrong in that the ShotExplosion shouldn’t be checking to see if it is interacting with an invader shot. It should pass on all interactions.

So in this brief session, about a half hour, I have been forced to find, via debugging, several places where I have to add interact_with_ methods, and have been bitten, not once but twice, by putting a method into the wrong class because I have two classes in the same file.

Let’s fix this. First, let’s make those methods abstract and allow PyCharm to help us discover what isn’t implemented.

And—again I hate to admit—the Flyer file also has multiple interface classes in it: Flyer, AsteroidFlyer, and InvadersFlyer. I think that is a bit more legit, because they are all interfaces, but I feel squidgy about it. I’ll let that ride, for now, but not the concretes. First, the methods.

I add interact_with_playershot as an abstract method to InvadersFlyer. I manage to create a new error:

  File "/Users/ron/PycharmProjects/firstGo/invader.py", line 24, in interact_with_playershot
    shot.hit_invader()
    ^^^^^^^^^^^^^^^^
AttributeError: 'ShotExplosion' object has no attribute 'hit_invader'

I also discovered that after the game crashed, I typed two “k” characters into the middle of my code. Hazard of running the game under PyCharm I guess.

Now that error. It comes from my having pasted, I think. The ShotExplosion interact_with should be this:

class ShotExplosion(InvadersFlyer):
    def interact_with(self, other, fleets):
        other.interact_with_playerexplosion(self, fleets)

And I had interact_with_playershot.

Anyway, aside from that, the game seems to compile and run, so I think all the abstract methods are implemented. Python and PyCharm may not always notice until you try to create an instance, but I’ve done the right thing with the new class.

What I need is a test that checks all the abstract methods. I had one but I think it is the one that is set to ignore, because it no longer handles all the cases I need. Better prioritize that.

Let’s break out the files.

OMG. PyCharm can just move a class to another file in one go. This is so trivial and I’ve been not doing it because it seemed tedious. I love the jetbrains people, but someone should have flapped my ears with a bladder with pebbles in it1 and told me to move my classes where they belong.

Now how do I figure out what files have more than one class? I decide to just click down the hierarchy pane, opening each file, and checking to see which ones have more than one class in them.

The other offender was InvaderGroup, in with InvaderFleet. It is a subordinate class but I have made the mistake of putting things in the wrong place in that file, so I split them apart.

I think we are good and can commit.

Famous last words.

  File "/Users/ron/PycharmProjects/firstGo/shot_explosion.py", line 42, in interact_with
    other.interact_with_playerexplosion(self, fleets)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Bumper' object has no attribute 'interact_with_playerexplosion'. Did you mean: 'interact_with_playershot'?

Is that one not abstract? It is not. Add it. Test fails.

    @pytest.fixture
    def shot(self):
        maker = BitmapMaker.instance()
>       return InvaderShot(u.CENTER, maker.squiggles)
E       TypeError: Can't instantiate abstract class InvaderShot with abstract method interact_with_playerexplosion

There’s one good reason to be sure of having at least an exists test for every class.

I add that method. Still things failing. InvaderFleet needs a method. I tick through all the tests that fail and implement the method everywhere.

This is why it is so tempting to make these methods more like subscriptions. It’s a two-edged sword, though. If I make them abstract, I have to implement them everywhere and sometimes, if there’s a weakness in my testing, they might not get caught until run time. If they are not abstract, and implemented as pass in the AsteroidsFlyer interface, there is the risk that I’ll need a non-null implementation and forget it.

I think I should return my attention to the ignored test and beef it up until it really reports everything I need to be sure about, all in one place. That will be for another time, however, not this morning. Probably. I am green. Can I commit? I think so.

Commit: invader shot and player shot mutually destroy, with player shot showing an explosion.

That is an 11 file commit. Again we see a downside to defining the abstract methods for each possible interaction: we have to change a lot of files.

The changes are all trivial, but it’s still a hassle, and one hates to see a lot of files in a commit like that: it just seems wrong.

Let me take a short break here, and then let’s reflect a bit and probably look at that ignored test and think about what we need.

Reflection

The good news with making player shots and invader shots mutually destroy one another is that it only required this much code:

class PlayerShot(InvadersFlyer):
    def interact_with_invadershot(self, shot, fleets):
        if self.colliding(shot):
            fleets.remove(self)
            fleets.append(ShotExplosion(self.position))

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

class InvaderShot(InvadersFlyer):
    def interact_with_playershot(self, shot, fleets):
        if self.colliding(shot):
            fleets.remove(self)

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

The duplication of colliding is of course a smell. I’m not sure what we’d do about it. We could implement it as a concrete method in the superclass, but that’s a bit naff, at least by some people’s standards.

Anyway, the actual implementation was quite simple and that’s one of the nice things about the design we have. If you want to do something when something happens, you implement a method and do the thing.

But the downside is that when we add a class, we must in principle create a new abstract method interact_with_thatclass, and all the existing subclasses and all the future subclasses, must implement it as a concrete method, almost always pass. It’s not even easy to find them all.

That brings me to the ignored test. What did it do?

    @pytest.mark.skip("needs updating")
    def test_all_interact_with_implemented_in_flyer(self):
        subclasses = get_subclasses(AsteroidFlyer)
        ignores = ["BeginChecker", "EndChecker", "InvaderFleet"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(AsteroidFlyer)
        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
            if "interact_with" in klass.__dict__:
                interact_with_method = klass.__dict__["interact_with"]
                interact_with_code = interact_with_method.__code__.co_code
                assert interact_with_code != pass_code, name + " has pass in interact_with"
            else:
                assert False, name + " does not implement `interact_with`"

That’s not obvious, and it just about can’t be. Probably can be better than it is. First thing, it’s only checking AsteroidFlyer now. It used to check Flyer and that got changed when we broke out the two game sub-flyers. So we need two tests one for Asteroids and one for Invaders, because they don’t have to implement all the same methods.

We probably need one for Flyer as well. I think it does still contain some abstract methods.

Then what we do is get all the attributes of the interface class (AsteroidFlyer in the current test). And we get the compiled code for “pass”. Then, for each subclass of AsteroidFlyer, we convert its name to lower case, “InvaderShot” to “invadershot” for example, and create the string “interact_with_invadershot”. We require that method to be in the superclass. If we were really good, we’d check to see if it is abstract, I suspect.

Then we look in the current subclass to be sure that it implements interact_with and not as pass.

I think we should turn this back on and see what happens. The AsteroidFlyer version should just work.

Yes. It passes. I’ll duplicate the test for invaders. That one fails. Let’s see why.

    def test_all_interact_with_implemented_in_invaders_flyer(self):
        subclasses = get_subclasses(InvadersFlyer)
        ignores = []
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(AsteroidFlyer)
        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
E           AssertionError: assert 'interact_with_invadershot' in ['__abstractmethods__', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', ...]

OK, what is it telling me? That assertion says that interact_with_invadershot is not defined in InvadersFlyer. But it sure doesn’t say it very clearly.

I add a message to the assertion:

E           AssertionError: InvadersFlyer does not implement interact_with_shotexplosion

That’s a bit more helpful. Anyway let’s implement that method.

If I add that as abstract 25 other tests fail. If I add it as concrete, some will look askance at me. I hate when that happens.

Added as concrete, I do get this new failure:

E               AssertionError: invaderfleet has pass in interact_with

That one is interesting. I really don’t want everyone having to interact with InvaderFleet, they have nothing to say to it. But rules are rules.

I implement it as it should be:

class InvaderFleet(InvadersFlyer):
    def interact_with(self, other, fleets):
        other.interact_with_invaderfleet(self, fleets)

I expected that to break something and it didn’t. Ah. Everyone already implements interact_with_invaderfleet.

We are green and have somewhat better checking. Let’s commit and then look into the duplication.

Commit: re-enable tests around interact_with implementation.

    # @pytest.mark.skip("needs updating")
    def test_all_interact_with_implemented_in_asteroid_flyer(self):
        test_class = AsteroidFlyer
        subclasses = get_subclasses(test_class)
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(test_class)
        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
            if "interact_with" in klass.__dict__:
                interact_with_method = klass.__dict__["interact_with"]
                interact_with_code = interact_with_method.__code__.co_code
                assert interact_with_code != pass_code, name + " has pass in interact_with"
            else:
                assert False, name + " does not implement `interact_with`"


    # @pytest.mark.skip("needs updating")
    def test_all_interact_with_implemented_in_invaders_flyer(self):
        test_class = InvadersFlyer
        subclasses = get_subclasses(test_class)
        ignores = []
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(test_class)
        pass_code = just_pass.__code__.co_code
        for klass in subclasses:
            name = klass.__name__.lower()
            print("checking", name)
            required_method = "interact_with_" + name
            assert required_method in attributes, "InvadersFlyer does not implement " + required_method
            if "interact_with" in klass.__dict__:
                interact_with_method = klass.__dict__["interact_with"]
                interact_with_code = interact_with_method.__code__.co_code
                assert interact_with_code != pass_code, name + " has pass in interact_with"
            else:
                assert False, name + " does not implement `interact_with`"

I think we can just extract a method after the assignment of test_class.

I’ve done something wrong. Revert. Do again, more careful to get the two bits exactly the same.

class TestFlyer:

    # @pytest.mark.skip("needs updating")
    def test_all_interact_with_implemented_in_asteroid_flyer(self):
        self.check_class(AsteroidFlyer)

    # @pytest.mark.skip("needs updating")
    def test_all_interact_with_implemented_in_invaders_flyer(self):
        self.check_class(InvadersFlyer)

    def check_class(self, test_class):
        subclasses = get_subclasses(test_class)
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(test_class)
        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, test_class.__name__ + " does not implement " + required_method
            if "interact_with" in klass.__dict__:
                interact_with_method = klass.__dict__["interact_with"]
                interact_with_code = interact_with_method.__code__.co_code
                assert interact_with_code != pass_code, name + " has pass in interact_with"
            else:
                assert False, name + " does not implement `interact_with`"

Green. Now what about that mess in the for statement? There are two things going on in there. Let me try another extract or two.

    def check_class(self, test_class):
        subclasses = get_subclasses(test_class)
        ignores = ["BeginChecker", "EndChecker"]
        subclasses = [klass for klass in subclasses if klass.__name__ not in ignores]
        attributes = dir(test_class)
        pass_code = just_pass.__code__.co_code
        for klass in subclasses:
            name = klass.__name__.lower()
            self.check_top_class_has_interact_with_each_subclass(attributes, name, test_class)
            self.check_interact_with_present_and_not_just_pass(klass, name, pass_code)

    def check_top_class_has_interact_with_each_subclass(self, attributes, name, test_class):
        required_method = "interact_with_" + name
        assert required_method in attributes, test_class.__name__ + " does not implement " + required_method

    def check_interact_with_present_and_not_just_pass(self, klass, name, pass_code):
        if "interact_with" in klass.__dict__:
            interact_with_method = klass.__dict__["interact_with"]
            interact_with_code = interact_with_method.__code__.co_code
            assert interact_with_code != pass_code, name + " has pass in interact_with"
        else:
            assert False, name + " does not implement `interact_with`"

That might be better. Those long names are irritating but the ideas are complicated, since this is a test that inspects our code. They might help next time we explore these tests and we just might be here again.

For now, commit: improve tests that check to ensure all the right methods are implemented in interfaces and subclasses.

Let’s sum up, I’m done for now.

Summary

The code to make the shots kill each other was nearly trivial. It does show some duplication, the colliding method. It would be nice to get rid of that. One possibility would be a framework change to tell the objects whether they are colliding … but we don’t really want to do any framework changes, and anyway the two games use entirely different collision logic. So that’s out.

I ran into issues with pushing the new interact methods into all the classes. That is a continuing thorn in my side, easily corrected by implementation inheritance, but that brings in its own problems, which will be that we can too easily forget an important case.

There may be a better way to deal with this, but if there is I haven’t thought of it yet. Or, if I have thought of it, I’ve forgotten it now.

I ran into trouble with the mask needed for collisions. Anything that can collide needs a mask, and some objects didn’t have one. Possibly we need a new interface / abstract class, Collidable, that would require that the mask property was present. I’m not sure if I can do an abstract property with a simple member or not. Would have to find out. Probably should. A search says yes, there is @abstractproperty. We could just add that right into InvadersFlyer, I should think. The InvaderFleet would have to provide it but no biggie. Or it could be a separate interface … but then we’d have to remember to inherit it. No, it’ll be better in the InvadersFlyer.

It might be better to create all the masks in BitmapMaker and provide them in a standard fashion.

Summary Summary

This morning was odd. The actual changes needed were small. But they had consequences that surprised me. Fixing up the ignored test would have found all the problems, one at a time, I think, but would have found them all. So maybe next time, which will be really soon now, we’ll see those tests helping us. If not … we’ll revisit the concern and see what else might help.

I had hoped to quickly hammer out the shot vs shot and then move on to dropping the shots from different columns and dealing with the shot timing. That didn’t happen, because the seemingly simple shot code cascaded into a lot of updates, done one at a time, and finally into some improved tests.

This sometimes happens, and is one reason why some folx think it’s completely useless to estimate stories. I don’t think it’s completely useless … but I do think that it’s just as good, and lots easier, to slice stories down to a minimal thickness and then just count them and see how fast they get implemented. It turns out that that number tracks actuals just fine. Law of large numbers or something.

Summary Summary Summary

We have a new feature and new tests. The sun is out and we have progress to show. Life is good!


  1. Ten points to your house2 if you recognize that reference. ↩

  2. Yes3, JR is a bigot. But the HP stories were enjoyable by a lot of people. ↩

  3. No, the original reference was not to HP. It was a reference to a climenole. ↩