Improvements?

The Repo on GitHub

With World using a separate Bot instance, separate from the ones we use on our nascent client side, we can start making some simplifications. Let’s look around. Results mostly good but not all good. Rat hole.

When a Bot is told to do_something, only the id of that bot is used in the World, to figure out how to respond to commands. The Bot accumulates a list of commands, and sends them to a DirectConnection that knows the world:

class Bot:
    def perform_actions(self, actions):
        connection = DirectConnection(self.world)
        for action in actions:
            match action:
                case 'take':
                    connection.take(self)
                case 'drop':
                    connection.drop(self, self.inventory[0])
                case 'step':
                    self._old_location = self.location
                    connection.step(self)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
                    connection.set_direction(self, action)
                case _:
                    assert 0, f'no case {action}'

You’ll note that the Bot apparently still knows the world. I can think of at least two three things that would be better:

• It could know a Connection;
• It could know some kind of parent collection that knows the connection;
• It could be passed the connection, or whatever object should deal with the Bot’s desired actions.

Of those, the last is probably best.

I note the reference to inventory[0]. I thought we had changed the Bot to only have one thing at a time. Better check.

Ah, yes. The Knowledge only has one thing but the Bot thinks of a collection:

class Bot:
    @property
    def inventory(self):
        if self._knowledge._entity:
            return [self._knowledge._entity,]
        else:
            return []

I think that I may have left it that way on the grounds that someday soon the Bot will be able to hold more than one thing.

I note in Bot a method: has_inventory, which is not used. Remove it. Commit: unused method What about that inventory thing? The code has no real purpose and it can’t possibly be difficult to replace it. On the other hand, I really do have a story in mind that will require the Bot to have more than one item. Perhaps. We might go another way. Let’s remove the inventory thing, refer directly to whatever knowledge knows, um _entity. Let’s rename that member to _held_entity.

I notice a large number of methods on Knowledge. Some of them just cover the members, which all begin with _ and are thus considered private. And there are methods that describe things that pertain to the Bot. When we review the init:

class Knowledge:
    drop_threshold = 4
    take_threshold = 7
    energy_threshold = 5

    def __init__(self, location, direction):
        # shared info
        self._direction = direction
        # world write / client read only
        self._held_entity = None
        self._id = None
        self._location = location
        self._scent = 0
        self._vision = Vision([], self.location, self.direction)
        # local info client
        self._energy = self.energy_threshold
        self._old_location = None

We see that Knowledge is highly focused on what the Bot knows, which is a superset of the information shared. We also see that the comments are no longer accurate, so:

    def __init__(self, location, direction):
        # world write / client read only
        self._direction = direction
        self._held_entity = None
        self._id = None
        self._location = location
        self._scent = 0
        self._vision = Vision([], self.location, self.direction)
        # local Bot client-side info
        self._energy = self.energy_threshold
        self._old_location = None

Green, commit: correctly identify world-provided vs local knowledge.

Commentary

We came in here with a general plan to simplify the World’s view of things on the client side, In order to prepare for that, we browse the code. As we browse the code, we see things that need just a little improvement. We could ignore them; we could make a note of them for “later”; we could just improve them. Since we are here with code improvement in mind, it makes more sense than ever to just improve them.

Note, however that if any given moment our tests are green, and we see something that needs changing, even a variable name, we can change it and quickly commit and push that change, and the issue is resolved there and then. It follows that what I’d like to do is to be green all the time, and make and commit quick changes as needed all the time. You might like to proceed differently … and if you do prefer another approach, I’d be interested to hear why you feel that way.

So, right, we were here about Bot.inventory. I think there is a sender in game, so we’ll check senders and see what we see. (Presumably we’d always do this before removing a public property.)

class Bot:
                ...
                case 'drop':
                    connection.drop(self, self.inventory[0])
                ...

class Game:
                ...
                if name == 'R':
                    if entity.inventory:
                        color = RED
                    else:
                        color = GREEN
                ...

Let’s give Bot a new property: holding.

    @property
    def holding(self):
        return self._knowledge._held_entity

Let’s not violate Demeter:

class Bot:
    @property
    def holding(self):
        return self._knowledge.holding

class Knowledge:        
    @property
    def holding(self):
        return self._held_entity

And use the new method in our two references to inventory:

class Bot:
                case 'drop':
                    connection.drop(self, self.holding)

class Game:
                if name == 'R':
                    if entity.holding:
                        color = RED
                    else:
                        color = GREEN

And remove inventory as not referenced. Green, game runs, commit: replace inventory notion with holding single item.

As long as we’re here, let’s scan Knowledge for anything odd.

There are lots of properties. At the time we wrote this, we were into using private members and then setting up properties, on the grounds that storing directly into another object is quite bad and fetching raw items from it isn’t considered best practice. I do note that the properties take up a lot of space in the file.

Past those, I find these methods:

class Knowledge:
    def has(self, entity):
        return entity == self._held_entity

    @property
    def has_block(self):
        return self._held_entity and self._held_entity.name == 'B'

    @property
    def has_moved(self):
        return self.location != self._old_location

In particular I note has_moved. Don’t we have special code in Bot for this? Two is probably one too many.

Where’s that code in Bot?

class Bot:
    def __init__(self, x, y, direction=Direction.EAST):
        self.world = None
        self.name = 'R'
        self.direction_change_chance = 0.2
        self._knowledge = Knowledge(Location(x, y), direction)
        self.state = Walking()
        self._old_location = None

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

    def perform_actions(self, actions):
        connection = DirectConnection(self.world)
        for action in actions:
            match action:
                case 'take':
                    connection.take(self)
                case 'drop':
                    connection.drop(self, self.holding)
                case 'step':
                    self._old_location = self.location
                    connection.step(self)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
                    connection.set_direction(self, action)
                case _:
                    assert 0, f'no case {action}'

    def move(self):
        if random.random() < self.direction_change_chance:
            self.change_direction()
        self._old_location = self.location
        self.step()

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

Looks like someone really wanted to be sure we saved the old location. Let’s first change the only code that actually checks the value:

    def update_for_state_machine(self):
        self._knowledge.gain_energy()
        if not self._knowledge.has_moved:
            new_direction = self.change_direction()
            return new_direction
        else:
            return []

Well! Two tests fail! Let’s see what’s up.

    def test_requests_direction_change_if_stuck(self):
        bot = Bot(10, 10)
        bot._old_location = Location(10, 10)
        actions = bot.update_for_state_machine()
        assert actions[0] in ["NORTH", "SOUTH", "WEST"]

That one just relies on the old implementation. We can make it even more invasive and it will pass.

    def test_requests_direction_change_if_stuck(self):
        bot = Bot(10, 10)
        bot._knowledge._old_location = Location(10, 10)
        actions = bot.update_for_state_machine()
        assert actions[0] in ["NORTH", "SOUTH", "WEST"]

Passes. The other failing one is that big long story one:

    def test_change_direction_if_stuck(self):
        def move_and_update():
            client_bot.perform_actions(['step'])
            world.update_client_for_test(client_bot)
            actions = client_bot.update_for_state_machine()
            client_bot.perform_actions(actions)

        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        move_and_update()
        assert client_bot.location == Location(10, 5)
        move_and_update()
        world_bot = world.map.at_id(client_bot.id)
        assert world_bot.direction != Direction.EAST
        assert client_bot.direction != Direction.EAST

I see no point in preserving that test at all, but we’ll leave it in for now. But, because I trust my tests at about 0.95, not 1.0, I’ll run the game.

Note

I really wish I had trusted the tests. I did find a bug in the way the Game displays things, but I’d rather have had the improvements. On the other hand, the game bug was hard to spot unless you had a certain frame of mind, so it was worth finding. Just wasn’t where I wanted to spend my time, and it triggered a roll back that was probably not required.

It’s all good. Not every step we take is a good one. That’s OK. And it’s why we try to keep them small and easily reversible.

Running the game shows me something very interesting. I definitely see red bots, the ones that are already carrying a block, bump into a block and just stand there a while: they should bounce off immediately if this feature works. And I think I have seen bots running off the screen entirely.

So this change is not righteous and we need a better test. But I think we can be assured that our current failing test, ugly though it may be, is failing legitimately.

    def test_change_direction_if_stuck(self):
        def move_and_update():
            client_bot.perform_actions(['step'])
            world.update_client_for_test(client_bot)
            actions = client_bot.update_for_state_machine()
            client_bot.perform_actions(actions)

        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert not client_bot.has_moved()  # <=== added check
        move_and_update()
        world_bot = world.map.at_id(client_bot.id)
        assert world_bot.direction != Direction.EAST
        assert client_bot.direction != Direction.EAST

class Bot:
    def has_moved(self):
        return self._knowledge.has_moved

I am not surprised to find that the test fails on the check for has_moved. That is suggesting to me that the Bot’s knowledge has not been updated correctly.

I home in on this:

class World:
    def update_client_for_test(self, client_bot):
        real_bot = self.map.at_id(client_bot.id)
        client_bot._knowledge = real_bot._knowledge

This just slams the knowledge from our world bot into the client one. Then why doesn’t the world bot know?

I add this check:

        world_bot = world.map.at_id(client_bot.id)
        assert not world_bot.has_moved()

And it fails: the world bot does not realize that it has not moved. Why is that? I’ll bet it’s because we don’t write the location value back if it hasn’t changed.

class World:
    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)  # changes world version
        real_bot = self.map.at_id(bot.id)
        self.set_bot_vision(real_bot)
        self.set_bot_scent(real_bot)

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

Sure enough, we leave it alone. We need to set it again, to trigger the saving of the old value. This is weird.

We’ll change Map thus:

    def attempt_move(self, id, location: Location):
        entity = self.contents[id]
        entity.location = self.valid_location(entity, location)

    def valid_location(self, entity, location: Location):
        if self.location_is_valid(location):
            return location
        else:
            return entity.location

    def location_is_valid(self, location: Location) -> bool:
        if self.is_occupied(location):
            return False
        return self.is_within_map(location)

Our test passes. Run game for better feels. I still think bots are walking off the lower edge.

OK we need to back away from this change. I’m glad I didn’t commit it. Roll it back.

I think briefly about rolling back only some of the changes, but it is too much. I do save a version of game that displays every move, not every ten as I usually use.

I think I have good news, in a manner of speaking: I think the bots are going off screen now. If that’s true, then this morning’s changes are not the cause and we need some tests.

My first move is to check the game display. I’ll put a block in each corner and make sure that’s where they show up.

Curiously, I find that a block with x or y at 0 does not display, which could account for bots seeming to vanish, and that blocks at height or width do display. So what is up with that? Is this just a display bug? I think I’d like that.

Ah!

    def draw_world(self):
        for entity in self.world.map:
            x = entity.x
            y = entity.y
            if x and y:

There’s yer problem right there.

            if x and y and x >= 0 and y >= 0:

We do have to check for existence because we set the location of a held block to None.

Further observation of the game tells me that a bot can walk to the west until it gets to cell x = 1 and then, it seems to vanish. It never gets to zero that I can see, but I think it has walked off the stage.

I need a test.

    def test_stop_at_west_edge(self):
        world = World(10, 10)
        client_bot = world.add_bot(3, 5)
        client_bot.direction_change_chance = 0.0
        client_bot.perform_actions(['WEST'])
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(2, 5)
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(1, 5)
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(0, 5)
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(0, 5)
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location != Location(0, 5)

This test walks a bot west, hits the edge. It does not move on the first attempt to continue west. Next time it attempts to move it succeeds and is no longer at (0, 5).

I think I have a display bug for bots, though I do not see how that could be, since we can display blocks on the edge.

I’m convinced at this point that the changes I made to the check for having moved were probably good, but I want to chase this display issue a bit further. I’ll instrument game.

Oh. Dammit. Zero is false.

    if x is not None and y is not None and x >= 0 and y >= 0:

Grr. That wasted our precious time.

Summary

Well, I have a nice new test, a bit wordy but very definitive, which showed me that bots were not walking off into space. That led me back to the check for displaying an entity in Game, which is downright odd but also correct.

I conclude that the changes made to the old location logic were probably valid, and I’ll make them next time. The confusion cost me one improvement. But it’s better to be sure, and I don’t trust the tests at the 1.0 level, so sometimes I want to watch the game run. You’ll notice that often, usually, I trust the tests. But sometimes a change just seems risky so I take that extra step.

For once, a rollback was probably not the right move. However, in finding the display bug, I did splatter changes around, so a commit after that would have been a bit nasty. Better to have a clean slate and do it again. I take it back: it probably was the right move, with no really good moves available.

It’s worth noting that I came in with a somewhat specific theory about improving World, but where we started browsing, we found opportunities to make things better and we took them immediately, changing, seeing green, committing. We got four commits, about one every half hour, which is a bit slow, but the fact is that we committed 15 minutes after we started, 10 minutes after that, 19 minutes after that … and one hour and 37 minutes after that. Part of that was time spent realizing that the setting of location was not proper. After that, a test failed. A better test would not have failed, and in fact at one point I had the failing one running with sensible adjustments, and I went down the display rat hole.

Overall, some good, some not so good. I’ve made a note about the Map bug: I don’t feel ready to fix it, and it’s covered by the fact that Bot holds an old location as well.

I writes ‘em the way they happens. Most of our days are a mixture. It’s all good … but some’s better!

See you next time!