Hello, loves!

I see one more thing that needs doing. And then I propose that we have a little fun. What am I saying, programming IS fun for me. CW: Brief LLM excursion.

Begin with a digression …
In fact, I do this simple programming thing, and this prolix writing about it thing, because I enjoy them. I enjoy the puzzles, the mental exercise, the fun of seeing the computer do what I set out to have it do, the occasional joy of someone engaging with me via one of these articles.

I have had the privilege, most of my life, to have work that I could do fairly well, and that I enjoyed doing, and when things became less than enjoyable, something better soon came along. Now that I’m retired, I continue to enjoy that privilege by doing what I love, and sharing it.

Frequent readers here or on Mastodon know that I often think about LLM-“AI”, and that I think, um, poorly of it. Most of my objections are systemic in nature, relating to climate, resources, extractive capitalism, job displacement, tool of the oppressor, you know what I mean. But I can see how one could get a similar kind of joy to successfully guiding a deeply flawed program with a conversational interface, until it produced a program sort of like what one had in mind.

I can see it. Paraphrasing Upton Sinclair: It is difficult to get someone to understand something, when their salary depends on them not understanding it. So I see why folx might do what they do. I still think it’s literally morally wrong. But I’ve done morally wrong things. Most of us have, and will continue to do so. I can dig it.

But I do this for fun. Enough of the downer talk. Let’s have some fun.

It was a classic case of Feature Envy, Watson …

Shortly after I closed out yesterday’s article, I noticed this code in Dungeon:

class Dungeon:
    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self.contents_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            self.contents_allow_move(self.player_cell)

    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        move_is_allowed = all(interactor.execute(item) for item in list(self.contents_at(cell)))
        return move_is_allowed

This code is all deeply concerned about the Interactor class, calling it repeatedly. Why isn’t Interactor more helpful? All the Dungeon really wants is to know whether all the interactions returned True. Let’s make Interactor a bit more helpful. It’s rather simple …

class Interactor:
    def __init__(self, dungeon, cell):
        self.dungeon = dungeon
        self.cell = cell

    def execute(self, item):
        return item.interact_with_player(self)

    def publish(self, event, caller_id, *args, **kwargs):
        self.dungeon.publish(event, caller_id, *args, **kwargs)

    def receive_content(self, item):
        self.dungeon.receive_content_from_cell(item, self.cell)

Give it a new method:

    def execute_all(self):
        return all(self.execute(item) for item in self.dungeon.contents_at(self.cell))

Use it:

Hey! I think I just found a defect. I bet that Python all will short-terminate if it hits a false. I know that I would. Fix that in the new method:

Something is wrong here. Reset and do again.

class Interactor:
    def execute_all(self):
        all_ok = True
        for item in list(self.dungeon.contents_at(self.cell)):
            all_ok = all_ok & self.execute(item)
        return all_ok

class Dungeon:
    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        move_is_allowed = interactor.execute_all()
        return move_is_allowed

That passes. I thought I had exactly that code when I said “something is wrong here”, but tests were failing then and they aren’t now. Is there some reason why we need to make that a list? Ah, yes there is! When we give something to Dot, it gets removed from the contents, so we need to iterate a copy. I didn’t have the list in the previous attempt, I’ll bet.

Let’s use copy as it is more explicit:

    def execute_all(self):
        all_ok = True
        for item in self.dungeon.contents_at(self.cell).copy():
            all_ok = all_ok & self.execute(item)
        return all_ok

Fine, but wouldn’t it be better if we did the copy inside the Dungeon code?

    def contents_at(self, cell):
        return self.contents[cell].copy()

That breaks a test. Interesting! What is it?

    def test_contents_to_player(self):
        layout = DungeonLayout(10, 10)
        room_cell = Cell(5, 5)
        room = Room([room_cell])
        layout.add_room(room)
        dungeon = Dungeon(layout)
        factory = ContentFactory()
        item_1 = factory.receivable(name="treasure", resource='none', scale=0.5)
        shape = [(0.33, 0.66), (0.66, 0.66), (0.5, 0.33)]
        item_2 = factory.receivable(name="more treasure", resource='none', scale=0.5)
        dungeon.place_content_at(room_cell,item_1)
        dungeon.place_content_at(room_cell,item_2)
        dungeon.place_player_at(room_cell)
        assert item_1 in dungeon.inventory()
        assert item_2 in dungeon.inventory()
        assert dungeon.contents_at(room_cell) == []

Ah. Because the removal logic doesn’t work on the base contents if we copy:

class Dungeon:
    def receive_content_from_cell(self, content, containing_cell):
        contents = self.contents_at(containing_cell)
        if content in contents:
            self.publish('remove_content', '', content=content)
            contents.remove(content)
        self.player_inventory.append(content)

OK, belay that idea for now but I’m not comfortable with the situation. Let’s see where we are and get a commit here.

class Dungeon:
    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        move_is_allowed = interactor.execute_all()
        return move_is_allowed

class Interactor:
    def execute_all(self):
        # explicit loop because python `all` short-circuits
        all_ok = True
        for item in self.dungeon.contents_at(self.cell).copy():
            # copy because we may remove content in this loop
            all_ok = all_ok & self.execute(item)
        return all_ok

    def execute(self, item):
        return item.interact_with_player(self)

Kent Beck has said “A comment is the code’s way of asking to be improved”, and I agree. If one pays attention to that notion, the result is, like it says on the label, improved code.

This is the only use of execute. Let’s rename:

    def execute_all(self):
        # explicit loop because python `all` short-circuits
        all_ok = True
        for item in self.dungeon.contents_at(self.cell).copy():
            # copy because we may remove content in this loop
            all_ok = all_ok & self.interact_with(item)
        return all_ok

    def interact_with(self, item):
        return item.interact_with_player(self)

Should we rename execute_all to something more explicit? No, wait.

Let’s remove cell from the constructor, and instead pass the cell to exxecute_all … but first, tiny fool, commit: refactoring interactor-dungeon relationship.

Now let’s see if we can pass the cell in and use it:

class Interactor:
    def __init__(self, dungeon, cell):
        self.dungeon = dungeon
        self.cell = cell

    def execute_all(self, cell):
        # explicit loop because python `all` short-circuits
        all_ok = True
        for item in self.dungeon.contents_at(cell).copy():
            # copy because we may remove content in this loop
            all_ok = all_ok & self.interact_with(item)
        return all_ok

class Dungeon:
    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        move_is_allowed = interactor.execute_all(cell)
        return move_is_allowed

Commit: passing cell to execute_all

Now remove it from the constructor. Darn. The tests tell me what we can’t do that. Interactor acts like a tiny dungeon and has this:

    def receive_content(self, item):
        self.dungeon.receive_content_from_cell(item, self.cell)

OK< reset that right back out.

How about a rename from execute_all, which seems pretty harsh even in the context of a dungeon, to … interact?

class Dungeon:
    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        move_is_allowed = interactor.interact()
        return move_is_allowed

Inline that.

    def contents_allow_move(self, cell):
        interactor = Interactor(self, cell)
        return interactor.interact()

Inline again:

    def contents_allow_move(self, cell):
        return Interactor(self, cell).interact()

Commit: refactoring.

Here’s a slightly bigger picture:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self.contents_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            self.contents_allow_move(self.player_cell)

    def place_player_at(self, cell):
        if self.contents_allow_move(cell):
            self.set_player_position(cell)

    def contents_allow_move(self, cell):
        return Interactor(self, cell).interact()

We could inline the contents_allow_move but I rather like having the name there. Does it look odd / confusing that the second call to contents_allow_move doesn’t pay attention to the result?

Let’s try this:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self.contents_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            _redo_interactions = self.contents_allow_move(self.player_cell)

No, not good enough.

Let me bloviate a bit on what I’m doing here. This section of the code is complicated, in that the interaction returns a result and has a side effect of quite some magnitude, since Dot may receive benefit or harm while this goes on, announcements could be made, perhaps bats could be released. Anything could happen. I’m trying to make the complexity clear enough, with enough hidden to keep it from being confusing, but enough shown to ensure that the casual reader won’t change things without extra care.

I want the code to be compact … but more than that, I want it to be clear.

Let’s try this:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self.contents_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            self.redo_interactions_in_current_cell()

    def redo_interactions_in_current_cell(self):
        _ignored =  self.contents_allow_move(self.player_cell)

    def place_player_at(self, cell):
        if self.contents_allow_move(cell):
            self.set_player_position(cell)

    def contents_allow_move(self, cell):
        return Interactor(self, cell).interact()

    def set_player_position(self, cell):
        self.player_cell = cell

The redo... method tries to express that we are executing the interactions again per the rules, and the _ignored makes it more clear that we don’t care about the result.

Commit that.

I think I want to change contents_allow_move, resulting in this:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self.interactions_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            self.redo_interactions_in_current_cell()

    def interactions_allow_move(self, cell):
        return Interactor(self, cell).interact()

    def redo_interactions_in_current_cell(self):
        _ignored =  self.interactions_allow_move(self.player_cell)

    def place_player_at(self, cell):
        if self.interactions_allow_move(cell):
            self.set_player_position(cell)

    def set_player_position(self, cell):
        self.player_cell = cell

Some of these should at least be private methods. Dungeon continues to have a lot of methods and every little hint helps. Commit, and then:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self._interactions_allow_move(new_cell):
            self.set_player_position(new_cell)
        else:
            self._redo_interactions_in_current_cell()

    def _interactions_allow_move(self, cell):
        return Interactor(self, cell).interact()

    def _redo_interactions_in_current_cell(self):
        _ignored =  self._interactions_allow_move(self.player_cell)

    def place_player_at(self, cell):
        if self._interactions_allow_move(cell):
            self.set_player_position(cell)

    def set_player_position(self, cell):
        self.player_cell = cell

Commit.

place_player_at does interactions and set_player_position does not. main uses place... but it means set: it doesn’t want the player interacting. Maybe it should, but for now:

main.py
    ...
    target = Cell(32, 28)
    dungeon.set_player_position(target)
    ...

Commit.

One more thing … let’s rename the place method to make clear that it will interact. And maybe rename the other? How about this?

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move(direction)
        if new_cell != self.player_cell and self._interactions_allow_move(new_cell):
            self.just_set_player_position(new_cell)
        else:
            self._redo_interactions_in_current_cell()

    def _interactions_allow_move(self, cell):
        return Interactor(self, cell).interact()

    def _redo_interactions_in_current_cell(self):
        _ignored =  self._interactions_allow_move(self.player_cell)

    def set_player_position_with_interaction(self, cell):
        if self._interactions_allow_move(cell):
            self.just_set_player_position(cell)

    def just_set_player_position(self, cell):
        self.player_cell = cell

Commit. Enough already. Let’s sum up.

Summary

I had a little fun feature in mind but never got there, but this article is more than long enough. We’ll try the feature next time. If you got here, congratulations.

We did actually make a small design improvement, moving the group interaction from Dungeon to Interactor. Then we entered into a long sequence of tiny nearly inconsequential changes, renames, tiny moves of a line from here to there. You have to ask yourself whether it was “worth it”, and, more importantly, whether that much care is “worth it” in your own work.

I feel confident that if I were under pressure to deliver feature upon feature, I would not do this much refactoring. I wish that I would, I think that I should, and I know that under pressure, I would fold like a wet tissue, knuckling under to The Man, toadying up, currying favor, caving in, like a good little minion.

It’s not that I am weak, although I surely am. It is that when in a team, with a leader or under a manager, one tries to do what is wanted, even if one would prefer otherwise, and, sadly, even if one’s own course would be better.

However, I would submit (not in the sense of kowtowing) that the code we have now is better, more clear, easier to change and harder to break than the code we started with. As such, this code would enable us, were we a production-oriented team, to go faster. It is surely better in some abstract sense, but I suggest that it’s quite likely better even in a team that needs to go fast.

I could be wrong, and you get to decide what you do.

The girls and I just want to have fun. I hope you’ve had some fun here too. See you next time!