Python 52 - More Code Review

Python Asteroids on GitHub

Features are important, but I learn more from code review, so let’s see what we can see. Timer object: Ron does not kill his darling.

The only features remaining that come to mind are hyperspace, which I sorely need as such a terrible player, and the small saucer. I’ve also written an “extra credit” Jira: Star Field. We might do that before we’re done.

From a playability viewpoint, I think we need to rescale the game. There is an oddity in the way that PyGame maps to my screen so the scale that has worked on other versions seems too large here:

The native dimensions of the screen in the picture above are 1536x1536, but the program is calling for 768x768. PyGame is doubling it. It looks better at 1024, which makes the game fill essentially the full height of my 32 inch monitor. I’ll try it at that scale for a while and see how it feels.

I’d like to add some secret debugging keys, such as one that erases all the asteroids so that I can work with the saucer, or one to give me an infinite number of ships, so that I can check scores greater than my average score of about 87.

That’s about all I can think of for features we lack. None of them seem difficult or deeply educational, although we always learn something as we work.

My conclusion this morning is that it’ll be more interesting (for me) to see about improving the code, where we’ll probably find opportunities to learn more about PyCharm’s refactoring tools, and more about good ways to program things in Python.

Some Ideas

I’d definitely like to make the Game object more cohesive, breaking it out into the basic PyGame operation, the Asteroids game operation, and quite probably a separate object to handle the collision logic.

Suppose we were going to change the underlying game system for some reason. Right now, the individual space objects know a bit more than we might like about PyGame:

class Missile:
    def draw(self, screen):
        pygame.draw.circle(screen, "white", self.position, 4)

class Saucer:
	def __init__ 
	    ...
        saucer_scale = 4 * self.size
        self.offset = raw_dimensions * saucer_scale / 2
        saucer_size = raw_dimensions * saucer_scale
        self.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
        ...

    def draw(self, screen):
        top_left_corner = self.position - self.offset
        screen.blit(self.saucer_surface, top_left_corner)

If we were to change out PyGame right now, we’d find PyGame-specific code like the code above, spread randomly around the code. This isn’t a very large program, so it would only be a moderate pain to do it, but it would be a pain. In a larger program, it could get messy.

Now I’m not smart enough to create a generalized graphics API that could then be implemented in PyGame or pyglet or whatever we might pick. But we would very likely want some kind of consolidated interface for drawing things, rather than have the drawing code spread around all the objects as it is now.

Of course, if we did go to replace PyGame, we could just change all the places. It’s just that in a large program that would be very time consuming and error-prone. So, possibly, we should be looking at ways to better isolate drawing from the rest of the logic of being an asteroid or saucer or missile.

I’m not ready for that. I’m glad to have listed the idea, given it a bit of space in my mind, but I’m not ready to do that yet.

Browsing

Let’s browse the code looking for opportunities and issues that we might want to address.

Looking at saucer, since I had it on the screen, I see these methods:

a_missile_is_available
angle_to
cannot_target_ship
check_zigzag
create_missile
destroyed_by
draw
fire_a_missile
fire_if_possible
firing_is_possible
init_for_new_game
missile_at_angle
missile_timer_expired
move
new_direction
ready
score_for_hitting
scores_for_hitting_asteroid
scores_for_hitting_saucer
suitable_missile

Are you wondering how I got that list? I wrote this test:

    def test_methods(self):
        methods = dir(Saucer)
        methods = [x for x in methods if callable(getattr(Saucer, x))]
        methods = [x for x in methods if not x.startswith("__")]
        for m in methods:
            print(m)
        assert False

Asserting False makes it easy to find in the test output, then I just copied and pasted. Yes, I know I could do that all in one nasty line. If I wanted that, I’d do Command+Option+N (Inline) a few times, or paste in some and and such. I don’t want that.

What I’m more likely to do is to build a convenient function for getting this information, perhaps even going so far as to pull in a module to copy to clipboard. I’d do that if I made these lists often, but I do not.

Reviewing that list we see a few areas that we could consider breaking out:

• initializing (init_for_new_game and ready);
• timing (check_zigzag and possibly missile_timer_expired)
• firing missiles
• zig-zagging
• colliding (the score methods)
• move
• draw

Of these, the missile-related items are by far the largest group. Shall we break missile firing away from Saucer? It all starts here:

    def move(self, delta_time, saucers, saucer_missiles, ships):
        self.fire_if_possible(delta_time, saucer_missiles, ships)
        self.check_zigzag(delta_time)
        self.position += delta_time * self.velocity
        x = self.position.x
        if x < 0 or x > u.SCREEN_SIZE:
            if self in saucers:
                saucers.remove(self)

    def fire_if_possible(self, delta_time, saucer_missiles, ships):
        if self.firing_is_possible(delta_time, saucer_missiles):
            self.fire_a_missile(saucer_missiles, ships)

    def fire_a_missile(self, saucer_missiles, ships):
        saucer_missiles.append(self.create_missile(ships))
        self.missile_timer = u.SAUCER_MISSILE_DELAY

Somewhere in there, we’d start deferring to another object, perhaps named FireControl or something like that. We could create the instance dynamically, right there, or create it once and for all when we create the saucer. I’d prefer to create it dynamically. I think it’s rule 37 that is “trust your garbage collector”, but I’m not sure of the number.

If the object were dynamic, we’d have to maintain the timer up in Saucer, but the fire control thing could be immutable.

A wild idea has appeared!

Maybe it’s time to invent the timer object. We could have a very small timer object, and we’d give it delta time and when the time expired, it would do its action and reset the timer.

It’s tricky, though: we only reset the timer if we actually fire a missile. The condition for firing is that the time has expired and we have a spare missile to fire.

The convention could be that the action returns True or False, depending on whether it did the action and therefore the timer should be reset.

We might even find that our Timer was useful for the Ship’s logic determining whether it’s safe to emerge.

In Kotlin, it would be easy to build an object that takes an action as a parameter. In Python, it’s probably also easy, but a) I haven’t done it yet and b) the syntax will be messy compared to Kotlin’s.

We Have a Plan

We’ll build a Timer object. We’ll TDD it, of course.

I think this calls for a new test file. I wrote out a whole test:

class TestTimer():
    def test_creation(self):
        happened = False
        def it_happened()
            nonlocal happened
            happened = True
        delay = 3
        timer = Timer(delay, it_happened)
        assert not happened
        timer.tick(0.1)
        assert not happened
        timer.tick(3)
        assert happened

I could have stopped sooner but I had a sense of what I wanted, so I wrote it all down. There’s more to worry about. In particular this it_happened function is going to return None, which is falsy. That tells me that a common error that I may make might be to forget to return the True / False. Maybe we should check explicitly for True and False not just truthiness.

Anyway, the test is red so I can code:

class Timer:
    def __init__(self, delay, action):
        self.delay = delay
        self.action = action
        self.elapsed = 0

    def tick(self, delta_time):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action()
            if action_complete:
                self.elapsed = 0

My test is green. Let’s check the reset code, which I already wrote.

    def test_reset(self):
        happened = False
        def action():
            nonlocal happened
            happened = True
            return True
        delay = 1
        timer = Timer(delay, action)
        assert not happened
        timer.tick(1)
        assert happened
        assert timer.elapsed == 0

That’s green as well. Should we do something about an action that doesn’t return literal True or False? Should we do something about None?

It might be useful to return something truthy, but returning None is almost certainly a mistake. I hate exceptions, but I think this calls for one.

    def tick(self, delta_time):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action()
            if action_complete == None:
                raise Exception("Timer action may not return None")
            if action_complete:
                self.elapsed = 0

Now, of course, my first test fails because its action does return None. I change that test to be correct, returning True, and add this one:

    def test_none_raises_exception(self):
        happened = False

        def action_without_return():
            nonlocal happened
            happened = True
        delay = 1
        timer = Timer(delay, action_without_return)
        with pytest.raises(Exception):
            timer.tick(1.5)
        assert happened

Tests are green. Commit: Implement new Timer object.

Now that we have the thing, lets try applying it. Here’s an opportunity:

    def check_saucer_spawn(self, saucer, saucers, delta_time):
        if saucers: return
        self.saucer_timer -= delta_time
        if self.saucer_timer <= 0:
            saucer.ready()
            saucers.append(saucer)
            self.saucer_timer = u.SAUCER_EMERGENCE_TIME

Added in Post:

It turns out below that I decide not to do this one, as it is too tricky for a first application of the new Timer object.

Let’s see where we first define saucer_timer:

    # noinspection PyAttributeOutsideInit
    def init_asteroids_game_values(self):
        self.asteroids_in_this_wave: int
        self.saucer_timer = 0
        self.saucer_zigzag_timer = 0
        self.ship_timer = 0
        self.ships_remaining = 0
        self.wave_timer = u.ASTEROID_TIMER_STOPPED

OK, we’ll want a Timer with delay = u.SAUCER_EMERGENCE_TIME and action doing the saucer.ready and saucers.append. And, hmm, we need access to the saucer and saucers collection.

I think I need to go back to my tests to see how to do this. What I’d like to do is to be able to pass arguments to the Timer, and have those passed on to the action. We couldn’t write to them, but we could call them and such. Here’s my test:

    def test_timer_with_args(self):
        saucer = Saucer()
        saucers = []

        def start_saucer(saucer, saucers):
            saucer.ready()
            saucers.append(saucer)
            return True
        delay = 1
        timer = Timer(delay, start_saucer, saucer, saucers)
        timer.tick(1.1)
        assert saucers

This doesn’t work because Timer’s init doesn’t expect those args.

I extend Timer:

class Timer:
    def __init__(self, delay, action, *args):
        self.delay = delay
        self.action = action
        self.args = args
        self.elapsed = 0

    def tick(self, delta_time):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action(*self.args)
            if action_complete is None:
                raise Exception("Timer action may not return None")
            if action_complete:
                self.elapsed = 0

The test runs. We can pass extra arguments to our Timer, and those arguments will be passed to the action function.

Now let’s see how we might use the Timer in the game.

I want to say this:

    # noinspection PyAttributeOutsideInit
    def init_asteroids_game_values(self):
        self.asteroids_in_this_wave: int
        self.saucer_zigzag_timer = 0
        self.ship_timer = 0
        self.ships_remaining = 0
        self.wave_timer = u.ASTEROID_TIMER_STOPPED

        def spawn_saucer(saucer, saucers):
            saucer.ready()
            saucers.append(saucer)
            return True
        self.saucer_timer = Timer(u.SAUCER_EMERGENCE_TIME, self.saucer, self.saucers)

But that’s not going to work, because saucer and saucers aren’t defined yet. We’ll create the timer with the wrong values.

I think I’d better put this somewhere else. I’ll add a separate init for timers:

    def __init__(self, testing=False):
        self.init_general_game_values()
        self.init_asteroids_game_values()
        self.init_space_objects()
        self.init_timers()
        self.init_pygame_and_display(testing)

Added in Post:

Here I decide on a simpler first application of Timer.

And I noticed the zigzag timer and I think it might be an easier first one. Let’s see how it works … interesting. It’s not used. zigzagging is actually inside the saucer.

Let’s do use Timer in the Saucer as our first live run.

class Saucer:
	def __init__...
	    ...
        self.zig_timer = 1.5
        ...

    def check_zigzag(self, delta_time):
        self.zig_timer -= delta_time
        if self.zig_timer <= 0:
            self.zig_timer = u.SAUCER_ZIG_TIME
            self.velocity = self.new_direction() * self.direction

Changing this will break some tests. So be it, we have tests for timer.

It turns out we init the timer twice, once in __init__ and once in ready. Here’s ready:

    def ready(self):
        self.direction = -self.direction
        self.velocity = self.direction * u.SAUCER_VELOCITY
        x = 0 if self.direction > 0 else u.SCREEN_SIZE
        self.position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
        self.missile_timer = u.SAUCER_MISSILE_DELAY

        def zig(saucer):
            saucer.velocity = saucer.new_direction()*saucer.direction
            return True
        self.zig_timer = Timer(1.5, zig, self)

I could use the word self in zig, but decided not to. And I could probably call a method directly, but I’ve not tried that yet.

Anyway, with this change, the saucer zigzags as it should.

Commit: use Timer in saucer zigzag.

Let’s reflect.

Reflection

The Timer object exists, and it’s a reasonable implementation. Let’s extract that code to a method to remove the duplication. And we need to decide what to do about the failing tests, which AARGH I’ve committed. Bad Ron, no biscuit!

class TestSaucer:
    def test_ready(self):
        saucer = Saucer()
        saucer.ready()
        assert saucer.position.x == 0
        assert saucer.velocity == u.SAUCER_VELOCITY
        assert saucer.zig_timer == u.SAUCER_ZIG_TIME
        assert saucer.missile_timer == u.SAUCER_MISSILE_DELAY
        saucer.zig_timer = 0
        saucer.missile_timer = 0
        saucer.ready()
        assert saucer.position.x == u.SCREEN_SIZE
        assert saucer.velocity == -u.SAUCER_VELOCITY
        assert saucer.zig_timer == u.SAUCER_ZIG_TIME
        assert saucer.missile_timer == u.SAUCER_MISSILE_DELAY

I modify the test and it finds a bug:

class TestSaucer:
    def test_ready(self):
        saucer = Saucer()
        saucer.ready()
        assert saucer.position.x == 0
        assert saucer.velocity == u.SAUCER_VELOCITY
        assert saucer.missile_timer == u.SAUCER_MISSILE_DELAY
        saucer.missile_timer = 0
        saucer.ready()
        assert saucer.position.x == u.SCREEN_SIZE
        assert saucer.velocity == -u.SAUCER_VELOCITY
        assert saucer.zig_timer.delay == u.SAUCER_ZIG_TIME
        assert saucer.zig_timer.elapsed == 0
        assert saucer.missile_timer == u.SAUCER_MISSILE_DELAY

The code in saucer __init__ isn’t using u.SAUCER_ZIG_TIME, it’s using 1.5, and I copied that down into ready. Wrong! Fix that and extract:

class Saucer:
    def __init__(self, position=None, size=2):
        self.position = position if position is not None else u.CENTER
        self.size = size
        self.velocity = u.SAUCER_VELOCITY
        self.directions = (self.velocity.rotate(45), self.velocity, self.velocity, self.velocity.rotate(-45))
        self.direction = -1
        self.radius = 20
        raw_dimensions = Vector2(10, 6)
        saucer_scale = 4 * self.size
        self.offset = raw_dimensions * saucer_scale / 2
        saucer_size = raw_dimensions * saucer_scale
        self.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
        self.missile_timer = u.SAUCER_MISSILE_DELAY
        self.set_zig_timer()

    def set_zig_timer(self):
        def zig(saucer):
            saucer.velocity = saucer.new_direction() * saucer.direction
            return True

        # noinspection PyAttributeOutsideInit
        self.zig_timer = Timer(u.SAUCER_ZIG_TIME, zig, self)

    def ready(self):
        self.direction = -self.direction
        self.velocity = self.direction * u.SAUCER_VELOCITY
        x = 0 if self.direction > 0 else u.SCREEN_SIZE
        self.position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
        self.missile_timer = u.SAUCER_MISSILE_DELAY
        self.set_zig_timer()

We’re green and with somewhat better code. Commit: fix bug in zig timer, improve test, make tests run. Sorry.

I’ve been at this for nigh on to three hours, and the article is long. Let’s sum up.

Summary

I’ve built a new object, Timer, that applies ab action and optionally resets the timer if the action returns True. If it were to return False, the action will be repeated on each tick of the Timer until it does return True.

The action can accept arguments, and can be rigged, using nonlocal, to refer to objects in the scope where it is defined. I’ve not yet tried calling a method directly but if we pass self to the Timer I think everything should work. We’ll test that before we rely on it.

The object has shortened the code for the saucer’s zig-zag timer a bit, from:

    def check_zigzag(self, delta_time):
        self.zig_timer -= delta_time
        if self.zig_timer <= 0:
            self.zig_timer = u.SAUCER_ZIG_TIME
            self.velocity = self.new_direction() * self.direction

To …

    def check_zigzag(self, delta_time):
        self.zig_timer.tick(delta_time)

I would certainly freely grant that the Timer setup is a bit more esoteric than the longer code above, as it looks like this:

    def set_zig_timer(self):
        def zig(saucer):
            saucer.velocity = saucer.new_direction() * saucer.direction
            return True

        # noinspection PyAttributeOutsideInit
        self.zig_timer = Timer(u.SAUCER_ZIG_TIME, zig, self)

I may be too in love with the Timer to assess it fairly, but I think that the additional complexity in setup is probably paid back by the easier ticking. We should get a better sense of that after a few more applications of Timer.

One thing seems clear: it should make all our timing code look similar, and when we look at the calls to tick they’ll be easy to ignore when browsing, if a bit more tricky to understand when we need the details. Needing the details will be rare, I hope.

Another wild idea has appeared!

If we provide a method for the Timer to call, we might find that the setup is easier and makes more sense. I’ll work on that soon.

We’ll see. This is a bit of a darling, but for now at least, we’ll allow it.

See you next time.