A Little Improvement

Added in Post

There is a bit of thrashing below, where I think I see a simpler approach and later discover that I’m mistaken. Thrashing happens sometimes. I’ll add more comments below so you’ll have a better sense of what is happening.

Hello, loves!

The new conditional-entry logic is clearly a hack. Let’s provide a little improvement. Some thrashing. Method object. Satisfactory but not amazing.

The hack we’d like to fix is embedded in just one method:

class Dungeon:
    def place_player_at(self, cell):
        old_cell = self.player_cell
        self.player_cell = cell
        move_allowed = all((
            item.interact_with_player(self)
                for item in list(self.contents_at(cell))))
        if not move_allowed:
            self.player_cell = old_cell

What a crock! Save where the player is, move the player, run the interactions, and if any object, move the player back where she was. Why?, you ask. Well, I’ll tell you why.

The interact_with_player is sent to dungeon contents, which can optionally give themselves to the player with code like this:

class ReceivableContent:
    def interact_with_player(self, dungeon) -> bool:
        dungeon.receive_item(self)
        dungeon.announce(f'You have received {self.name}!')
        return True

The dungeon, in its turn, puts the object in inventory, and then removes it from the dungeon, since it has been given:

class Dungeon:
    def receive_item(self, item):
        self.player_inventory.append(item)
        content = self.contents_at(self.player_cell)
        if item in content:
            self.publish('remove_item', '', item)
            content.remove(item)

DungeonView subscribes to remove_item, so that it’ll know to remove the item from the sprite lists. But the dungeon’s own job is to remove the item from the contents of the cell that it is in … and this code assumes that the item in question is in the player’s cell.

Thus the hack. The ideal fix would be to have receive_item be passed both the item and the cell from which it is to be removed. That would keep both sides of the transfer in the same method. But to do that, we’d need to pass the cell to all the content items, since they do not know where they are, and according to our design standards, they should not.

So we could pass the cell to all the interact implementors and we could change all their implementations to send it back if they want to give themselves to the player.

I think we can do better. What we’ll try is the Method Object pattern, where we implement a method by passing off our concerns to a separate object that does the work. I’m not sure what we’ll call this object, but I am starting to suspect that it might be a rudimentary Player object. Let’s find out.

I believe we should TDD this new object. We’ll call it PlayerMover as a nonce name.

class TestPlayerMover:
    def test_initial(self):
        assert False

Fails, as intended. Now let’s see what we can do. We are replacing the logic in these two methods:

class Dungeon:
    def place_player_at_offset(self, cell, offset):
        # seems like this check should be in place_player_at
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            new_cell = cell
        self.place_player_at(new_cell)

    def place_player_at(self, cell):
        old_cell = self.player_cell
        self.player_cell = cell
        move_allowed = all((
            item.interact_with_player(self)
                for item in list(self.contents_at(cell))))
        if not move_allowed:
            self.player_cell = old_cell

Added in Post

I am mistaken here, thinking I see a better thing to do. I think what threw me off was that the test was looking hard to write, and in thinking about how to refactor Dungeon to make it easier, I thought there was a path that didn’t need the Method Object. That was a mistake. See the next “Added in Post”.

No, wait! I think we can do better right here. Belay the Method Object idea, at least for now.

place_player_at_cell is used by Dungeon itself, but also by outside callers, all of whom intend to place the player at the provided cell unconditionally. I think place_player_at_offset is only called from the move methods.

    def move_player_north(self):
        self.place_player_at_offset(self.player_cell, (0, 1))

    def move_player_east(self):
        self.place_player_at_offset(self.player_cell, (1, 0))

    def move_player_south(self):
        self.place_player_at_offset(self.player_cell, (0, -1))

    def move_player_west(self):
        self.place_player_at_offset(self.player_cell, (-1, 0))

Let’s rename that method to indicate that it might not work:

    def move_player_north(self):
        self.attempt_move_to_offset(self.player_cell, (0, 1))

    def move_player_east(self):
        self.attempt_move_to_offset(self.player_cell, (1, 0))

    def move_player_south(self):
        self.attempt_move_to_offset(self.player_cell, (0, -1))

    def move_player_west(self):
        self.attempt_move_to_offset(self.player_cell, (-1, 0))

    def attempt_move_to_offset(self, cell, offset):
        # seems like this check should be in place_player_at
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            new_cell = cell
        self.place_player_at(new_cell)

Now if we can’t move because there is no cell or the cell is available (i.e. not allocated in the dungeon), there can be no interactions. So we can just return.

    def attempt_move_to_offset(self, cell, offset):
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            return
        self.place_player_at(new_cell)

That’s safe. Commit: refactoring player placement.

Added in Post

This change was righteous enough, and better expresses what’s going on. But I’m still chasing an idea that won’t work out.

I want to wind up with place_player_at doing nothing but putting the player in the desired cell. We could go one of two ways, either only calling it if we move her, or always calling it. We’ll try only calling it if we move her and then see what we think.

The thing we need to do now is inherently complex as it is conditional. We want Dot to interact with the content in the new cell, and, if allowed, to enter it. I really don’t want to name a method interact_with_contents_and_enter_if_allowed. Perhaps you can see why. Nonetheless, I’m actually going to do that.

I’ll make a copy of the current place_player_at with the new name, then carve down the place and use it:

Added in Post

It just got to feeling messy and I knew I was losing the thread. Wisely decided to reset.

That doesn’t work. Unclear why not. Doesn’t feel good. Reset. We are here:

    def attempt_move_to_offset(self, cell, offset):
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            return
        self.place_player_at(new_cell)

    def place_player_at(self, cell):
        old_cell = self.player_cell
        self.player_cell = cell
        move_allowed = all((
            item.interact_with_player(self)
                for item in list(self.contents_at(cell))))
        if not move_allowed:
            self.player_cell = old_cell

One issue is that there are users of place_player_at who expect it to just work, and the dungeon and some tests expect it to be checking. I think further refactoring here won’t help. And I don’t quite think I can TDD the object we need, although I do think I can write it.

Added in Post

I felt a bit of defeat here, because I couldn’t see an easy way to TDD the object. As it turns out, it’s so simple as almost not to need it, with only one method that isn’t a pure forwarder. Still, I’d like to have direct tests. At this writing, there are none.

Let’s look over at receive_item again:

    def receive_item(self, item):
        self.player_inventory.append(item)
        content = self.contents_at(self.player_cell)
        if item in content:
            self.publish('remove_item', '', item)
            content.remove(item)

I’m planning to have a method that knows the cell from which the item is coming. We wish that the items themselves knew that but I think our method object will handle things. Move line down:

    def receive_item(self, item):
        content = self.contents_at(self.player_cell)
        if item in content:
            self.publish('remove_item', '', item)
            content.remove(item)
        self.player_inventory.append(item)

Extract variable:

    def receive_item(self, item):
        containing_cell = self.player_cell
        content = self.contents_at(containing_cell)
        if item in content:
            self.publish('remove_item', '', item)
            content.remove(item)
        self.player_inventory.append(item)

Extract Method:

    def receive_item(self, item):
        containing_cell = self.player_cell
        self.receive_item_from_cell(item, containing_cell)

    def receive_item_from_cell(self, item, containing_cell):
        content = self.contents_at(containing_cell)
        if item in content:
            self.publish('remove_item', '', item)
            content.remove(item)
        self.player_inventory.append(item)

OK. Time to stop typing, reflect, regroup.

Added in Post

The changes above were good and probably should have been committed. Probably I wasn’t confident that they were good steps. Stopped to think, which was good.

Pause for Thought

We wish that when the player tries to move into a cell, and the interact_with_player is called on the cell’s contents, items that want to give themselves to the player would call receive_item_from_cell, so that we wouldn’t assume that the player was standing amid the treasures. But the items cannot do that, because they don’t know what cell they are in, and we don’t want to refactor the whole content hierarchy to add a parameter that they shouldn’t know anyway.

So we plan to place an intermediate object into the equation. The dungeon will provide that object with everything it needs to know, namely the player’s current cell and the cell to which she is trying to move, and this object will run the interactions, receive the items, pass them on to the dungeon, providing the cell it needs, and then return the cell in which the player should be placed, or perhaps just place her there.

I still don’t see how to TDD this. I’ll see if I can code it and then we’ll see what can be done about testing. If I were smarter, maybe I’d see it now.

Added in Post

I think what’s here is an example of a fairly strong move. When we’re not sure what kind of object we need, we write the code that will use it, as it if existed. I call this “Wishful Thinking”, and Kent Beck called it “Coding by Intention” if I’m not mistaken.

Ideally one would code it in a test. In this case, less ideal, I code it where I want it.

Let’s see how we’d like to use the object, assuming it exists. It’ll do its job somewhere around here:

    def attempt_move_to_offset(self, cell, offset):
        new_cell = self.layout.at_offset(cell.xy, offset)
        if new_cell is None or self.layout.is_available(new_cell):
            return
        self.place_player_at(new_cell)

    def place_player_at(self, cell):
        old_cell = self.player_cell
        self.player_cell = cell
        move_allowed = all((
            item.interact_with_player(self)
                for item in list(self.contents_at(cell))))
        if not move_allowed:
            self.player_cell = old_cell

Added in Post

This is an interesting move. As we’ll see below, we wind up with a comprehension. But we need to change what’s inside it, and I couldn’t see clearly. So we expand it, and, if it seems right later, replace the open loop with a comprehension again. So we expand the code out, improve it, make it compact again.

All the trouble occurs in the move_allowed loop. I think I want to unwind my lovely comprehension here, because what we need to do seems to me to be down inside the loop.

    def place_player_at(self, cell):
        old_cell = self.player_cell
        self.player_cell = cell
        move_allowed = True
        for item in list(self.contents_at(cell)):
            ok_with_me = item.interact_with_player(self)
            move_allowed &= ok_with_me
        if not move_allowed:
            self.player_cell = old_cell

Added in Post

Finally, Wishful Thinking about what we want (well, almost what we want).

Now my plan, still, is to have a method object to deal with the old new issue. I think I’ll call it Interactor for now:

    def place_player_at(self, cell):
        move_allowed = True
        for item in list(self.contents_at(cell)):
            interactor = Interactor(item, cell)
            ok_with_me = interactor.execute()
            move_allowed &= ok_with_me
        if move_allowed:
            self.player_cell = cell

Now for Interactor, first this:

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

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

Tests fail. I expect them to fail because Interactor doesn’t understand receive_item. That is the case:

  def interact_with_player(self, dungeon) -> bool:
>       dungeon.receive_item(self)
        ^^^^^^^^^^^^^^^^^^^^
E       AttributeError: 'Interactor' object has no attribute 'receive_item'

Added in Post

Often, as we do the Wishful Thinking thing, we will find that the code we are wishing for needs more information than we originally realized. I find that this happens often and it doesn’t even surprise me that it happens here.

And I realize that it needs the dungeon as well, so:

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

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

    def receive_item(self, item):
        self.dungeon.receive_item_from_cell(item, self.cell)

And in the call:

class Dungeon:
    def place_player_at(self, cell):
        move_allowed = True
        for item in list(self.contents_at(cell)):
            interactor = Interactor(self, item, cell)
            ok_with_me = interactor.execute()
            move_allowed &= ok_with_me
        if move_allowed:
            self.player_cell = cell

Added in Post

Glad to have the tests here, else we’d have failed in the game.

Tests fail because Interactor doesn’t understand announce.

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

    def announce(self, message):
        self.dungeon.announce(message)

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

    def receive_item(self, item):
        self.dungeon.receive_item_from_cell(item, self.cell)

Added in Post

We need a test that fails for want of publish, and don’t have one. I’ve made a note and maybe we’ll write one.

Tests are green. Let me test in the game. Instant failure, Interact needs publish. Add it. Green. Game works. Commit: Interactor used when moving player.

Added in Post

It works. We take a look at making it more right.

However, I don’t think this is quite right yet. First, let’s review the Interactor:

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

    def announce(self, message):
        self.dungeon.announce(message)

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

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

    def receive_item(self, item):
        self.dungeon.receive_item_from_cell(item, self.cell)

We don’t need the item instance variable, if we pass it to execute, so we refactor:

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

    def announce(self, message):
        self.dungeon.announce(message)

    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_item(self, item):
        self.dungeon.receive_item_from_cell(item, self.cell)

And the call changes:

    def place_player_at(self, cell):
        move_allowed = True
        for item in list(self.contents_at(cell)):
            interactor = Interactor(self, cell)
            ok_with_me = interactor.execute(item)
            move_allowed &= ok_with_me
        if move_allowed:
            self.player_cell = cell

Added in Post

This clearly works. It’s iffy because reusing the Interactor was not part of my original understanding of it, it just fell out that it is reusable. I think that this is OK but <waves hands a bit>.

And we can reuse the interactor. Should we? I think yes. Move it up.

    def place_player_at(self, cell):
        interactor = Interactor(self, cell)
        move_allowed = True
        for item in list(self.contents_at(cell)):
            ok_with_me = interactor.execute(item)
            move_allowed &= ok_with_me
        if move_allowed:
            self.player_cell = cell

Commit. (Missed the commit for changing the signatures. Slow down, Ron.)

Rename the class to CellInteractor? Not now, but might be a good idea.

Put the comprehension back, because the local standard is to use them.

    def place_player_at(self, cell):
        interactor = Interactor(self, cell)
        move_allowed = all(interactor.execute(item) for item in list(self.contents_at(cell)))
        if move_allowed:
            self.player_cell = cell

Dare we inline that? Let’s do and see how we fell about it.

    def place_player_at(self, cell):
        interactor = Interactor(self, cell)
        if all(interactor.execute(item) for item in list(self.contents_at(cell))):
            self.player_cell = cell

I think we’re mostly good here. Everything works. Commit.

Summary

There was thrashing. I thought I saw a way to do without the method object, tried it, and discovered that I was mistaken. Reset –hard. As it should be. I’ve added lots of “Added in Post” comments, in the hope that you could better follow what was happening.

I think it felt worse to me than it was. Most of the changes I made were reasonable, but it felt to me like I was fussing around the edges and not getting closer to the central idea. Sometimes it do be like that.

Existing tests are checking the method object well enough, but they are themselves involved story tests with lots of setup. This is all part of Dungeon being so central to things.

I think our little method object, Interactor, helps just a bit. It makes a connection from content items that want to give themselves away, and we avoid a new structure mapping item to cell, which would make Dungeon even more complicated. It’s a small improvement. I’d like to see more. But every little bit helps.

Se you next time!