The Repo on GitHub

We’ll take a look at some small changes to improve the World class. If only we could refactor the Real World so easily. Maybe we can?

Here’s the entire World class. We’ll just scan it, not read in detail, and see what we notice.

class World:
    def __init__(self, max_x, max_y):
        self.width = max_x
        self.height = max_y
        self.map = Map(max_x, max_y)
        self.ids_used = set()

    def add_block(self, x, y, aroma=0):
        return self._add_entity(WorldEntity.block(x, y, aroma))

    def add_bot(self, x, y, direction = Direction.EAST):
        return self._add_entity(WorldEntity.bot(x, y, direction))

    def _add_entity(self, entity):
        self.map.place(entity)
        return entity.id

    def entity_from_id(self, bot_id):
        return self.map.at_id(bot_id)

    def execute(self, actions_list):
        self.ids_used = set()
        self.execute_actions(actions_list)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.unpack_and_execute(**action)

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object=None, **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',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step(entity_object)
            case {'verb': 'take'}:
                self.take_forward(entity_object)
            case {'verb': 'turn',
                  'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object,direction)
            case {'verb': 'turn', 'direction': bad_direction}:
                raise AttributeError(f'unknown direction {bad_direction}')
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

    def add_bot_action(self, x, y, direction):
        bot_id = self.add_bot(x, y, Direction.from_name(direction))
        self.ids_used.add(bot_id)

    def drop_forward_action(self, entity, holding):
        self.drop_forward(entity, self.entity_from_id(holding))

    def drop_forward(self, bot, entity):
        if self.map.place_at(entity, bot.forward_location()):
            bot.remove(entity)

    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)

    def take_forward(self, bot):
        is_block = lambda e: e.name == 'B'
        if block := self.map.take_conditionally_at(bot.forward_location(), is_block):
            bot.receive(block)

    def turn(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)

    def fetch(self, entity_id):
        return self.entity_from_id(entity_id).as_dictionary()

    def set_bot_vision(self, bot):
        bot.vision = self.map.vision_at(bot.location)

    def set_bot_scent(self, bot):
        aroma_to_seek = 0
        if bot.holding:
            aroma_to_seek = bot.holding.aroma
        bot.scent = self.map.scent_at(bot.location, aroma_to_seek)

    def is_empty(self, drop_location):
        return not self.map.at_xy(drop_location.x, drop_location.y)

  • I notice one private method, _add_entity. That’s an odd number. I seem to be ambivalent about using the underbar-private notation in Python. Improving consistency would be good.

  • Some of the methods that execute_action calls are named ending in action and some are not. Consistency here would be good. If we named them action_xyz instead of xyz_action, it would make sense to keep them together. That might be an improvement. Maybe something more verb-like would be even better.

  • The top-level execution calls go execute to execute_actions to unpack_and_execute to execute_action. Those names don’t quite tell the story. We’ll come back to that below, I think. We’re scanning here, collecting impressions and ideas.

  • drop_forward_action calls drop_forward and nothing else. We’ve allowed other verbs to have a conditional such as we find in drop. Perhaps we should inline this one. No. drop_forward_action is making a key transformation from an id to an object. But let’s consider some parameter names below.

  • The method is_empty is used only in tests. We could remove it by changing the tests.

  • set_bot_scent could use a ternary, which some might consider an improvement.

Enough scanning. What have we learned? Well, I think we have learned that there is almost always room for improvement in my code, and my experience suggests that that’s true for just about any code. We do not always choose to make those improvements: there is often something more important on our mind. But some of these things are so quick to do that we can do them as soon as we notice them. Almost every day while I’m writing one of these screeds, I see something trivial and improve it without mentioning it. When it’s a machine refactoring, and the tests run, why not?

Let’s look a bit harder at this:

    def drop_forward_action(self, entity, holding):
        self.drop_forward(entity, self.entity_from_id(holding))

    def drop_forward(self, bot, entity):
        if self.map.place_at(entity, bot.forward_location()):
            bot.remove(entity)

The meaning of entity swaps between those two methods. In fact, in drop_forward_action and elsewhere, the so-called entity is a bot, not an arbitrary entity. Let’s do some renaming. We’re going to find that we need a fair amount, I think, but the changes should all be pretty local. We can commit each one if we want to. And, according to GeePaw Hill, we should. Micro-changes.

    def drop_forward_action(self, bot, holding):
        self.drop_forward(bot, self.entity_from_id(holding))

    def drop_forward(self, bot, entity):
        if self.map.place_at(entity, bot.forward_location()):
            bot.remove(entity)

Renamed drop_forward_action parameter entity to bot. Now the parameters match. Commit: minor renaming.

Rename:

    def drop_forward(self, bot, held_entity):
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)

Why not block? Because someday a bot may be able to hold other things than blocks. Commit again.

Rename step to step_action for consistency. Commit. Rename take_forward. Commit. Rename turn. Commit.

Now all the methods called from execute_action are named xyz_action for values of xyz. They are grouped together already, except for this pair, which troubles me:

    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)

This is the only one that requires an extra method / step. That may be OK: it’s the only one that involves a second entity. But the drop_forward is called from tests. Let’s see if we can make them call the action method instead.

    def test_bot_can_drop_on_empty_space(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5, Direction.NORTH)
        bot = world.entity_from_id(bot_id)
        block = WorldEntity.block(4, 4)
        bot.receive(block)
        assert bot.has(block)
        world.drop_forward(bot, block)
        assert not bot.has(block)
        assert world.map.at_xy(5, 4) is block

This should be legit:

    def test_bot_can_drop_on_empty_space(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5, Direction.NORTH)
        bot = world.entity_from_id(bot_id)
        block = WorldEntity.block(4, 4)
        bot.receive(block)
        assert bot.has(block)
        world.drop_forward_action(bot, block.id)
        assert not bot.has(block)
        assert world.map.at_xy(5, 4) is block

The test fails. The bug is that the block was not added to the world by the test. The test is wrong.

    def test_bot_can_drop_on_empty_space(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5, Direction.NORTH)
        bot = world.entity_from_id(bot_id)
        block_id = world.add_block(4, 4)
        block = world.entity_from_id(block_id)
        bot.receive(block)
        assert bot.has(block)
        world.drop_forward_action(bot, block.id)
        assert not bot.has(block)
        assert world.map.at_xy(5, 4) is block

That works as intended. The test was valid before, I think, but just by luck.

I make the corresponding change to the other uses of drop_forward. Everyone calls the action now. This is good. Commit: adjust tests to call drop_forward_action.

Now let’s see if we want to do an inline here:

    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)

Curiously, PyCharm acts as if it is going to do the inline and then does nothing. I submit a bug report. That’s quite rare.

Shall we inline it by hand? I extract variable:

    def drop_forward_action(self, bot, holding):
        held_entity = self.entity_from_id(holding)
        self.drop_forward(bot, held_entity)

    def drop_forward(self, bot, held_entity):
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)

I’ll try the inline again. Still no go.

I move the code myself:

    def drop_forward_action(self, bot, holding):
        held_entity = self.entity_from_id(holding)
        if self.map.place_at(held_entity, bot.forward_location()):
            bot.remove(held_entity)

And inline the variable … no … we can’t do that, it’s used twice now.

Having seen this result, I do not prefer it. Undo.

I think that’s a sign that we should stop. But first I want to review the higher level flow again:

    def execute(self, actions_list):
        self.ids_used = set()
        self.execute_actions(actions_list)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.unpack_and_execute(**action)

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object=None, **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',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step_action(entity_object)
            case {'verb': 'take'}:
                self.take_forward_action(entity_object)
            case {'verb': 'turn',
                  'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn_action(entity_object, direction)
            case {'verb': 'turn', 'direction': bad_direction}:
                raise AttributeError(f'unknown direction {bad_direction}')
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn_action(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

Only the top-level execute is really public. It might be better named execute_request or execute_requests. Let’s do the latter.

Maybe execute isn’t the best word. Let’s think about this sequence:

execute_requests to execute_actions to unpack_and_execute to execute_action.

Perform? Do? Carry out? Let’s try something. Let’s inline the unpack. No, we can’t do that, the **parameters thing won’t work.

Bah. I’ve stared at this long enough. Tried a few refactorings, broke something every time. Good sign that we’ve done enough: today was supposed to be small improvements, nothing requiring deep thought. Let’s sum up:

Summary

On the bright side, we’ve seen tiny changes, each taking moments to do and commit, each making (what we consider to be) small improvements to the code. They’ve added up to improve the consistency of the World’s request executing code.

There are still some things I don’t like: the name sequence down the execute_requests chain is a bit bumpy. Probably doesn’t matter. the one step unpack_and_execute is the largest bump: a method with an “and” in its name always suggests a need for some kind of improvement.

And the big match/case above is a large blob of code. No real way around that as far as I can see. Would some spacing help? No, I like that even less. We might do well to encapsulate the raise cases. We’ll probably deal with that soon, I have error handling on the list of things we need.

I’ll mention again, that while in real life we might never have a morning to spend like this one, just making small improvements, almost every day in real life will have us encounter many opportunities to make a single small improvement. If we take the opportunity, especially when our IDE will do the bulk of the work, our code will be more likely to stay alive and flexible.

I think it’s worth making the small changes, and worth stealing a couple of hours to do a few, when—if it ever happens—the pressure lowers. And sometimes, when the pressure is high, it’s a good move to decompress with a little refactoring anyway,

You get to use your own judgment, of course. Judgment, not just reaction, that’s the ticket.

See you next time!

Last Minute Idea
How about this for a rename:
    def execute_actions(self, actions_list):
        for action in actions_list:
            self.execute_with_entity(**action)

    def execute_with_entity(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

I think I prefer that! Woot! Committed.