More & Better

The Repo on GitHub

We’ll start with some more error handling. Where we’ll go, no one knows.

Reflection

I’ve done a bit of reflecting—or, as I call it, thinking—on our first cut at error handling. Recall that I’d been imagining adding an error list to each bot, to be filled in only if that bot encountered difficulties. One issue that I didn’t have a solution for was when to clear a Bot’s error messages. Perhaps it could be done after the bot has been fetched into the results list, but I don’t like things that have to come up behind sweeping up the debris: they are too easily forgotten.

I think the answer is this: keep the error messages in a separate list, as they are now, and give each message a bot id, zero if it is not associated with a bot, otherwise the id of the bot affected. We leave it up to the client to deal with the messages in any way it sees fit.

This will transform the individual messages from a simple string to a dictionary with two keys, probably ‘message’ and ‘bot_id’. Something like that.

Let’s go ahead and change our single error and its tests to adhere to that formula.

class TestWorldMessages:
    def test_list_exists(self):
        world = World(10, 10)
        requests = []
        result = world.execute_requests(requests)
        assert result['messages'] == []
        assert result['updates'] == []

    def test_requests_must_be_list(self):
        world = World(10, 10)
        requests = { 'entity': 0, 'verb': 'add_bot'}
        messages = world.execute_requests(requests)['messages']
        assert any('must be a list' in message 
            for message in messages)

The first one is fine. The second could use some improvement. I think it goes like this:

    def test_requests_must_be_list(self):
        world = World(10, 10)
        requests = { 'entity': 0, 'verb': 'add_bot'}
        messages = world.execute_requests(requests)['messages']
        assert any('must be a list' in message['message]'] 
            for message in messages)

That fails, of course so let’s see where we issue our one and only message:

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

We’re going to need a bit more code there in the except, so let’s extract a method. We’ll call it add_message. We want the string to be a parameter so we’ll take three steps: extract variable, extract method, inline variable. Like this:

Extract variable:

        except AssertionError:
            msg = 'requests must be a list of actions'
            self.messages.append(msg)
            ...

Extract method:

        except AssertionError:
            msg = 'requests must be a list of actions'
            self.add_message(msg)
        ...

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

Inline variable:

    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 add_message(self, msg):
        self.messages.append(msg)

Now of course you, or even I, could have typed those changes in, and probably correctly. But the three steps were all done by PyCharm, and all I had to do was make up a name. Far better, in my view, and good practice for seeing how to do other transformations that aren’t quite that simple.

If I had been truly clever, I’d have foreseen this need and done it before I made the test fail. As things stand, I can’t commit until I make the test run. Well, I could commit the World file and not the test, but no.

In this case, the change to add_message will be quite simple, so we’ll go ahead. Other times, I might revert the test and commit, or revert the whole sequence and do again, better. Today:

    def add_message(self, msg):
        message_dict = { 'message': msg}
        self.messages.append(message_dict)

I am quite set back on my heels when the test still fails, until I look at the failure:

>   assert any('must be a list' in message['message]'] for message in messages)
E   KeyError: 'message]'

Perhaps the square bracket in the lookup string is an issue. Perhaps. Fix the test:

    def test_requests_must_be_list(self):
        world = World(10, 10)
        requests = { 'entity': 0, 'verb': 'add_bot'}
        messages = world.execute_requests(requests)['messages']
        assert any('must be a list' in message['message'] 
            for message in messages)

Green. Commit: messages is now a list of dictionaries.

Reflection

I seem to have a very good chance of typing foo['key]'] when I mean foo['key']. It has happened before, frequently. It always shows up quickly, so far, but it’s troubling, because I can’t think of any better advice than “be careful” that will help prevent that particular mistake.

Doing the extraction before the test would have been better: it would have allowed earlier commits. On another day, I might have done that. Today, I didn’t. I suspect that if I had started with the code, I might have made the same decision: we’re going to add lines here, so let’s extract, but the new test would not have been in place.

What a person could do would be to revert this commit, and so the whole sequence over, better. I can’t bring myself to do that, at least not today. We’ll move on.

We want a second key in the dictionary, for the id. And I think the best thing is to provide it always, using the zero identifier when the message is not specific to a bot. Maybe I’m wrong about that. Yes, I am wrong about that. Having a magic value like that isn’t a good thing. We’ll make the other key optional and see how we feel about it.

Let’s look for a place where we need it, and write a new test.

    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)

Our current code demands an ‘entity’ key in every action request, with a zero expected for the ‘add_bot’ command.

I think there is a flaw in our logic here. Suppose that someone were to send this request:

{ 'entity': 101, 'verb': 'add_bot'}

We would add 101 to our list of bots processed. We’d then go on to add_bot_action:

    def add_bot_action(self, x, y, direction):
        bot_id = self.add_bot(x, y, Direction.from_name(direction))
        self.ids_used.add(bot_id)

We’d create the new bot and add its id as well. So when the request sequence is over, we’d return the info on the new bot, but also on bot 101.

This is probably harmless, but it’s not really right. Let’s see where we could detect the situation:

    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)
            ...

No. This is too deep for a next step. Let’s find a place for a bot-related message that doesn’t ask us to change a lot of code. Inch by inch.

class World:
    def drop_forward_action(self, bot, holding):
        self.drop_forward(bot, self.entity_from_id(holding))

    def drop_forward(self, bot, held_entity):
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)

Here’s a nice one. Presently, if you can’t drop what you’re holding, we just don’t remove it. Let’s see what’s in place_at:

class Map:
    def place_at(self, entity, drop_location):
        if self.location_is_open(drop_location):
            entity.location = drop_location
            self.place(entity)
            return True
        else:
            return False

So you can’t drop off-world or on top of something else. That’s worthy of a message, I think.

We have tests for trying to drop off world, calling this check:

    def test_bot_cannot_drop_off_world_west(self):
        self.check_cannot_drop_off_world(0, 5, Direction.WEST)
    ...
    def check_cannot_drop_off_world(self, x, y, direction):
        world = World(10, 10)
        bot_id = world.add_bot(x, y, direction)
        bot = world.entity_from_id(bot_id)
        block_id = world.add_block(4, 4)
        block = world.entity_from_id(block_id)
        bot.receive(block)
        world.drop_forward_action(bot, block_id)
        assert bot.has(block), 'drop should not happen'

We could check the messages right here, with a little care.

    def check_cannot_drop_off_world(self, x, y, direction):
        world = World(10, 10)
        bot_id = world.add_bot(x, y, direction)
        bot = world.entity_from_id(bot_id)
        block_id = world.add_block(4, 4)
        block = world.entity_from_id(block_id)
        bot.receive(block)
        world.drop_forward_action(bot, block_id)
        assert bot.has(block), 'drop should not happen'
        messages = world.messages
        assert len(messages) > 0

That’s enough to fail four times, NORTH, SOUTH, EAST, WEST. So now:

    def drop_forward(self, bot, held_entity):
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)
        else:
            self.add_message('drop location was not open')

Test passes. Shall we commit? I think GeePaw would. I think we won’t.

Let’s enhance the test:

    def check_cannot_drop_off_world(self, x, y, direction):
        world = World(10, 10)
        bot_id = world.add_bot(x, y, direction)
        bot = world.entity_from_id(bot_id)
        block_id = world.add_block(4, 4)
        block = world.entity_from_id(block_id)
        bot.receive(block)
        world.drop_forward_action(bot, block_id)
        assert bot.has(block), 'drop should not happen'
        messages = world.messages
        assert len(messages) == 1
        message = messages[0]
        assert 'was not open' in message['message']

This continues to pass, of course. But now let’s demand the id:

        ...
        assert len(messages) == 1
        message = messages[0]
        assert 'was not open' in message['message']
        assert message['bot_id'] == bot_id

Now we need a way to add a message with a bot id in it. We could have an optional parameter on add_message, or a new method. Today I feel like a new method is better:

    def drop_forward(self, bot, held_entity):
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)
        else:
            self.add_bot_message(bot, 'drop location was not open')

    def add_bot_message(self, bot, msg):
        message_dict = { 'bot_id': bot.id, 'message': msg}
        self.messages.append(message_dict)

And we’re green. Commit: drop on non-open location issues bot-specific error.

Reflection

We have accomplished what we set out to do, adding a bot-specific message to the messages. We made the decision to make the ‘bot_id’ key optional rather than give it a magic value.

We went in a series of small steps, supported by tests all the way. First we refactored out an add_message method, then enhanced it to produce a dictionary, then to produce a message about improper dropping, then finally to include the offending bot’s id.

It all went quite smoothly, although preparing the add_message method first might have been even better.

I decide to mark those two methods as private, since they are deep in the class. Commit: mark methods private.

Reflection (more)

We noticed yesterday that we extract the messages in DirectConnection, and that since DC doesn’t really know about what’s going on, that should be done in the cohort:

class DirectConnection:
    def run_request(self, cohort, rq):
        results = self.world.execute_requests(rq)
        cohort.update(results['updates'])

We’ll change that to send the results to Cohort, breaking lots of things and then fix Cohort:

class DirectConnection:
    def run_request(self, cohort, rq):
        results = self.world.execute_requests(rq)
        cohort.update(results)

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

A test fails. It’s creating its own results:

    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.location == Location(5, 6)

What is this testing? Oh, it’s ensuring that if a new bot id arrives from World, we add it properly. We just need to format the input correctly:

    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()
        updates = [ dict ]
        messages = []
        results = { 'messages': messages, 'updates': updates }
        cohort.update(results)
        client_bot = cohort.bots[new_bot.id]
        assert client_bot.location == Location(5, 6)

Green. Commit: DC sends full results to Cohort, Cohort pulls updates.

OK, good session. Let’s wrap up.

Summary

We now have the ability to pass diagnostics and other messages back to our clients. The format is simple but can be extended to include additional fields if we need it to. It’s JSONable, as a dictionary, and its creation is covered by a couple of methods that will probably be useful as we add messages.

As things stand, both those methods know the format of the message, so there is a bit of duplication going on, but unless we resolve that with a variable argument list, I don’t see how we might address that. Perhaps a better idea will appear.

Our changes were all very small and we had frequent commits. Preparing the space before writing the test would have allowed us to commit after each extract, but all that was only a matter of minutes.

The refactoring, extract variable, extract method, inline variable: that was pretty neat, wasn’t it? It’s fun when we can make a change that way, and it’s more reliable than doing it by hand. Quite pleasing in a small way.

Bottom line, progress on error messages. We can now look for more ways to make the World impervious to weird messages from clients. This is the kind of time when I’d like to have some canny tester on line to think of ways to break it. Looking at you, Michael Bolton!

See you next time, where we’ll think of something to do!