A Better Way

I think I see a better way to express the World’s handling of actions. Let’s find out if I’m right.

The handling of actions currently consists of a few calls, each one going deeper into the problem of executing the request from the bot side of things:

class World:
    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        self.execute_valid_list(actions_list)
        return { 'updates': self.updates, 'messages': self.messages }

    def execute_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
        else:
            self.get_updates(actions_list)

    def get_updates(self, actions_list):
        self.execute_actions(actions_list)
        self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.execute_with_entity(**action)

    def execute_with_entity(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            ...

We call down and down and down, ensuring that we have a list, then calling to execute the list (and fetching the updates), doing one action, assigning an entity if we need one, then finally sorting out the verbs and nouns and calling the specific operations. This has always seemed odd to me, causing me to try to rename things, and a few attempts to move things up and down that chain. It just seemed to me not to be right.

This morning, for no good reason, I had what I think is a better idea. I’m thinking that we should have a sequence something like this:

    # SKETCH
    def execute_rquests(self, action_list):
        valid_list = self.validate_list(action_list)
        self.updates = self.execute_actions(valid_list)

    def execute_actions(self, actions):
        for action in actions_list:
            self.execute(action)

    def execute(self, action):
        action_with_entity = self.assign_entity(**action)
        self.execute_action(action_with_entity)

The basic idea is that instead of checking or transforming the input and then calling on down, we’ll do our check or transform, and return the result of that operation, and the higher level will call the next method. If I’m right, the new code will tell the story better than the existing code.

Would you really do this?

Well, the fact is, I probably would do it in real life. I might be wrong to do so. But anyway, this is not real life, and so we have the luxury of trying multiple ways of doing things, building up our intuition and our skills. So we’re going to try it.

I am going to work in small steps, and I’m going to commit them. If this isn’t a good idea, well, we can always go back. But I think we’ll prefer it. Well, I think that I will prefer it. I would love to hear your views, so hit me up on mastodon if you wish.

Round Tuit

The basic scheme is that the various methods we call will transform the input they’re given to be the input that the next stage needs. As such, since most of the methods return nothing now, we can probably go in two mini-steps per step. Let’s find out.

We start here:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        self.execute_valid_list(actions_list)
        return { 'updates': self.updates, 'messages': self.messages }

    def execute_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
        else:
            self.get_updates(actions_list)

If the list is valid (that is, it’s a list) we’ll return it. Otherwise we’ll file the message and return an empty list. That should let the rest of the code just spin through the empty list doing nothing. We’ll see shortly.

    def execute_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
            return []
        else:
            self.get_updates(actions_list)
            return actions_list

Commit: refactoring toward new design.

Move the get_updates up:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        self.execute_valid_list(actions_list)
        self.get_updates(actions_list)
        return { 'updates': self.updates, 'messages': self.messages }

Tests break of course. Use the result:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        valid_actions = self.execute_valid_list(actions_list)
        self.get_updates(valid_actions)
        return { 'updates': self.updates, 'messages': self.messages }

Green. Commit. Rename the method:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        valid_actions = self.get_valid_list(actions_list)
        self.get_updates(valid_actions)
        return { 'updates': self.updates, 'messages': self.messages }

    def get_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
            return []
        else:
            return actions_list

Commit. Now we look on down:

    def get_updates(self, actions_list):
        self.execute_actions(actions_list)
        self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.execute_with_entity(**action)

    def execute_with_entity(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            ...

I think we want to do the same trick to the execute with entity. First return the result:

    def execute_with_entity(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)
        return parameters

Commit. Move the line up, expecting a failure.

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.execute_with_entity(**action)
            self.execute_action(**parameters)

    def execute_with_entity(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        return parameters

Use the returned result:

    def execute_actions(self, actions_list):
        for action in actions_list:
            parameters = self.execute_with_entity(**action)
            self.execute_action(**parameters)

Green. Commit. Rename method and result.

    def execute_actions(self, actions_list):
        for action in actions_list:
            action_with_parameters = self.assign_parameters(**action)
            self.execute_action(**action_with_parameters)

    def assign_parameters(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        return parameters

Green. Commit.

Here we are now:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        valid_actions = self.get_valid_list(actions_list)
        self.get_updates(valid_actions)
        return { 'updates': self.updates, 'messages': self.messages }

    def get_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
            return []
        else:
            return actions_list

    def get_updates(self, actions_list):
        self.execute_actions(actions_list)
        self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]

    def execute_actions(self, actions_list):
        for action in actions_list:
            action_with_parameters = self.assign_parameters(**action)
            self.execute_action(**action_with_parameters)

    def assign_parameters(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        return parameters

I think we should inline get_updates up into execute_requests and see what that tells us.

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        self.updates = []
        valid_actions = self.get_valid_list(actions_list)
        self.execute_actions(valid_actions)
        self.updates = [self.fetch(bot_id) for bot_id in self.ids_used]
        return { 'updates': self.updates, 'messages': self.messages }

Commit. We can put the updates into a temp and eliminate the attribute.

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        valid_actions = self.get_valid_list(actions_list)
        self.execute_actions(valid_actions)
        updates = [self.fetch(bot_id) for bot_id in self.ids_used]
        return { 'updates': updates, 'messages': self.messages }

That method would be a bit better if we were to extract the first two statements, but I kind of like the fact that we can see the two attributes initialized and used.

Note: I failed to commit here. Then I went on with the RequestSummary idea and lost this update when I roll back below. I’ve made the change a second time, somewhere down below, preserving this improvement.

The fact that two of the three operational calls return results and one does not strikes me as odd.

It would be just about marvelous if the execute could return the messages and ids used. Or, perhaps we should pass the collections down into the methods. Or perhaps there is some tiny object waiting to be born? A RequestResult or RequestSummary or something like that.

I think the pattern I’m looking for is Accumulating Parameter, a parameter passed into subordinate methods that, well, accumulates results.

Let’s try it. I’ll create a tiny local class.

class RequestSummary:
    def __init__(self):
        self.ids = set()
        self.messages = []

    def add_message(self, message):
        self.messages.append(message)

    def add_id(self, id):
        self.ids.add(id)

PyCharm prompted most of the code for this, after I defined the __init__. And, no, I do not have the AI component turned on.

Let’s use this:

    def execute_requests(self, actions_list):
        summary = RequestSummary()
        valid_actions = self.get_valid_list(actions_list, summary)
        self.execute_actions(valid_actions, summary)
        updates = [self.fetch(bot_id) for bot_id in summary.ids]
        return { 'updates': updates, 'messages': summary.messages }

I think that will be correct when the called methods are fixed up. So far, of course, they don’t expect a summary parameter.

Change is a bit too large.
Roll back.

I’ve spent a few minutes pushing the parameter down and down. I still have a lot of tests failing. They all seem to be failing because they don’t provide the summary parameter. Does the game run? It nearly does but finally threw an error trying to drop a block where it couldn’t. I think that was a legitimate message, because when we’re running multiple bots one might drop where another plans to drop, or move where it plans to drop. But the changes are too deep to push though. Roll back.

We’ve done enough, let’s reflect and sum up.

Reflection

Scanning the code, I don’t love this:

    def get_valid_list(self, actions_list):
        try:
            assert isinstance(actions_list, list)
        except AssertionError:
            self._add_message('requests must be a list of actions')
            return []
        else:
            return actions_list

Let’s do this:

    def get_valid_list(self, actions_list):
        if isinstance(actions_list, list):
            return actions_list
        else:
            self._add_message('requests must be a list of actions')
            return []

We’ll see below how that fits in, but it’s simpler this way. This is why we reflect and look at the code again.

The changes to the new sequencing were straightforward, smooth, easy, and quite small. We did seven commits separated by no more than five minutes. Here’s what we have now:

    def execute_requests(self, actions_list):
        self.ids_used = set()
        self.messages = []
        valid_actions = self.get_valid_list(actions_list)
        self.execute_actions(valid_actions)
        updates = [self.fetch(bot_id) for bot_id in self.ids_used]
        return { 'updates': updates, 'messages': self.messages }

    def get_valid_list(self, actions_list):
        if isinstance(actions_list, list):
            return actions_list
        else:
            self._add_message('requests must be a list of actions')
            return []

    def execute_actions(self, actions_list):
        for action in actions_list:
            action_with_parameters = self.assign_parameters(**action)
            self.execute_action(**action_with_parameters)

    def assign_parameters(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        return parameters

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            ...

Summary

I think these changes have improved things, but there are a few items that I think could still be improved:

It still seems to me that an accumulating parameter would be a good idea, but it seems to need to be passed down too far. I spent maybe five minutes pushing on the idea and then rolled back. A tiny bit disappointing. It felt like a good idea but got messy down in the weeds.
Another possibility is the Method Object pattern. Let’s explore that idea tomorrow. It deserves its own setting.
I’d feel better if the call to execute_actions was returning something, since the calls preceding and following return things. It’s OK now but just looks odd to me.
A fanatic might look at execute_actionwith an eye to extracting a method, for a perfect Composed Method pattern. I’m not quite convinced. We’ll have fresh eyes tomorrow and take another look.

Overall, I definitely prefer this to the version we started with. What do you think? See you next time!