(Working Toward) WorldEntity
Armed with a bit less ignorance we’ll try to create and use a new world-side entity. I have … ideas. Result: This is why story estimation is so fraught.
As I imagined might happen, yesterday’s experiment plus a little time provides me with a less-tentative plan for the WorldEntity class. Ideas include:
-
Since DirectConnection expects a dictionary (easy to JSON), and since the client-side Knowledge can update from a dictionary, we might as well base our WorldEntity on the
dictclass. With a covering class, of course; -
We’ll cover the dictionary with the same accessors that we use to get and set the values we set in the existing Knowledge class. Probably even borrow some code from it.
-
We’ll TDD the class. It’s trivial but even so it’s better to have some tests, if only so no one says “where are the tests for this?”
-
The existing code is setting a
worldpointer. I should think that that is obsolete and if it is not, should be made so. So we’ll do that.
- Nota Bene
- I just barely remembered that
worldpointer. It really does need to go: we cannot see the World over the network. We do the right thing today, to remove it. And yet … I surely wasn’t thinking of it a few lines back when I said we’d be working on the WorldEntity. -
I could, of course, revise this article to make it look like things went according to plan. But my point, and I do have one, is that things very rarely go according to plan.
-
That said, the work today goes quite nicely. It’s just not the work we expected.
Let’s begin with that world pointer:
class World:
def add(self, entity):
entity.world = self
World.next_id += 1
entity.id = World.next_id
self.map.place(entity)
That’s reflected 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
But wait! We don’t ever pass that entity to the client side. No … DirectConnection is updating it:
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
OK, what is this used for? Ah. This is odd and somewhat perverted:
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.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}'
That’s the only access in the production code. There are some tests referencing it as well. We’ll deal with those as needed. It seems clear (to me) that the perform_actions should be passed a DirectConnection. It has no business creating one, and we’re trying to get it not to know the world, which is right out. perform_actions is called from here:
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()
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 step(self):
self.perform_actions(['step'])
It’s also called from some tests, plus there are tests calling move and step. Here’s one:
def test_nothing_near(self):
world = World(10, 10)
client_bot = world.add_bot(5, 5)
real_bot = world.map.at_id(client_bot.id)
client_bot.direction_change_chance = 0.0
real_bot.direction_change_chance = 0.0
client_bot.vision = None
client_bot.move()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(6, 5)
vision = client_bot.vision
assert ('R', 6, 5) in vision
Can we replace this with a call do do_something? If so, we can funnel a DC into that method and pass it where it needs to go.
When I change the move to do_something, the test fails. The None is no good. Give the test an empty list. Green. Commit: remove direct call from tests to move.
There are now no calls to move. Remove it, commit: remove move.
Other calls to step?
def test_stop_at_edge(self):
world = World(10, 10)
client_bot = world.add_bot(9, 5)
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
Those two calls to do_something used to say step. Test runs. No references to step. Remove it. Green. Commit: adjust tests, remove step method.
Where were we? Oh, yes, working to get rid of the world pointer in Bot. Let’s think a moment. Big UpFront Design right here:
We use world so that perform_actions can create a DirectConnection to the world, which we then use to, well, perform our actions:
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}'
So, clearly, we want perform_actions to be passed a connection. Can we just move the creation of the connection up to do_something and pass it in? Not quite, there are tests calling perform_actions. Best inspect those. Here’s one offender:
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
What would happen if we replaced that perform_actions with do_something? With this change, the tests run
def test_change_direction_if_stuck(self):
def move_and_update():
client_bot.do_something()
actions = client_bot.update_for_state_machine()
client_bot.perform_actions(actions)
...
What is that second thing doing? What if we just remove that entirely? Test passes. Inline.
def test_change_direction_if_stuck(self):
world = World(10, 10)
client_bot = world.add_bot(9, 5)
client_bot.direction_change_chance = 0.0
client_bot.do_something()
assert client_bot.location == Location(10, 5)
assert client_bot.direction == Direction.EAST
client_bot.do_something()
assert client_bot.location == Location(10, 5)
client_bot.do_something()
world_bot = world.map.at_id(client_bot.id)
assert world_bot.direction != Direction.EAST
assert client_bot.direction != Direction.EAST
Green. Commit: remove direct test of perform_actions
Continuing to find references to the perform_actions:
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()
assert client_bot.location == Location(2, 5)
client_bot.do_something()
assert client_bot.location == Location(1, 5)
client_bot.do_something()
assert client_bot.location == Location(0, 5)
client_bot.do_something()
assert client_bot.location == Location(0, 5)
client_bot.do_something()
assert client_bot.location != Location(0, 5)
Ah, this one is a bit tricky. We wanted to force the bot to move west. I wrote that test when I thought Bots were walking off the left side of the screen. That behavior is tested elsewhere now. Remove that test entirely. Green. Commit: remove redundant test.
One more test:
def test_three_blocks_near(self):
from block import Block
def turn_move_and_update():
client_bot.perform_actions(['WEST', 'step'])
world.update_client_for_test(client_bot)
actions = client_bot.update_for_state_machine()
client_bot.perform_actions(actions)
world = World(10, 10)
world.add(Block(4, 4))
world.add(Block(6, 6))
world.add(Block(4, 5))
client_bot = world.add_bot(6, 5)
client_bot.direction_change_chance = 0.0
client_bot.vision = None
turn_move_and_update()
assert client_bot.location == Location(5, 5)
vision = client_bot.vision
assert ('R', 5, 5) in vision
assert ('B', 4, 5) in vision
assert ('B', 6, 6) in vision
assert ('B', 4, 4) in vision
This is just a test of vision. I don’t know why I put in all the wandering around stuff. I “know” that vision works. I’m tempted to remove this test. Let’s see how it gets created:
class World:
def step(self, bot: Bot):
self.map.attempt_move(bot.id, bot.forward_location()) # changes world version
self.set_bot_vision(bot)
self.set_bot_scent(bot)
def set_bot_vision(self, bot):
bot.vision = self.map.vision_at(bot.location)
class Map:
def vision_at(self, location):
result = []
for dx in (-1, 0, 1):
for dy in (-1, 0, 1):
found = self.at_xy(location.x + dx, location.y + dy)
if found:
result.append((found.name, found.x, found.y))
return result
How about we convert this test to test Map directly? Would that be satisfactory?
def test_three_blocks_near(self):
from block import Block
from bot import Bot
world = World(10, 10)
world.add(Block(4, 4))
world.add(Block(6, 6))
world.add(Block(4, 5))
world.add(Bot(5, 5))
vision = world.map.vision_at(Location(5, 5))
assert ('R', 5, 5) in vision
assert ('B', 4, 5) in vision
assert ('B', 6, 6) in vision
assert ('B', 4, 4) in vision
Green. I think there’s just the one call to perform_actions. Yes. Green. Commit: no tests use perform_actions.
Now I can push the creation of the DC up one level:
def do_something(self):
connection = DirectConnection(self.world)
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, connection)
def perform_actions(self, actions, connection):
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}'
Green. Commit: DC now created in do_something. Yikes, a test just broke! I pushed a broken test I think.
Oh, another intermittent!
def test_stop_at_edge(self):
world = World(10, 10)
client_bot = world.add_bot(9, 5)
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
Sometimes the bot decides to change direction.
def test_stop_at_edge(self):
world = World(10, 10)
client_bot = world.add_bot(9, 5)
client_bot.direction_change_chance = 0
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
client_bot.do_something()
world.update_client_for_test(client_bot)
assert client_bot.location == Location(10, 5)
That should fix it. Commit: fix intermittent test.
Now then. What we really want1 is to have the DirectConnection passed to do_something. And we do now have about a million calls testing do_something. Many of which should be smaller more direct tests but we are here now.
If we change signature of do_something to expect a DirectConnection, and we default it properly, this might go readily. I’ll try:
I have PyCharm change the signature with default DirectConnection(world). Since the tests all create a temp world before they call do_something, all the tests link up. They all fail wanting import of DirectConnection, no surprise there, and when I provide it, the tests are green. Commit: do_something now receives a DirectConnection and uses it.
Did game get updated correctly? Yes, though it needed the DC import. Commit: give game DirectConnection.
Now can we remove the world member from Bot (and Block)? All the uses are assignments, no reads.
Oh, wait, there’s a test in the Proxy stuff. That whole test class is suspect. We’ll remove it. Green, with a few fewer tests of classes we don’t use and never will. Commit: remove test_population file.
I go through one more find cycle and remove all the references to Bot.world. And then Block.world, which is set and never accessed (of course). Commit: remove world member from Bot and Block.
Wow. Look where we are. Let’s sum up.
Summary
I really had expected to TDD the WorldEntity and possibly even get it in place. However, as so often happens, something that I almost forgot needed doing first. Now we could certainly have ignored the world member question, but I think if we had, we’d have had a half dozen or more tests that would have broken when we weren’t expecting it. So it was good that I remembered that world membet, and good that I decided to get rid of it.
Every step along the way was simple, and even though a lot of tests needed to be updated, the updates were simple, mostly coming down to calling do_something instead of something else. They’d probably have been even simpler, or even unnecessary, had someone not written all those story-like tests in the past.
On the bright side, we replaced one and simply removed another as redundant. So the tests are improved as a side effect of this.
Deciding to replace the world member led directly to passing the DirectConnection to do_something and that is, I think, just exactly what should happen. In the client server mode, some external god-like object will own all the bots and when the time is right (TBD) it will loop over them asking them to do something. So it will make sense that that object, whatever it turns out to be, is on the wire between client and server.
So today had these really interesting aspects:
- It was surely almost exactly the right thing to do next;
- It went very smoothly, modulo discovery of an intermittent test;
- It was COMPLETELY NOT WHAT WE SET OUT TO DO.
This, my dear friends, is why story estimation is so fraught and why so many of us recommend not trying to estimate stories at all. We can’t really get it right at the beginning, because we simply cannot predict or remember all the things the code needs. And, if we feel pressured to meet the estimate, we’re very likely to take shortcuts and leave the system in a worse state than it should be.
If I had told by the Big Boss that I just had to get the WorldEntity into the system today, I almost certainly could have done it. We’ll see, tomorrow, whether that’s true, because I’ll probably really do it tomorrow. But if I had done it today, I would not have had time to remove the world reference and I’d have either coded around it or mashed something, rather than do what needed to be done.
And don’t forget that this WorldEntity is only a step on the way to another story, the ability for our Bots to group like things together in separate groups. I’m really glad no one is holding me to my thinking on how long that would take!
Is it possible to use story estimates well? I can’t say it’s impossible. I’ve seen them used fairly effectively and without undue pressure. Once. And I’ve seen them useless, or misused, many times. And what I find here chez Ron2 is that even all by myself, with no team issues or undue pressure … I still almost never do, by 10 AM, what I thought at 7 AM I would do.
There’s a lesson there, somewhere.
And next time … or maybe not next time … we’ll do the WorldEntity. See you then!