Details

The Repo on GitHub

Let’s review our new action handling and see what needs improvement. We rename around a dozen or fewer things. Was it worth it? Toot me up and tell me.

Our new execute_action method is far simpler than the previous one. I think it looks pretty good, but there is at least one issue there that concerns me. Relatedly, the methods called have somewhat strange and inconsistent names. I think we can do better. We’ll start with execute_action:

    def execute_action(self, verb=None, entity_object=None, **action_dictionary):
        if entity_object or verb == 'add_bot':
            match verb:
                case 'add_bot': self.action_add_bot(**action_dictionary)
                case 'step': self.step_action(entity_object)
                case 'drop': self.action_drop(entity_object, **action_dictionary)
                case 'take': self.take_forward_action(entity_object)
                case 'turn': self.action_turn(entity_object, **action_dictionary)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    self.turn_action(entity_object, direction)
                case _:
                    self._add_message(f'Unknown action {verb=} {action_dictionary=}')
        else:
            self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')

Take a moment. What do you see here? What raises questions?

OK. Here’s my list of observations:

1. Some calls get **action_dictionary as a parameter and some do not.
2. Some methods are named action_something and some are named something action.
3. In the case of “turn”, we see both versions.
4. The verbs NORTH, EAST, etc., are handled a bit differently from everything else.

We can “explain” the situation to some degree:

1. Some verbs require additional information, and some do not. “step” does not, but “drop” needs to be told what to drop. The ones that need more information are called with **action_dictionary.

2. The methods that need additional information from **adciont_dictionary are the ones named action_something and the others are named something_action

3. The “turn” calls follow this rule, the action_turn needs to get the turn direction and the turn_action receives it.

4. This could be written out longhand, one verb per line but this code might be preferable.

Now let’s think about naming. What was the author trying to say with the names like action_turn and turn_action? Probably one concern was that it should be clear that all these methods are part of doing actions. But the whole class, World, is about handling requests, which are lists of actions. So we probably could remove the action prefix and suffix and be just as well off.

I’m a bit concerned about the name action_dictionary. It is a dictionary, but we don’t usually go around calling things by their data type. What this dictionary is is everything we know about one action. When execute_action is called, the thing contains all the key-value pairs of the original action request, plus possibly the ‘entity_object’, which is the WorldEntity to which the action applies.

Inside execute_action, we have stripped out the ‘verb’ and ‘entity_object’, so that action_dictionary contains all the rest of the parameters of the desired action, if there are any. Maybe it should be called remaining_parameters or additional_parameters_if_any. Maybe it’s additional_details or further_info. Or just action_details.

Why are we even struggling with this?

We are struggling with this now, because right now we know as much about this code as we ever have, and quite like more than the next person to read it will know—even if that person is ourselves. At this moment, we are best equipped to craft this code so that when it comes time to change it, the team has the best possible chance of changing it rapidly and correctly.

And, we are struggling with it now because the knife of our mind is not as sharp as it could be and we want to sharpen it, so that every naming decision we make will be just a bit more precise.

I’m going to rename that parameter details. The fact that it relates to action is pretty clear.

    def execute_action(self, verb=None, entity_object=None, **details):
        if entity_object or verb == 'add_bot':
            match verb:
                case 'add_bot': self.action_add_bot(**details)
                case 'step': self.step_action(entity_object)
                case 'drop': self.action_drop(entity_object, **details)
                case 'take': self.take_forward_action(entity_object)
                case 'turn': self.action_turn(entity_object, **details)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    self.turn_action(entity_object, direction)
                case _:
                    self._add_message(f'Unknown action {verb=} {details=}')
        else:
            self._add_message(f'verb {verb} requires entity parameter {details}')

Now let’s look at the turn case, because we have them both. I’ll format for a better comparison.

case 'turn': 
    self.action_turn(entity_object, **details)
case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
    self.turn_action(entity_object, direction)

I think we’ll look also at what action_turn does:

    def action_turn(self, entity_object, direction=None, **details):
        match direction:
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
                self.turn_action(entity_object, direction)
            case _:
                self._add_message(f'unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST')

Aside from the error messaging, all this method really does is strip the direction key out of the details and call the other method, turn_action. This suggests strongly that the other methods that take details will strip out what they need and then do a more primitive operation that does the job given the details. We could verify that with a glance at ‘drop’:

    def action_drop(self, entity_object, holding=None, **action_dictionary):
        self.drop_forward_action(entity_object, holding)

Yes, same pattern.

I think we should do some renaming. Let’s rename all the something_action methods to just something. turn_action to turn and so on. We can do that unless we already have a method called turn, etc.

    def execute_action(self, verb=None, entity_object=None, **details):
        if entity_object or verb == 'add_bot':
            match verb:
                case 'add_bot': self.action_add_bot(**details)
                case 'step': self.step(entity_object)
                case 'drop': self.action_drop(entity_object, **details)
                case 'take': self.take_forward(entity_object)
                case 'turn': self.action_turn(entity_object, **details)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    self.turn(entity_object, direction)
                case _:
                    self._add_message(f'Unknown action {verb=} {details=}')
        else:
            self._add_message(f'verb {verb} requires entity parameter {details}')

PyCharm does the work. The tests are green. Commit: improve naming.

Now what about the others: action_add_bot, action_drop, and action_turn? They should be named add_bot_XYZ, drop_XYZ, turn_XYZ, to focus on what they are about, with XYZ being a suffix indicating that we have to strip details out before we can get down to the actual work.

We’ve looked at ‘drop’ and ‘turn, let’s look at the other one, action_add_bot:

    def action_add_bot(self, x=None, y=None, direction=None):
        if self.check_add_parameters(x, y, direction):
            self.add_bot_action(x, y, direction)

    def check_add_parameters(self, x, y, direction):
        if not x or not y:
            self._add_message('add_bot command requires x and y parameters')
            return False
        if direction not in ['NORTH' , 'EAST' , 'SOUTH' , 'WEST']:
            self._add_message(f'add_bot has unknown direction {direction}, should be NORTH, EAST, SOUTH, or WEST')
            return False
        return True

Same idea as with the turn with a slightly different approach to the error checking, because it was so messy.

I notice one other thing here. We do not have a final parameter on action_add_bot to absorb spurious parameters. Let’s write a test to show what happens.

    def test_add_bot_with_spurious_parameter(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'add_bot',
                   'x': 5,
                   'y': 6,
                   'direction': 'EAST',
                   'spurious': 'extra'}
        rq = [ command ]
        result = world.execute_requests(rq)['updates']
        assert len(result) == 1
        bot_info = result[0]
        assert bot_info['eid'] == WorldEntity.next_id
        assert bot_info['location'] == Location(5, 6)

This test fails:

    def execute_action(self, verb=None, entity_object=None, **details):
        if entity_object or verb == 'add_bot':
            match verb:
>               case 'add_bot': self.action_add_bot(**details)
E               TypeError: World.action_add_bot() got an unexpected keyword argument 'spurious'

We need to add the collecting parameter to the method:

    def action_add_bot(self, x=None, y=None, direction=None, **ignored):
        if self.check_add_parameters(x, y, direction):
            self.add_bot_action(x, y, direction)

The test passes. As do all the others. I change the collecting parameters in the other two action methods to be **ignored, since they are. Commit: add collecting parameter to action_add_bot; new tests; tidying.

Now I’d just like these action_verb method names to be better. The methods themselves check parameters from their input details parameter (some not very diligently) and then do the action. I’m just not thinking of a great name. Let’s think of another way one of these could almost be written:

    case 'add_bot': self.action_add_bot(**details)

We could almost do this:

    case 'add_bot':
        x, y, direction = details
        self.add_bot(x, y, direction)

We can’t quite do that, because the parameters need checking.

We could do this:

    case 'add_bot':
        x, y, direction = self.check_add_parameter(**details)
        self.add_bot(x, y, direction)

Now our strict rules of coding would require us to extract those two lines as a method and its name is the name we’re looking for.

Is it add_bot_checking(**details)? add_bot_after_checking(**details)?

I almost like that. Let’s rename:

    def execute_action(self, verb=None, entity_object=None, **details):
        if entity_object or verb == 'add_bot':
            match verb:
                case 'add_bot': self.add_bot_after_checking(**details)
                case 'step': self.step(entity_object)
                case 'drop': self.drop_after_checking(entity_object, **details)
                case 'take': self.take_forward(entity_object)
                case 'turn': self.turn_after_checking(entity_object, **details)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    self.turn(entity_object, direction)
                case _:
                    self._add_message(f'Unknown action {verb=} {details=}')
        else:
            self._add_message(f'verb {verb} requires entity parameter {details}')

Would turn_after_parsing be better than turn_after_checking? turn_providing? turn_supplying? turn_with(**details)? turn_using(**details)?

Let’s try using.

    def execute_action(self, verb=None, entity_object=None, **details):
        if entity_object or verb == 'add_bot':
            match verb:
                case 'add_bot': self.add_bot_using(**details)
                case 'step': self.step(entity_object)
                case 'drop': self.drop_using(entity_object, **details)
                case 'take': self.take_forward(entity_object)
                case 'turn': self.turn_using(entity_object, **details)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                    self.turn(entity_object, direction)
                case _:
                    self._add_message(f'Unknown action {verb=} {details=}')
        else:
            self._add_message(f'verb {verb} requires entity parameter {details}')

I’m going to go with that, I think turn_using_details(entity_object, **details) would be better, but it’s longer, doesn’t add much, and generally we do not name the parameters in the name of the method. We’ll stick with what we have here. Commit: rename three methods.

Summary

This morning, we did almost nothing.

We renamed about six methods; we renamed a few parameters; we wrote a test that demonstrated a potential defect and fixed it. Counting the writing of the article, we spent about an hour.

Was it worth it? Well, if at some future time a new programmer, or an old one with a poor memory, needs to add another action to the things that the World can do, or needs to change how an existing action is handled they’ll find code that is simpler, better named, and easier to understand than had we left it as it was. tI believe that they will have fewer questions of the “????” kind when they read the code above.

I think there’s no question that the code is a bit better and a bit easier to understand. It’s more consistent and the names make more sense. The code itself didn’t change a bit: only the names.

Was it worth it? You tell me! See you next time!