Keep on Moving

Hello, loves!

We improve some code, reduce overall size and breadth of interfaces, make things better. CW: Brief LLM-“AI” remarks.

I’m beginning with a review of the code around the new Connection idea, to see what can be removed and what needs improvement. I’ll show you what I spot, but spare you as many big code dumps as I can.

In Cell’s # movement section, I find this nice little method:

class Cell:
    def attempt_move(self, cell):
        connection = self.get_connection(cell)
        return connection.move()

That method is used in five test checks and not used at all in production code. That’s curious. What goes on in prod?

class Dungeon:
    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):
        new_cell = cell.get_connection_offset(offset).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

Those move_player_north methods are called from the View, e.g.

class DungeonView:
        if symbol == arcade.key.RIGHT:
            self.dungeon.move_player_east()

Wouldn’t it be better to have the keyboard dispatch say something like self.dungeon.move_player(EAST), and have Dungeon say something very like self.player_cell.attempt_move(EAST) and have the Cell manage the details?

Or maybe we’d say Direction.east instead of EAST. An Enum kind of thing. I think we’ll try that and I think we’ll put the Enum in the params file, where we have a few other useful constants.

params.py

class Direction(Enum):
    EAST = (1,0)
    WEST = (-1,0)
    NORTH = (0,1)
    SOUTH = (0,-1)

Now let’s do just a bit of wishful thinking.

class DungeonView:
        if symbol == arcade.key.RIGHT:
            self.dungeon.move_player(Direction.EAST)

If we wish really hard that that will work, and we also type two lines:

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_to_offset(self.player_cell, direction.value)

Our wish comes true: it works. A test has broken, however, using a FakeDungeon that needs updating. I guess we’ll do that now.

The test update is simple enough, looks like this:

class FakeDungeon:
    def move_player(self, direction):
        self.received = direction

    def subscribe(self, *args, **kwarg):
        pass


class TestHandlers:
    def test_keydown(self):
        fake = FakeDungeon()
        view = DungeonView(fake, testing=True)
        
        event = arcade.key.UP
        view.on_key_press(event, 0)
        assert fake.received == Direction.NORTH
        view.on_key_release(event, 0)
        ...

It fails until I complete the changes in DungeonView:

class DungeonView:
        if symbol == arcade.key.RIGHT:
            self.dungeon.move_player(Direction.EAST)
        elif symbol == arcade.key.LEFT:
            self.dungeon.move_player(Direction.WEST)
        elif symbol == arcade.key.UP:
            self.dungeon.move_player(Direction.NORTH)
        elif symbol == arcade.key.DOWN:
            self.dungeon.move_player(Direction.SOUTH)

Now can we remove all those move_player_north methods in Dungeon? Not quite, there are senders, all in test_dungeon.

They just need the obvious updates. I remove the methods and let the tests guide me to the edits. They’re all the same deal. I cleverly multi-cursor the lot of them. And I add the lower-case terms to the Enum. We’ll prefer EAST but allow east.

We can commit all this: use enum for direction, eliminate methods in Dungeon.

Now we have this:

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_to_offset(self.player_cell, direction.value)

    def attempt_move_to_offset(self, cell, offset):
        new_cell = cell.get_connection_offset(offset).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

    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

This class has no reason to fetch the value from the direction. Let’s push the direction down into Cell to be sorted out there. We’d rather not think about it here in the dungeon.

There are multiple users of attempt_move_to_offset. I expect they are all in tests but for this one but we’ll create a new method anyway. Again, a bit of wishful thinking.

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_in_direction(self.player_cell, direction)

The tests all scream in pain but then:

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_in_direction(self.player_cell, direction)

    def attempt_move_in_direction(self, cell, direction):
        new_cell = cell.get_connection_offset(direction.value).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

I sure wish we could say this:

class Dungeon:
    def attempt_move_in_direection(self, cell, direction):
        new_cell = cell.get_connection_in_direction(direction).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

Again, tests cry out in anguish until …

class Cell:
    def get_connection_in_direction(self, direction):
        return self.get_connection_offset(direction.value)

Digression

In the olden days, when we were working with Smalltalk, you might change some code to refer to a method that didn’t exist. You’d run the code and get a “walkback”, which is like a combined error and breakpoint, basically leaving you browsing the offending line saying “Cell doesNotUnderstand: get_connection_in_direction”. You’d type in the method … and continue the debugger, which would just carry on, finding the new method, as if nothing had happened.

That may not sound all that marvelous, but in fact it is, because often these days we know that in order to send move(NORTH) we need to be ready to receive it …. and so we wind up working bottom up, first putting in the method in Cell and then the one in Dungeon and then the one in View, which essentially inverts the order we would naturally think, which is the view says this so the dungeon gets this and says that so the cell gets that and …

In Smalltalk we could think in the forward direction, and by the time we got down to Cell we would have already discovered that what we thought it needed wasn’t quite what we expected. When we do things bottom up, we often get the bottom method not quite right, because we aren’t clear on what will go in the middle because we haven’t been there yet.

Smalltalk is so fine. So very fine. My life has been good, so it makes no sense to have regrets, but I sure miss Smalltalk.

That’s why sometimes I do the “wishful thinking” approach: it’s not as smooth, as things break, and we don’t get a walkback, but we can at least proceed in the natural order, from need to implementation top down.

Reflection

OK, having distracted myself with this digression, let’s commit this and see where we stand. Commit: refactoring movement code, pushing capability down toward Cell.

In Cell, we still have this, with five users, all in tests

class Cell:
    def attempt_move(self, cell):
        connection = self.get_connection(cell)
        return connection.move()

Up in Dungeon:

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_in_direction(self.player_cell, direction)

    def attempt_move_in_direction(self, cell, direction):
        new_cell = cell.get_connection_in_direction(direction).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

    def attempt_move_to_offset(self, cell, offset):
        new_cell = cell.get_connection_offset(offset).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

    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

We have two users of attempt_move_to_offset, both in tests. I think we’ll likely edit them again in what follows, but let’s fix them up so we can remove this method. I just change them to say Direction.EAST instead of (1,0). remove the method. Commit.

Avoidance

I think I’ve been avoiding dealing with an obvious anomaly, which I’ve mentioned but done nothing about. There is a method attempt_move in Cell, which is even tested. And then Dungeon has its own attempt_move_in_direction, which doesn’t even call a corresponding method in Cell, but instead gets a connection and exercises it.

Sometimes when it’s not clear quite what the code wants, I like to improve the code, massage it a bit, calm it down, tell it it’s pretty, and then things can become more clear. So now I’m looking at this:

class Dungeon:
    def move_player(self, direction):
        self.attempt_move_in_direction(self.player_cell, direction)

    def attempt_move_in_direction(self, cell, direction):
        new_cell = cell.get_connection_in_direction(direction).move()
        if new_cell != cell:
            self.place_player_at(new_cell)

I do think we’re going to be back here messing things up a bit but that’s not a good reason to leave this anomaly: it’s a good reason to get rid of it so that things will be as clear as possible. Clearly this statement is FeatureEnvy:

        new_cell = cell.get_connection_in_direction(direction).move()

This time we’ll go bottom up. Built the feature:

class Cell:
    def attempt_move_in_direction(self, direction):
        return self.get_connection_in_direction(direction).move()

Then use it:

    def move_player(self, direction):
        self.attempt_move_in_direction(self.player_cell, direction)

    def attempt_move_in_direction(self, cell, direction):
        new_cell = cell.attempt_move_in_direction(direction)
        if new_cell != cell:
            self.place_player_at(new_cell)

Green. Commit: moving capability to Cell.

Now inline. Hmm, PyCharm can’t figure out that this is OK. It thinks the method references itself. I wonder if a type hint would help.

PyCharm was right, so I had to fix up two tests to call move_player instead of attempt_move_in_direction, but now we have this:

    def move_player(self, direction):
        new_cell = self.player_cell.attempt_move_in_direction(direction)
        if new_cell != self.player_cell:
            self.place_player_at(new_cell)

    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

Perfect time for a break.

But first, I find the tests using attempt_move and convert them to use attempt_move_in_direction and remove attempt_move.

Commit. But now we have the method attempt_move_in_direction. Let’s rename it to attempt_move! And while we’re at it, make all the get_connection... methods private. Commit: rename method to attempt_move.

OK, enough.

Summary

In a series of small steps we’ve done some good things.

• We’ve removed a half-dozen methods or more from Dungeon, and added, what, net one method to Cell to cover the same functionality.

• We’ve simplified a lot of code that refers to direction pairs (1, 0) to refer to the Enum equivalent, much more clear and safer, in that no one is going to give us a (2,2) by mistake.

• We’ve brought the tests more in line with the code, and it wasn’t even particularly tedious.

• There’s now only one public method in Cell to support movement, attempt_move.

A nice morning’s relaxing work.

I remain concerned about the wild idea about rerunning the interactions, because if the move is possible, we’ll run the interactions in the new cell. But the interactions can decline the move at the last minute, so we’ll have to recognize the case and also run them in the old cell. (In principle the old cell could now refuse, but if it does we’ll ignore it because we got in there somehow.)

The logic is tricky and I’m not clear on its impact on the things we’ve been doing here.

I’m sure we’ll sort it out, but I think I have to code it to understand it. This is often the way, and is, by the way, one of my concerns with the use of LLM-“AI” systems. The “AI” doesn’t understand the code, much less the need. And if the LLM does the writing, the human isn’t going to understand the code either, because we understand the code by working with it, not just by reading it.

That’s a big concern for code quality, if that matters, and for security and privacy, which do matter.

Of course my real objections are much heavier, including the transfer of money from those who need it to those who have way too much, the consumption of water we need to drink, of power we need to keep warm and cool,