Dungeon 139
We have a defect to fix, and, I suspect, a lesson to re-learn. Then, maybe we’ll look at poison and antidotes. And liddle lamzy divey.
Narrator (v.o.): We don’t get to poison. We do get something interesting.
When testing the new code that sends the princess back to room one when she loses a battle, I found a tile in the dungeon that I couldn’t enter, but that didn’t have anything visible on it. That is what we call in my trade a defect. It came to me last night that when we moved the princess to the center of room one, we did not remove her from whatever tile she was on before, so that the TileArbiter wouldn’t let anyone enter.
I did a quick test patch here:
function Player:youAreDead(combatRound)
self.runner:placePlayerInRoom1()
end
I patched in
self.tile:removeContents(self)
And the problem went away. However, I suspect that’s not the “right fix”, and I’d like to explore that notion.
It is a fundamental notion of the design that a Tile knows its contents, all the entities and items that are on that tile. No entity or object can be on more than one tile qt a time. That means that whenever an entity moves, it needs to be removed from the old tile and added to the new one.
This is what we call an “invariant”. It must always be true that an entity is in the contents of exactly one tile, the one that it is “on”. We use invariants all the time, and I don’t know about you, but I mostly don’t think about them very deeply. Which means that I have experiences like last night’s that sound like “Oh, yeah, we have to remove that thing from that other thing before we put it into that third thing”. We keep the invariants in our head, and if you’re like me, you’re not very formal about it. That’s programming, moving all the stuff around correctly.
What’s really best is to write code that maintains invariants automatically, so that we don’t have to remember them, and so that it’s harder to get it wrong. Since I wrote that defect yesterday, we can be certain that this code right here isn’t maintaining the invariant.
So my mission this morning isn’t just to fix the defect, it’s to fix it so that we are protected from breaking the invariant again in the future. Now nothing is powerful enough to prevent me from writing a defect, but it is possible to make it harder to do. If it’s hard enough, my laziness will protect me.
So let’s do this.
Make Good Behavior Invariant
We begin by looking at the bottom-level methods and their senders.
function Tile:addDeferredContents(anEntity)
self.contents:put(anEntity)
end
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addDeferredContents(anEntity)
end
function Tile:removeContents(anEntity)
self.contents:remove(anEntity)
end
Well, look at that, will ya? It would have worked correctly if I had called moveEntrant, somewhere around here:
function GameRunner:placePlayerInRoom1()
local r1 = self.rooms[1]
local tile = r1:centerTile()
self.player = Player:cloneFrom(self.player, tile,self)
end
function Player:cloneFrom(oldPlayer, tile, runner)
local player = Player(tile,runner)
if oldPlayer then player:cloneCharacterSheet(oldPlayer) end
return player
end
Now part of our problem is that the placePlayerInRoom1 method expects that it’s creating a new dungeon, so there’s no need to clear the old contents, because we’ve made all new tiles.
function Player:init(tile, runner)
self.runner = runner
self:initTile(tile)
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
end
function Player:initTile(tile)
self.tile = tile
self.tile:illuminate()
self.tile:addDeferredContents(self)
end
Are there even more calls to addDeferredContents? It turns out that there are a lot of them, because al the various objects (and the monsters) add themselves to tiles.
There are fewer calls to removeContents, but there are a few, various objects leaving the system, loots being given, and monsters being removed. (The PathFinder removes itself when it “goes down” the WayDown.)
Hm. How can we make this tile rule invariant? I had hoped that I could funnel everyone through moveEntrant and make the add and remove private. But we have people who move from where they are to nowhere, and people who come from nowhere and go somewhere, as well as people who just move.
OK, first things first. Cloning a new Player to move her to room one was a hack. Let’s fix that first. Change this:
function Player:youAreDead(combatRound)
self.runner:placePlayerInRoom1()
end
To this:
function Player:youAreDead(combatRound)
self.runner:movePlayerToRoom1()
end
Now we can implement this correctly, if not well:
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
self.player:moveTo(tile)
end
That moveTo doesn’t exist yet.
function Entity:moveTo(aTile)
self.tile:moveEntrant(aTile)
end
I think that closes the loop. I am mistaken:
Tile:333: attempt to index a nil value (local 'newTile')
stack traceback:
Tile:333: in method 'moveEntrant'
Entity:59: in method 'moveTo'
GameRunner:330: in method 'movePlayerToRoom1'
Player:250: in field '?'
Provider:28: in method 'execute'
Provider:38: in method 'getItem'
Floater:44: in method 'fetchMessage'
Floater:55: in method 'increment'
Floater:39: in method 'draw'
GameRunner:220: in method 'drawMessages'
GameRunner:177: in method 'draw'
Main:33: in function 'draw'
Got the calling sequence wrong, should be:
function Entity:moveTo(aTile)
self.tile:moveEntrant(entity, aTile)
end
No, again:
DeferredTable:46: table index is nil
stack traceback:
DeferredTable:46: in method 'remove'
Tile:341: in method 'removeContents'
Tile:332: in method 'moveEntrant'
Entity:59: in method 'moveTo'
GameRunner:330: in method 'movePlayerToRoom1'
Player:250: in field '?'
Provider:28: in method 'execute'
Provider:38: in method 'getItem'
Floater:44: in method 'fetchMessage'
Floater:55: in method 'increment'
Floater:39: in method 'draw'
GameRunner:220: in method 'drawMessages'
GameRunner:177: in method 'draw'
Main:33: in function 'draw'
What a fool. I am so tempted to erase this mistake, it’s so inept.
function Entity:moveTo(aTile)
self.tile:moveEntrant(entity, aTile)
end
Can’t you write one line of code correctly?? Apparently not..
function Entity:moveTo(aTile)
self.tile:moveEntrant(self, aTile)
end
Now then, what happens? Well, ,what happens is that we don’t move.
The right thing to do at this point is to revert and do over. Am I going to do the right thing? Maybe, but I’m going to look to see what happened first. I got the crawl about princess being down and finding oneself in a familiar room. She just didn’t move. That says we must have used the original tile somehow.
Here’s what we did:
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
self.player:moveTo(tile)
end
function Entity:moveTo(aTile)
self.tile:moveEntrant(self, aTile)
end
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addDeferredContents(anEntity)
end
The Entity does not have its tile set unless we set it.
function Entity:moveTo(aTile)
self.tile:moveEntrant(self, aTile)
self.tile = aTile
end
Now this better work or I definitely will revert.
It moves me to the new room correctly, but it doesn’t illuminate the tiles until I move. This is the wrong solution. Revert.
Well, That’s Discouraging
It’s particularly discouraging because I know a one-line fix for the problem. Let’s do the fix and then decide what to do about this hi-falutin “invariant” idea.
function Player:youAreDead(combatRound)
self.tile:removeContents(self)
self.runner:placePlayerInRoom1()
end
Test. Works fine. Princess moves, room illuminates, when I go back to the room I lost in, the cell isn’t blocked.
Commit: fix defect where player’s defeat cell was blocked after waking up in room one.
OK. What we have here is an interesting situation. If I’m under product development pressure, I’m going to move on and call this good. But the fact remains, there is an idea in my head, that at most one tile can have any given object in its contents, and that idea is not well expressed in the code. That means that when things like this happen again, such as monsters or player teleporting around or running fast, or treasures moving around, or some idea not even yet contemplated … there’s a good chance that I’ll make this mistake again.
One thing that made this error more likely, and that makes it harder to get the code right, is that the tile knows its contents, and the contents know the tile. The problem in my first “fix” this morning was that in focusing on getting the tile contents right, I lost sight of the need for the entity to know the tile as well as the tile to know the entity.
That sort of relationship is always tricky to manage. A rule of thumb is not to have things pointing at each other. When something we point at needs to know us, we pass it a reference to us, in the method that needs it.
In our present design, that ship has sailed, gotten stuck in the canal, and dug bag out. Our design is very much based on there being an “array” of tiles, with contents, and the contents all know the tile they are in.
Just for fun, let’s look at a simple case, a key.
function Key:init(tile, runner)
self.tile = tile
self.tile:addDeferredContents(self)
self.runner = runner
end
function Key:draw(tiny)
if tiny then return end
pushMatrix()
pushStyle()
spriteMode(CENTER)
local g = self.tile:graphicCenter()
sprite(asset.builtin.Planet_Cute.Key,g.x,g.y, 50,50)
popStyle()
popMatrix()
end
function Key:getTile()
return self.tile
end
function Key:take()
self.tile:removeContents(self)
end
I wonder who calls take, and getTile.
function Player:startActionWithKey(aKey)
self.characterSheet:addKey(aKey)
sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
aKey:take()
end
Already we see the concern. This method just gets the key, and if the key doesn’t know the tile, we’d have to tell it, which means we’d need this method to be given the tile:
That’s triggered out of TileArbiter:
t[Key][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithKey}
That’s called from here:
function TileArbiter:moveTo()
local entry = self:tableEntry(self.resident,self.mover)
local action = entry.action
if action then action(self.mover,self.resident) end
local result = entry.moveTo(self)
return result
end
So we could have a new “rule” that the actions pass in the tile, but the TileArbiter doesn’t even know which tiles are being looked at.
function Tile:attemptedEntranceBy(enteringEntity, oldRoom)
local ta
local acceptedTile
local accepted = false
local residents = 0
for k,residentEntity in pairs(self.contents) do
residents = residents + 1
ta = TileArbiter(residentEntity,enteringEntity)
acceptedTile = ta:moveTo()
accepted = accepted or acceptedTile==self
end
if residents == 0 or accepted then
return self
else
return oldRoom
end
end
We could pass down the tiles to moveTo, and then we’d have the tiles accessible.
I hope you begin to see the problem. We’ve woven the “tile knows thing and thing knows tile” idea so deeply into the design that it’s really difficult to get it back out.
So here we are. We have a design that seems to be open to a particular kind of mistake, changing an object’s tile without adjusting contents, and at first blush, fixing the problem “right” seems to involve undoing a rather central design aspect. We don’t see at this moment what a better design would be, much less how to get there. And we’ve just made the code a tiny bit worse with our insertion of yet another chunk of code that spreads the idea around instead of centralizing it.
It may be that I should be smarter, but that ship has sailed. It may be that I should have thought harder, or listened to my elders, but my elders are all mostly dead and I wasn’t listening anyway. So we’re faced with what seems like a lose-lose choice:
- Commit to a major set of changes that we can’t even imagine;
- Commit to making the code a bit worse every time this happens.
The latter loss seems smaller, and so we are likely to choose the easy one-line fix and move on, with another defect now hanging over our head every time we do something similar.
Maybe there’s a “middle path” that will at least centralize this invariant. I had thought we had found it but it didn’t work.
I think I’d like to have a method, somewhere, that moves an object from a tile it’s on (which may not exist) to a tile it’s moving to (which may not exist). If such a thing existed, it could manage the updating of the contents, and the object … if the object understood a setTile method (or if we let the method jam the tile in like a dagger striking to the heart of the object … but I become emotional.
Because the source tile and target tile are both optionally absent, the method can’t be a Tile instance method, but it could be a class method. (Or we could put it somewhere else, like GameRunner, but Tile’s invariants ought to belong to Tile.)
Let’s try that. I should TDD it, but … no, I’ve had to revert today, that tells me doing things by the numbers is a good thing for the day.
I don’t like having to use fake objects for this, but Tiles are so intricate that I feel I have no choice. In any case, I’m just trying to sort out the protocol.
_:test("both exist", function()
local from = FakeTile2()
local to = FakeTile2()
local thing = FakeThing(from)
Tile:moveEntity(thing, from, to)
_:expect(thing:getTile()).is(to)
end)
Now I can make this work, and I will:
function Tile:moveEntity(thing, from, to)
from:removeContents(thing)
thing:setTile(to)
to:addContents(thing)
end
We need to put the add and remove on our fake tiles. Should be doable. Along the way I remember that we need deferred
function FakeTile2:addDeferredContents(thing)
self.contents = thing
end
function FakeTile2:removeContents(thing)
self.contents = nil
end
function Tile:moveEntity(thing, from, to)
from:removeContents(thing)
thing:setTile(to)
to:addDeferredContents(thing)
end
Test runs. Beef it up.
_:test("both exist", function()
local from = FakeTile2()
local to = FakeTile2()
local thing = FakeThing(from)
Tile:moveEntity(thing, from, to)
_:expect(thing:getTile()).is(to)
_:expect(from.contents).is(nil)
_:expect(to.contents).is(thing)
end)
Still runs. Let’s do the other cases:
_:test("no from", function()
local from = nil
local to = FakeTile2()
local thing = FakeThing(from)
Tile:moveEntity(thing, from, to)
_:expect(thing:getTile()).is(to)
_:expect(to.contents).is(thing)
end)
Should fail sending to nil.
2: no from -- TestTileMover:45: attempt to index a nil value (local 'from')
Improve the code:
function Tile:moveEntity(thing, from, to)
if from then from:removeContents(thing) end
thing:setTile(to)
to:addDeferredContents(thing)
end
Test runs.
Now test the other case:
_:test("no to", function()
local from = FakeTile2()
local to = nil
local thing = FakeThing(from)
Tile:moveEntity(thing, from, to)
_:expect(thing:getTile()).is(to)
_:expect(from.contents).is(nil)
end)
3: no to -- TestTileMover:56: attempt to index a nil value (local 'to')
As expected. Enhance:
function Tile:moveEntity(thing, from, to)
if from then from:removeContents(thing) end
if to then
thing:setTile(to)
to:addDeferredContents(thing)
end
end
That fails and should make me think.
3: no to -- Actual: table: 0x28009bcc0, Expected: nil
We haven’t cleared the tile out of the object. I think we should do so. Change the method:
function Tile:moveEntity(thing, from, to)
if from then from:removeContents(thing) end
if to then to:addDeferredContents(thing) end
thing:setTile(to)
end
Test runs. We have our new class method.
I think its name shouldn’t be “entity” though. Maybe for now we’ll just use it for those guys. Funny thing, though, I haven’t even looked at how it should be used. I was so preoccupied with whether or not I could, I didn’t stop to think if I should.
Let’s look at our original concern, the player discovering she’s dead.
function Player:youAreDead(combatRound)
self.tile:removeContents(self)
self.runner:placePlayerInRoom1()
end
We want to call a method on GameRunner to move us rather than use the place method. We tried this before, but now we have a better way:
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
Tile:moveEntity(self.player, self.player:getTile(), tile)
end
We’re going to have that issue with illuminate. Let’s see how it goes.
function Player:youAreDead(combatRound)
self.runner:movePlayerToRoom1()
end
Oops;
function Entity:setTile(tile)
self.tile = tile
end
I feel that I’m sloppy today. This is concerning. Try to cope.
OK, yes, that works except for illumination. Where’s that done?
function Player:initTile(tile)
self.tile = tile
self.tile:illuminate()
self.tile:addDeferredContents(self)
end
function Player:moveBy(aStep)
self.runner:clearTiles()
self.tile = self.tile:legalNeighbor(self,aStep)
self.tile:illuminate()
end
Duplication!
A dead giveaway that we need to reorganize things a bit. Also we note that we really don’t want to illuminate where the monsters are, so monster and player need their own setTile. At least player does, and I’d rather not override.
That initTile looks like a candidate to be the new setTile. Senders? Just one. Change it:
function Player:init(tile, runner)
self.runner = runner
self:setTile(tile)
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
end
function Player:setTile(tile)
self.tile = tile
self.tile:illuminate()
self.tile:addDeferredContents(self)
end
And move the one from entity down to Monster:
function Monster:setTile(tile)
self.tile = tile
end
Test. This ought to work. And it does. Also I should learn to wait until I heal before heading back out.
OK. We can commit this: GameRunner movePlayerToRoom1 uses new Tile:moveEntity method.
Now let’s look for other spots to use this … such as in placePlayerInRoom1:
function GameRunner:placePlayerInRoom1()
local r1 = self.rooms[1]
local tile = r1:centerTile()
self.player = Player:cloneFrom(self.player, tile,self)
end
Well, there is no move here. We could change the cloneFrom. Let’s look at it:
function Player:cloneFrom(oldPlayer, tile, runner)
local player = Player(tile,runner)
if oldPlayer then player:cloneCharacterSheet(oldPlayer) end
return player
end
We should really clear the old player’s tile if it’s there. So we should really do this with our new moveEntity, if we can do it without jumping through our own orifice.
We could clone the whole object and then move it. But we originally did the clone so as not to deal with the Player’s many member variables (too many, probably). What if we did this:
function Player:cloneFrom(oldPlayer, tile, runner)
if oldPlayer then
Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
end
local player = Player(tile,runner)
player:cloneCharacterSheet(oldPlayer)
return player
end
No, that won’t do. Belay that.
function Player:cloneFrom(oldPlayer, tile, runner)
local player = Player(tile,runner)
if oldPlayer then
Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
player:cloneCharacterSheet(oldPlayer)
end
return player
end
This works. It’s a bit weird, because we ask why we’re moving this old player even though we don’t need it.
Arguably we should revert this because it’s obscure. But if we do, we leave ourselves open to another instance of leaving the player lying about in a cell to be tripped over.
I think we should leave this but make it more clear what’s going on:
function Player:cloneFrom(oldPlayer, tile, runner)
local player = Player(tile,runner)
if oldPlayer then
self:makeSureOldPlayerIsRemoved()
player:cloneCharacterSheet(oldPlayer)
end
return player
end
function Player:makeSurePlayerIsRemoved()
Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
end
This pattern is “Explaining Method Name”. We use it when the call we have isn’t clear in its purpose. It’s like a comment, except that it’s executable.
I’m not entirely happy, but I think it’s better this way.
Let’s look for some more removeContents calls. No, first commit: Placing new player ensures old player is cleared out.
Now …
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addDeferredContents(anEntity)
end
Can we either forward this to our new moveEntity or change its caller:
function Tile:validateMoveTo(anEntity, newTile)
if newTile:isRoom() then
local tile = newTile:attemptedEntranceBy(anEntity, self)
self:moveEntrant(anEntity,tile)
return tile
else
return self
end
end
function Monster:basicMoveToTile(tile)
self.tile:moveEntrant(self,tile)
self.tile = tile
end
We can certainly redo that last one:
function Monster:basicMoveToTile(tile)
Tile:moveEntity(self, self:getTile(), tile)
end
Test this carefully. Works fine. Commit: Monster:basicMoveTo uses Tile:moveEntity.
The validateMoveTo is more problematical.
function Tile:legalNeighbor(anEntity, aStepVector)
local newTile = self:getNeighbor(aStepVector)
return self:validateMoveTo(anEntity,newTile)
end
function Player:moveBy(aStep)
self.runner:clearTiles()
self.tile = self.tile:legalNeighbor(self,aStep)
self.tile:illuminate()
end
Ah. The methods return whatever tile should be used, and then we just set it. Let’s defer this whole notion to the tile, including the setting.
We could write this as a step in the right direction:
function Player:moveBy(aStep)
self.runner:clearTiles()
local tile = self.tile:legalNeighbor(self,aStep)
Tile:moveEntity(self, self:getTile(), tile)
end
That’s better already. Does anyone else call legalNeighbor? Monster does:
function Monster:basicMoveRandomly(ignoredDungeon)
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
self.tile = self.tile:legalNeighbor(self,move)
end
We can do the same thing here.
function Monster:basicMoveRandomly(ignoredDungeon)
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tile = self.tile:legalNeighbor(self,move)
Tile:moveEntity(self, self:getTile(), tile)
end
This works. Commit: basicMoveRandom uses Tile:moveEntity
Grr, there were two changes in that commit. Oh well, no harm.
I’m noticing something:
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
Tile:moveEntity(self.player, self.player:getTile(), tile)
end
function Monster:basicMoveRandomly(ignoredDungeon)
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tile = self.tile:legalNeighbor(self,move)
Tile:moveEntity(self, self:getTile(), tile)
end
function Monster:basicMoveToTile(tile)
Tile:moveEntity(self, self:getTile(), tile)
end
function Player:makeSurePlayerIsRemoved()
Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
end
function Player:moveBy(aStep)
self.runner:clearTiles()
local tile = self.tile:legalNeighbor(self,aStep)
Tile:moveEntity(self, self:getTile(), tile)
end
Everyone is calling getTile on the entity they’re moving. Let’s change the calling sequence thus:
function Tile:moveEntity(thing, to)
local from = thing:getTile()
if from then from:removeContents(thing) end
if to then to:addDeferredContents(thing) end
thing:setTile(to)
end
And all the callers. That works. I also noticed this:
function Monster:basicMoveToTile(tile)
Tile:moveEntity(self, tile)
end
But we also have a method like this:
function Monster:basicMoveRandomly(ignoredDungeon)
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tile = self.tile:legalNeighbor(self,move)
Tile:moveEntity(self, tile)
end
And several like this:
function Monster:basicMaintainRangeToPlayer(dungeon)
local tiles = dungeon:availableTilesSameDistanceFromPlayer(self.tile)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
function Monster:basicMoveAwayFromPlayer(dungeon)
local tiles = dungeon:availableTilesFurtherFromPlayer(self.tile)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
function Monster:basicMoveAwayFromTile(dungeon, tile)
local tiles = dungeon:availableTilesFurtherFromTile(self.tile, tile)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
function Monster:basicMoveTowardTile(dungeon, tile)
local tiles = dungeon:availableTilesCloserToTile(self.tile, tile)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
These can be folded together. First a commit, though: remove unneeded parm from moveEntity.
Now a bunch of those methods choose a random move and they all do it themselves.
Change this one:
function Monster:basicMaintainRangeToPlayer(dungeon)
local tiles = dungeon:availableTilesSameDistanceFromPlayer(self.tile)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
To this:
function Monster:basicMaintainRangeToPlayer(dungeon)
local tiles = dungeon:availableTilesSameDistanceFromPlayer(self.tile)
self:basicRandomMove(tiles)
end
function Monster:basicRandomMove(tiles)
self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end
Then use basicRandomMove in all the others:
function Monster:basicMoveAwayFromPlayer(dungeon)
local tiles = dungeon:availableTilesFurtherFromPlayer(self.tile)
self:basicRandomMove(tiles)
end
function Monster:basicMoveAwayFromTile(dungeon, tile)
local tiles = dungeon:availableTilesFurtherFromTile(self.tile, tile)
self:basicRandomMove(tiles)
end
function Monster:basicMoveTowardTile(dungeon, tile)
local tiles = dungeon:availableTilesCloserToTile(self.tile, tile)
self:basicRandomMove(tiles)
end
Then change the base method:
function Monster:basicRandomMove(tiles)
local tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
self:basicMoveToTile(tile)
end
Test extensively. We’re good. Commit: refactor Monster basic moves.
I’m not entirely satisfied yet here. There’s still this stuff with legalNeighbor and validateMoveTo, and their now unnecessary returning of a tile that we then send into the new moveEntity method. We can probably crunch that down a bit more, but we’ll leave it for today.
We have partially closed the door on the defect of moving an entity without adjusting the tiles underneath it. We did it in a slightly irritating way, with a class method on Tile.
However, we no longer pass in two tiles, we found that we could fetch the old tile from the entity. So in two cases, the one where both the old tile and the new one exist, and the case where the newTile exists, we could use an instance method. Do we use the case where there is no new tile?
I think we do not. Let’s change our tests and then replace the method:
function Tile:moveEntity(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addDeferredContents(thing)
thing:setTile(self)
end
That went fine. I find this problem and fix it:
function Player:cloneFrom(oldPlayer, tile, runner)
local player = Player(tile,runner)
if oldPlayer then
self:makeSureOldPlayerIsRemoved(tile)
player:cloneCharacterSheet(oldPlayer)
end
return player
end
function Player:makeSurePlayerIsRemoved(tile)
tile:moveEntity(oldPlayer)
end
I wasn’t passing in the tile. The case never happens anyway, so I am not entirely happy with my explaining method name pattern.
Game is good. Commit: Tile:moveEntity becomes instance method.
Let’s sum up.
Summary
It’s 1250, so I’ve been at this from around four or four and a half hours. And I had the bug fixed at least two hours before now, and it was a one line change.
So was this “worth it”? Honestly, it’s hard to say. I think the code is better now, but I’d have to say that at most an hour of the four and change was spent on the bug, and in fact, since I knew exactly what the one line was, I could have fixed it in less than ten minutes even if Working Copy was slow.
I’m sure that under pressure to deliver, I’d have cut the change and run. The code would be a tiny bit worse, but just a tiny bit. Now it’s more than a tiny bit better but not an amazing amount better.
I think, though, that we are close to “substantially better”, in that we are close to isolating the need to change the entity’s tile pointer and the tile contents collections in sync, maintaining an even large invariant than the one we set our sights on:
A tile knows its contents, and its contents know the tile. When one of these changes, so does the other.
We can only consider this truly done when all tile setting methods and all collection changing methods, specifically enforce the invariant. We are closer to that, and we might be able to get there. That would be a substantial design improvement, and since it’s in the middle of a rather complex interaction of objects, it would be a good thing to accomplish.
So I’m glad I did this, but I freely grant that it used up half a day. In your world, that might not be OK. And, if in your world that’s not OK, maybe there’s too much pressure in your world, and maybe there’s more code that has been given short shrift than just one little bit.
I hope those things aren’t true, and I know that for me, in the past, they have been true. We can’t entirely escape things like that, but we can stay alert and take opportunities to improve things.
Next time, let’s try to get around to poison. See you then!