Message Pool
It would be nice if we didn’t have to duplicate message strings in the code and tests. We’re not going to internationalize, but something better than copying the strings would be good.
Here’s an example of the problem:
def execute_action(self, verb=None, bot=None, **details):
if bot or verb == 'add_bot':
match verb:
case 'add_bot': self.add_bot_using(**details)
case 'step': self.step(bot)
case 'drop': self.drop_using(bot, **details)
case 'take': self.take_forward(bot)
case 'turn': self.turn_using(bot, **details)
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
self.turn(bot, direction)
case _:
self._add_message(f'Unknown action {verb=} {details=}')
else:
self._add_message(f'verb {verb} requires bot_key parameter {details}')
def test_zero_id(self):
WorldEntity.next_key = 100
world = World(10, 10)
command = {'bot_key': 0,
'verb': 'step'}
rq = [ command ]
messages = world.execute_requests(rq)['messages']
assert 'requires bot_key parameter' in messages[0]['message']
As written, the test doesn’t check the full string, which might be OK, but if we decide to change the message text, the test will fail.
What would we like to have? Well, naively, some pool of messages with names, either as declared variables or a table, so that we could say something like this:
def execute_action(self, verb=None, bot=None, **details):
if bot or verb == 'add_bot':
match verb:
...
else:
self._add_message('BOT_KEY_NEEDED', verb, details)
def test_zero_id(self):
WorldEntity.next_key = 100
world = World(10, 10)
command = {'bot_key': 0,
'verb': 'step'}
rq = [ command ]
messages = world.execute_requests(rq)['messages']
assert match_message(messages, 'BOT_KEY_NEEDED')
Where the match_message function is magically able to compare the message in the message list with the official BOT_KEY_NEEDED message.
- Tidying
- I noticed that the
execute_actionmethod would look better with the short line at the beginning like an escape clause. Invert the condition:
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_message(f'verb {verb} requires bot_key parameter {details}')
else:
match verb:
case 'add_bot': self.add_bot_using(**details)
case 'step': self.step(bot)
case 'drop': self.drop_using(bot, **details)
case 'take': self.take_forward(bot)
case 'turn': self.turn_using(bot, **details)
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
self.turn(bot, direction)
case _:
self._add_message(f'Unknown action {verb=} {details=}')
Better. Commit: tidying.
How would you feel about extracting the else clause, in the spirit of Composed Method?
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_message(f'verb {verb} requires bot_key parameter {details}')
else:
self.execute_verb(verb, bot, details)
def execute_verb(self, verb, bot, details):
match verb:
case 'add_bot':
self.add_bot_using(**details)
case 'step':
self.step(bot)
case 'drop':
self.drop_using(bot, **details)
case 'take':
self.take_forward(bot)
case 'turn':
self.turn_using(bot, **details)
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
self.turn(bot, direction)
case _:
self._add_message(f'Unknown action {verb=} {details=}')
That’s better by my lights, and them’s the only lights we have here chez Ron. Commit: tidying, extract method.
- Note
- Using a powerful IDE like PyCharm makes changes like those above nearly trivial. It inverted the condition for me. It extracted the method, requiring me to think of a name and decide the order of the parameters if I didn’t like its choice. Bada bing bada boom, nothing to it. This makes it much easier to keep the code alive, because when we notice a small issue, it’s a small operation to fix it up.
But messages. The thing is this: for messages with values interpolated in, Python f-strings are super nice. These lines:
self._add_message(f'turn has unknown direction {direction}, '
f'should be NORTH, EAST, SOUTH, or WEST')
self._add_message(f'Unknown action {verb=} {details=}')
produces messages like this:
unknown direction LEFT, should be NORTH, EAST, SOUTH, or WEST
Unknown action verb='whatever' details={'x': 10, 'direction': 'easy'}
As in the second example above, some of our messages include a dictionary, showing the values of parameters sent to the particular verb. I’m not sure how valuable that is: the dictionary is often empty, but it would be nice to provide for the case.
It turns out that an f-string’s interpolations rely on the string being defined in the context of the variables passed in. We can’t have an f-string external to what we want to plug in. However, Python’s str.format can do the job this way:
def test_interpolation(self):
d = { 'x': 10, 'direction': 'EAST'}
s = 'message {verb} does not like {direction}'
s1 = s.format(verb = 'turn', direction = 'RIGHT')
assert s1 == 'message turn does not like RIGHT'
s2 = s.format(verb = 'turn', direction = d)
assert s2 == "message turn does not like {'x': 10, 'direction': 'EAST'}"
I wonder whether we can make a variable-length message method to do the job for us. I’ll write a test.
def test_varargs(self):
s = 'message {verb} does not like {direction}'
def message(msg_name, **kwargs):
prototype = s if msg_name == 's' else None
return prototype.format(**kwargs)
d = { 'x': 10, 'direction': 'EAST'}
m = message('s', verb = 'turn', direction = d, unexpected = "boo!")
assert m == "message turn does not like {'x': 10, 'direction': 'EAST'}"
This is a bit more elaborate than needed to test the idea but I wanted to at least sketch how I think the thing might work. The assertion there shows that we can plug in the verb and the direction dictionary, and that unexpected parameters do not disturb format.
We can live with this. Commit this new test: new test for interpolation
Reflect: How should we proceed?
I think we can refactor this capability in incrementally, one way or another. But how can we best check a message for being the right one? How about this: we’ll have some kind of key for each message, that’s the whole point of the idea. So when we display a message … oh, heck, let me type one in and show you what I mean:
def turn_using(self, bot, direction=None, **ignored):
match direction:
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
self.turn(bot, direction)
case _:
self._add_keyed_message("TURN_DIRECTION", direction=direction)
We’ll have a new method, for now, _add_keyed_message. We expect to pass this test:
def test_invalid_turn_message(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5)
requests = [{'bot_key': bot_id, 'verb': 'turn', 'direction': 'LEFT'}]
messages = world.execute_requests(requests)['messages']
assert 'TURN_DIRECTION:' in messages[0]['message']
Note that all we check in this test is the key of the message. And we implement thus:
def _add_keyed_message(self, key, **kwargs):
msg = self._get_message(key)
formatted = msg.format(**kwargs)
self._add_message(formatted)
def _get_message(self, key):
return key + ": " + self.error_messages.get(key, 'unknown message')
And we have a dictionary of error messages:
class World:
error_messages = {
'TURN_DIRECTION':
'unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST',
}
The test passes. Commit: first use of keyed error messages.
Reflection
I think the main concern we’ll have is in the code to add a message to the list, and the code to do that isn’t much worse than we used to have:
self._add_keyed_message("TURN_DIRECTION", direction=direction)
Instead of:
self._add_message(f'turn has unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST')
I’ve had a Clever Idea. Let me express it here to get it out of my system.
The process of creating a new error message has steps something like this:
- Decide what the message should say;
- Decide what terms will be plugged in;
- Create the string with interpolated
{term}indicators; - Make up a key name for the message.
- Add the name and message to the
error_messagesdictionary; - Type in the
_add_keyed_messagestatement.
The Clever Idea is that if the key is not found in the list, the default message should be whatever was passed in.
Like this:
def _get_message(self, key):
return key + ": " + self.error_messages.get(key, key)
Now let me rename some methods here. This is getting too cute but trust me.
def _add_message(self, msg):
message_dict = { 'message': msg}
self.messages.append(message_dict)
That becomes this:
def _add_completed_message(self, msg):
message_dict = { 'message': msg}
self.messages.append(message_dict)
I’m changing direction in mid-flight here. Let’s fix up another message given what we have here. Here’s one that needs a parameter:
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_completed_message(f'verb {verb} requires bot_key parameter {details}')
else:
self.execute_verb(verb, bot, details)
Now we change this to refer to the other method, and we remove the f.
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_keyed_message('verb {verb} requires bot_key parameter {details}')
else:
self.execute_verb(verb, bot, details)
Tests fail. I don’t check: I think I know what I’m doing. I may be wrong. Provide the values:
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_keyed_message(
'verb {verb} requires bot_key parameter {details}',
verb=verb, details=details)
else:
self.execute_verb(verb, bot, details)
Tests pass, because the message serves as its own key. We make up a key and move the string to the error messages:
class World:
error_messages = {
'TURN_DIRECTION':
'unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST',
'VERB_NEEDS_BOT_KEY':
'verb {verb} requires bot_key parameter {details}'
}
def execute_action(self, verb=None, bot=None, **details):
if not bot and verb != 'add_bot':
self._add_keyed_message(
`VERB_NEEDS_BOT_KEY`,
verb=verb, details=details)
else:
self.execute_verb(verb, bot, details)
The tests pass. They are not checking the key, however, they are checking the string. We can change them:
def test_no_entity(self):
world = World(10, 10)
requests = [ {'verb': 'step'}]
messages = world.execute_requests(requests)['messages']
assert 'VERB_NEEDS_BOT_KEY' in messages[0]['message']
My clever idea was that I could edit the things in place, in small steps, and that allowing the message to serve as its own key might be a good thing. I think I was chasing my own tail: it wasn’t particularly smooth. Anyway, it’s not very hard, however we do it.
Summary
I’m ready to stop and we can certainly commit this: add another message to the error_messages scheme.
I think we have a basic scheme for messages that have a key name, allowing them to be stored in a dictionary, and that include the key name as part of the message so that our tests can work independently of the specific string value of the message. This would even allow for internationalizing our messages if we were into that sort of thing.
Is there a lesson for me in this? I think so. I created more than one error message after becoming aware that the issue of testing them was fraught, since tests would require updating whenever a message was improved. Even doing them the first time was tricky, since we either had to replicate the exact contents, which is at best tedious, or compare on a subset. It required more thinking than it should, making it less likely that I’d write the test.
There are now five remaining messages that need to be updated. I’ll probably do those behind the curtain, since they are just the same as what we’ve done. Had I moved sooner to the keyed message idea, I might have saved a bit of tedium converting those five. That said, until I experimented with string.format, I didn’t quite know how I’d do the thing. I had not used string.format, having moved directly from the fixed-format kinds of strings to f-strings, so I didn’t know that it was so close to what we already were doing.
I’m not here to “should” on myself, but I do think I’d do well to be a bit more sensitive to the quality of what I’m doing. I do like the fact that I can always find something that needs improvement, so that I can enjoy improving it and write about it, but I feel that I’m leaving things less nice than I might. It’s to my own detriment, because cruft slows us down, and with a little more care now we can go faster later on. So we will continue to defer design decisions as long as is reasonable, but we’ll try not to let the lack of a decision produce reams (or fives) of code that needs improvement.
Teams rarely do too much code improvement.
Teams have often asked me “how do we know we’re not doing too much code improvement?” I always smile and say that I’ve never encountered a team that did too much code improvement, so I don’t know. I would go on to say that one’s management and product owners will keep the pressure on, and that so long as we avoid trying to get permission for a “big refactoring” and do everything in tiny steps, we’ll probably be OK.
I probably do too much, but what we do here is special.
Here, on these pages, I probably do do too much code improvement. That’s because one of our fundamental topics is to show code that needs improvement and how to improve it. No one really cares how long it takes to get the Bot World to have a new feature. In your world, I’m pretty sure you get more than enough pressure to do more features, so your best move is probably to focus on features, with good code, with small improvements as you spot them, and to recognize when it’s time to refactor a bit to make room for things.
The danger, on a product team, is to try to go too fast. That will build up cruft, the cruft will cause friction, the friction will slow us down, and soon it’s very difficult to dig back out. We have to find that safe balance. My own experience is that most times most teams don’t do enough cleaning, but each team is different and needs to sort it out for themselves. I’d offer to come in and observe and tell you what I see, but that would require me to leave my house and probably get on an airplane, and those days are gone.
But feel free to write: I answer toots and email.
See you next time!