Block?
I believe that the client-side code includes a Block class. I further believe that we have no use for it. Even further, I suspect there are tests that use it, which should not. Let’s sort this out. Simple changes, nothing to see here. Refactoring works.
Back when all this started, I intentionally did not make a client-server distinction, on the grounds that we didn’t need it yet and that not worrying about it would make development easier and wouldn’t do much harm. I was confident that we could refactor and install the client-server notions later.
I was nearly right about all that, in that we’re in a position now where I’m confident that we can split into client and server as soon as I learn enough about sockets to actually do it. But I did wait longer than I should have, or perhaps thought a bit less than I should have, with the upshot being that the server side logic was manipulating client-side objects (Bots and Blocks), when clearly, it shouldn’t be able to do that.
I say “clearly”. There certainly are, or have been, distributed object systems where objects on one computer could send messages to objects on another and they could send messages back. But even the small amount of reasoning that I can muster would suggest that such a scheme works best if there is a lot of work being done on one side in between inter-machine messages, or else the network delays will become intolerable. Anyway, realistically, while we might have chosen to copy the Bot and Block back and forth, using the actual instances really needed to be right out.
The delay in addressing the issue did not kill the project, which was my point, really, but it was probably somewhat more difficult than it would have been had I started sooner. There’s no way to really know that, but it feels to me like I did some things twice that I might have had to do only once … except that if we’re keeping client and server separate … don’t we have to do a lot of things twice? Probably yes. Anyway it has gone well enough.
But I do think there is a Block class on the client side and I think that the production code probably does not use it, and I am quite sure that there are tests on the world side that do use it. This situation is not ideal, by which I mean it is bad, and today we’re going to go after it.
And without further ado, let me introduce to you, the Block class:
class Block:
def __init__(self, x, y):
self.id = None
self.name = 'B'
self.location = Location(x, y)
@property
def x(self):
return self.location.x
@property
def y(self):
return self.location.y
def __str__(self):
return f'{self.name} {str(self.location)}'
Not what you’d call a lot of behavior. Where is it used?
I am delighted to find only three places net of import statements, one in TestConnection and two in TestKnowledge.
class TestConnection:
def test_drop_block_on_open_cell(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
bot_id = client_bot.id
world_bot = world.map.at_id(bot_id)
block_id = world.add_block(1, 9)
client_block = Block(1, 9)
client_block.id = block_id
world_block = world.map.at_id(block_id)
world_bot.receive(world_block)
assert len(world.map.contents.keys()) == 2
connection.drop(client_bot, client_block.id)
assert len(world.map.contents.keys()) == 2
assert not world_bot.has(world_block)
assert not world.is_empty(Location(6, 5))
Reading that, I do fondly wonder what it’s really testing and how we might better do that thing.
Are we testing the ability of the World to add a block and get it into the map at the right place? That check belongs in TestWorld.
Are we testing whether DirectConnection properly formats a drop command? That’s trivial and should be a test of that very thing.
Are we testing whether, given a valid drop command, the world drops the block. That, too, is a world test and if I recall what we were looking at yesterday, we have direct tests of that.
Looking at TestWorld, I do not see a drop test that actually drops. I see quite a few demonstrating all the ways in which one cannot drop. Let’s look at one of those and allow it to drop.
def test_bot_can_drop_on_empty_space(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5, Direction.NORTH)
bot = world.entity_from_id(bot_id)
block = WorldEntity.block(4, 4)
bot.receive(block)
assert bot.has(block)
world.drop_forward(bot, block)
assert not bot.has(block)
assert world.map.at_xy(5, 4) is block
We could do them in the other three directions, but I see no point, But I do see a point to checking that we cannot drop if there is already something there.
def test_bot_cannot_drop_on_used_space(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5, Direction.NORTH)
bot = world.entity_from_id(bot_id)
blocker_id = world.add_bot(5, 4)
blocker = world.entity_from_id(blocker_id)
block = WorldEntity.block(4, 4)
bot.receive(block)
assert bot.has(block)
world.drop_forward(bot, block)
assert bot.has(block)
assert world.map.at_xy(5, 4) is blocker
With those two tests in place, I’m confident that world behaves correctly. Yes, in principle we could check all the directions, and in principle we could try putting a block in as blocker instead of a bot, but these are not black-box tests and I am more than confident enough that these will deal with any interesting mistakes I may make in this area.
I could be wrong, of course. I’m sure we could imagine a mistake that would make dropping NORTH work but EAST not. I’m not concerned enough about that to test it If I were, I would. If you were here and wanted to test it, we’d test it.
OK, back to that connection test:
def test_drop_block_on_open_cell(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
bot_id = client_bot.id
world_bot = world.map.at_id(bot_id)
block_id = world.add_block(1, 9)
client_block = Block(1, 9)
client_block.id = block_id
world_block = world.map.at_id(block_id)
world_bot.receive(world_block)
assert len(world.map.contents.keys()) == 2
connection.drop(client_bot, client_block.id)
assert len(world.map.contents.keys()) == 2
assert not world_bot.has(world_block)
assert not world.is_empty(Location(6, 5))
Those assertions are all about the world and its map. That is none of DC’s business.
I think I remember a reason why this test might exist. There was a time when, if a block was in the map, and a bot took it, the block was removed from the map entirely. It somehow became clear that that isn’t a good idea, because once it was out of the map, the world had no knowledge of it. What it if was a smart block and after a few moves it began to struggle? So we changed the world to set the block’s location to (None, None), but to keep it in the map by id:
class Map:
def remove(self, id):
entity = self.contents[id]
entity.location = Location(None, None)
Note that we do not remove it from contents, we just set its location. A quick look tells me that there is no direct test that checks that the object was not removed. Perhaps this DC test was the one intended to do that. Let’s do a world test and remove this test entirely.
def test_remove_does_not_forget(self):
world = World(10, 10)
block_id = world.add_block(5, 5)
block = world.entity_from_id(block_id)
assert world.map.at_xy(5, 5) is block
world.map.remove(block_id)
assert world.map.at_xy(5, 5) is None
assert world.map.at_id(block_id) is block
OK, remove the DC test and commit: move testing from TestConnection to TestWorld where it belongs.
Now those other tests that look at Block.
class TestKnowledge;
def test_has_block(self):
knowledge = Knowledge(None, None)
assert not knowledge.has_block
block = Block(3, 3)
knowledge.receive(block)
assert knowledge.has_block
knowledge.remove(block)
assert not knowledge.has_block
def test_bot_uses_knowledge_inventory(self):
bot = Bot(5, 5)
block = Block(3, 3)
bot.receive(block)
assert bot._knowledge.has_block
These are, well how can I put this, fundamentally wrong. We don’t really reconstitute a block and store it in our knowledge instance.
On the world side, when a Bot takes a Block, we have a WorldEntity instance at the take location, and we pass that to the bot’s WorldEntity:
class World:
def receive(self, entity):
self.holding = entity
@holding.setter
def holding(self, value):
self._dict['held_entity'] = value
When we fetch the bot via its id, per DirectConnection, we return the entity as_dictionary:
class WorldEntity:
def as_dictionary(self):
held = 0 if not self.holding else self.holding.id
return {'direction': self.direction,
'held_entity': held,
'eid': self.id,
'location': self.location,
'scent': self.scent,
'vision': self.vision}
Ah, there we are. We only put the object id into the returned information. So that’s OK. But we shouldn’t be using the Block in those tests, we should use a WorldEntity block.
def test_has_block(self):
knowledge = Knowledge(None, None)
assert not knowledge.has_block
block = WorldEntity.block(3, 3)
knowledge.receive(block)
assert knowledge.has_block
knowledge.remove(block)
assert not knowledge.has_block
def test_bot_uses_knowledge_inventory(self):
bot = Bot(5, 5)
block = WorldEntity.block(3, 3)
bot.receive(block)
assert bot._knowledge.has_block
Now, modulo a couple of unnecessary imports, we should have no references to Block class.
We issue a safe delete. Commit: replace spurious reference to Block. Remove unused Block class.
Reflection
So that went smoothly, except that at first I failed to notice that first line of the as_dictionary, which sent me on a snipe hunt worrying about stuffing a WorldEntity coming over to the client side. No such snipe exists.
I wonder if inlining that temp would make it more clear, by drawing attention to the anomalous assignment:
def as_dictionary(self):
return {'direction': self.direction,
'held_entity': (0 if not self.holding else self.holding.id),
'eid': self.id,
'location': self.location,
'scent': self.scent,
'vision': self.vision}
I think I prefer that because that big long line draws the eye to the special case.
Of course, since we’re reflecting … we could just put the id into held-entity right off the bat. If we did, we’d have an issue with the other side of the property:
class WorldEntity:
@property
def holding(self):
try:
return self._dict['held_entity']
except KeyError:
return None
@holding.setter
def holding(self, value):
self._dict['held_entity'] = value
There are tests that check just what we’re holding, and the world uses the holding property to get the aroma of the block we’re holding. So we’ll let that ride for now, but it is a bit strange.
What’s Next?
I think that what we ought to be doing Real Soon Now is to move our classes under some folders, one for server, one for client, and possibly one for common code or data structures that we might want to share. That should make it easier to find places where any residual cross-wire references exist: the file would have to import from both sides. I’ll do that offline, no need to show you much.
Well, my first attempt to do that, which was to tell PyCharm to move the file, seems to have removed it entirely. Roll back.
Doing it over seems to have worked. Once in a while PyCharm couldn’t unwind the imports and needed me to type a folder name somewhere. Something about an imported class instance looking different from a local one. Commented out the unnecessary checks.
I moved all the tests into a single ‘test’ directory. That was probably a mistake, I probably should have built two directories, server and world, with src and tst underneath each. We can do that at some future unspecified date. This will do for now. All nice and committed.
Summary
I think this is closer to the structure that we should have … and I think it is no less difficult than it was before to determine whether there are any cross-wire connections. I’d have to check all the imports to see if any refer to both client. and server. Using PyCharm’s find, searching for client\. and server\., everything looks OK. I think I need a review from people who have more sense than I do about organizing Python projects, but this will serve for now.
I’ll live with it a while and if it doesn’t seem good, we can always change it.
Confidence that we are no longer using objects from the other side of the wire is up around 0.99 now.
Lesson, if any, is that, yes we can refactor incrementally from just about any decent design to any other, and that even so, a bit of thinking ahead, if not coding ahead, is always worth doing.
A useful session if not exciting. See you next time!