The Last Thread?

The Repo on GitHub

The demo went well. Someone spotted a dangling thread of connection between server and client. Let’s fix that.

While we might allow certain structure definitions, even certain classes, such as Location and Direction, to be used on both the client and server sides, we surely cannot pass mutable objects back and forth over the wire.

Well, we could …

No, that’s not quite correct. We surely could pack up any object on the client side, copy it across to server, reinflate it, modify it, and copy it back. At that point, the reinflated object would not be the same object that we started with, except with changes. It would be a new object. We might have a repository of objects on the client side where we’d replace the old one with the new one, or copy its contents over, whatever. We could make things appear as if “the same” object could be manipulated by server or client.

But we probably shouldn’t …

While we could probably invent fully distributed objects for this little game, that would almost certainly not be an ideal design. Simply put, there would surely be methods, and perhaps data, in that object, that were specific to server, and other methods and data that were specific to client, even if some of the methods and data were used jointly. The issue is that an object that has some server code and data, and some client code and data is not cohesive. Objects, ideally, have a single focus. They handle one concept, and they handle all aspects of that concept.

Of course, the art comes down to identifying the concepts, because we don’t want one big object that handles the concept “game involving robots and other objects interacting in a large world”. The art is in dividing up that big idea into smaller ideas so that each class manages an identifiable sub-notion of that larger concept.

Well. Suffice it to say that we are on the path of having a WorldEntity on one side, and a Bot class on the other side, and that the twain only meet via a dictionary of data passed back and forth.

What was spotted last night, was that we still have code on the server side that is referencing the client-side Bot and Block classes. Here’s the Bot case:

class DirectConnection:
    def add_bot(self, x, y):
        client_bot = self.world.add_bot(x, y)
        self.update_client(client_bot)
        return client_bot

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

class World:
    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        returned_client_entity = Bot(x, y, direction)
        return self.add_and_return_client_entity(entity, returned_client_entity)

We see there that the World, defiantly server-side, is creating a Bot instance, which is furiously client-side. Clearly, the DirectConnection should do the creation and return the Bot to the client. Let’s fix that.

What should be returned? Given how things work right now, it would seem that World.add_bot should return an id, and the DirectConnection should fetch the result dictionary by id, and fill in the Bot from that.

Musing / Design Thinking

On the surface, this seems simple enough. I’m a bit concerned that there is more to it than we might think, having to do with what is, and is not, in the result dictionary. And should that object be elaborated into something more testable? How deeply must we dig in the code to be sure this works?

I think that we can safely do the naive thing. We’ll find out.

We’ll change World so that it does not create a Bot and just returns the id of the WorldEntity created. That will break some tests, no doubt about it. Then we’ll pick up the slack in the DirectConnection.

I don’t see any machine refactorings that will help directly. Let’s see if we can ease into it. Perhaps a Wishful Thinking new method, add_bot_returning_id, which we’d rename after things work.

No. I’m just going to change how add_bot works and deal with the fallout. I see something that may help:

class World:
    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        returned_client_entity = Bot(x, y, direction)
        return self.add_and_return_client_entity(entity, returned_client_entity)

    def add_and_return_client_entity(self, entity, returned_client_entity):
        self.map.place(entity)
        id = entity.id
        returned_client_entity.id = id
        return returned_client_entity

I’ll inline that second method into the first, leaving the method alone for its other user. We’ll deal with that one shortly. (For values of shortly. Probably tomorrow.)

    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        returned_client_entity = Bot(x, y, direction)
        self.map.place(entity)
        id = entity.id
        returned_client_entity.id = id
        return returned_client_entity

Now I just edit the method so that it doesn’t create the Bot and so that it just returns the id. That will break things.

    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        self.map.place(entity)
        return entity.id

21 tests fail. That’s rather a lot. But I think we’ll find the changes easy. First change the DC code:

    def add_bot(self, x, y):
        bot_id = self.world.add_bot(x, y)
        result_dict = self.world.fetch(bot_id)
        bot = Bot(x, y)
        bot._knowledge.update(result_dict)
        return bot

Now only 20 tests fail. I bet we have 20 tests calling World.add_bot. We’ll see. Here’s the first one.

    def test_bot_in_world_gets_next_id(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        bot = world.add_bot(5, 5)
>       assert bot.id == 101
E       AttributeError: 'int' object has no attribute 'id'

If we do this, the test should run:

    def test_bot_in_world_gets_next_id(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        bot = DirectConnection(world).add_bot(5, 5)
        assert bot.id == 101

We can go thru the tests and make that change everywhere. I assume there is a global replace in PyCharm, this might be just the time to try it.

Conscience Speaks

This is going to work, but it seems that it makes our tests even less reasonable than they already are. We’re surely not making the tests better. But but but … it will probably work. I’ll try paying attention to the tests as the replace goes through them, and maybe even not change ones where I can see a simpler way.

A brief time passes …

The global replace has fixed all but one test. I did improve a number of them, consolidating multiple uses of the connection with an extracted variable. Here’s our broken one:

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

The original test created the bot with Direction.NORTH and DC.add_bot doesn’t accept direction. Let’s improve it:

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

class DirectConnection:
    def add_bot(self, x, y, direction=Direction.EAST):
        bot_id = self.world.add_bot(x, y, direction)
        result_dict = self.world.fetch(bot_id)
        bot = Bot(x, y)
        bot._knowledge.update(result_dict)
        return bot

We are green. Commit: World.add_bot no longer references Bot class. One more reference to fix.

I did a quick search for references to Bot and found that, to my dismay, there is another one in World:

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

I don’t think this type hint is accurate. I think it’ll be a WorldEntity.

class World:
    def command(self, action, bot_id, parameter=None):
        world_bot = self.map.at_id(bot_id)
        if action == 'step':
            self.step(world_bot)
        elif action == 'take':
            self.take_forward(world_bot)
        elif action == 'drop':
            block = self.map.at_id(parameter)
            self.drop_forward(world_bot, block)
        elif action == 'turn':
            self.set_direction(world_bot, parameter)
        else:
            raise Exception('Unknown command')

Right. That’s just an early attempt at a type hint. I no longer remember why I even did that. Remove it.

    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)

The old_location was a leftover, so I removed it. Removed the import of Bot. Green. Commit: World no longer references Bot class.

Let’s glance at a test or two, the ones that I “improved”.

    def test_take_a_block(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        bot_id = client_bot.id
        world.add_block(6, 5)
        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))

All that really happened here was that the connection was set up down by the take, so I moved it up and used it in the creation of the client bot. There was one test, maybe two, that actually created a DirectConnection more than once, which is harmless but wasteful, and I changed those to create one and use it. Extract Variable handles that nicely.

Reflection

We have done a good thing, removing the reference from World to Bot. We see by inspection that no real Bots are being sent across the connection in either direction. It is possible that a test could be passing a real Bot into the world. There’s no real harm done by that, except that the test probably isn’t testing what we think it is.

Becoming more certain we’re disconnected

What “should” happen, I think, is that we “should” ensure that any test that deals with a Bot instance only talks to the world through DirectConnection. I think that would require a bit of searching and such, although a good start might be to look at tests that import Bot, or refer to World methods.

Private methods might help

It seems very likely to me that almost all the methods of World should be private, which might give us some leverage for finding tests that reference them. I’ve made a note of that.

Block remains to be dealt with

We have references to Block in World, via its add_block method. We should change that similarly to what we’ve done this morning, and that should be straightforward, but there’s more.

The client, as presently envisioned, can really only create bot instances. Blocks are aspects of the world. We have not addressed how the world should configure itself, but presumably it needs some kind of constructor or factory methods, that would belong on the server side, although they might be invoked by a client when requesting a world to play in.

Tests could be criticized

The subjunctive is not appropriate here. They really could use improvement. They are very story-like, setting up world situations and then fiddling with client objects to see what happened. There should be at least these kinds of tests:

Some tests would ensure that the world does what it should do. Others should ensure that the Bots do what they should do. Some tests should ensure that the world and client share the right information back and forth. Some tests should ensure that the client creates properly formatted requests. Some should check that the world properly understands properly formatted requests.

It seems to me that if the tests were really great, there would be very few that actually tested across the connection. There might want to be some, but if we know that the world works, and the client works, and the connection works, what other testing do we need?

If I were a good person, I would go after that issue. And if I were to do that, we might all learn something. So, I’ll put it on the list. We don’t have to do it all at once, so a bit of exploration and improvement would surely be easy enough, and enlightening.

Summary

We did a good thing today, and although it did require a lot of changes to tests, they were all just the same substitution, with one exception that identified the need for a direction parameter in DirectConnection.add_bot, which was readily added.

Now there’s one less thing to criticize, and the two sides, client and server, are almost completely separated.

There’s some duplication to clean up in World, but since next time I plan to do with Block what we did with Bot today, we’ll wait until we’ve done them so that we can see what really needs changing.

I’ll permit myself a gentle “woot”, and a nice iced chai latter. Drink ‘em if ya got ‘em. See you next time!