Browsing & Musing

The Repo on GitHub

We have one known thing to work on. Some things to put on the list. And let’s see what the code wants.

My early morning thoughts included:

• The Bot presently holds just one object at a time. That’s OK for now. However, I think we’re passing a world-side object over to the client side in that case. Not entirely OK.

• The World only really needs to save, by id, a Bot’s direction, location, and holdings. It has no need to store the vision list nor the scent value. We might want to address that.

• The Knowledge object, which is increasingly client-side, should reflect both of the observations above.

I think we can keep those matters “on the list” for now, and proceed with the leftovers from yesterday, which is how the Bot knows whether or not it moved since last time. We presently have two ways of knowing that and should bring that down to one. Yesterday, I thought that I knew which one it should be. Today, like all days, we’ll think a bit more.

The way that I think we should use is in Knowledge:

class Knowledge:
    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

    @property
    def location(self):
        return self._location

    @location.setter
    def location(self, location):
        self._old_location = self.location
        self._location = location

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

However there is also an old location stored and updated in Bot:

class Bot:
    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 move(self):
        if random.random() < self.direction_change_chance:
            self.change_direction()
        self._old_location = self.location
        self.step()

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

And it’s read just here:

    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 []

There is also an interesting issue that we ran across yesterday:

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

Here on the world side, we do not set a location into the entity unless it has been found to be valid. If it’s not valid, we just don’t move the entity. What is the effect of that on the client side? It depends on how we set the entity on the client side, and the news there is not as good as it might be:

class DirectConnection:
    def step(self, bot):
        self.world.command('step', bot.id)
        result = self.world.fetch(bot.id)
        bot._knowledge = copy.copy(result)

class World:
    def fetch(self, entity_id):
        return self.map.at_id(entity_id)._knowledge

Presently, we simply copy the Knowledge from the World into the client bot. (DC has a client bot instance.)

Now, if we changed the attempt_mve to always store the location, then the old location would be set on the world side and copied to the client side correctly. I think that’s a pretty thin thread. I think we should, instead, provide a new method on Knowledge, to copy itself to another instance.

Note

When all this goes fully client-server, it’s not even clear what will come back across the wire, but whatever it is, there will be plenty of copying going on. It’ll basically be some kind of JSON representation of the necessary data. My present inclination would be that it will be a dictionary kind of thing, key-value data.

Maybe this is a good time to deal with that. What if we change fetch to return a dictionary now, and then DC will have to decode it and punch it into a knowledge.

Ah, is the location setter trick even a good one here? Yes, I think it’ll hold water, so long as the old location is outside the knowledge dictionary. Which it is, at least conceptually.

Let’s do this. We’ll change fetch to get a dictionary from the Knowledge and DC to copy the dictionary back into the Knowledge. Later on, World may create its dictionary some other way, but DC won’t care.

Here goes:

class World:
    def fetch(self, entity_id):
        return self.map.at_id(entity_id)._knowledge.as_dictionary()

A dozen tests fail, should be no surprise there. We have no such method.

class Knowledge:
    def as_dictionary(self):
        return {'direction': self._direction,
                'held_entity': self._held_entity,
                'id': self._id,
                'location': self._location,
                'scent': self._scent,
                'vision': self._vision}

Only ten tests fail now. I almost wonder how two of them passed but we’re here to get them all to run. We need to fix up all that copy stuff in DC. Let’s centralize it:

class DirectConnection:
    def set_direction(self, bot, direction_string):
        self.world.command('turn', bot.id, direction_string)
        self.update_client(bot)

    def update_client(self, bot):
        result = self.world.fetch(bot.id)
        bot._knowledge = copy.copy(result)

All the methods now call update_client instead of copying those two lines, which, up until moments ago, they all did.

Now …

    def update_client(self, bot):
        result_dict = self.world.fetch(bot.id)
        bot._knowledge.update(result_dict)

And back in Knowledge:

    def update(self, update_dictionary):
        self.direction = update_dictionary['direction']
        self.receive(update_dictionary['held_entity'])
        self.id = update_dictionary['id']
        self.location = update_dictionary['location']
        self._scent = update_dictionary['scent']
        self.vision = update_dictionary['vision']

That’s a bit nasty, because we don’t quite have setters for everything, and because we have receive to set the held entity, but it should be correct. Make it work, make it right, make it fast, Kent Beck used to tell us.

One test still fails and I want to know the reason why. I guess I’ll have to look at it.

    def test_connection_add_bot(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        bot = connection.add_bot(5, 5)
        assert bot.location == Location(5, 5)

class DirectConnection:
    def add_bot(self, x, y):
        from bot import Bot
        id = self.world.add_world_bot(x, y)
        client_bot = Bot(x, y)
        client_bot.id = id
        client_bot.world = self.world
        result = self.world.fetch(id)
        client_bot._knowledge = copy.copy(result)
        return client_bot

So that’s just wrong, innit? We should be calling our update method. I wonder why PyCharm didn’t detect it when I did the refactoring. No matter, that’s why we have tests.

class DirectConnection:
    def add_bot(self, x, y):
        from bot import Bot
        id = self.world.add_world_bot(x, y)
        client_bot = Bot(x, y)
        client_bot.id = id
        client_bot.world = self.world
        self.update_client(client_bot)
        return client_bot

And we are green. Commit: world.fetch returns a dictionary, DirectConnection updates knowledge from it.

Now then, where were we? Oh, right, the old location thing. We should be able to begin by changing that one place that actually checks the old location to refer to the knowledge instead:

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

Two tests fail. I am hoping they are just poorly written. Let’s find out. Here’s the first one:

    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"]

Right, that won’t do at all. What if we did this:

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

We set it to be what it is, so marking that we have not moved. Test runs. Now the other:

    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)

I wrote this one yesterday when I thought the bots were wandering off to the left. They are not. I’ll make it work, and then remove it as redundant. I’ll make it work first, just in case I learn something. I think the issue will be found in that update_client_for_test method, which is probably out of date with the change to the dictionary.

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

Yeah, no, that’s not how we do that any more.

class World:
    def update_client_for_test(self, client_bot):
        result_dict = self.fetch(client_bot.id)
        client_bot._knowledge.update(result_dict)

I really thought that was the necessary change but now three tests are broken.

After some deliberation …

… and a bit of debugging … I think this change will not work. Why? Because as written, the DirectConnection is updating the client bot on every command. So, for example, if the location is (3, 5) and we set direction WEST, the entire dictionary will be updated, and the old location will be set to (3, 5). Then, if we do_something, the first thing that happens is that we check to see if we have moved and it appears that we have not (because we haven’t even tried to move).

OK that’s good news, we have a better understanding of the issue. Roll back that one line.

We did change some tests and that update method and those are solid. Commit: modify tests and change test update for new dictionary return from fetch.

Reflection

So, the trick of caching old location in knowledge just isn’t going to cut it. Let’s remove the clever bit and the has_moved method.

class Knowledge:
    @location.setter
    def location(self, location):
        self._old_location = self.location
        self._location = location

That becomes:

class Knowledge:
    @location.setter
    def location(self, location):
        self._location = location

This fails:

    def test_no_move(self):
        knowledge = Knowledge( Location(10, 10), Direction.NORTH)
        knowledge.location = Location(10, 10)
        assert not knowledge.has_moved

That just doesn’t apply, and gets removed. There are actually three tests for has_moved and they all get to go away. Green, commit: remove has_moved, old location from Knowledge.

We should see if we can choose just one place for setting the old location instead of quite so many.

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

Let’s remove it from do_something and leave it in step. Green. Commit: remove redundant setting of old location.

And let’s sum up:

Summary

Twice now, I’ve gone after the logic that tells the Bot whether it has moved since its last time in the barrel. I really thought that detecting a change inside Knowledge was the trick, but it didn’t work. So, for now, we’re leaving the decision at the higher level, in the Bot.

But perhaps the problem was in how it was done, not in the idea itself. Knowledge saved an “old location” and compared that to its current location to decide whether it had moved. What if, instead, we noticed when we actually set a location different from the current location and then set a flag saying we have moved? That would seem to be better … but then when do we clear that flag?

I was thinking vaguely this morning that we may be looking at the first of a number of “conditions” that the world might want to return to a client, such as “you tried to move but couldn’t”, “you tried to drop but couldn’t”, “your mother dresses you funny”, “I am a teapot”, and so on. We could have a sort of bitmap thing, or a more complex error result, that comes back when you fetch. We’ll keep that in mind.

Part of the difficulty we had with this issue was due to the fact that on the client side we think of a single operation do_something, and that gets translated, currently, into a series of operations, each of which fetches a result and updates the client. It would seem that we would at least send all the commands for a Bot as a batch and do a single fetch at the end.

I believe that that will almost happen “automatically”. Currently, in Bot, we have:

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

In the first method shown, we have a nice collection of actions. Then in the second, we unwind that nice collection and send them, one at a time. to our connection. I think we will naturally evolve, by which I mean I think I’ll see it as suitable and cause it to happen, to a situation where we pass that package to DirectConnection and when DC thinks the time is right, it will execute that whole package, or perhaps even more than one such package, and update the Bot just once per package.

Which, by the way, would allow us to push has_moved back down to Knowledge. Where is Alanis Morrissette when you need her?

Anyway, that is for another day. For today, we’ve settled on one place for keeping track of motion, although it was not the one we thought we’d keep, and we’ve further isolated the server and client by passing a dictionary across the DirectConnection and updating the client from that.

I am not entirely pleased with that construction, by the way, since every time we add a notion to the shared information between world and bots, we’ll have to update both sides in a perfectly corresponding way. And, additionally, the code just looks a bit weird as it stands. Again, that is for another day.

Summary Summary

We proceed, day by day, session by session, to change the program to make it look more like the program we want. Lately we have been changing its design to look more like the design we want, driven by a desire to make it more suitable for the client-server mode of operation. That has taken a while, arguably because I pushed too far in a different direction. Possibly we would have gone faster had I not held back from client server thinking for so long.

But—like your cat when it falls off the shelf—I did that on purpose. No, I did, actually: I wanted to let the design grow as it wished without the client-server notion, so as to see how much trouble we got in. And while we did have to make a lot of changes, we were not in any kind of “big trouble”. We just refactored and layered in changes and now we are quite well set up to do client-server when we want to.

In my view all but the most egregious, awful, terrible design mistakes can be refactored, in small steps, to a design that we can readily live with. That doesn’t prove that rewriting is always slower: there can be no proof of that even if it is true. But what it does indicate is that when our design is bad, we should very seriously consider refactoring to improve it rather than go with our first notion, which is often to throw it all away and start over. In my view, that trick works very rarely and refactoring works just about every time. And if it’s not going to work, we’ll find out pretty quickly.

But, in the case at hand, and every case I can think of, refactoring to a better design does work.

We’ll make this design better next time. See you then!