We’ll review our list and pick some steps toward our overall goals. We might even remind ourselves what those goals are. Or we might trust that we’ll find our way.

Here’s the current list of ideas:

  • Work out a real Cohort object and use it;
  • Find, create, or stumble onto a reasonable protocol between DC, Cohort, and Bot;
  • Make it easier to create the little action dictionaries;
  • Provide and use an entry in DirectConnection to take a list of many actions;
  • Deal with errors, including avoiding exceptions and passing error info in the knowledge.
  • Location in result dictionary (JSON?)
  • Refactor execute (I think I meant do_something here, but we’ll see.)
  • Sort out adding bots
  • [Possible] Move to better separation between Bot and World in tests.

Yesterday we drove out improvements in Cohort, and adjusted Bot so that it normally returns a list of actions that it would like to perform. The Bot needs a bit of refactoring: I think we’ll start with that and a look at Cohort. I believe that Cohort is nearly ready to deal with a list of Bots, but that it has no way to get such a list.

Musing on that, and our need to have a way to add a Bot, it seems to me that we “just” need a new verb to send to World, ‘add_bot’ or something, passing the desired location for the proposed bot. Then World would create a Bot and add its id to the list of ids whose knowledge will be returned at the end of the command sequence. The new knowledge would show up, and the Cohort would discover that it doesn’t have a Bot of that id, create one, and fill in the blanks.

I imagine that we’ll make some discoveries along the way. In fact, I confirmed one thing last night in Pyto: we cannot just pass a Location back through JSON. We could rig up a specialized way of doing it, but I think we’ll do better to just pass ‘x’ and ‘y’ and wrap them as needed.

Anyway, let’s get down to cases, starting in the Bot and how it does things now.

class Bot:
    def do_something(self, connection):
        actions = []
        actions += self.update_for_state_machine()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        if connection:  # <===
            self.perform_actions(actions, connection)
        return actions

    def perform_actions(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'take':
                    connection.take(cohort, self.id)
                case 'drop':
                    # self.holding is an id
                    connection.drop(cohort, self.id, self.holding)
                case 'step':
                    self._old_location = self.location
                    connection.step(cohort, self.id)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
                    connection.turn(cohort, self.id, action)
                case _:
                    assert 0, f'no case {action}'

We slammed that if connection: in there, because Cohort doesn’t want the Bot to perform commands, because Cohort will batch them up and do it. But the tests are not yet converted to work that way, so they do want the commands sent to the World. Let’s refactor this to get rid of the if. I started thinking about ideal names, and who gets to call do_something and who has to call something else. Let’s try this:

Extract a method to get the actions:

class Bot:
    def do_something(self, connection):
        actions = self.get_actions()
        if connection:
            self.perform_actions(actions, connection)
        return actions

    def get_actions(self):
        actions = []
        actions += self.update_for_state_machine()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        return actions

Change Cohort to call get_actions:

class Cohort:
    def create_message(self):
        message = []
        for bot in self.bots:
            actions = bot.get_actions()  # <===
            for verb in actions:
                action = self.create_action(bot, verb)
                message.append(action)
        return message

This breaks tests, because my FakeBot has no get_actions method. This is one reason why I like to avoid fake objects, but I still like this one.

class FakeBot:
    def get_actions(self):
        return self.actions

That replaces do_something in the FakeBot. Green. One more change though. This is what the refactoring step left us:

class Bot:
    def do_something(self, connection):
        actions = self.get_actions()
        if connection:
            self.perform_actions(actions, connection)
        return actions

We no longer need the return, nor the conditional. And inline.

class Bot:
    def do_some
    def do_something(self, connection):
        self.perform_actions(self.get_actions(), connection)

A test has failed. It appears that one test expects do_something to return the actions:

    def test_stop_at_edge(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(9, 5)
        client_bot.direction_change_chance = 0
        actions = client_bot.do_something(connection)
        assert actions == ['step']
        assert client_bot.location == Location(10, 5)
        actions = client_bot.do_something(connection)
        assert actions == ['step']
        assert client_bot.location == Location(10, 5)
        actions = client_bot.do_something(connection)
        assert actions[0] in ["NORTH", "SOUTH", "EAST", "WEST"]
        assert actions[1] == 'step'
        assert client_bot.location != Location(10, 5)

Oh, right. I kind of like that test though it is round trip, because it directly checks what’s to be done.

Since do_something is now for tests only, let’s belay that “improvement” and let it return the actions.

    def do_something(self, connection):
        actions = self.get_actions()
        self.perform_actions(actions, connection)
        return actions

And let’s rename the method. It is only used in tests (and once in game, but we’ll be fixing that up).

    def do_something_only_for_tests(self, connection):
        actions = self.get_actions()
        self.perform_actions(actions, connection)
        return actions

OK, now commit: refactor separating prod execution via Cohort from test modes.

Now a quick scan of Cohort, which is pretty trivial and still a bit stubbed out:

class Cohort:
    def __init__(self, bot):
        self._bot = bot

    @property
    def bots(self):
        return [self._bot]

    def create_message(self):
        message = []
        for bot in self.bots:
            actions = bot.get_actions()
            for verb in actions:
                action = self.create_action(bot, verb)
                message.append(action)
        return message

    def create_action(self, bot, verb):
        match verb:
            case 'drop':
                return {'entity': bot.id, 'verb': verb, 'param1': bot.holding}
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                return {'entity': bot.id, 'verb': 'turn', 'param1': direction}
            case _:
                return {'entity': bot.id, 'verb': verb}

    def update(self, results):
        for result_dict in results:
            self._bot._knowledge.update(result_dict)

As you can see, it’s just hacked to have a collection to run over, but that means it’s ready to have a real collection. And currently it just assumes that whatever comes back is intended for its only bot.

Since our tests all create it with a single bot, I’m inclined to retain that “feature”. Or we could give it a factory method. Same-oh-same-oh?

Let’s give it a dictionary of Bots by id:

class Cohort:
    def __init__(self, bot=None):
        self.bots = {}
        if bot:
            self.bots[bot.id] = bot

    def create_message(self):
        message = []
        for bot in self.bots.values():
            actions = bot.get_actions()
            for verb in actions:
                action = self.create_action(bot, verb)
                message.append(action)
        return message

    def create_action(self, bot, verb):
        match verb:
            case 'drop':
                return {'entity': bot.id, 'verb': verb, 'param1': bot.holding}
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                return {'entity': bot.id, 'verb': 'turn', 'param1': direction}
            case _:
                return {'entity': bot.id, 'verb': verb}

    def update(self, results):
        for result_dict in results:
            bot_id = result_dict['eid']
            bot = self.bots[bot_id]
            bot._knowledge.update(result_dict)

That’s green. Doesn’t handle missing ids yet. And why oh why is that key ‘eid’? Let’s write a test to drive out some of the bot-adding behavior.

    def test_adding_via_surprise_knowledge(self):
        cohort = Cohort()
        WorldEntity.next_id = 100
        new_bot = WorldEntity.bot(5, 6, Direction.EAST)
        dict = new_bot.as_dictionary()
        cohort.update([ dict ])
        client_bot = cohort.bots[new_bot.id]
        assert client_bot.x == 5
        assert client_bot.y == 6

I think that should work if things worked. Of course right now it had better be failing on a key error.

>           bot = self.bots[bot_id]
E           KeyError: 101

Perfect. We need to improve this:

    def update(self, results):
        for result_dict in results:
            bot_id = result_dict['eid']
            bot = self.bots[bot_id]
            bot._knowledge.update(result_dict)

Extract a method:

    def update(self, results):
        for result_dict in results:
            bot_id = result_dict['eid']
            bot = self.by_id(bot_id)
            bot._knowledge.update(result_dict)

    def by_id(self, bot_id):
        return self.bots[bot_id]

Enhance the method:

    def by_id(self, bot_id):
        from client.bot import Bot
        try:
            return self.bots[bot_id]
        except KeyError:
            new_bot = Bot(0, 0)
            new_bot.id = bot_id
            self.bots[bot_id] = new_bot
            return new_bot

Let’s extract a method for that bottom part.

    def by_id(self, bot_id):
        try:
            return self.bots[bot_id]
        except KeyError:
            return self.create_new_bot(bot_id)

    def create_new_bot(self, bot_id):
        from client.bot import Bot
        new_bot = Bot(0, 0)
        new_bot.id = bot_id
        self.bots[bot_id] = new_bot
        return new_bot

Tests are green. Commit: Cohort creates new Bot on unrecognized id. Irritating that it knows Bot class.

I notice this:

    def update(self, results):
        for result_dict in results:
            bot_id = result_dict['eid']
            bot = self.by_id(bot_id)
            bot._knowledge.update(result_dict)

You’d think that a Bot would know how to update itself. You’d think we’d have the decency not to grab the bot by its _knowledge.

class Cohort:
    def update(self, results):
        for result_dict in results:
            bot_id = result_dict['eid']
            bot = self.by_id(bot_id)
            bot.update(result_dict)

class Bot:
    def update(self, result_dict):
        self._knowledge.update(result_dict)

Commit: tidying.

I observe this:

    def get_actions(self):
        actions = []
        actions += self.update_for_state_machine()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        return actions

    def update_for_state_machine(self):
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

That method name isn’t helping me. What we’re doing here is turning if we didn’t move. Let’s try a name like that.

    def get_actions(self):
        actions = []
        actions += self.turn_if_we_didnt_move()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        actions += ['step']
        return actions

    def turn_if_we_didnt_move(self):
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

Commit: rename method.

Reflection

We’ve made a bit of progress toward our Cohort properly handling multiple bots, and we’ve improved a bit of the campground in which we worked. Good for us. It’s only about an hour and a half since we started. Shall we take a try at creating a bot in the world and returning it?

Let’s think of the pieces and parts for this.

Cohort will need a new method telling it to add a bot (or N bots?). That method will add a special message to the request list, consisting of something like an empty or special id, a verb like add_bot and the x and y coordinates desired. And probably a direction as well, since World requires one for creation. We could provide a default if we need to.

This will create a two- or three-parameter action. Currently they are zero or one. So that’ll be interesting.

World will need to parse the command, add a bot, add it to the list of responses, and carry on.

Let’s look at World and how it executes:

    def execute(self, actions_list):
        ids_used = set()
        for action in actions_list:
            id = action['entity']
            ids_used.add(id)
            entity = self.entity_from_id(id)
            verb = action['verb']
            try:
                param1 = action["param1"]
            except KeyError:
                param1 = None
            self.execute_action(entity, verb, param1)
        return [ self.fetch(bot_id) for bot_id in ids_used ]

    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}')

If we choose, we can create the three parameters and pass them in. Or we could handle the ‘add_bot’ very separately, which might be a bit less messy.

I’m not entirely sure that we have divided up the responsibility ideally between those two methods.

Let’s see what we have in the way of tests of execute. I’ll spare you the details but we have four fairly nice ones. Let’s just write the add test and see it fail.

    def test_add_bot(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'add_bot',
                   'x': 5,
                   'y': 6,
                   'direction': 'EAST'}

I found myself thinking: why do we have a generic parameter param1 that everyone has to use? Why don’t we have parameters suited to the verb?

Let’s refactor to allow that:

    def execute(self, actions_list):
        ids_used = set()
        for action in actions_list:
            id = action['entity']
            ids_used.add(id)
            entity = self.entity_from_id(id)
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in ids_used ]

    def execute_action(self, entity, verb, action):
        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 {verb}')

Equivalent. Looks a bit more fragile and we should provide a better way of fetching parameters, but it’ll hold water for now.

Let’s complete our test and make it work. This is enough to fail:

    def test_add_bot(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'add_bot',
                   'x': 5,
                   'y': 6,
                   'direction': 'EAST'}
        rq = [ command ]
        result = world.execute(rq)
        assert len(result) == 1

It’ll be failing on an unknown verb, I reckon:

    def at_id(self, entity_id):
>       return self.contents[entity_id]
E       KeyError: 0

Ah, wrong. Failing on the zero id. I’m tempted to do something clever but not yet.

    def execute(self, actions_list):
        ids_used = set()
        for action in actions_list:
            id = action['entity']
            if id:
                ids_used.add(id)
                entity = self.entity_from_id(id)
            else:
                entity = None
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in ids_used ]

A bit nasty but let’s get the vine over the chasm before we worry about paving the roadway.

            case _:
>               raise Exception(f'Unknown action {verb}')
E               Exception: Unknown action add_bot

Ah, as expected. Now to implement:

    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                bot_id = self.add_bot(action['x'], action['y'], action['direction'])
                # arrgh

We can’t return add the id because the list is local to the execute. I guess we’ll have to change that:

    def execute(self, actions_list):
        self.ids_used = set()
        for action in actions_list:
            id = action['entity']
            if id:
                self.ids_used.add(id)
                entity = self.entity_from_id(id)
            else:
                entity = None
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                bot_id = self.add_bot(action['x'], action['y'], action['direction'])
                self.ids_used.add(bot_id)
            case 'step':
                self.step(entity)
            ...

Green. Commit: initial add_bot working. Let’s reflect.

Reflect

We had to mash the execute twice to get this to work. We had to change it to allow the input entity id to be zero, which is a risky choice on its own, and then to bump the used ids up to a member variable, so that we could update it inside the verb operation.

Don’t you mean “thrice”?

Right. As always, I was wrong about the two, it was thrice. We also weakened the fetching of the parameters a bit, although I am sure the code would have ultimately raised some exception on a malformed action.

This code is not production-worthy. But error handling is still on our list, and that’s where we will deal with exceptions in code like this. So I’ll allow it, given what we’re trying to do here, but it’s really not satisfactory even as interim code.

I think I’d argue that it works as well as it did before we started, and that it has a new capability, which also works. It is ready for us to implement a method in Cohort on the client side to add bots.

In my view, it’s only when we can see the code that we can see where it’s not quite what it ought to be. I don’t believe that if I had just thought a little more or a little harder, I’d have come up with something ever so clean and nice. I think that I need to get something in place, actually working, doing at least the happy path, before I can see what would do the job better.

A wild Snorlax idea appears!

It seems to me that perhaps we are looking at a separate object wishing to be born. World manages bots and blocks and things. And this code mostly just calls methods on world to tell it to step a bot or turn it or whatever. So maybe this execute and execute_action and the used_ids list are really part of another object, the WorldActionAuthority or something.

We’ll explore that possibility later, perhaps as soon as tomorrow. I think it will help. We’ll have all this strangeness nicely isolated, where we can munge it around more freely, and think of better ways of expressing things.

I can’t help thinking that the initial dispatch might best be done based on the verb, but it is certainly true that all the current verbs require a valid bot id, except for our new one. We’ll see. There are a number of things we might try, including dispatching more than once.

For now, I think we’re OK. Let’s sum up.

Summary

We have left the code in a more capable state, but a state that is a bit less ragged than we’d like. We have committed it, because it runs all the tests and we keep code that works at HEAD. Our current list of things to do might look like this:

  • Continue to refine protocol between DC, Cohort, and Bot;
  • Finish addition of bots
  • Enhance Cohort to add bots;
  • Use Cohort in game;
  • Provide and use an entry in DirectConnection to take a list of many actions;
  • Deal with errors, including avoiding exceptions and passing error info in the knowledge;
  • Deal with fact that Location cannot be JSONized;
  • Refactor execute (I think I meant do_something here, but we’ll see.)
  • Clean up execute in World, possible new object.
  • [Possible] Move to better separation between Bot and World in tests.
  • [Possible] Make it easier to create the little action dictionaries;

There is certainly overlap among these and some are at a higher level of abstraction than others. No matter, we are fairly intelligent entities, some recent events notwithstanding, and we seem to be able to pull sensible steps out of this pile of miscellany.

Decent steps today, no trouble to speak of, and we have a new capability coming on line.

See you next time!