Simpler!

A wild idea has appeared! This one’s so nice I rolled out of bed early to work on it.

The currently-planned structure for a command batch makes some sense. It’s a list of requests. Each request has a bot identifier, and a list of commands. Each command has a verb and optional parameters. We have a nice little builder class for constructing these things, and yesterday created another nice set of classes for interpreting them. I’ve been pleased with them.

This morning, in the early hours of waking up, as is my habit, I was lazily thinking about the request processing classes, and how one might pass down the world instance, and the bot instance, while allowing the processing classes not to know much about each other or what was going on. Lists of classes, lambdas, things like that.

At one point, I was envisioning the “bottom” of things, wishing each bottom-level action could somehow be passed the relevant bot id and somehow convert the verb, a string, to a method to be called. Suddenly I saw it!

The World does not want all that structure. It is configured to execute a single command for a single entity, then move on to the next command for whatever entity it might use. The DirectConnection, as yet untouched by batch thinking, gets a call passing the bot id and the verb to be performed. The Bot, upon do_something, presently makes one or more calls to the connection, specifying its id and the verb to be performed.

No one in the whole chain of events wants that nested batched-up structure!

Suddenly I realized that a simple list of (id, verb, parameter), one after another, was perfectly compatible with the World. While it’s true that the Bot does more than one command at a time, it does pass itself to each one, so it is essentially asking for (id, verb, parameter) as well.

We just need a simple list of single-verb requests, containing id, verb, parameter. We do not need any of that builder, or any of those very lovely Processor classes.

This is such an obviously correct, and much simpler idea, that I actually rolled out a few minutes early to work on it.

Waste?

So have I just “wasted” a week’s worth of sessions doing my builders and the new processors, based on an overly complex design for a batch of commands? Well, we’ll be deleting a lot of code, for small values of “a lot”. And had I thought of this idea earlier, I wouldn’t have worked out how to do the builder and processor. On the other hand, I am happy to have them as tools in my kit, and I’m certainly better equipped to do that sort of thing than I was.

For me, with my limitations, one of which is that I really kind of like clever ideas like builders and the multi-class processor thing, those were necessary steps to get where we are, which is on the cusp of a much better solution to the problem we have, messaging between client and server.

Today, I can see a more direct path from where we were a few days back to where we’ll be in a day or so. A few days back, that path was not visible to me. There are certain rules that prohibit me from rolling back the clock to a few days ago, and those of you to whom something good happened over that interval, I hope all of you, should be grateful, since you might not be so lucky if we started over. If you see what I’m saying.

Programming is the business of changing code, as Professor GeePaw Hill tells us, but it is also the business of learning a bit every day, about what we need and how to get it. We take small steps in the direction we sense will get us closer to what we want, and, since we’re pretty good at those steps, we tend to get to better places. And sometimes, and today is one of those days, we get to a simpler place, a nicer place, than we had been building a few days, weeks, or months ago.

In our case today, we haven’t fully installed the builder / processor objects, so in the production code we don’t have very much to undo. But, because we keep our code pretty well structured, even if we had more of this in there, I suspect the basic structure would be such that we could unwind it pretty readily.

Anyway, this idea is tasty. Let’s get to it.

Looking Around

Let’s take the position that the World gets to say what it wants, and it’s going to say that what it wants is a simple list of (id, verb, parameter). It will run through that list, executing whatever verb on whatever bot, until done. It will keep track of all the bot ids it processed in the list, and it will return a list of the knowledge dictionaries for each of the bots that it dealt with.

It will be the job of the clients to send over lists of one or more requests for one or more bots. They can send as long or short a list as they wish, perhaps subject to limits we may come up with later. They’ll get back a list of knowledge dictionaries to distribute among the troops.

Our mission today, and perhaps over a few upcoming days, will be to get this scheme in place. So let’s see how things are happening now. I think we’ll start with a look at what the DirectConnection does now. Here’s a typical command:

class DirectConnection:
    def step(self, cohort, client_bot_id):
        rq = dict()
        rq['entity'] = client_bot_id
        step_action = {'verb': 'step'}
        rq['actions'] = [step_action,]
        self.world.execute(rq)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

And that of course really gets all JSONized and sent over the wires and unJSONized until World:

class World:
    def execute(self, request):
        id = request["entity"]
        entity = self.entity_from_id(id)
        actions = request["actions"]
        for action in actions:
            self.execute_action(entity, action)

    def execute_action(self, entity, action):
        verb = action["verb"]
        match verb:
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action["param1"]
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'turn':
                direction = action["param1"]
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {action}')

So here we are assuming a dictionary with two elements, ‘entity’ and ‘actions’, where ‘entity’ points to an id and ‘actions’ points to a list of dictionaries with ‘verb’ and ‘action’, which are dispatched to methods corresponding to the verbs.

Small Steps?

How can we do this in small steps, keeping everything working? Naively, we have to change World to expect the new simplified list, and DC to create such a list. If we do that, we have to get the code working in two classes at once. Instead, let’s do something like this:

We know we want World to work on a simple list of (id, verb, param). I think we want the elements to be little dictionaries with keys ‘entity’, ‘verb’, ‘param1’. Can we arrange World so that it retains the entry point execute, which accepts the old format, and transforms that format to the new list format, and continues to work? If we can, and if the code is properly factored, there will be new entry points that DC can use to pass in a list in the new form.

Let’s refactor so that execute_action is given all three parameters, unstructured, entity, verb, and param1. (I’m not sure whether the entity or the id will be more valuable but we seem always to use the entity so we’ll pass it.)

    def execute(self, request):
        id = request["entity"]
        entity = self.entity_from_id(id)
        actions = request["actions"]
        for action in actions:
            verb = action["verb"]
            try:
                param1 = action["param1"]
            except KeyError:
                param1 = None
            self.execute_action(entity, verb, param1)

    def execute_action(self, entity, verb, param1):
        match verb:
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = param1
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'turn':
                direction = param1
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

We are green. Can commit. Let’s do: execute_action now takes verb and parameter, not a dictionary.

One possible next step is to change execute to expect a list. But it has nine users, half of which are tests. Let’s instead posit a new method, execute_actions that takes the list, and change just one test at a time to use it, leaving execute in place until it’s no longer needed.

I’m tempted to rename execute but I think it won’t long endure, so we’ll leave it. Here’s a test to fix up:

    def test_one_step(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        rq = {
            'entity': bot_id,
            'actions': [
                {'verb': 'step'}
            ]
        }
        world.execute(rq)
        world_bot = world.entity_from_id(bot_id)
        assert world_bot.location == Location(6, 5)

We convert this to call the missing method with a list containing our one action, like this:

    def test_one_step(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        rq = { 'entity': bot_id, 'verb': 'step' }
        rq_list = [rq]
        world.execute_list(rq_list)
        world_bot = world.entity_from_id(bot_id)
        assert world_bot.location == Location(6, 5)

Failing for want of execute_list, which we provide:

class World:
    def execute_list(self, actions_list):
        for action in actions_list:
            id = action['entity']
            entity = self.entity_from_id(id)
            verb = action['verb']
            try:
                param1 = action["param1"]
            except KeyError:
                param1 = None
            self.execute_action(entity, verb, param1)

Green. Commit: new method execute_list accepts list of entity_id, verb, optional param1

Let’s reflect a moment before we unwind all this. Clearly this is a bit fragile. I mistakenly used ‘id’ as the key for the entity id, and got a failure until I asked for ‘entity’. And if anyone dares to send us a malformed action, the code will raise an exception and we’ll all fall down. That would be bad. We need to deal with that sort of thing.

I generally do not concern myself with exceptions, instead coding and testing until I’m sure they won’t happen. (This is risky and I would not do it in situations where exceptions would be seriously harmful.) But when we’re dealing with outside inputs, as we are here, we need to be prepared to get malformed input, so we’ll have to improve in this area.

But it is working. We can even change the DirectConnection calls if we wish:

class DirectConnection:
    def step(self, cohort, client_bot_id):
        rq = dict()
        rq['entity'] = client_bot_id
        rq['verb'] = 'step'
        rq_list = [rq]
        self.world.execute_list(rq_list)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

That works. Commit: step uses new list format requests.

Refactor step:

    def step(self, cohort, client_bot_id):
        rq_list = [{'entity': client_bot_id, 'verb': 'step'}]
        self.world.execute_list(rq_list)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

Commit again: refactoring.

Change another one:

    def take(self, cohort, client_bot_id):
        rq_list = [ {'entity': client_bot_id, 'verb': 'take'}]
        self.world.execute_list(rq_list)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

Green. Commit: take uses new list format requests.

Another:

    def drop(self, cohort, client_bot_id, holding_id):
        rq = [ {'entity': client_bot_id, 'verb': 'drop', 'param1': holding_id}]
        self.world.execute_list(rq)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

Green, commit. Another:

    def turn(self, cohort, client_bot_id, direction_string):
        rq = [ { 'entity': client_bot_id, 'verb': 'turn', 'param1': direction_string}]
        self.world.execute_list(rq)
        result_dict = self.world.fetch(client_bot_id)
        cohort.update(result_dict)

Green. Commit: all verbs use new format.

You’ll note that I started just using rq as the name of the list. I’ll go back and update the two where I didn’t yet have that pattern.

Now let’s sort out the names. I would like this new method, execute list to be named execute, so let’s rename the other method, which is still used in some tests, to execute_old_style_dictionary. PyCharm manages that just fine.

Now rename the new one to execute. That works. Commit: renaming methods.

Now let’s fix up the tests that use the old one. I’ll spare you the code, it all went like we did above.

Commit: tests all use execute not old style.

There is one more user of the old style request, the process method. It is used only in the tests for our RequestBuilder class and the tests for the new process classes. All that can go away now, I guess. Thank them for their service and remove them.

Safe Delete test_request_builder. Commit: removing old request builder

Remove test_request_processor. Remove request_builder. Remove test_world_batch. Commit: removing old tests and unneeded builders and request objects.

Whew. Let’s sum up.

Summary

A simpler idea appeared, passing a list of simple dictionaries containing entity id, verb and parameter, instead of our more more convoluted list of dictionaries of lists of dictionaries from before. We were able to refactor such that we could implement the feature for just one case, get it working, and the incrementally change each individual test and actual usage, one at a time.

Really small steps, everything working all the time. Yay, me!

It all went swimmingly, except for when I mistakenly used the wrong key, which has identified the need to bullet-proof things a bit. That is yet to be done. There are other improvements possible and work yet to be done, including but not limited to:

• Make it easier to create the little action dictionaries;
• Provide and use an entry in DirectConnection to take a list of many actions;
• Look for duplicated code in DirectConnection and take advantage of it;
• Cause World to return all the knowledge dictionaries for all the entities in an execute list;
• Deal with errors, including avoiding exceptions and passing error info in the knowledge.

Doubtless there’s more.

But this idea has worked out very nicely. It has simplified the existing code, the tests were easy to revise (and really didn’t have to be, as we could have left the old scheme in place, even changed it to call the new scheme for full confidence).

I’m really quite pleased with this and it was worth getting up a few minutes early to work on it.

Now for a nice iced chai latte and perhaps a tasty granola bar. See you next time!