Python Asteroids on GitHub

A Timer-related idea. I have a little time, let’s try something. It goes very nicely, I think.

As I was spiking to see if Timer would help Thumper, I had an idea, and it goes like this:

The current Timer object takes a delay time, and a function (or method). You tick the timer, giving it delta_time, and when the delay has elapsed, the timer calls the method. If the method returns None or True, the timer resets. Otherwise, it will keep calling on subsequent ticks, until the method does return None or True.

The return bit allows you to call a method that can check conditions before carrying out its action. Simply by returning False, it can ask to be called again in few moments.

There is some fancy rigmarole in the tick, so that if your designated function requires arguments, you can provide them at the time of the tick. It’s a nice little object, and I am proud of it.

It is a bit obscure in one regard: when you tick the timer, you don’t see what it’s going to call, as that is defined when it is created. It looks like this:

class SaucerMaker(Flyer):
    def tick(self, delta_time, fleet, fleets):
        if self._saucer_gone:
            self._timer.tick(delta_time, fleets)

What does this timer do? We have to look up in the __init__:

class SaucerMaker(Flyer):
    def __init__(self):
        self._timer = Timer(u.SAUCER_EMERGENCE_TIME, self.create_saucer)
        self._saucer_gone = True

Then, we can look at create_saucer:

    @staticmethod
    def create_saucer(fleets):
        fleets.add_flyer(Saucer())

What if, instead, we told the timer when we tick it, what to call, like this:

    def tick(self, delta_time, fleet, fleets):
        if self._saucer_gone:
            self._timer.tick(delta_time, self.create_saucer, fleets)

Would that be better, because it’s more clear in the call to Timer.tick what it’s going to do?

Note
We can’t say …
    self._timer.tick(delta_time, self.create_saucer(fleets))
Because
… that would unconditionally call create_saucer before ever calling tick. We must pass a function, not a result.

Honestly, I think that would be better. It seems that whenever I want to know what a given timer will call, usually because I want to revise how that function works, I have to go from the tick to the Timer, to the method, instead of from the tick directly to the method.

I think that what I’ll do is create the other kind of Timer, with TDD, of course, and then if that works out, try it in a few places to see if I really like it.

We have some pretty decent Timer tests:

class Checker:
    def __init__(self, extra):
        self.happened = 0
        self.extra = extra if extra else 0

    def set(self, value):
        self.happened = value + self.extra
        return True


class TestTimer:
    def test_creation(self):
        happened = False

        def it_happened():
            nonlocal happened
            happened = True
            return True

        delay = 3
        timer = Timer(delay, it_happened)
        assert not happened
        timer.tick(0.1)
        assert not happened
        timer.tick(3)
        assert happened

    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

    def test_returning_none_resets_timer(self):
        happened = False

        def action_without_return():
            nonlocal happened
            happened = True

        delay = 1
        timer = Timer(delay, action_without_return)
        timer.tick(1.5)
        assert happened

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

        def start_saucer(a_saucer, the_saucers):
            the_saucers.append(a_saucer)
            return True

        delay = 1
        timer = Timer(delay, start_saucer, saucer, saucers)
        timer.tick(1.1)
        assert saucers

    def test_with_method(self):
        checker = Checker(19)
        another = Checker(9)
        some_value = 31
        timer = Timer(1, checker.set, some_value)
        timer2 = Timer(1, another.set, 21)
        timer.tick(1.1)
        assert checker.happened == 31 + 19
        timer2.tick(1.1)
        assert another.happened == 21 + 9

    def test_tick_args(self):
        result = ""

        def action(action_arg, tick_arg):
            nonlocal result
            result = action_arg + " " + tick_arg
            return True

        timer = Timer(1, action, "action arg")
        timer.tick(1.1, "tick arg")
        assert result == "action arg tick arg"

I note in that last test that Timer allows us to provide arguments both at the time of defining the Timer, and at the time of ticking it. I wonder if we ever use the first aspect. A quick Command+B tells me that the only use of that feature is in the tests. Interesting. Someone built this object without a real case in mind, or for some other reason added an unused capability. The horror! The waste! The fun!

It occurs to me that the existing Timer could probably be extended to allow us to provide a function as the first parameter to the tick.

Let’s write a test to see.

    def test_tick_with_function(self):
        result = ""

        def action(arg):
            nonlocal result
            result = arg

        timer = Timer(1)
        timer.tick(1.1, action, "hello")
        assert result == "hello"

This won’t run because Timer demands a function as its first argument.

class Timer:
    def __init__(self, delay, action: Callable[[...], Union[None, bool]], *args):

I walked through fire to figure out that type prompt, by the way.

Can we just give it a default? Let’s follow that path.

class Timer:
    def __init__(self, delay, action: Callable[[...], Union[None, bool]] = None, *args):
        """action is callable returning None or bool"""
        self.delay = delay
        self.action = action if action else self.call_back
        self.args = args
        self.elapsed = 0

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action(*self.args, *tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

    def call_back(self, *args):
        return args[0](*args[1:])

My test actually runs! I don’t really think this is very robust. I’m not sure what would happen if I were to define parameters in the Timer creation but not provide a function. I’ll test that.

    def test_tick_with_function_and_timer_args(self):
        result = ""

        def action(arg):
            nonlocal result
            result = arg

        timer = Timer(1, None, "howdy and")
        timer.tick(1.1, action, "hello")
        assert result == "howdy and hello"

Python required me to provide the None, because it knew I needed a function there, not a string. But when I test, I get this:

self = <timer.Timer object at 0x105717dd0>
args = ('howdy and', <function TestTimer.test_tick_with_function_and_timer_args.<locals>.action at 0x1056f28e0>, 'hello')

    def call_back(self, *args):
>       return args[0](*args[1:])
E       TypeError: 'str' object is not callable

I think I can explain this and I even think that I could fix it. In trying to explain it, I think I should decide that this is not a good path. I think the issue is this:

When the timer is created, the action is None and the args are ("howdy and"). When we tick, our call_back just gets all the parameters in a sequence, which will be ("howdy and", action, "hello"). The function is in the middle of things.

Could I unstring them? Probably yes.

A bit of horsing around and a small update to the test and I get this:

    def test_tick_with_function_and_timer_args(self):
        result = ""

        def action(timer_arg, call_arg):
            nonlocal result
            result = timer_arg + call_arg

        timer = Timer(1, None, "howdy and ")
        timer.tick(1.1, action, "hello")
        assert result == "howdy and hello"

class Timer:
    def call_back(self, *args):
        args_len = len(self.args)
        return args[args_len](*args[:args_len], *args[args_len+1:])

That obscenity assumes that the args consists of len(self.args) objects, followed by the function to be called, followed by any arguments supplied in the tick. And it slices accordingly, calling the function that it finds embedded in the sequence passing all the other elements of the sequence to it.

I am proud of this only in the sense that I made it work. And it does show that the idea of moving the callback to the tick is feasible and I do think that if we can in fact feas1 it, it will make Timer code easier to write and read.

Things I do not like:

  1. I had to create the Timer with an explicit None as the function to call. That makes using the new supposed to be simpler form less simple.

  2. The code is error-prone. If I do not pass a function on the tick, it will not be diagnosed by Python or PyCharm, but it will fail trying to call some random thing. And it will only fail when the timer ticks out. If there are good tests for that particular timer, the tests will find the problem, but otherwise it will crash at run time.

  3. The slicing is not obvious. (I could probably make it more clear, or live with it, if the other two concerns were not present.)

What could we do?

There are a number of possibilities.

  1. We could skip the whole idea. I hate to do that: it might actually be a good one, since we don’t use all the Timer’s capability.

  2. We could create a new SimpleTimer class that works as we intend here, and leave Timer alone. We could probably actually use Timer in the SimpleTimer implementation.

  3. We could simplify Timer so that it never accepts the method to call during creation, and never accepts parameters at that time, just accepting the delay. That would require us to change all the existing uses of Timer to specify the function to call in tick, rather than at creation time. On the upside, that would be a bit more clear than things are now.

In a real product, maybe.

If this were a real product and it was nearly done, we should skip the whole thing. If it were a real product with lots more timing to do, either creating a new kind of timer, or simplifying the existing one and changing it uses might be worth it.

For us here, yes.

For my purposes here, which are to explore solutions, and to explore discovering places in our code that could be improved, and showing that we can improve them step by step, safely, it could of course be worth doing. Whether it works or not, we get to see what happens. Sometimes the bear bites me. That’s OK, that’s what happens in real life, too.

So for us here, the question in this case is “how”. (Excuse me, I have to take another call from Dr Ian Malcolm …) As I was saying, what if we put in this somewhat iffy code that is running now in my spike, perhaps beefed up a bit. Perhaps first we would remove Timer’s initial parameter list, since no one is using it and we can do that with impunity.

Then we could enable the new feature, with the original simpler “just call the first argument” formulation. Then over however much time it takes, we change existing timer usage over to the new form. Then we remove the old form, because the new form is more clear.

Our tests are green. Let’s double check to be sure no one is using the ability to provide arguments when creating a Timer. Command+B on the class name. I confirm, the only uses of that are in the tests. We’ll remove the feature and scrap the corresponding tests.

All I do below is remove the args parameter from __init__, and its one reference:

class Timer:
    def __init__(self, delay, action: Callable[[...], Union[None, bool]] = None):
        """action is callable returning None or bool"""
        self.delay = delay
        self.action = action if action else self.call_back
        self.elapsed = 0

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action( *tick_args) # <===
            if action_complete is None or action_complete:
                self.elapsed = 0

    def call_back(self, *args):
        return args[0](*args[1:])

Four tests fail. I remove two and recode two that seemed useful.

We are green. Test the game just for additional comfort. Game’s good. Commit: Timer no longer accepts arguments when created, no one used the feature.

Now as you can see above, I did leave in the call_back function, which allows users to provide the call_back in tick. I should mention that in the Commit. I’ll make some trivial change in the tests and recommit: “Timer can now accept None for callback function on creation, and accept a provided function in tick.”

Now, anyone who creates a timer can remove the callback from its creation and put it in the tick. Let’s do that for … Saucer.

Old:

class Saucer(Flyer):
    def _set_zig_timer(self):
        # noinspection PyAttributeOutsideInit
        self.zig_timer = Timer(u.SAUCER_ZIG_TIME, self.zig_zag_action)

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

This guy is factored down to his nubbins, but that’s fine for my purposes here. I just do this:

New:

    def _set_zig_timer(self):
        # noinspection PyAttributeOutsideInit
        self.zig_timer = Timer(u.SAUCER_ZIG_TIME)

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

Tests are green. Run the game, I like to watch. Works a treat. Commit: Saucer converted to provide callback in tick rather than Timer creation.

I noticed in doing it that all I have to do is cut the second argument from the Timer() and paste it into the tick, right after delta_time. Repeat for the other Saucer timer:

    def _set_firing_timer(self):
        # noinspection PyAttributeOutsideInit
        self.missile_timer = Timer(u.SAUCER_MISSILE_DELAY)

    def fire_if_possible(self, delta_time, fleets):
        self.missile_timer.tick(delta_time, self.fire_if_missile_available, fleets)

That’s all there is to it. Test, Commit: Saucer uses callback in tick for both timers.

Who else uses Timers?

Nota Bene
These changes could be going on for months, whenever someone comes near a Timer or has a random moment to do some refactoring. I just happen to be doing them all right now. The Timer is fine as it stands.
class Fragment(Flyer):
    def __init__(self, position, angle=None, speed_mul=None, fragments=None):
    	...
        self.timer = Timer(u.FRAGMENT_LIFETIME)
        ...

    def tick(self, delta_time, fragments, _fleets):
        self.timer.tick(delta_time, self.timeout, fragments)

Test fails until I paste the call into tick, so I know we’re good. Commit: Fragment uses callback in tick not Timer creation.

Very similar change in Missile, not worth showing. Commit: Missile uses callback in tick not Timer creation.

Exact same thing in SaucerMaker. Commit with bad message: Saucer uses callback in tick not Timer creation. Sorry, I was going too fast. Won’t amend because I find that amending a pushed commit is a pain.

Exact same thing in ShipMaker. This time commit: ShipMaker uses callback in tick not Timer creation. Previous commit should have said SaucerMaker, not Saucer.

Fine, so sue me.

The only remaining one is in WaveMaker.

class WaveMaker(Flyer):
    def __init__(self):
        self._need_asteroids = None
        self._timer = Timer(u.ASTEROID_DELAY)
        self._number_to_create = 2

    def tick(self, delta_time, fleet, fleets):
        if self._need_asteroids:
            self._timer.tick(delta_time, self.create_asteroids, fleets)

As with each of these, a test fails when I do the cut from creating the timer, and runs again when I complete the paste. Commit: WaveMaker uses callback in tick not Timer creation.

Months Later …

There are no more calls to Timer that pass anything other than delta_time.

We can adjust its __init__ accordingly, and we should also improve the tick. In fact, we should have done that a while ago, as you’ll see when I do it now.

Going in, this is Timer:

class Timer:
    def __init__(self, delay, action: Callable[[...], Union[None, bool]] = None):
        """action is callable returning None or bool"""
        self.delay = delay
        self.action = action if action else self.call_back
        self.elapsed = 0

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action( *tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

    def call_back(self, *args):
        return args[0](*args[1:])

First, enhance the call_back with type annotation, as we should have already done.

    def call_back(action: Callable[[...], Union[None, bool]], *args):
        return action(*args)

We should have made the callback explicit long ago. I just wasn’t smart enough to think of it. Maybe I was moving too fast: I’ve been known to do that.

Commit: required callback action parameter in call_back is annotated with type information.

Now we can remove the action parameter from __init__, as unused, unwanted, and generally unneeded. This is a bit more tricky than it might be. We have callers who need fixing and callers who do not.

First let’s force it always to go to call_back.

No. Fix up all the tests. Don’t be silly.

Yes
Yes. If I make up my mind and then quickly change it, I mention it. This is to remind us all that it is OK to change our mind. We don’t have to do whatever thing first comes to us. We could even start and then roll back.

In the tests, I just do my standard cut the action from Timer(), paste it into tick and we’re all green. Now no one is using the call_back parameter in Timer.__init__.

Let’s review things and see what we need. I really want to do this in small, safe, steps.

class Timer:
    def __init__(self, delay, action: Callable[[...], Union[None, bool]] = None):
        """action is callable returning None or bool"""
        self.delay = delay
        self.action = action if action else self.call_back
        self.elapsed = 0

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action( *tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

    @staticmethod
    def call_back(action: Callable[[...], Union[None, bool]], *args):
        return action(*args)

We can remove the parameter and set action unconditionally.

class Timer:
    def __init__(self, delay):
        """action is callable returning None or bool"""
        self.delay = delay
        self.action = self.call_back
        self.elapsed = 0

Commit: Timer uses self.call_back unconditionally.

We’re left with this:

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.action( *tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

    @staticmethod
    def call_back(action: Callable[[...], Union[None, bool]], *args):
        return action(*args)

I’d like to do this in one move, but I can’t see how. I have to change two lines, the tick line and the action_complete line. Hm. Will it inline for me?

No but can’t I just call call_back where I call action? Yes.

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = self.call_back(*tick_args) # <===
            if action_complete is None or action_complete:
                self.elapsed = 0

    @staticmethod
    def call_back(action: Callable[[...], Union[None, bool]], *args):
        return action(*args)

Green. Commit: moving toward action in tick.

Ah I think I see a safe next step, if I can extract a variable:

    def tick(self, delta_time, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action = self.call_back
            action_complete = action(*tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

Meh. That’s solid and I could commit: extract variable action.

But there’s still a two-line change needing to be done. OK, give up and do it.

    def tick(self, delta_time, action, *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = action(*tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

Add the type hint, and remove call_back method:

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

    def tick(self, delta_time, action: Callable[[...], Union[None, bool]], *tick_args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = action(*tick_args)
            if action_complete is None or action_complete:
                self.elapsed = 0

Rename tick_args as we no longer need the distinction:

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

    def tick(self, delta_time, action: Callable[[...], Union[None, bool]], *args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = action(*args)
            if action_complete is None or action_complete:
                self.elapsed = 0

Commit: Timer now expects action callback in tick and so specifies. General refactoring to simplify.

I am tempted to check explicitly for True in the final bit there. The basic idea is this:

You can return True or False from your callback function, returning False to indicate that you didn’t complete your action and want an immediate callback on the next tick. This allows us to do things like check for safety before we emerge.

However, most functions, it seemed to me at the time, would just complete and be happy, and it was nearly certain that if I wasn’t thinking in terms of might or might not, I’d forget to return True, thus returning None. So, the Timer accepts either None or anything truthy as indicating that it should reset.

It seems to me that this code is at least asking for a comment. And I was taught by Kent Beck that “a comment is the code’s way of asking to be made more clear”.

How could we do that? We reset unless you explicitly return False. Or, if you were to return anything falsy, which would be unfortunate if you were to accidentally return zero.

However … the type hint might warn you if you did that wrong … but those aren’t reliable yet in Python, if they ever will be.

How about this:

    def tick(self, delta_time, action: Callable[[...], Union[None, bool]], *args):
        self.elapsed += delta_time
        if self.elapsed >= self.delay:
            action_complete = action(*args)
            self.reset_unless_explicitly_false(action_complete)

    def reset_unless_explicitly_false(self, action_complete):
        if action_complete is not False:
            self.elapsed = 0

I tried in-lining the action call but then it’s less clear that the point is to call the action, and the method name would have to be something like call_action_and_reset_unless_it_explicitly_returns_false and life is too short even to read that let alone type it in.

I think we’ll leave it this way. It’s just a tiny bit more explicit and explicit is good.

Commit: refactor for improved clarity.

Let’s Sum Up

This was a rather long exercise, 3 1/2 hours with all the writing and pasting. I pity you if you tried to read every word. But it was an easy 3 1/2 hours, with breaks and random chats with my dear wife and at least one visit from the cat. The changes spanned quite a few classes, but the tests broke every time they should have, giving me great confidence that my timers were all working correctly.

Of course it was clear that they “should”, but seeing the test go red when I did the cut and green when I did the paste was quite nice. And once, I forget where, I pasted in the wrong place and the test not turning green told me I’d done it wrong.

What have we accomplished?

We have made it more explicit what will happen when a Timer ticks to completion, because the function to be called is right there in the tick, instead of hidden away in the Timer creation. I think that’s just flat better. I’d have to think pretty hard to think of a case where I’d prefer it the old way.

Would we do this in real life? Toward the end of a project, it’s probably not worth it. In the middle, it might well be.

I’m here to find out what’s possible and how hard it is. From that angle, I’m happy, because it was quite possible and quite straightforward. I think that almost every design change we need to make is like this. If our code is well-structured, changes are generally not troubling.

And conversely, or contrariwise or whatever the term is, when changes are hard to make, it’s a clue that our code isn’t as well-structured as it might be. The thing to do might be to improve the structure a bit and then make the change.

In my experience, if we make a point of making the code just a bit better every time we pass through it, our code base will improve, our skills will improve, and our work experience will improve. Perhaps you’d like to try it and let me know what you find.

Anyway, more than enough words for today.

See you next time!


  1. Surely if something is feasible you can feas it? ↩