Strangler?
Let’s try that possibly better idea. I think we can do it step by step, in a strangler kind of style. I think it’s going to work out!
I think it was Michael Feathers who coined the term Strangler Vine for the practice of doing a large design change by moving small bits of code over to the new scheme one at a time, essentially strangling out the old code bit by bit. Anyway, that’s what we’ll try to do.
We’re starting with this:
def execute_action(self, action_dictionary):
match action_dictionary:
case {'verb': 'add_bot',
'x': x, 'y': y, 'direction': direction}:
self.add_bot_action(x, y, direction)
case {'verb': 'drop',
'entity_object': entity_object,
'holding': holding}:
self.drop_forward_action(entity_object, holding)
case {'verb': 'step',
'entity_object': entity_object}:
self.step_action(entity_object)
case {'verb': 'take',
'entity_object': entity_object}:
self.take_forward_action(entity_object)
case {'verb': 'turn',
'entity_object': entity_object,
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn',
'direction': bad_direction}:
self._add_message(f'unknown direction {bad_direction}, should be NORTH, EAST, SOUTH, or WEST')
case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction,
'entity_object': entity_object}:
self.turn_action(entity_object, direction)
case { 'verb': verb } if verb not in self.all_verbs:
self._add_message(f'unknown verb: {verb}')
case { 'verb': verb} if verb not in ['add_bot',]:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case _:
self._add_message(f'Unknown action {action_dictionary}')
And the idea is to get to something somewhat like this:
def execute_action(self, action_dictionary):
verb = action_dictionary.get('verb')
match verb:
case 'add_bot': self.add_bot(**action_dictionary)
case 'drop': self.drop(**action_dictionary)
case 'step': self.step(**action_dictionary)
...
case _: self._add_message('Unknown verb {verb}')
I’m hopeful that this will lead to something better than the Wall Of Code that we see above. It should at least be a smaller wall.
So. How to do the strangling? We’ll probably want something like this:
def execute_action(self, action_dictionary):
if not self.new_way(action_dictionary):
self.old_way(action_dictionary)
Where old_way is what we have now, and we’ll have to figure out some easy way to return True if we handled the action, and False otherwise. I think we can probably do that. After everything is moved over—or if we decide to have two ways of doing things—we’ll probably have some final renaming and cleanup to do. It’s too soon to tell.
We’ll start out thinking this is an experiment but frankly I figure I’ll start committing as soon as it looks like it’s going to work. I might even select difficult ones to do, if there are any.
Let’s first make the split.
def execute_action(self, action_dictionary):
self.execute_action_old(action_dictionary)
def execute_action_old(self, action_dictionary):
match action_dictionary:
case {'verb': 'add_bot',
'x': x, 'y': y, 'direction': direction}:
self.add_bot_action(x, y, direction)
To no one’s surprise, we’re still green. We could commit if we wanted to. We’ll hold off a bit.
Add a bit:
def execute_action(self, action_dictionary):
if not self.execute_action_new(action_dictionary):
self.execute_action_old(action_dictionary)
def execute_action_new(self, action_dictionary):
return False
def execute_action_old(self, action_dictionary):
match action_dictionary:
case {'verb': 'add_bot',
'x': x, 'y': y, 'direction': direction}:
self.add_bot_action(x, y, direction)
Our strangler is set up. Let’s try something easy. Not ‘add_bot’. How about ‘step’?
def execute_action(self, action_dictionary):
if not self.execute_action_new(**action_dictionary):
self.execute_action_old(action_dictionary)
def execute_action_new(self, verb=None, **action_dictionary):
match verb:
case 'step':
self.action_step(**action_dictionary)
case _:
return False
return True
def action_step(self, entity_object=None, **action_dictionary):
if entity_object:
self.step_action(entity_object)
else:
self._add_message(f'verb step requires entity parameter {action_dictionary}')
This is running green. I foresee that we’ll have to duplicate the ‘requires entity parameter’ message a lot but there will probably be ways to deal with that. Does the game work? It does, with a couple of changes that tell me I’ve not run it for a while. It needs to refer to the map to get width and height. Commit that.
OK, let’s think about what we have here.
Reflection
I think it’s clearly going to work. So what can we do about the need everyone will have to check entity object? Let’s assume for now that everyone needs one and worry about ‘add_bot’ separately. So we can do a check at the beginning.
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if not entity_object:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
match verb:
case 'step':
self.step_action(entity_object)
case _:
return False
return True
One test fails.
def test_invalid_turn_message(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5)
requests = [{'verb': 'turn', 'direction': 'LEFT'}]
messages = world.execute_requests(requests)['messages']
expected = 'unknown direction LEFT, should be NORTH, EAST, SOUTH, or WEST'
assert expected in messages[0]['message']
This one is getting the complaint about entity object and in fact it should. The test is mistaken: I left the entity object out.
def test_invalid_turn_message(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5)
requests = [{'entity': bot_id, 'verb': 'turn', 'direction': 'LEFT'}]
messages = world.execute_requests(requests)['messages']
expected = 'unknown direction LEFT, should be NORTH, EAST, SOUTH, or WEST'
assert expected in messages[0]['message']
Green now. I think we’re going to be OK here, let’s press on.
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if not entity_object:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
match verb:
case 'step':
self.step_action(entity_object)
case 'drop':
self.action_drop(entity_object, **action_dictionary)
case _:
return False
return True
def action_drop(self, entity_object, holding=None, **action_dictionary):
self.drop_forward_action(entity_object, holding)
def drop_forward_action(self, bot, holding):
self.drop_forward(bot, self.entity_from_id(holding))
def drop_forward(self, bot, held_entity):
if self.map.place_at(held_entity, bot.forward_location()):
bot.remove(held_entity)
else:
self._add_bot_message(bot, 'drop location was not open')
Clearly we need a naming convention here. action_drop and drop_forward_action and drop_forward aren’t going to cut it as clear names. Best think about that soon. But this one works as well, so the basic plan is working out just fine.
Let’s reformat the main method and see how it shapes up:
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if not entity_object:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
else:
match verb:
case 'step': self.step_action(entity_object)
case 'drop': self.action_drop(entity_object, **action_dictionary)
case _: return False
return True
Might be OK. What if we invert the if?
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if entity_object:
match verb:
case 'step': self.step_action(entity_object)
case 'drop': self.action_drop(entity_object, **action_dictionary)
case _: return False
return True
else:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
I think we’ll find that’s better, after a couple of extracts. We’ll let it ride for now. Let’s do one more.
Ah, there’s an issue. Take ‘turn’ for example. It requires a direction, ‘NORTH’, ‘EAST’, and so on. If we send it ‘WHATEVER’, we need an error. To return an error, we need to return FALSE from execute_action_new. So whatever we do in case: 'turn' we need a way to signal happiness or sadness. We could make them all return T/F, or we could just have the ones that can fail do it, like this, perhaps:
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if entity_object:
result = True
match verb:
case 'step': self.step_action(entity_object)
case 'drop': self.action_drop(entity_object, **action_dictionary)
case 'turn': result = self.action_turn(entity_object, **action_dictionary)
case _: result = False
return result
else:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
That will do the job, but I’m not sure I love it. Let’s see about finishing ‘turn’.
def execute_action_new(self, verb=None, entity_object=None, **action_dictionary):
if entity_object:
result = True
match verb:
case 'step': self.step_action(entity_object)
case 'drop': self.action_drop(entity_object, **action_dictionary)
case 'turn': result = self.action_turn(entity_object, **action_dictionary)
case _: result = False
return result
else:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
return False
def action_turn(self, entity_object, direction=None, **action_dictionary):
match direction:
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
self.turn_action(entity_object, direction)
return True
case _:
self._add_message(f'unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST')
return False
def turn_action(self, world_bot, direction_name):
# no change on unrecognized name
if direction_name in ['NORTH', 'EAST', 'SOUTH', 'WEST']:
world_bot.direction = Direction.from_name(direction_name)
We probably don’t need that final check but it’s harmless and safer.
We are green with this code. I think we’re going to prefer it, and also think it’s going to need some refactoring to make the called methods more regular and better named. But I think it’s going to work. So we’ll commit: Strangling execute_action to more compact verb-first format. Looks good so far.
Summary
I think we might have to do something special with the ‘add_bot’ message, but worst case we can have two execute methods, one for commands that need no entity, of which we just have the one, and all the rest. We’ll see. It might remain as a small bump on the code but with any luck at all we can smooth it right in.
I predict that some renaming will help a lot. It seems that there are up to three levels we go through, one to pull out additional parameters, needed by commands like ‘turn’ and ‘drop’, One to translate a value, like for ‘drop’, and one to actually do the work. We may find that we want to refactor them differently. We can always inline them up to one method and then refactor. Or maybe we’ll see a decent pattern.
We could also pull out the parameters directly rather than using the dictionary unpacking that a call gives us. That would be more explicit but I think the code would be messier and more nested. We might try it just to see, or we might go with my intuition.
Overall … I do think this will be better. We’ll continue next time: see you then!