Cohesion
Bot is not as cohesive as it could be. I plan to change it. To make the change easy, a lot of small steps remove about 40 percent of the class. Nice, but it did make the article kind of long.
- Advice to Reader
- The following is rather long, because, after discussing the overall plan, we discover a long series of very small improvements that set us up better for what we think we want to do. So my suggestion is that you kind of skim along, noticing that most of the big chunks of code that you see are tests that are going to be removed, or just slightly tweaked.
- What to Look For
- All this is in aid of making Bot more cohesive. Its methods could be more cohesive, and so could its data. My thinking, just below, focused on the data. But by the time we’re done this morning, the methods contributing to lack of cohesion will all be gone. The data issue remains but is less troubling and certainly easier to think about.
- Make the Change Easy
- Kent Beck tells us that when faced with a difficult change, we can often make the change easy and then make the resulting easy change. What we do today is to clear the space around the change we think we want to make, the creation of a better separation between world info, client info, and between info that varies rapidly vs info that varies infrequently.
I’m concerned about cohesion.
I mentioned yesterday that our Bot class has at least two responsibilities, one as the creator of the action list for each turn, and one as a sort of inspection point for our tests. The object’s responsibilities are not cohesive; they do not hang together. Additionally, the second responsibility is questionable all on its own, because we’d rather not have to specialize our objects to support tests. They should be easily testable, but that should be an inherent result of good design, not something added on,
There is another concern around Bot that is troubling. Its supporting object, Knowledge is made up almost entirely of values that are updated by the World server, but there is one value, the energy, that is not a result from the World, but instead is a client-side value, and changes during the process of computing a new Bot state. This, too, is not cohesive, having many values from one source, with one set of timing characteristics—once per turn—and another member that is preserved across turns, and that changes at other times according to local rules.
Finally, if that were not enough, the Bot’s own member variables raise issues:
class Bot:
def __init__(self, x, y, direction=Direction.EAST):
self.name = 'R'
self.direction_change_chance = 0.2
self._knowledge = Knowledge(Location(x, y), direction)
self.state = Walking()
self._old_location = None
THe variables name, direction_change_chance, and _knowledge are assigned when the Bot is created and never change. (The contents of _knowledge may change, but the instance is persistent.)
The state member changes on every turn, based on the decisions of the Bot’s state machine, and _old_location changes on essentially every turn, on every turn where the Bot tries to take a step.
All these observations are clues, hints, suggestions, indications, or signals that the allocation of responsibilities may not be quite right. Generally speaking, things go better when things that change together live together. Things go better when objects have only one overall responsibility, not several.
Too much together;
Not enough separate.
These concerns, at least as I’m looking at them this morning, are all about cohesion, the notion that like things belong together and disparate things are best kept separated. When we can raise concerns like the ones we’ve just talked about, it suggests that our objects aren’t quite right. It often suggests that we need at least one new object, so that everything has its place.
Let’s see if we can identify places that might suggest objects.
- Bot static information, such as name and chance of changing direction. (Yes, we can imagine the latter changing, but right now it doesn’t.)
- Bot’s own changeable information, such as state, old location, its energy, and possibly its world-side knowledge.
- The world-side knowledge, which is updated on every turn.
Some design planning
So, what to do? Well, imagine this: Suppose that the Bot had two information-storing attributes, one for world-provided info, and one for its own purposes. From a theoretical viewpoint, we might see that as better, because it would keep related things together.
One implication of such a change would be that the state machine would need access to both of those objects. Presently, we only pass in one of them:
class Looking:
def action(self, knowledge):
if knowledge.can_take:
return ['take']
return []
def update(self, knowledge):
if knowledge.has_block:
knowledge.use_energy()
return Walking()
else:
return Looking()
There would be another possibility: we could pass in the Bot and let it forward to the subordinates. We can make a good case for this, because of the fact that we pushed the energy down into Knowledge, which is basically a hack to give the state machine access to it.
However. There is another option that we should consider, if only because we have all seen a lot of code that uses this idea:
Why not just keep everything in the Bot class?
Yep, we could do that, and we could do it “easily”. In essence, we’d just put all the members of Knowledge back up in Bot, we’d move its methods up, remove the Forwarders, and just have one big class that knows everything.
In our case, we’d have about a dozen members in Bot and maybe 180 lines of code counting white space. My brother GeePaw Hill is struggling with a class that has over 8000 lines in it, so this would be nothing. I’d wager that many of you know of classes that are far more broad than a dozen members and a couple of hundred lines.
The argument in favor of this idea is generally that it is “simpler”. I suppose it is simpler for Python to find an attribute in the same class as to find it in a separate one. But to be honest, I am not here to make Python’s job easier. Python is here to make my job easier, and I’d rather read fewer lines of code, not more.
I’m not even going to do it that way to show what it would look like. I’ve been around the block a few times, and I am quite confident that I prefer my classes small and cohesive. So we’ll go that way.
OK, well, how should we proceed?
One way would be to create two knowledge holders, one local and one from the world, and pass both to the state machine. But the state machine doesn’t care what’s local and what isn’t. So let’s not confuse it by making its job harder.
So we want one thing to pass into the state machine. But we want the Bot to have two knowledge holders. For the one from the world, we would really prefer that no client-side object has write access to it, and for the client-side one, we can allow reading and writing.
We could insert a special object on top of the local and world knowledge objects, so that the state machine only sees one thing … or we could pass the Bot to the machine. I am leaning toward the latter.
But before that … Why does Bot have the forwarders that it already has?
class Bot:
direction = Forwarder('_knowledge')
holding = Forwarder('_knowledge')
id = Forwarder('_knowledge')
location = Forwarder('_knowledge')
vision = Forwarder('_knowledge')
Bot refers to the location, so that it can check to see if it has managed to take a step, and if it hasn’t to change direction. Does it use direction, holding, id, or vision`?
Consider the code.
Slight change of focus.
Let’s not focus on users of the class. I think we’d do well to see whether we can simplify the existing code before building out any new objects. And let’s start from the code, not the forwarders. If we can actually remove some methods, we’re sure to make the subsequent problem easier. (See Make the Change Easy above.)
Bot has properties named x and y. Are they used? One test uses them, and I change it to use location:
def test_adding_via_surprise_knowledge(self):
cohort = Cohort()
WorldEntity.next_id = 100
new_bot = WorldEntity.bot(5, 6, Direction.EAST)
dict = new_bot.as_dictionary()
cohort.update([ dict ])
client_bot = cohort.bots[new_bot.id]
assert client_bot.location == Location(5, 6)
Commit: remove x and y from Bot.
- Worth Noting
- This was a near trivial change. Its value isn’t just in getting rid of two properties on the class, although that certainly has value. But the real value is that it got me started. I’ve moved from having a fairly grand idea, separating out the data and removing lots of methods, down to a very simple thing: remove this x right here; remove this y right here.
-
The transition to a focus on one small thing at a time has a fine result by the time we’re done.
What about this has method?
class Bot:
def has(self, entity):
return self._knowledge.has(entity)
Senders are all tests and I think most of them are testing the world entity. Remove the method, see what breaks.
Nothing breaks. That’s weird. Ah. The test is marked skip, as being too weird. Remove it. Commit: remove has method and weird test.
Was that too abrupt?
I don’t think so. We have lots of tests testing taking and dropping, so that losing this one does not concern me much at all. I admit that I did run the game to see it dropping things. Not surprised to see it works fine.
What’s next here? These two methods look suspicious.
class Bot:
def receive(self, entity_id: int):
self._knowledge.receive(entity_id)
def remove(self, entity):
self._knowledge.remove(entity)
I think we’ll find that these can only be used in tests, because the knowledge updates its held objects during the update call, and it does not call back to Bot to do its work.
I find all the senders of remove and they seem not to be talking to this method. Remove it and see. Green. Commit: remove ‘remove’.
How about receive? Again all the usages seem to be in world-related tests. Remove it and see. One fail. The test is this:
def test_bot_uses_knowledge_inventory(self):
bot = Bot(5, 5)
block = WorldEntity.block(3, 3)
bot.receive(block)
assert bot._knowledge.has_block
This just tests the receive method to see that it passes the block down to knowledge … there is literally no other use of the ‘receive’ method: it only exists to satisfy this test. Remove the test. Commit: remove useless test and ‘receive’ method.
There is only one more small questionable method in Bot:
def has_block(self):
return self._knowledge.has_block
It appears that there are two tests using this method. I’ll remove it to be sure. Yes:
def test_bot_cant_take_diagonally(self):
world = World(10, 10)
client_bot = DirectConnection(world).add_bot(5, 5)
world_bot = world.map.at_id(client_bot.id)
world.add_block(4, 4)
world.add_block(6, 4)
world.add_block(4, 6)
world.add_block(6, 6)
# world.add_block(6, 5) # uncomment to see test fail
world.take_forward(world_bot)
world.update_client_for_test(client_bot)
assert not client_bot.has_block()
def test_bot_drops_and_world_receives(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
cohort = Cohort(client_bot)
block_id = world.add_block(6, 5)
assert (block := world.map.at_xy(6, 5)) is not None
self.do_take(cohort, connection, client_bot)
assert client_bot.has_block()
assert world.map.at_xy(6, 5) is None
self.do_drop(cohort, connection, client_bot, block_id)
assert world.map.at_xy(6, 5) is block
These tests are both in the TestBot class. My guess is that it’s a historical artifact. Certainly the first test is testing the world, not the bot.
We might want a test for update, but we already have plenty of tests that won’t run if the update isn’t right. Here we can just check the world bot to see if it has the block or not. We revise:
def test_bot_drops_and_world_receives(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
cohort = Cohort(client_bot)
block_id = world.add_block(6, 5)
assert (block := world.map.at_xy(6, 5)) is not None
self.do_take(cohort, connection, client_bot)
world_bot = world.map.at_id(client_bot.id)
assert world_bot.has_block()
assert world.map.at_xy(6, 5) is None
That one is green. We’ll fix the other similarly:
def test_bot_cant_take_diagonally(self):
world = World(10, 10)
client_bot = DirectConnection(world).add_bot(5, 5)
world_bot = world.map.at_id(client_bot.id)
world.add_block(4, 4)
world.add_block(6, 4)
world.add_block(4, 6)
world.add_block(6, 6)
# world.add_block(6, 5) # uncomment to see test fail
world.take_forward(world_bot)
assert not world_bot.has_block()
That still fails. Why? Because:
class WorldEntity:
def has_block(self):
return self.holding.kind is EntityKind.BLOCK
holding can be None. Fix the method:
def has_block(self):
return self.holding and self.holding.kind is EntityKind.BLOCK
Green. Commit: remove ‘has_block’, improve related tests.
So, this is interesting. Looking at its methods, Bot now seems very cohesive, except for two Very Questionable Methods. See if you can spot them in the full class listing:
class Bot:
direction = Forwarder('_knowledge')
holding = Forwarder('_knowledge')
id = Forwarder('_knowledge')
location = Forwarder('_knowledge')
vision = Forwarder('_knowledge')
def __init__(self, x, y, direction=Direction.EAST):
self.name = 'R'
self.direction_change_chance = 0.2
self._knowledge = Knowledge(Location(x, y), direction)
self.state = Walking()
self._old_location = None
def change_direction(self):
return [self._knowledge.new_direction().name()]
def check_expectations(self):
if self.location == self._old_location:
return self.change_direction()
else:
return []
def do_something_only_for_tests(self, connection):
actions = self.get_actions()
self.perform_actions_only_for_tests(actions, connection)
return actions
def get_actions(self):
actions = self.get_intended_actions()
self.record_expectations(actions)
return actions
def get_intended_actions(self):
actions = []
actions += self.check_expectations()
actions += self.updated_state_action()
actions += self.possibly_change_direction()
actions += ['step']
return actions
def possibly_change_direction(self):
if random.random() < self.direction_change_chance:
return self.change_direction()
else:
return []
def record_expectations(self, actions):
if 'step' in actions:
self._old_location = self.location
def perform_actions_only_for_tests(self, actions, connection):
cohort = Cohort(self)
for action in actions:
match action:
case 'drop':
rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
case 'step' | 'take' | 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as verb:
rq = [ {'entity': self.id, 'verb': verb} ]
case _:
assert 0, f'no case {action}'
connection.run_request(cohort, rq)
def update(self, result_dict):
self._knowledge.update(result_dict)
def updated_state_action(self):
self.state = self.state.update(self._knowledge)
return self.state.action(self._knowledge)
The questionable methods are of course do_something_only_for_tests and perform_actions_only_for_tests.
def do_something_only_for_tests(self, connection):
actions = self.get_actions()
self.perform_actions_only_for_tests(actions, connection)
return actions
def perform_actions_only_for_tests(self, actions, connection):
cohort = Cohort(self)
for action in actions:
match action:
case 'drop':
rq = [ { 'entity': self.id, 'verb': 'drop', 'holding': self.holding}]
case 'step' | 'take' | 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as verb:
rq = [ {'entity': self.id, 'verb': verb} ]
case _:
assert 0, f'no case {action}'
connection.run_request(cohort, rq)
The perform part is a hacked-together replica of code in Cohort and code in DirectConnection:
class Cohort:
def create_action(self, bot, verb):
match verb:
case 'drop':
return {'entity': bot.id, 'verb': verb, 'holding': bot.holding}
case _:
return {'entity': bot.id, 'verb': verb}
class DirectConnection:
def run_request(self, cohort, rq):
results = self.world.execute(rq)
cohort.update(results)
- Note
- I almost stopped here, but then I thought maybe there’d be something simple that I could fix up. And there was.
I think these will need fresh eyes to sort out. Let’s see, however, if we can find a simple example. Maybe it’s easy.
def test_wandering(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(5, 5)
client_bot.do_something_only_for_tests(connection)
loc = client_bot.location
assert loc != Location(5, 5)
OK, this one is just really dumb. It tests to see whether the bot took a step.
We can test that by checking the request.
def test_wandering_better(self):
bot = Bot(5, 5)
actions = bot.get_actions()
assert 'step' in actions
Remove the prior wandering test, keep this one. Here are the next two tests in the file:
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_only_for_tests(connection)
assert client_bot.location == Location(10, 5)
assert client_bot.direction == Direction.EAST
client_bot.do_something_only_for_tests(connection)
assert client_bot.location == Location(10, 5)
client_bot.do_something_only_for_tests(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.check_expectations()
assert actions[0] in ["NORTH", "SOUTH", "WEST"]
The first one tests less than the second. It may have been a scaffolding test, with the second, better one, coming along next. Remove the first one.
Here’s another sender of our weird methods:
def test_stop_at_edge(self):
world = World(10, 10)
connection = DirectConnection(world)
client_bot = connection.add_bot(9, 5)
client_bot.direction_change_chance = 0
actions = client_bot.do_something_only_for_tests(connection)
assert actions == ['step']
assert client_bot.location == Location(10, 5)
actions = client_bot.do_something_only_for_tests(connection)
assert actions == ['step']
assert client_bot.location == Location(10, 5)
actions = client_bot.do_something_only_for_tests(connection)
assert actions[0] in ["NORTH", "SOUTH", "EAST", "WEST"]
assert actions[1] == 'step'
assert client_bot.location != Location(10, 5)
These are, of course, “story tests”. They are hard to set up, exercise way too much code, and, in the current case, are hardly testing anything that is part of the actual system.
What we want to know is whether, if you’re up against the east wall and try to step, you’ll issue a direction change next time around. But we already know that you’ll change direction if you’re stuck: we have that tiny test up above. So what we really want to know is whether your location will change if you try to step off the world. How does World do this?
class World:
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)
class Map:
def attempt_move(self, id, location: Location):
entity = self.contents[id]
if self.location_is_open(location):
entity.location = location
def location_is_open(self, location: Location):
return self.is_within_map(location) and not self.is_occupied(location)
def is_within_map(self, location):
return 0 <= location.x <= self.width and 0 <= location.y <= self.height
We see that we’ll not move if the location is not open and that an off-world location is not open. Do we have a test for that logic in Map?
We have some map tests but as it happens, none for the edges. I’ll be satisfied if we check the off-world locations for being open. I write this tiny new test:
def test_map_off_world_not_open(self):
map = Map(10,10)
for x,y in ((-1, 5), (5, -1), (11, 5), (5, 11)):
location = Location(x, y)
assert not map.location_is_open(location)
That tests all four sides for not being open. The other test is now redundant and also duplicates this test’s actual effect, while going around the horn to do so. Remove it. Green: Commit: replace huge test with microtest.
There are only two more tests that use those methods, one for scent, one for vision. Here’s scent:
def test_we_get_scent(self):
world = World(10, 10)
world.add_block(5, 5) # comment?
client_bot = DirectConnection(world).add_bot(4, 6)
client_bot.direction_change_chance = 0.0
client_bot.do_something_only_for_tests(DirectConnection(world))
assert client_bot._knowledge._scent == 3
Is this trying to check whether scent is computed correctly, or whether we pass it over to the client? The test will only run if both are true.
We have a comprehensive test that assesses whether we compute scent correctly:
def test_scent_all_around(self):
map = Map(10, 10)
for x in range(11):
for y in range(11):
block = WorldEntity.block(x, y, 2)
map.place(block)
scent = map.scent_at(Location(5, 5), 2)
assert scent == 40
Do we have a test that asserts that scent goes into the info packet from the entity?
class World:
def fetch(self, entity_id):
return self.entity_from_id(entity_id).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}
def test_set_and_fetch(self):
# not much of a test really
entity = WorldEntity.bot(0, 0, Direction.EAST)
assert entity.direction == Direction.EAST
entity.location = Location(6, 4)
assert entity.location == Location(6,4)
block = WorldEntity.block(9, 9, 0)
entity.receive(block)
assert entity.holding == block
entity.scent = 37
assert entity.scent == 37
entity.vision = []
assert entity.vision == []
assert self.is_valid(entity)
fetched = entity.as_dictionary()
assert fetched['eid'] == entity.id
assert fetched['direction'] == Direction.EAST
assert fetched['held_entity'] == block.id
assert fetched['location'] == Location(6, 4)
assert fetched['scent'] == 37
assert fetched['vision'] == []
That giant test only assures us that scent is in the dictionary the world fetches. Is scent stored into the Knowledge on the other side?
class Bot:
def update(self, result_dict):
self._knowledge.update(result_dict)
class Knowledge:
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']
Is that tested? It is not. Does it work? Obviously. But if we are to remove that last test that calls the weird test-only methods, we should write a real test for the update.
def test_knowledge_update(self):
bot = WorldEntity.bot(5, 5, Direction.NORTH)
block = WorldEntity.block(3, 3)
bot.holding = block
bot.scent = 234
bot.vision = [('R', 5, 5)]
d = bot.as_dictionary()
k = Knowledge(bot.location, bot.direction)
k.update(d)
assert k._scent == 234
assert k.vision.vision_list == bot.vision
This test now covers the concerns of the more complex test for scent. Down to one test that uses the only_for_test methods:
def test_nothing_near(self):
world = World(10, 10)
client_bot = DirectConnection(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.do_something_only_for_tests(DirectConnection(world))
world.update_client_for_test(client_bot)
assert client_bot.location == Location(6, 5)
vision = client_bot.vision
assert ('R', 6, 5) in vision
All this just to check that getting the vision works? And we already have this test:
def test_three_blocks_near(self):
world = World(10, 10)
world.add_block(4, 4)
world.add_block(6, 6)
world.add_block(4, 5)
DirectConnection(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
That’s more robust and more clear than the other. Remove the other. Green. Remove the only_for_tests methods from Bot. Green. Commit: adjust and improve tests, remove only_for_tests methods from Bot.
I’m in for 2.5 hours, with a surprising outcome. Perfect. Let’s sum up.
Summary
I really thought that I’d refactor Bot one way or another to make its methods and its attributes more cohesive. I was expecting to adjust its knowledge and to add another object to deal with the attributes that change a lot and the ones that do not and the ones from world and the local ones.
To prepare the ground, we started looking at methods that seemed not to relate to Bot’s primary job, creating the actions for the next turn, and dealing with the results. One by one, we found that the methods were only used in tests, and we found ways to improve the tests or replace them. One by one, the methods dissolved. None of the changes was particularly profound, but Bot is now down to a svelte 60ish lines from around 100.
And we have not even dealt with the member variables yet.
Let’s do look at the forwarders however:
class Bot:
direction = Forwarder('_knowledge')
holding = Forwarder('_knowledge')
id = Forwarder('_knowledge')
location = Forwarder('_knowledge')
vision = Forwarder('_knowledge')
I wonder how many of those are used.
class Bot:
# direction = Forwarder('_knowledge')
# holding = Forwarder('_knowledge')
id = Forwarder('_knowledge')
location = Forwarder('_knowledge')
# vision = Forwarder('_knowledge')
I tried commenting those lines out. Three of the five are no longer accessed. Fine, remove them.
The Bot uses location itself:
def check_expectations(self):
if self.location == self._old_location:
return self.change_direction()
else:
return []
def record_expectations(self, actions):
if 'step' in actions:
self._old_location = self.location
So that’s certainly legitimate. What about id? Bot itself does not use it, but Cohort stores Bots by their id, and Bots are of course identified over in World based on the id which World has assigned. So that one is pretty legitimate. there are some tests that access it as well, and we might want to look at that but since the member makes sense on the production side, we don’t mind the tests looking at it.
- Admission
- I freely grant that I am relieved that there are two legitimate Forwarders left. If we were down to one it would seriously call into question whether we need the class, especially in view of its somewhat high cleverness factor. But two is enough to justify it, I think. The properties corresponding to a forwarder take up at least six lines counting white space, and perhaps more. Think of the savings!
-
No, not really, but they do communicate better. And I think we might wind up with a couple more. We’ll see.
We’re making the change easy, and along the way we removed an entire responsibility from Bot, its specialized methods that really only supported testing.
I was not expecting this sequence of small changes.
Coming in this morning, I wasn’t aware that I could improve the code so much by small changes, one method and one test at a time. But there it was, unfolding before me. Each change just led to the next. Clip clop, I walked along the road, and now we’re in a better place.
I love it when a plan takes me somewhere I didn’t expect to go. At least when it takes me to a good place. I’m not so into plans that take me to a bad place. Call me fickle.
A pleasant morning. I apologize for the length of this, but my articles reflect what actually happens and sometimes we skim through a lot of code and a lot of little thoughts. The much cleaner Bot class will be that much easier to refactor if we decide to go ahead with the information-related changes. But the real lesson, if there is one, is that we made a substantial improvement with no confusion, no difficulty, just a lot of very small and easy changes.
This is the way. See you next time!