P-238 - Good Ideas

Python Asteroids+Invaders on GitHub

Not my ideas, mind you. Two good ideas from Tomas set me on a course for last evening and this morning. Are there rats in here?

Tomas Aschan offered two thoughts that have set up my next few steps:

@RonJeffries I don’t think I’d have implemented it as a feature of Fleets, but rather as a thing of its own that any flyer can compose with as needed. But my lifecycle is a little bit different than yours, too (eg I don’t pass fleets to interact_with), so that probably pushes me in slightly different directions.

What if you, instead of multipart reminder lists, use a lambda that just calls one method? Ie fleets.remind_me(lambda: self.set_foo(bar)) - does that simplify someone of the plumbing?

You may recall that I tried my reminders with lambda and for some reason couldn’t make it work. I was sure that it would be nicer, but somehow I couldn’t get the syntax right. So I tried again last night and this time the odds were in my favor. Now the calls look like this:

    def die_on_collision(self, flyer, fleets):
        if self.colliding(flyer):
            fleets.remind_me(lambda: self.die(fleets))

That used to be this more awkward formula:

    def die_on_collision(self, flyer, fleets):
        if self.colliding(flyer):
            fleets.remind_me(self.die, fleets)

The lambda form is more clear, I think, given that you recognize the lambda syntax, which of course all here do or are willing to learn¹.

The reminders code itself gets simpler. I’m not finished with it yet, so follow along. Here’s where it is now, making the lambda work with minimal changes, so far, to the reminder code:

class _Reminder:
    def __init__(self, func, _args):
        self.func = func

    def execute(self):
        self.func()

Here, I haven’t even removed the args yet, I’ve just marked them ignored and recorded the lambda, named func because lambda would be confusing at best.

The code in Fleets looks like this:

    def begin_interactions(self):
        self.reminders = []
        for flyer in self.all_objects:
            flyer.begin_interactions(self)

    def end_interactions(self):
        for flyer in self.all_objects:
            self._execute_reminders()
            flyer.end_interactions(self)

    def remind_me(self, reminder: Callable, *args):
        reminder = _Reminder(reminder, args)
        self.reminders.append(reminder)

    def _execute_reminders(self):
        for reminder in self.reminders:
            reminder.execute()
        self.reminders = []

Here again, I’ve not even removed the args yet. I think we can do that with impunity, but a quick Command+B assures me that no one is using additional arguments. Good for them, because they don’t do anything. I’ll remove that and commit what I have: switch to using lambda in reminders.

Now to Tomas’s other idea, making the reminders notion separate from Fleets. While adding the idea to Fleets has certainly been convenient, and is providing the deferred operation we want, it has certainly made Fleets less cohesive than it was before. It now has this additional appendix of responsibility “oh, right, and would you please remember some stuff for me?”.

So let’s provide a separate class for doing reminders, and objects that want to defer things can use it as they wish. I think the conventional use in a Flyer will be to create or init reminders in begin_interactions, and unwind them in end_interactions, but there may well be other places where the facility could be useful.

I think we’ll build it for Fleets, because for backward compatibility we’ll want to leave it in fleets for a little while, so that we can take smaller steps. Then we’ll find users of the Fleets capability, fix them up one at a time, and then remove the reminder calls from Fleets. (Of course, Fleets might want its own reminders. Once we have this hammer object, everything may start looking like a nail something to do later.)

We will of course² TDD the thing into existence. Remind me that I have an interesting additional idea for it.

I decide to call it TodoList:

class TestReminders:

    def test_exists(self):
        TodoList()

class TodoList:
    pass

Fantastic! Ship it! In fact we can. Let’s see how many commits we can do. Commit: initial test and class for TodoList.

OK, what do we want to say to this TodoList. I really like remind_me well enough to use it. Let’s stick with that and execute.

I suspect I’ll need some kind of little test classes for this thing. We’ll find out quickly:

class TestReminders:
    def __init__(self):
        self.value = None

    def save_value(self, value):
        self.value = value

    def test_exists(self):
        TodoList()

    def test_remembers_things(self):
        todo = TodoList()
        todo.remind_me(lambda: self.save_value(17))
        todo.execute()
        assert self.value == 17

Maybe we can do everything here in the test class. We need some methods on TodoList.

class TodoList:
    def __init__(self):
        self.todos = []
        
    def remind_me(self, func):
        self.todos.append(func)

    def execute(self):
        for func in self.todos:
            func()

We are green. Commit: TodoList keeps list and iterates it.

I went beyond the test here. I could have just saved the one callable. “Fake it till you make it.” But it seemed silly. Let’s do test further.

class TestReminders:
    def __init__(self):
        self.value = None
        self.total = 0

    def save_value(self, value):
        self.value = value

    def accumulate(self, value):
        self.total += value

    def test_has_list(self):
        todo = TodoList()
        todo.remind_me(lambda: self.save_value(86))
        todo.remind_me(lambda: self.accumulate(13))
        todo.remind_me(lambda: self.accumulate(13))
        todo.execute()
        assert self.value == 86
        assert self.total == 26

That runs perfectly, of course. I should rename the test class to TestTodoList. First let’s commit this new capability: additional test.

Do the rename:

class TestTodoList:

Commit: rename test class.

Now an important test:

    def test_clears_list(self):
        todo = TodoList()
        todo.remind_me(lambda: self.accumulate(13))
        todo.execute()
        assert self.total == 13
        todo.execute()
        assert self.total == 13

I expected that test to fail, because I execute twice, and it does not. I am surprised!

I am even more surprised. None of these tests are running!!!

I think that renaming the class broke it. Or were they never running???

They were never running. My init has broken things. Live and learn.

Now they are running and failing. Let’s see if I can make this init work. No. Some research tells me that I cannot: pytest ignores classes with inits. OK, we need to recast these a bit.

class TestTodoList:

    def save_value(self, value):
        self.value = value

    def accumulate(self, value):
        self.value += value

    def test_exists(self):
        TodoList()

    def test_remembers_things(self):
        todo = TodoList()
        todo.remind_me(lambda: self.save_value(17))
        todo.execute()
        assert self.value == 17

    def test_has_list(self):
        todo = TodoList()
        todo.remind_me(lambda: self.save_value(13))
        todo.remind_me(lambda: self.accumulate(13))
        todo.remind_me(lambda: self.accumulate(13))
        todo.execute()
        assert self.value == 39

We’ll just have the one saved value, value and accumulate into it. Inconvenient but life is tough.

These are green. Commit: fix issue where todo list tests were not running.

    def test_clears_list(self):
        todo = TodoList()
        todo.remind_me(lambda: self.save_value(31))
        todo.remind_me(lambda: self.accumulate(13))
        todo.execute()
        assert self.value == 44
        self.value = 0
        todo.execute()
        assert self.value == 0

This fails, with a 44 because the list executed again. I do not want that: once things are done, they are done. If you want to do them again, schedule them again. Them’s the rules.

class TodoList:
    def __init__(self):
        self.todos = []
        
    def remind_me(self, func):
        self.todos.append(func)

    def execute(self):
        for func in self.todos:
            func()
        self.todos.clear()

Green. Commit: todolist is cleared after execution.

Let’s reflect.

Reflection

So that went well. Odd that the tests weren’t running and I didn’t realize it. A pure TDD person would say that a test should always be seen to fail before seeing green. That habit would have served me well this morning, but it’s not a habit that I have formed. I’ll try to be more alert in the future and I will definitely be more aware that you can’t init a test class.

I like the name remind_me although arguably append would be more standard. I prefer the more meaningful name here. I do not like execute much at all, but didn’t have a better idea. How about do, or do_all?

How about finish? Yes, I think I like that one. Let’s change it. Change Signature breaks other users of the same word. I’ll do it the hard way. Commit: rename execute to finish.

I think we should extract the clear into a separate method, and keep it public. Let’s test that into existence:

    def test_can_clear_list(self):
        todo = TodoList()
        self.value = 0
        todo.remind_me(lambda: self.save_value(31))
        todo.clear()
        todo.finish()
        assert self.value == 0

class TodoList:
    def __init__(self):
        self.todos = []
        
    def remind_me(self, func):
        self.todos.append(func)

    def finish(self):
        for func in self.todos:
            func()
        self.clear()

    def clear(self):
        self.todos.clear()

Commit: todolist has clear method.

Let’s reflect further.

Reflection

Are we done? I think almost. Done for now, let’s say. I do foresee a need and could provide for it now, but I think it’s generally better to wait until we have a real need. Sometimes our foresight isn’t perfect.

The additional idea is this: allow the user of TodoList to provide an optional name for a task (hmm task is a really good name, isn’t it?) and then allow them later to tell the list todos.remove(name="foo") and have that task removed.

Why? Because in things like ShipMaker, we’ll want to schedule create_new_ship in begin_interactions and then, if we see an existing ship, we want to cross that item off the list. We’ll wait and see for that, so that we do it in a fashion that’s good for the user. We could guess now and be OK, but waiting is.

This discussion makes me think that “task” is a better term. Let’s do some renaming:

class Tasks:
    def __init__(self):
        self.tasks = []
        
    def remind_me(self, func):
        self.tasks.append(func)

    def finish(self):
        for func in self.tasks:
            func()
        self.clear()

    def clear(self):
        self.tasks.clear()

Commit: rename TodoList to Tasks.

I considered renaming remind_me to task or add_task, but I like remind_me. I did not consider honeydo[^h.

OK, now it seems that Fleets could use this class for its reminders. Let’s do that. We currently have this:

class _Reminder:
    def __init__(self, func):
        self.func = func

    def execute(self):
        self.func()


class Fleets:
    def __init__(self):
        self.flyers = list()
        self.reminders = []

    def begin_interactions(self):
        self.reminders = []
        for flyer in self.all_objects:
            flyer.begin_interactions(self)

    def end_interactions(self):
        for flyer in self.all_objects:
            self._execute_reminders()
            flyer.end_interactions(self)

    def remind_me(self, reminder: Callable):
        reminder = _Reminder(reminder)
        self.reminders.append(reminder)

    def _execute_reminders(self):
        for reminder in self.reminders:
            reminder.execute()
        self.reminders = []

We can just plug Tasks in here, I think.

class Fleets:
    def __init__(self):
        self.flyers = list()
        self.tasks = Tasks()

Tests are breaking. Steady on, I’m not done yet.

    def begin_interactions(self):
        self.tasks = Tasks()
        for flyer in self.all_objects:
            flyer.begin_interactions(self)
    def end_interactions(self):
        for flyer in self.all_objects:
            self._execute_reminders()
            flyer.end_interactions(self)

    def remind_me(self, reminder: Callable):
        self.tasks.remind_me(reminder)

    def _execute_reminders(self):
        self.tasks.finish()

And green. Commit: Fleets now uses Tasks for its remind_me capability.

Reflect again.

Reflection

I probably should have mentioned in the commit that I removed the private _Reminder class.

This is a super stopping point if we wanted to stop. And with 400 lines in, maybe we should stop. We have a new class, Tasks, and another class, Fleets, is using it happily. We really want to offload that responsibility from Fleets, but that can be done incrementally, over time, bit by bit, in small steps.

We’ve profited from two more ideas from Tomas, just in this article. He’s becoming a good partner in my coding. I can only imagine how useful it would be to pair with him, or with anyone, getting better ideas even sooner. The way it works now is that I do some p-baked thing, he offers an improvement, and a time or two later, I pick up on the idea.

Truth is, that is often the way of it. I suppose it’s possible to “do it right the first time”, but let’s be realistic, that’s a very high bar and wherever possible in life, we try to arrange things to allow for adjustment.

Programming is difficult. No matter how small we make our steps, we are still managing a lot of ideas at the same time, and trying to get it perfect right out of the box is unrealistic. It is also unrealistic to imagine later that what’s in the code is perfect. When you encounter a programmer who claims their code is perfect, my advice³, if I ever gave advice, would be to recognize that inside they are very likely quaking with fear that their code is not up to snuff compared with the rest of the team, and that they might be found out as the charlatan they suspect they are. They’re not defending their code, they’re defending themselves.

What we need to do is to let go. The code is what it is. It was the best we could do when we wrote it. We had a lot on our minds at the time. Maybe that was the day we had a fire drill. Perhaps cats came in last night and tossed it around a bit. Some of this code looks strange. Surely it must have been leprechauns fiddling with it. Possibly there are rats in here. Anyway, let’s improve it a bit. No blame, no harm, no foul, no penalty, just make it better.

Today, with Tomas’s help, we made it a little bit better. Thanks, Tomas!

Oh, and we made 11 commits. Pretty good performance by my sluggish standards.

See you next time!