We’ll check our list again, to see what might be fun to do. I did get a glimmer of an idea that I’d like to explore.

  • 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;

My small-p-baked idea1 has to do with representing the dispatch of verbs with a data structure rather than the growing match statement we now use. Let’s start by reviewing all the uses of match in the code so far: I’m sure it will lead us somewhere where we need to go.

Digression
It might seem that what one ought to do is to look at the list of tasks above, pick one based on its being high priority and reasonable difficulty, and work on it. Frequent readers will note that I do something like that sometimes, but that more frequently I will pick a place to work and proceed from there. Is this a reasonable thing to do in Real Life™?

If we were looking at a list of features that we wanted, I’d say to pick the one with the most value, and slice off a bit that we can do in a day or two. But when we are in the midst of working on a story, we might do a slice and then figure out another slice. But what happens to me is that once I’m in the midst of building a larger notion, my list of things to do is helpful, but what really works, for me, is just to jump into the code near the problem and solution, and let it lead me to more capability.

Let me freely grant that my purpose here is to work with code and show us what happens when I use the various practices, techniques, notions, and weird ideas that I have, so that readers can be amused, and perhaps even once in a while get an idea to try something I did … or to avoid something2 that I did. So you might not want to work in quite as random a fashion as I do. Then again … it seems to work for me.

But I digress. Here are our most notable match statements:

class World:
    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)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'turn':
                direction = action['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

    def set_direction(self, world_bot, direction_name):
        match direction_name:
            case 'NORTH':
                world_bot.direction = Direction.NORTH
            case 'EAST':
                world_bot.direction = Direction.EAST
            case 'WEST':
                world_bot.direction = Direction.WEST
            case 'SOUTH':
                world_bot.direction = Direction.SOUTH
            case _:
                pass

class Bot:
    def perform_actions_only_for_tests(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}'

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

Let me try to express my concern about these. They are just syntactic sugar for a big nest of if-elif-else statements. They are a procedure that is executed. In object-oriented programming, I was taught to avoid conditionals, and to replace them with other constructs, such as polymorphism, or message dispatch. The thought was that making such changes made the code “more object-oriented”, and that that was a good thing. And I have found it to be true.

There is a caveat: not everyone is really deeply comfortable with all the aspects of OO, in particular dynamic dispatch of messages based on what particular class receives the message. Many programmers using object-oriented languages have been taught to use them in a very procedural form. That being true, some of the transformations a “good OO programmer” might make are troubling to the OO programmers we meet on the ground.

Be that as it may, I’m here to do what I think is best. It may turn out that I do something you would not prefer: that’s OK, you get to do you, and if you think about something you see here and decide to do otherwise, I’m good with that. And sometimes, rather often, I do something, and I don’t even prefer it, so I put it back or do it yet another way.

One more thing:

There’s something odd about how we handle direction. The Cohort above receives the strings ‘NORTH’ and so on, because the Bot just emits the direction it wants to go. Cohort turns that into a verb ‘turn’, with the direction as a parameter. The Bot’s perform_action_only_for_tests method does the same thing. (See how squidgy that name makes you feel? It’ll help us want to get rid of it.)

But the World, upon seeing ‘turn’, calls set_direction, which then turns the ‘NORTH’ back into a direction. And so on.

Why don’t we do ourselves a favor here? World could implement ‘NORTH’ and the other directions as verbs, and it would be no less happy than it is now, maybe even happier. And Bot and Cohort would be just as happy.

Let’s do that.

Design Improvement? Now?

Yes, now. When we’re working in an area and see a design improvement that won’t take long to do, I think we should do it. That’s what keeps the design from getting crufty.

It’s worth nothing, however, that this is a change to the protocol, the message structure, between client and server. If we were running a server with client bots running around, we would almost certainly not remove the ‘turn’ capability, since it would break clients. We might add the new facility … but we might argue that having two ways to do the same thing is one too many, so we might not do it at all.

Fortunately, we don’t have that concern yet: we’re still just fleshing out the world and its operations, so I think we can do it. Let’s see.

Let’s test-drive the capability in World. We have a related test to look at:

    def test_bot_turns(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        bot = world.entity_from_id(bot_id)
        assert bot.direction == Direction.EAST
        rq = [ { 'entity': bot_id, 'verb': 'turn', 'direction': 'NORTH'}]
        world.execute(rq)
        assert bot.direction == Direction.NORTH

Our new one will want to test the strings as verbs. I’ll go out on a limb and build the test with a built-in loop. Very fancy. I may regret this, but probably won’t. Maybe. Possibly.

    def test_bot_direction_verbs(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        bot = world.entity_from_id(bot_id)
        choices = [ ('NORTH', Direction.NORTH),]
        for verb, result in choices:
            rq = [ { 'entity': bot_id, 'verb': verb } ]
            world.execute(rq)
            assert bot.direction == result

I figured that doing just ‘NORTH’ would fail and drive out the form of the solution. It does fail:

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

So we need a new verb roughly here:

    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)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'turn':
                direction = action['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

Hm, how about this:

    case 'NORTH' as direction:
        self.set_direction(entity, direction)

Green. Commit: implemented ‘NORTH’ verb.

Finish the test:

    def test_bot_direction_verbs(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        bot = world.entity_from_id(bot_id)
        choices = [ ('NORTH', Direction.NORTH),
                    ('EAST', Direction.EAST),
                    ('SOUTH', Direction.SOUTH),
                    ('WEST', Direction.WEST) ]
        for verb, result in choices:
            rq = [ { 'entity': bot_id, 'verb': verb } ]
            world.execute(rq)
            assert bot.direction == result

Fails, of course, so we finish the case:

    case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
        self.set_direction(entity, direction)

Green. Commit: NORTH, EAST, SOUTH, WEST are all verbs now

Now we can find the people creating ‘turn’ commands and have them just use the direction. We can change this:

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

To this:

    def create_action(self, bot, verb):
        match verb:
            case 'drop':
                return {'entity': bot.id, 'verb': verb, 'holding': bot.holding}
            case _:
                return {'entity': bot.id, 'verb': verb}

Very nice simplification, I like it! One test fails. I’m guessing we actually tested the transformation from a direction to a turn.

    def test_bot_turning(self):
        bot = FakeBot(101)
        bot.do(['NORTH', 'EAST', 'SOUTH', 'WEST'])
        cohort = Cohort(bot)
        message = cohort.create_message()
        assert len(message) == 4
        assert message[0] == {'verb': 'turn', 'direction':'NORTH', 'entity': 101}
        assert message[1] == {'verb': 'turn', 'direction':'EAST', 'entity': 101}
        assert message[2] == {'verb': 'turn', 'direction':'SOUTH', 'entity': 101}
        assert message[3] == {'verb': 'turn', 'direction':'WEST', 'entity': 101}

Might as well change that, it’s a decent test, although not terribly important.

    def test_bot_turning(self):
        bot = FakeBot(101)
        bot.do(['NORTH', 'EAST', 'SOUTH', 'WEST'])
        cohort = Cohort(bot)
        message = cohort.create_message()
        assert len(message) == 4
        assert message[0] == {'verb': 'NORTH', 'entity': 101}
        assert message[1] == {'verb': 'EAST', 'entity': 101}
        assert message[2] == {'verb': 'SOUTH', 'entity': 101}
        assert message[3] == {'verb': 'WEST', 'entity': 101}

Commit: Cohort uses direction verbs, not ‘turn.’

Where else? Bot, if I recall.

class Bot:
    def perform_actions_only_for_tests(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}'

Ah, nasty. This relies on DirectConnection. And I think this whole method should be changed to use the DirectConnection’s new way of handling requests. I think Cohort should be willing to help us here but it doesn’t seem to quite have the stuff. We’ll make the changes here and then see if some refactoring is needed.

Let’s just do the direction case first.

    def perform_actions_only_for_tests(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' as direction:
                    rq = [ {'entity': self.id, 'verb': direction} ]
                    connection.run_request(cohort, None, rq)
                case _:
                    assert 0, f'no case {action}'

Green. Commit: Bot no longer uses ‘turn’ in perform_actions_only_for_tests.

Now let’s convert all the cases in the above.

    def perform_actions_only_for_tests(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'take':
                    rq = [ { 'entity': self.id, 'verb': 'take'}]
                    connection.run_request(cohort, None, rq)
                case 'drop':
                    # self.holding is an id
                    rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
                    connection.run_request(cohort, None, rq)
                case 'step':
                    self._old_location = self.location
                    rq = [ { 'entity': self.id, 'verb': 'step'}]
                    connection.run_request(cohort, None, rq)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    rq = [ {'entity': self.id, 'verb': direction} ]
                    connection.run_request(cohort, None, rq)
                case _:
                    assert 0, f'no case {action}'

Green. Commit: perform_actions_only_for_tests uses only run_request, not specific DirectConnection verbs.

Now we can improve the code above. First factor out the duplication of calls to run_request:

    def perform_actions_only_for_tests(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'take':
                    rq = [ { 'entity': self.id, 'verb': 'take'}]
                case 'drop':
                    # self.holding is an id
                    rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
                case 'step':
                    self._old_location = self.location
                    rq = [ { 'entity': self.id, 'verb': 'step'}]
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    rq = [ {'entity': self.id, 'verb': direction} ]
                case _:
                    assert 0, f'no case {action}'
            connection.run_request(cohort, None, rq)

There is still duplication to be removed, though I have concern about that special line in ‘step’, setting _old_location. I don’t think we set that in our regular code. Is old_location handled in world now?

class World:
    def step(self, bot):
        self.map.attempt_move(bot.id, bot.forward_location())  # changes world version
        self.set_bot_vision(bot)
        self.set_bot_scent(bot)

class Map:
    def attempt_move(self, id, location: Location):
        entity = self.contents[id]
        if self.location_is_open(location):
            entity.location = location

It appears not. But that makes me wonder whether we properly change direction, when we’re running via the new request scheme. Cohort calls this:

class Bot:
    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 []

In World, if we can’t move you, we just don’t. There is no indication given that nothing happened. It’s up to the client to check. (It might be a kind service to put something in the error messages, when we start providing those.)

I think we need a round-trip test using the Cohort, to see if the direction changes. We do have at least one test that is close to doing that.

    def test_change_direction_if_stuck(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        client_bot.do_something_only_for_tests(connection)
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        client_bot.do_something_only_for_tests(connection)
        assert client_bot.location == Location(10, 5)
        client_bot.do_something_only_for_tests(connection)
        assert client_bot.location != Location(10, 5)
        world_bot = world.map.at_id(client_bot.id)
        assert world_bot.direction != Direction.EAST
        assert client_bot.direction != Direction.EAST

Ah, this will do. I suspect that if I remove the setting of old location from the only for tests code above, this test will start to fail. Yes, it does! Excellent. Remove it, fix the bug. But how?

The Bot only decides to step in one place:

    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

I think we can put the code there and have things work. But I am not entirely comfortable. “First, make it work.”

    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()
        self._old_location = self.location
        actions += ['step']
        return actions

Green. Commit: set _old_location before step. “Then, make it right.”

Wait! This change is needed. But I don’t think I can get rid of the one in perform..., because it’s not calling get_actions. Tests are working now, but that’s probably just because there is no test using the perform method that happens to do a step that won’t work.

I’m going to accept this change as it stands, since we are green. But I’m going to bump the priority of the issue, and ask that World inform the Bot that it was unable to comply to the step command in the event that it can’t.

I forgot to remove DirectConnection’s turn method. I do so and commit.

One more thing, even though time is fleeting.

    def perform_actions_only_for_tests(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'take':
                    rq = [ { 'entity': self.id, 'verb': 'take'}]
                case 'drop':
                    # self.holding is an id
                    rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
                case 'step':
                    rq = [ { 'entity': self.id, 'verb': 'step'}]
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    rq = [ {'entity': self.id, 'verb': direction} ]
                case _:
                    assert 0, f'no case {action}'
            connection.run_request(cohort, None, rq)

I think we can simplify this. First, transform some cases:

    def perform_actions_only_for_tests(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'take' as verb:
                    rq = [ { 'entity': self.id, 'verb': verb}]
                case 'drop':
                    # self.holding is an id
                    rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
                case 'step' as verb:
                    rq = [ { 'entity': self.id, 'verb': verb}]
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as verb:
                    rq = [ {'entity': self.id, 'verb': verb} ]
                case _:
                    assert 0, f'no case {action}'
            connection.run_request(cohort, None, rq)

Consolidate:

    def perform_actions_only_for_tests(self, actions, connection):
        cohort = Cohort(self)
        for action in actions:
            match action:
                case 'drop':
                    rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
                case 'step' | 'take' | 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as verb:
                    rq = [ {'entity': self.id, 'verb': verb} ]
                case _:
                    assert 0, f'no case {action}'
            connection.run_request(cohort, None, rq)

Green. Commit: combine common cases.

Our two hours is up, and our article is long. Let’s sum up.

Summary

We set out with a concern for our match statements, hoping to replace them somehow with something less procedural. In preparing the ground for that activity, we began to simplify the matches as they were, simplifying the ‘turn’ verb out of existence.

We have reduced our reliance on the special methods, step, drop, and so on in DirectConnection. They are now only used by one or two tests each, and not used in the production code at all. We should go after those. Note made.

I am pleased with what we’ve done, but a bit troubled by the _old_location thing. It’s a bit of a hack, and as such, really needs improvement. In addition, I really wanted to work on getting rid of at least one match statement, and didn’t get there. So that’s a bit of a disappointment, even though I know that simplifying them is a good step toward removing them.

I still think we’ll find some interesting possibilities for reducing the procedural character of our match statements, and I’m sure we’ll have fun working on it. I think the changes we made today may actually be more valuable than what we do to make them less procedural, because we have simplified the whole system.

But I wish we’d gotten further. It was not to be. See you next time!


  1. The existence of half-baked ideas implies the existence of p-baked ideas for 0 <= p <= 1. Presumably avoiding ideas with very low p-value would be a p-baked idea for large p. ↩

  2. If I can only serve as a bad example, but I’m a really bad example, perhaps that’s good enough. ↩