Breaking Free

Let’s move further toward separating the World side from the Bot side. There are options and issues.

Among the many topics I listed last time, which I am not reviewing, we could probably find evidence of these two:

Currently both sides, World and Bot, use the same class to hold their bot information. These should be separated, because they have different needs, and because we want, ultimately, to share little or no source code¹ between the client and server.
We are in mid-stream on moving all client-side access to the World to using a Connection, currently DirectConnection. We’ll have a clear separation between server and client only when we have all access moving through that connection.

I am of a mind to work on the latter, for two reasons. First, it feels more like progress. Client-server could work just fine even if the two sides have common code, so long as they do not retain pointers to each others’ objects. But it cannot work until all the access goes through the connection. Second, once all access goes through the connection, it should be more clear which bits of information belong in the three sets of client-side, server-side, and shared.

Well, there is no one here to stop me, so let’s have a look.

Direct Connection

Just now, DirectConnection is just a very small class and it is still resident in with its tests. The situation looks like this:

class TestConnection:
    def test_exists(self):
        world = World(20, 20)
        connection = DirectConnection(world)

    def test_step(self):
        world = World(20, 20)
        bot = world.add_bot(10, 10)
        connection = DirectConnection(world)
        connection.step(bot)
        assert bot.location == Location(11, 10)
        assert world.map.at_xy(11, 10).id == bot.id

    def test_step_north(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5, Direction.NORTH)
        location = client_bot.location
        connection = DirectConnection(world)
        connection.step(client_bot)
        assert client_bot.location == location + Direction.NORTH

We just have step so far, and it works like this:

class DirectConnection:
    def __init__(self, world):
        self.world = world

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

And in World:

    def command(self, action, parameter):
        if action == 'step':
            bot = self.map.at_id(parameter)
            self.step(bot)
        else:
            raise Exception('Unknown command')

I am not certain that we need to go through the process of sending a text command to World. There is no reason why we couldn’t have our DirectConnection merely forward the operation to the relevant World method. But, somehow, I feel a bit better doing it this way. I think I’m trying to lean well away from the client-side doing things that only the server side should do. We’ll continue with that scheme.

Let’s test the take operation.

Now here comes a question. We have tests for the take operation already, calling World, for example, in TestBot:

class TestBot:
    def test_take_a_block(self):
        world = World(10, 10)
        bot = world.add_bot(5, 5)
        block = Block(6, 5)
        world.add(block)
        assert not world.is_empty(Location(6, 5))
        world.take_forward(bot)
        assert bot.has(block)
        assert world.is_empty(Location(6, 5))

I think one could make the case that this is a TestWorld test, not a TestBot test. Let me do a rename here for clarity:

    def test_take_a_block(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        block = Block(6, 5)
        world.add(block)
        assert not world.is_empty(Location(6, 5))
        world.take_forward(client_bot)
        assert client_bot.has(block)
        assert world.is_empty(Location(6, 5))

With that rename in place, we see that this is a round-trip test for the client bot, but that mostly it is about whether the World is right. Let’s copy this test to the connection tests, make it right for there, make it work, and then think about how we should deal with this situation. The test, as it is now, stands too far back from the objects. It is a typical “story test”, the kind I’ve been writing, and those tests are more fragile, harder to write, and more indirect than seems ideal.

We’ll be better able to compare when we get a few of them sorted. Let’s start with the one above, re-cast to work with DirectConnection:

    def test_take_a_block(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        connection = DirectConnection(world)
        block = Block(6, 5)
        world.add(block)
        assert not world.is_empty(Location(6, 5))
        connection.take(client_bot)
        assert client_bot.has(block)
        assert world.is_empty(Location(6, 5))

This is not particularly different. I feel that I’ve not gone far enough. In particular, why should we pa any attention to the client bot at all. If the world has the right info and the info transfer works, there is no need to check the client bot. Re-cast again:

    def test_take_a_block(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        connection = DirectConnection(world)
        block = Block(6, 5)
        world.add(block)
        assert not world.is_empty(Location(6, 5))
        connection.take(client_bot)
        world_bot = world.map.at_id(bot_id)
        assert world_bot.has(block)
        assert world.is_empty(Location(6, 5))

Now it’s even a bit more remote. The test feels so far away from what I care about and the code I’m about to write. One issue is that add_bot returns a Bot to the Client. We’ll ultimately return only an id and let him make his own. I’m not sure our add_bot idea was quite righteous. But that’s OK, we’re finding a path to better places, not trying to step to some platonic ideal plane. I can’t see that far.

Let’s make this test work. We need the take command. This seems reasonable:

class DirectConnection:
    def __init__(self, world):
        self.world = world

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

    def take(self, client_bot):
        self.world.command('take', client_bot.id)
        result = self.world.fetch(client_bot.id)
        client_bot._knowledge = copy.copy(result)

Now we need the command in World:

    def command(self, action, parameter):
        if action == 'step':
            bot = self.map.at_id(parameter)
            self.step(bot)
        elif action == 'take':
            bot = self.map.at_id(parameter)
            self.take_forward(bot)
        else:
            raise Exception('Unknown command')

The test is green. Commit: implemented take.

Encouraged by this, I decide to do drop.

Would you believe that there appear to be no tests for doing a drop that will actually drop? We have four tests that check that you cannot drop something off the edge of the world, such as this one:

    def test_bot_cannot_drop_off_world_north(self):
        world = World(10, 10)
        bot = world.add_bot(5, 0)
        block = Block(4, 4)
        bot.receive(block)
        bot.direction = Direction.NORTH
        world.drop_forward(bot, block)
        assert bot.has(block), 'drop should not happen'

That is … not good. Someone, and I suspect that I know who it was, has implemented the drop capability without a direct test of it. Or so it seems. OK, well, we are here now, let’s write one for connection. In fact, we’ll write a couple but one at a time.

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world_bot.receive(block)
        connection = DirectConnection(world)
        connection.drop(client_bot, block)
        assert not world_bot.has(block)
        assert not world.is_empty(Location(5, 4))

I think this is close to what we need. Close enough to see what happens, if it isn’t quite right.

    def drop(self, client_bot, block):
        self.world.command('drop', client_bot.id, block.id)
        result = self.world.fetch(client_bot.id)
        client_bot._knowledge = copy.copy(result)

The biggest issue I see here is that I have two parameters and our command method only expects one:

    def command(self, action, parameter):
        if action == 'step':
            bot = self.map.at_id(parameter)
            self.step(bot)
        elif action == 'take':
            bot = self.map.at_id(parameter)
            self.take_forward(bot)
        else:
            raise Exception('Unknown command')

Let’s do this:

    def command(self, action, parm1, parm2=None):
        if action == 'step':
            bot = self.map.at_id(parm1)
            self.step(bot)
        elif action == 'take':
            bot = self.map.at_id(parm1)
            self.take_forward(bot)
        elif action == 'drop':
            bot = self.map.at_id(parm1)
            block = Block(99, 99)
            block.id = parm2
            self.drop_forward(bot, block)
        else:
            raise Exception('Unknown command')

This was as close as I could get to what needs to happen. In the current scheme, recall the test above for ‘take’, we actually remove the block from the world entirely and then put it back when the client returns it. That won’t do. We’re going to need to cache it in an off-world list and get it back from its id. I’m hoping that creating a new block will almost work here. But the test does not agree, and it tells me so.

>       assert not world_bot.has(block)
E       assert not True
E        +  where True = has(<block.Block object at 0x106447440>)
E        +    where has = <bot.Bot object at 0x106447230>.has

The world-bot reportedly still has a block. Let us explore. Ah, yes:

class World:
    def drop_forward(self, bot, entity):
        drop_location = self.bots_next_location(bot)
        if drop_location != bot.location and self.is_empty(drop_location):
            entity.location = drop_location
            self.add(entity)
            bot.remove(entity)

class Bot:
    def remove(self, entity):
        if self._entity == entity:
            self._entity = None

We do not have the same block as we came in with, because we had to reconstitute it. So the remove does not remove.

I think it is time to fix the take and remove so that things are not lost forever.

class Map:
    def remove(self, id):
        del self.contents[id]

Instead of removing the item, what if we just set its location off the map?

class Map:
    def remove(self, id):
        entity = self.contents[id]
        entity.location = Location(None, None)

And …

    def command(self, action, parm1, parm2=None):
        if action == 'step':
            bot = self.map.at_id(parm1)
            self.step(bot)
        elif action == 'take':
            bot = self.map.at_id(parm1)
            self.take_forward(bot)
        elif action == 'drop':
            bot = self.map.at_id(parm1)
            block = self.map.at_id(parm2)
            self.drop_forward(bot, block)
        else:
            raise Exception('Unknown command')

I really rather thought that was going to work. What does the test say? Ah, we did not find our block, because we never added it to the world in the test. The test needs improvement.

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world.add(block)
        world_bot.receive(block)
        connection = DirectConnection(world)
        connection.drop(client_bot, block)
        assert not world_bot.has(block)
        assert not world.is_empty(Location(5, 4))

Yes, well. The block has been removed from the bot but the world does not have it at 5,4. We do, however, have two blocks at location 6,5, which is east of the bot.

Let’s change the test to actually place the block and take it, and then drop it.

I notice that the take test is taking to the east. My test is wrong. Change the check:

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world.add(block)
        world_bot.receive(block)
        connection = DirectConnection(world)
        connection.drop(client_bot, block)
        assert not world_bot.has(block)
        assert not world.is_empty(Location(6, 5))

This passes, but I happen to know that there are probably two blocks in there with separate ids.

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world.add(block)
        world_bot.receive(block)
        connection = DirectConnection(world)
        connection.drop(client_bot, block)
        assert not world_bot.has(block)
        assert not world.is_empty(Location(6, 5))
        assert len(world.map.contents.keys()) == 2

That fails, getting 3 not 2, as I suspected. How did this come to pass?

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world.add(block)
        world_bot.receive(block)
        connection = DirectConnection(world)
        assert len(world.map.contents.keys()) == 2
        connection.drop(client_bot, block)
        assert len(world.map.contents.keys()) == 2
        assert not world_bot.has(block)
        assert not world.is_empty(Location(6, 5))

The first check finds two, and we could make a case that we should have removed it with a take or by hand, but how are we coming up with a new id?

Ah. I’ll bet drop_forward creates a block, because, up until now, we had removed the old one from the universe entirely. No, that can’t be, we are passing in the block that we got from the map.

Ah, no, it is:

    def drop_forward(self, bot, entity):
        drop_location = self.bots_next_location(bot)
        if drop_location != bot.location and self.is_empty(drop_location):
            entity.location = drop_location
            self.add(entity)
            bot.remove(entity)

We add it, which will give it a new id. Since it has not been removed, we need not add it back. Remove that line and hope.

Life is good, all tests pass. Commit: take implemented in DirectConnection

I am curious whether game main demo still works. It does not, crashing with no x and y here:

    def draw_world(self):
        for entity in self.world.map:
            x = entity.x
            y = entity.y
            name = entity.name
            scale_x = x * self.scale
            scale_y = y * self.scale
            ...

We must have things in there that have no x and y. Certainly taken blocks have that characteristic, though I didn’t notice the screen coming up at all. Anyway …

    def draw_world(self):
        for entity in self.world.map:
            x = entity.x
            y = entity.y
            if x and y:
                name = entity.name
                scale_x = x * self.scale
                scale_y = y * self.scale
                color = WHITE
                if name == 'R':
                    if entity.inventory:
                        color = RED
                    else:
                        color = GREEN
                self.text((scale_x + 1, scale_y + 1), name, 16, color, BLACK)

With that, the game runs. Commit: Fix game demo to skip objects with location None, None (taken blocks).

Reflection

OK, on the one hand, the addition of take and drop to the connection went nicely. We did have to adjust things so that objects taken do not just vanish from the world. That wasn’t a big surprise: I’m sure I’ve mentioned it before. It did take me a while to notice that add that was compensating for the del that we replaced.

And it was no big surprise that the demo, which is very ad hoc, needed to be adjusted for objects with no location. I do think it would be better to have set location to none, rather than to Location(None, None) since the latter is more invalid than the former, but it’s ok for now I think.

But the tests are still very much like story tests. Let’s look at those and think what would be better if only we could have something better.

    def test_step_north(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5, Direction.NORTH)
        location = client_bot.location
        connection = DirectConnection(world)
        connection.step(client_bot)
        assert client_bot.location == location + Direction.NORTH

    def test_take_a_block(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        connection = DirectConnection(world)
        block = Block(6, 5)
        world.add(block)
        assert not world.is_empty(Location(6, 5))
        connection.take(client_bot)
        world_bot = world.map.at_id(bot_id)
        assert world_bot.has(block)
        assert world.is_empty(Location(6, 5))

    def test_drop_block_on_open_cell(self):
        world = World(10, 10)
        client_bot = world.add_bot(5, 5)
        bot_id = client_bot.id
        world_bot = world.map.at_id(bot_id)
        block = Block(1, 9)
        world.add(block)
        world_bot.receive(block)
        connection = DirectConnection(world)
        assert len(world.map.contents.keys()) == 2
        connection.drop(client_bot, block)
        assert len(world.map.contents.keys()) == 2
        assert not world_bot.has(block)
        assert not world.is_empty(Location(6, 5))

These are in the TestConnection class, so, one would think, they should be testing the connection code, which is trivially small:

class DirectConnection:
    def __init__(self, world):
        self.world = world

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

    def take(self, client_bot):
        self.world.command('take', client_bot.id)
        result = self.world.fetch(client_bot.id)
        client_bot._knowledge = copy.copy(result)

    def drop(self, client_bot, block):
        self.world.command('drop', client_bot.id, block.id)
        result = self.world.fetch(client_bot.id)
        client_bot._knowledge = copy.copy(result)

What really needs testing in the code above? Well, we should really check that each one of those methods properly does the fetch and copy, I suppose. However, I’m not certain quite where the fetch and copy belong, because, in the fullness of time, we’ll be batching commands.

We need to be sure that the connection makes a correct call to the World. And, given how we’re doing it now, we need to be sure that this code is correctly aligned with the connection:

    def command(self, action, parm1, parm2=None):
        if action == 'step':
            bot = self.map.at_id(parm1)
            self.step(bot)
        elif action == 'take':
            bot = self.map.at_id(parm1)
            self.take_forward(bot)
        elif action == 'drop':
            bot = self.map.at_id(parm1)
            block = self.map.at_id(parm2)
            self.drop_forward(bot, block)
        else:
            raise Exception('Unknown command')

Why do we have this extra step here? We’ll need something similar for a network connection, but even so, that will be a responsibility of the connection to call the world correctly. I think we should inline all the string code and get rid fo the strings. I did those to give myself a sense that everything was properly indirect but this string stuff isn’t really needed.

We do need entry points that use the id, not the object, however. But maybe that’s an issue for those who call the connection? Ultimately they are going to have to use the id anyway, as they will have their own data structure for the details.

When we look at this test, our simplest:

    def test_step(self):
        world = World(20, 20)
        bot = world.add_bot(10, 10)
        connection = DirectConnection(world)
        connection.step(bot)
        assert bot.location == Location(11, 10)
        assert world.map.at_xy(11, 10).id == bot.id

And then this:

class World:
    def add_bot(self, x, y, direction = Direction.EAST):
        bot = Bot(x, y, direction)
        self.add(bot)
        returned_bot = Bot(x, y, direction)
        returned_bot.id = bot.id
        returned_bot.world = self
        return returned_bot

We see that world is creating a bot instance for the caller. That’s the caller’s business. The world can create a Bot for itself and should return an id to the client, which should make its own instance of whatever class it wants, with that id.

However, and this makes me unhappy, we are calling add_bot now in all our tests, all over. There are 23 callers of this method.

What should really happen, I think, when we have the connection, is that we’ll ask the connection to add a bot, it will ask the world to add a bot and (somehow) get an id back, and then either return that id, or create a bot and return it. I think the connection should not know anything about the situation, it should just forward things back and forth. So, I’m sorry, but add_bot should return a bot id and everyone who uses it should create their own bot from the id.

Well, no, that’s not right either: they need more info than just the id, they need the knowledge. Ah. That’s why we have fetch. Let’s try a test:

Note: The article is getting long and so is the time. I’ll just do this one and then wrap up, I think. I have a meeting soon anyway.

    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)

And let’s try it this way:

class DirectConnection:
    def add_bot(self, x, y):
        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

class World:
    def add_bot(self, x, y, direction = Direction.EAST):
        id = self.add_world_bot(x, y, direction)
        returned_bot = Bot(x, y, direction)
        returned_bot.id = id
        returned_bot.world = self
        return returned_bot

    def add_world_bot(self, x, y, direction = Direction.EAST):
        bot = Bot(x, y, direction)
        self.add(bot)
        return bot.id

That runs green. Safe to commit: added idea for add_bot to DirectConnection.

I think this can be unwound to be better, but I think we’ll need to revise a lot of tests. Probably at least 23. It won’t be hard but when we need to revise tests it is a sign that either the tests, or the objects, or quite likely both, are not correctly partitioned.

So that will need some thinking. But somehow this article got quite long, so let’s sum up.

Summary

The good news is that the DirectConnection now handles step, take, and drop, and that they went in without much difficulty at all. The less good news is that we have not worked out the initial setup logic, where a client will populate the world with its desired objects, and get back enough information to maintain on the client side.

GeePaw tabled the idea the other day of one big batch of info coming back, since we’ll probably batch commands anyway, and certainly it could make sense for the World to have a command similar to fetch that would fetch all the client’s known objects at once. So the client could add, add, add and then fetch_all to bring itself in sync. Maybe we’ll do that.

The tests still feel like they are testing too much. A take sets up a world and a connection and a bot and a block and then tries the take and then checks that the block is gone from the world and that the bot has it. That seems like a lot.

Idea: Maybe there could be a shorter form of world configuration that would make setting up a test more compact and less distracting? It might help …; Perhaps some kind of fixture, standard setup would make it feel better, but it just seems there is a lot to check. I need some consultation. Perhaps I can entice GeePaw to join me tomorrow. I think today is used up.

We’re progressing … but it still feels that we can’t move as quickly as we’d like. But nothing is breaking when we make changes, and we don’t spend long hours wondering what’s up, so maybe everything is OK and I’m just writing too many words. Few would disagree with that, I’d wager.

See you next time!

There is no real harm, down the road, if a Python client were to use the same basic class definition as the server side. In some cases, such as enumerating things, it might even be desirable. But the two sides do have different needs and its surely best to separate the needs into server-side, client-side, and shared. ↩