Still Concerned
The Knowledge class and its usage have something to teach us, I think. There are issues here that aren’t uncommon. Perhaps some changes will spark ideas. Theme: battling entropy, battling chaos.
Let’s refresh our memory with a quick scan of the Knowledge class, with an eye to things we might not like. Some of these concerns may be unique to Python, and some not. Here’s the whole thing: give it a scan, see what leaps out at you.
class Knowledge:
drop_threshold = 4
take_threshold = 7
energy_threshold = 5
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
def as_dictionary(self):
held = 0 if not self._held_entity else self._held_entity.id
return {'direction': self._direction,
'held_entity': held,
'id': self._id,
'location': self._location,
'scent': self._scent,
'vision': self._vision}
def update(self, update_dictionary):
self.direction = update_dictionary['direction']
self.receive(update_dictionary['held_entity'])
self.id = update_dictionary['eid']
self.location = update_dictionary['location']
self._scent = update_dictionary['scent']
self.vision = update_dictionary['vision']
@property
def holding(self):
return self._held_entity
@property
def id(self):
return self._id
@id.setter
def id(self, id):
self._id = id
@property
def vision(self) -> Vision:
return self._vision
@vision.setter
def vision(self, vision_list):
self._vision = Vision(vision_list, self.location, self.direction)
@property
def direction(self):
return self._direction
@direction.setter
def direction(self, direction):
self._direction = direction
@property
def location(self):
return self._location
@location.setter
def location(self, location):
self._location = location
@property
def has_block(self):
return self._held_entity # and self._held_entity.name == 'B'
@property
def can_take(self):
is_scent_ok = self._scent <= self.take_threshold
return is_scent_ok and self.vision.match_forward_and_one_side('B', '_')
@property
def can_drop(self):
vision_ok = self.vision.match_forward_and_one_side('_', 'B')
scent_ok = self._scent >= self.drop_threshold
return vision_ok and scent_ok
def has(self, entity):
return entity == self._held_entity
def has_energy(self):
return self._energy >= self.energy_threshold
def use_energy(self):
self._energy = 0
def gain_energy(self):
self._energy += 1
def receive(self, entity):
self._held_entity = entity
def remove(self, entity):
if self._held_entity == entity:
self._held_entity = None
First thing to notice is that we don’t need as_dictionary, since we make no production use of it. We’ll check that. World sends as_dictionary, but not to our object. It sends it to its own WorldEntity instances, as part of fetch. Bot sends it here:
class Bot:
def as_dictionary(self):
return self._knowledge.as_dictionary()
Are there senders of that? I cannot find them. So, starting in Bot, let’s remove those methods and see if anything breaks. Nothing should. Green. Commit: remove unused as_dictionary.
So we just lost ten lines we didn’t need. Life is good. What else do we notice about Knowledge?
It seems that whoever wrote it (pretty sure we know who that was) was thinking in terms of private member variables with setters and getters, probably because there are a couple of cases where what we save is not what we’re sent. In particular, vision, as it comes over from the world side, is a list of some kind, not worth looking at right now, and we wrap that in an instance of our bot-side class Vision, which has convenient pattern-matching methods.
- Added in Post
- The question of “why” is always a good one, though we have to be careful with it. Throughout what follows, we find constructs and approaches that were taken in the past (by me, of course) whose purpose isn’t quite clear. Standing back, I think I was, and continue to be struggling with a collection of concepts that want to be immutable over long periods of time, and then replaced with a new more current set. But the objects and code don’t quite support that.
-
See what you think as you read. If you think I’ve missed something important, toot me up.
But overall, the setters and getters both exist, and both just refer to the private variable of the same name prefixed with underbar.
I do not see the point of that. Do you see the point of that? I’m almost tempted to go back and find the article where I wrote about this, just to see what the [DELETED] I was thinking. But no, I’m more interested in what I’m thinking now.
It seems to me that we can remove these getters and setters, one at a time, by just renaming the member and deleting the properties. From this:
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
@property
def direction(self):
return self._direction
@direction.setter
def direction(self, direction):
self._direction = direction
To this:
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
(The properties are gone, which is why you can’t see them.)
We’re green. Let’s commit each time. Commit: rename _direction, removing accessors.
I did the same thing with id / _id, and noticed an interesting thing. I first renamed the _id by just removing the underbar in the init method. All the tests still ran. Why? Because the accessors were still setting and getting self._id, and Python, in its wisdom, will just randomly add a member of that name to the instance.
- Added in Post
- Another twinge, wishing for immutability? Probably.
It’s not too difficult to imagine a defect where, in some code, we refer to a member that doesn’t exist, and do so in such a way that that code appears to run, but other code that expects that value still refers to the “real” member, and a subtle defect arises, where the Bot seems to sometimes forget which way it’s going.
It would be nice if there were a way to ensure that we can’t add any more members than we intended. It turns out there is, and I plan to use it. Soon.
I’ll commit this and do the rest of the easy members without further comment, unless something odd comes up.
I only get id and location done and then notice scent, which has no accessors, but there are direct accesses to the allegedly private _scent. Bot has both properties:
class Bot:
@property
def scent(self):
return self._knowledge._scent
@scent.setter
def scent(self, scent):
self._knowledge._scent = scent
I’d expect that those are only used in tests. We’d better look. I see no accesses to the getter. The only uses of scent are in WorldEntity tests and World. I remove the properties and we’re still green.
Here’s where Knowledge stands now:
def __init__(self, location, direction):
# world write / client read only
self.direction = direction
self.id = None
self.location = location
self._held_entity = None
self._scent = 0
self._vision = Vision([], self.location, self.direction)
# local Bot client-side info
self._energy = self.energy_threshold
def update(self, update_dictionary):
self.direction = update_dictionary['direction']
self.id = update_dictionary['eid']
self.location = update_dictionary['location']
self.receive(update_dictionary['held_entity'])
self._scent = update_dictionary['scent']
self.vision = update_dictionary['vision']
@property
def vision(self) -> Vision:
return self._vision
@vision.setter
def vision(self, vision_list):
self._vision = Vision(vision_list, self.location, self.direction)
- Added in Post
- The next idea takes a fatal blow in the same paragraph as created it. Such is the life of some ideas. The next few short paragraphs are me, discovering that I’ve done the easy bits and the rest seem more difficult than we can do today.
I was thinking that we’d have some public members and some private. But no, let’s have them all be private until we know the reason why. Rename those three, one at a time. Meh! Bot has direction accessors. Who’s using them?
A scan of Bot tells me that it has accessors for almost everything in knowledge. That’s too broad, I think.
I think we’re going to have to change strategy here. Let’s see.
There are a few uses of Bot.direction, including this:
class Bot:
def change_direction(self):
direction = self.direction
while direction == self.direction:
direction = random.choice(Direction.ALL)
return [direction.name()]
Do we even use this? Yes …
class Bot:
def do_something(self, connection):
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 update_for_state_machine(self):
if self.location == self._old_location:
return self.change_direction()
else:
return []
However, it’s Knowledge that actually knows our direction, so those references to self.direction in change_direction are bogus. We should be asking Knowledge for a new_direction. We’ll do that “by intention” (wishful thinking):
class Bot:
def change_direction(self):
return [self._knowledge.new_direction().name()]
This breaks a few tests, no surprise there. And then …
class Knowledge:
def new_direction(self):
possibles = [d for d in Direction.ALL if d != self.direction]
return choice(possibles)
Why did I construct the possibles that way? Because Python .remove doesn’t produce a new list, it removes from the list it is given, so we’d be removing directions from Direction.ALL. That leads to trouble very soon.
OK, why did we do all that? Oh, right, because we’d like to get rid of the direction accessors in Bot, if we can.
Now there are still references to Bot.direction, getter and setter, and they are all in bot tests.
Here are two tests that appear side by side in test_bot.py:
def test_change_direction_if_stuck(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(9, 5)
client_bot.direction_change_chance = 0.0
client_bot.do_something(connection)
assert client_bot.location == Location(10, 5)
assert client_bot.direction == Direction.EAST
client_bot.do_something(connection)
assert client_bot.location == Location(10, 5)
client_bot.do_something(connection)
assert client_bot.location != Location(10, 5)
world_bot = world.map.at_id(client_bot.id)
assert world_bot.direction != Direction.EAST
assert client_bot.direction != Direction.EAST
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"]
The second test arranges things so that the bot thinks it hasn’t moved, then checks to see that update requests a change.
The first test arranges things, drives the bot around until it is stuck then checks to see if it has changed direction.
I think that first test is mostly redundant. What does it check that the second one doesn’t? Well, it checks that the world accepted and executed the turn command, and that the information got back from the world.
I think we ought to be able to do better than that. However, I’m not prepared to sort that out right now. I’m in the middle of another quest, which was to clean up Knowledge.
Let’s do a stop-gap thing. Let’s rename the Bot.direction setters and getters to Bot.direction_test_only. That will both express that they’re not to be used in production, and they might be ugly enough to induce us to fix up the tests that use them.
- Added in Post
- Turns out that I forgot to do that, I got interested in the tests. Not sure if we’ll need to do that or not. I think we should, but it didn’t happen.
I do find one use of direction in Bot:
class Bot:
def forward_location(self):
return self.location + self.direction
We can forward that to Knowledge. But first, who’s using this? World is the only sender of this message. It cannot talk to us. We can remove the method.
Whoa! Nine tests break when we do that. Here’s one:
def test_take_a_block(self):
world = World(10, 10)
client_bot = DirectConnection(world).add_bot(5, 5)
world.add_block(6, 5)
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))
That test passes a client bot to take_forward in World. World doesn’t deal with client bots. This test should ensure that World deals with WorldEntity. I think this is the right formula:
def test_take_a_block(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
world.add_block(6, 5)
assert not world.is_empty(Location(6, 5))
world.command('take', client_bot.id)
connection.update_client(client_bot)
assert client_bot.has_block()
assert world.is_empty(Location(6, 5))
I think we can do better. DC should be able to help. Yes:
def test_take_a_block(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
world.add_block(6, 5)
assert not world.is_empty(Location(6, 5))
connection.take(client_bot)
assert client_bot.has_block()
assert world.is_empty(Location(6, 5))
OK, we know what we have to do to disconnect these tests from real Bots. However, there are still 8 of them to unwind, and I need a break before I do the rest. And I think I’ll spare you the tedium, though if I can break these down into smaller checks, I’ll report that. Let’s put the method back for now, and commit: improving tests to allow removing forward_location from Bot, which is only used erroneously in tests.
Let’s sum up. I need a chai, and probably a snack.
Summary
We’ve improved Knowledge a bit: it’s down to 79 lines from 109, and a quick glance makes me think that it’s close to as small as we’ll get. We may be able to remove the two vision accessors, and we’ll look at that next time. And I’d definitely like to protect it from people dropping random new variables into it. I’m not sure why I feel that way, but I think that it’s because we’d like the part that comes back from World to be immutable. Since we don’t pass those values to World, we should have no reason to change them.
Possibly what we should do is create a new Knowledge from the dictionary that comes back from World and never change it. But preventing storing new members won’t prohibit changing values that are already there. There may be a way to make it reasonably safe from mutation. We’ll explore that.
Chaos
The morning was a bit more chaotic than I expected, with my attention drawn off to Bot and even to World, as well as discovering some tests that are using the client-side objects in World code, which is right out, even though it used to be what we did.
We are suffering the same kind of pain that we would in a larger application that had been built up over a longer period of time. I have done a very good job of building legacy code and tests here: better than I actually set out to do.
Hill referred to “sloppy code”—actually he accused me of sometimes being a sloppy coder, which was really terribly judgmental on his part I’m sure you’ll agree—and it’s a fair cop: some of this code is how can I put this not as nice as it might be. And some of the tests are really only verifying things that could be—and therefore should be—verified in much smaller simpler less story-like tests.
This is the kind of thing that we see in real programs that have been around a while. Changes we need in one area have impact on tests that by rights would still run. Things that by rights should be removed have to be left in because some code somewhere, or worse, just a test somewhere relies on it. It takes time, and lots of small changes before we can get things right.
- Caveat
- Don’t get me wrong here. I’m not claiming that I made this mess on purpose so as to be able to write about it. No, most of the mess came about by virtue of two or three decisions that made sense at the time but were mistakes as seen from here. One was that I chose not to break apart the client-server notion. That one I did make knowing that it would lead to trouble.
-
But another was whatever led me to write those long involved intricate story tests. That wasn’t a choice aimed at learning a future lesson. That was just plain poor practice, probably induced by too much target fascination trying to get the Bots and World to do something. I tried to go too fast, not to teach a lesson, but because I am flawed.
Generally, we never get things right. Once things get out of whack, we may then be forever in the process of making them better, but never getting all the way to “right”. If there even is such a state.
But that’s the point. We are, as Hill puts it, “in the business of changing code”, and we try always to change it not just to have more capability but also to be better than we found it. If we don’t do that, it will inevitably be worse than we found it, and that way is definitely the Way of Pain. It’s also the Way of Defects.
Kind of a battle against entropy. We may never win, but we can push all the entropy into smaller and smaller corners, making most of our working area more pleasant than it would otherwise be.
So, yes, today turned chaotic. They usually do. We just try to keep it together long enough to leave things less chaotic than they were. And I think we managed that.
See you next time!