The Repo on GitHub

I’ve had an uneasy feeling about the project. A kind reader suggested a book. The book suggests what my concern should be. In a long article I express concerns … and alleviate them a bit!

Reader Ajahn J.R. suggested the book Fluent Python, by Luciano Ramalho. I am enjoying the book, and am very grateful for the suggestion: I don’t think I’d have found the book any other way, since I wasn’t looking,

Fluent Python is an advanced Python book, and it goes in depth into how Python works, and how it is best used. I’m only partway through it, and I freely grant that I’m not studying every detail even of the parts I have read. Frequent readers will know that I like to read a bit and then try things. Fluent Python is so full of information that there’s no other way for me to approach it.

The book seems to take some topic, such as collections, and then it describes how the various collection types work, how they differ, and then why they differ, digging into the underlying structures and code, the dunder methods involved, even the memory layouts. It’s fascinating, and equips the reader to use existing Python well, and to build objects and classes that better serve one’s needs.

One detailed area that I was scanning last night, when I should have been sleeping, addressed Python’s packing and unpacking of sequences. I knew that given a sequence swe can unpack it like this:

>>>s = (3, 5)
>>>a, b = s
>>>a
3
>>>b
5

But there is so much more that Python can do, from unpacking some elements are returning the rest, to nested unpacking, and more. It was close to midnight when I was reading that bit, so I can’t begin to explain all of it. The book explains it better anyway.

Super book. If you like books like this, get it. But my point here is a not the book.

“Each of you is perfect the way you are and you can use a little improvement.”
– Shunryu Suzuki

A primary theme of my writing is to take some code that I’ve written and improve it. It is a deliberate practice of mine to write code right in front of you, code that works, that’s as good as I can get it in the moment. Then, later, we revisit that code, either because it needs extension from a later story, or just because we look around in the code base for something that needs improvement.

That’s well and good, and I think that from a general object-oriented outlook, the code I first write is OK, and the code I improve is better. Sometimes, perhaps, it’s nearly good. I’m not displeased with where we are in our Python journey together … but I am uneasy.

A little story

A long time ago, my friend and hero Ward Cunningham had a chance to review a program that a team of mine was working on. He declared the code we were writing—the code I was writing—to be “mediocre”. To my recollection, I was not insulted when I found that out. Ward is so far beyond me that I figure he couldn’t be more than two percent off, so if he says “mediocre”, that’s what it is.

I suspect that the work I’m doing here, on the Robot thing, probably has a lot of “mediocre” in it. I hope there are spots of “pretty good”, maybe even the occasional “nice”. But I don’t find it satisfying. It’s not up to the standards I’d like to reach. I don’t expect ever to reach the level of my personal hero programmers. But I know I’m better than this.

Why are we where we are? Well, there are forces acting that I’ll list here, not by way of excuse, but to see if they explain anything and if there’s something to do about them.

Disappointment About the Project
With some pals, I had expected that this would be a joyous group effort. That has not happened, and it is at least partly my fault, if we wanted to lay blame. I work at hours when most people are not awake, and at least some people find me difficult to pair with. But what’s significant is that I’m a bit sad because there was supposed to be more together fun in here.
Lack of Pairing / Mobbing
A concrete effect of working with others on code is that the code becomes much better, through the exchange of ideas and the vision of multiple people with varied understanding and ability. This would be especially valuable if any of them happened to know things about Python that I do not. However, I discount this concern, because nothing I do here has had the benefit of pairing or mobbing for years, so this is no change.
Inexperience with Python
Well, I started with Python in March 2023, so I’m about 20 months in, which might not take one to the level of Mastery, but honestly I feel I should be doing better than I am.
Advanced Age
I’m old. Very old. I could be losing it. Surely some of the “little grey cells” have retired by now. But even if that’s true, there are plenty left. I can do better, even if I have to try harder. I don’t mind trying harder.
Lazy Careless Useless
It me. I am inherently lazy, always have been. I study things that interest me, ignore things that don’t. I never work hard, although I have spent innumerable hours reading programming books, reading code, writing code, analyzing code … but that wasn’t hard work, that was fun. Maybe I’ve not been having enough fun.
Challenge
I’ve just been cruising along. This program is much like every other program, because all programs are much like every other program. Maybe I need more of a challenge. However, I want to wave off challenges like “OK, make it really client-server”. Not that that wouldn’t be a challenge: it is and will continue to be, until I do it. But it’s not one that will make me a better Python programmer, if that’s the objective. Sure, I’ll know another thing, but will I do it better than I’m doing now? I don’t think so. I want to do better.
None of the Above
OK, that’s about the whole list that comes to mind, and none of them seem to address just why I find myself where I am. At this writing none of those topics seems to offer me much leverage of the kind I’m looking for. They each offer a bit of something I might draw from, but none of them springs out at me with “That’s it!”


—

At this writing, I envision a two-part plan.

Study / Report
I’ll study an area of Python, from Fluent Python and whatever other sources I may discover. From time to time, I’ll write about what I’ve learned, even though it’s surely better explained wherever I was studying. I’ll be sure to mention my sources.
Improve Code
I’ll review existing code and assess it in the light of what I’m learning, and see how to improve it. I’ll write new code, try to get it better before I abandon it, and come pack to it later and see how to improve it. (I really do think that leaving and coming back is good. We just shouldn’t leave too soon.)

This isn’t very different from what I’ve been doing, is it? And that is as it should be, I think. I want to continue to do the good things that I know of, and to continue to learn good things , and to continue to practice, practice, practice.

“Ever tried. Ever failed. No matter. Try again. Fail again. Fail better.”
– Samuel Beckett

That’s my plan, and that’s good enough.


—

Now, let’s look at some code. We haven’t looked at World lately. Here are the first bits:

class World:
    next_id = 100

    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):
        entity = WorldEntity.block(x, y, aroma)
        self.map.place(entity)
        return entity.id

    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        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()
        for action in actions_list:
            id = action['entity']
            if id:
                self.ids_used.add(id)
                entity = self.entity_from_id(id)
            else:
                entity = None
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                direction_string = action['direction']
                direction = Direction.from_name(direction_string)
                bot_id = self.add_bot(action['x'], action['y'], direction)
                self.ids_used.add(bot_id)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.set_direction(entity, direction)
            case 'turn':
                direction = action['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

What do we see here? The first thing is next_id. I am suspicious. I ask PyCharm for references. It finds none, zero, not any. I think we assign ids elsewhere now, but we left this here to confuse people.

Remove. Green. Commit: remove unused next_id.

Next, these:

    def add_block(self, x, y, aroma=0):
        entity = WorldEntity.block(x, y, aroma)
        self.map.place(entity)
        return entity.id

    def add_bot(self, x, y, direction = Direction.EAST):
        entity = WorldEntity.bot(x, y, direction)
        self.map.place(entity)
        return entity.id

I note duplication. Remove it.

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

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

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

Let’s consider inlining:

    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

Let’s take a glance at place:

    def place(self, entity):
        assert isinstance(entity, WorldEntity)
        self.contents[entity.id] = entity

I was wondering where the id comes from. Must be in the WorldEntity constructors. We pop over there.

class WorldEntity:
    next_id = 100

    def __init__(self, kind: EntityKind, x: int, y: int, direction: Direction):
        self._dict = dict()
        WorldEntity.next_id += 1
        self._dict['eid'] = self.next_id
        self._dict['kind'] = kind
        self.aroma = 0
        self.location = Location(x, y)
        self.direction = direction
        self.holding = None
        self.scent = 0
        self.vision = []

OK, that’ll do for now. I just wanted to be sure what was going on. Review our changed code:

class World:
    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

I consider that to be an improvement. I think it meets my current coding standards quite well. Green. Commit: tidying, remove duplication.

Now this:

class World:
    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                direction_string = action['direction']
                direction = Direction.from_name(direction_string)
                bot_id = self.add_bot(action['x'], action['y'], direction)
                self.ids_used.add(bot_id)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.set_direction(entity, direction)
            case 'turn':
                direction = action['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

What’s wrong with this? The difference between these two cases tells the story:

    case 'add_bot':
        direction_string = action['direction']
        direction = Direction.from_name(direction_string)
        bot_id = self.add_bot(action['x'], action['y'], direction)
        self.ids_used.add(bot_id)
    case 'step':
        self.step(entity)

The second one just does a call. The first does some work. They should all do calls. I am also inclined to improve the names to indicate private methods as I did above. Did you notice that?

Noticed in Post
I did not follow through on the private method idea. I should do it or not do it. This is an area needing improvement, and someone will probably put it on my permanent record.

The obvious name for that chunk of code is add_bot, but we already have that method, we were just looking at it above. Let me try a name: I think it’s important to isolate this:

class World:
    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                self.add_bot_action(action)
            ...

    def add_bot_action(self, action):
        direction_string = action['direction']
        direction = Direction.from_name(direction_string)
        bot_id = self.add_bot(action['x'], action['y'], direction)
        self.ids_used.add(bot_id)

If we read this code carefully, we see that action is a dictionary kind of thing and that it needs to contain ‘x’, ‘y’, and ‘direction’. And we convert the direction string to a Direction instance. I think we can do better:

    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                self.add_bot_action(**action)
            ...

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

What’s happening here? Well, as I learned just last night, Python will unpack a dictionary into a function’s arguments by name, if you prefix it by ** in the call.

I might inline that third parameter in the call to add_bot:

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

I like that well enough to commit: Tidying via dictionary unpacking.

It would be nice if we didn’t have to deal with the entity and verb, which are already fetched and used above.

    def execute(self, actions_list):
        self.ids_used = set()
        for action in actions_list:
            id = action['entity']
            if id:
                self.ids_used.add(id)
                entity = self.entity_from_id(id)
            else:
                entity = None
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                self.add_bot_action(**action)

A bit of review tells me that I can do this:

    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)

The **_ says to put any remaining key-value pairs into the parameter _, as a dictionary. In essence, for our purposes, we mean “ignore”.

I like where this is going. Let’s continue. Shall we commit? Yes, we are green, we shall. Commit: further tidying with unpacking.

I want to give this same treatment to the execute method above. Extract Method:

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

    def unpack_and_execute(self, action):
        id = action['entity']
        if id:
            self.ids_used.add(id)
            entity = self.entity_from_id(id)
        else:
            entity = None
        verb = action['verb']
        self.execute_action(entity, verb, action)

Now change the unpack_and_execute calling sequence:

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

    def unpack_and_execute(self, entity, verb, **parameters):
        if entity:
            self.ids_used.add(entity)
            entity_object = self.entity_from_id(entity)
        else:
            entity_object = None
        self.execute_action(entity_object, verb, parameters)

    def execute_action(self, entity, verb, parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = parameters['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.set_direction(entity, direction)
            case 'turn':
                direction = parameters['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

We’re green, and I like where this is going. Commit: further tidying via unpack.

I note that our messages pass id integers under names like ‘entity’ and ‘holding’. Those keywords would be better as ‘entity_id’ and ‘holding_id’, but I’m not going to go after that just now. One issue, if we were in the real world, would be that the keywords are part of our published API between World and client, so we cannot lightly change them.

Let’s do fix up the drop case to use our new scheme, see if it’s not better.

    def execute_action(self, entity, verb, parameters):
        match verb:
            ...
            case 'drop':
                self.drop_forward_action(entity, parameters)
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.set_direction(entity, direction)
            ...

    def drop_forward_action(self, entity, parameters):
        holding_id = parameters['holding']
        holding = self.entity_from_id(holding_id)
        self.drop_forward(entity, holding)

Green, of course. Commit: extract method. Now revise drop_forward_action to unpack.

    def drop_forward_action(self, entity, holding, **_):
        object_held = self.entity_from_id(holding)
        self.drop_forward(entity, object_held)

And inline:

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

I like that. It looks the way I’ve been taught to make OO code look.

Commit: tidying via unpacking.

One more thing, arranging the execute_action method a bit more nicely:

    def execute_action(self, entity, verb, parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity, **parameters)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'turn':
                self.turn(entity, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

I alphabetized (mostly), renamed the set_direction method to turn. I also reordered some of the methods we call.

Committed: tidying.

Let’s sum up: we have a long article here.

Summary

I thought about my discomfort with the quality of my work, and determined to study and practice a bit more carefully.

Then we looked at the code in World. It started out like this:

class World:
    def execute_action(self, entity, verb, action):
        match verb:
            case 'add_bot':
                direction_string = action['direction']
                direction = Direction.from_name(direction_string)
                bot_id = self.add_bot(action['x'], action['y'], direction)
                self.ids_used.add(bot_id)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action['holding']
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.set_direction(entity, direction)
            case 'turn':
                direction = action['direction']
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

Now it looks like this:

class World:
    def execute_action(self, entity, verb, parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity, **parameters)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'turn':
                self.turn(entity, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

By my lights, from study, practice, and advice from my betters, that code is what that code should look like. Nearly. I think maybe the two turn cases could be better, but I don’t see how.

We also changed this:

class World:
    def execute(self, actions_list):
        self.ids_used = set()
        for action in actions_list:
            id = action['entity']
            if id:
                self.ids_used.add(id)
                entity = self.entity_from_id(id)
            else:
                entity = None
            verb = action['verb']
            self.execute_action(entity, verb, action)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

To this:

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

    def unpack_and_execute(self, entity, verb, **parameters):
        if entity:
            self.ids_used.add(entity)
            entity_object = self.entity_from_id(entity)
        else:
            entity_object = None
        self.execute_action(entity_object, verb, parameters)

Now that doesn’t please me quite as much. What do I see there?

  1. The execute method isn’t quite in Composed Method form. This might be better:
    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)

Better. We could perhaps change the return to call a method, maybe get_results or something. Commit: tidying.

And the unpack_and_execute is a bit awkward. Probably needs a better name as well. Plus we have the issue that the name ‘entity’ in the dictionary is the name of the entity’s id, not the entity. And the name of the song is called “Haddock’s Eyes”.

The thing that I like best about this morning’s work is that we made use of Python’s simple yet very powerful ability to unpack a dictionary into a function’s parameter list. That is certainly new to me: I really don’t think I was aware of it at all before last night. But it’s not deep in the bag of tricks from a Python viewpoint, even if it is new to me and other readers who may not be all about Python. On the contrary, it is a very carefully crafted feature of the language.

I suspect that some readers, or people that some readers know, would object to single line methods like this one:

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

My experience is that when OO code starts looking like that, it’s really getting good. When it’s Very Good, the name of the method itself is usually all you need to know. If not, the name of the method that it calls, and the parameters provided, is enough. The fear that some programmers have about these tiny methods is that you have to bounce bounce bounce down down down for ages to find out what happens.

When we do it well, we rarely have to bounce down: we stop at the top, thinking “OK, that’s gonna take the action for dropping forward”. Only if we are going to work on the details do we look further, and the fact that everything is so short means that we can skip right over the bits that are not our current topic.

I invite comments and questions on Mastodon concerning this topic or whatever comes to your mind.

Bottom line, I feel that this morning I’ve done a much more workmanlike job with the code. I am pleased. I hope that you are as well. Stop by tomorrow to see what happens!