Dungeon 140

Musing on Technical Debt. A very interesting series of tiny commits leading to an important change: my invariant.

My understanding of technical debt as Ward originally defined it is that it is the difference between the design we have, and the better design we now have in our head. Ward was not thinking of crufty code, poorly-written code, untested code. He was comparing the design that exists with the design we now see.

This morning as that cat tried to get me to get up and feed her, I was thinking about the Tile. At the time I built the basics of the Tile, I didn’t fully realize how central it would become, though I think I did see that it would be an important component of the system. And I was thinking about two of the Tile’s main methods, addDeferredContents and removeContents. Yesterday I recognized an invariant that we need in the system, namely that an object in the dungeon can be in one and only one tile. Yesterday’s work was aimed at ensuring that could happen.

We wound up with this method:

function Tile:moveEntity(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

When I wrote that method yesterday, it was at first a class method that took two tiles, from and to. I realized that we were always using the tile the thing was on as the from tile, so I changed the method to be sent to the to tile and to fetch the from from the thing.

As the last thing I did with that method yesterday, I moved it into the mostly alphabetic group of instance methods. This morning, as I grabbed that method to display it, I noticed this one right below it:

function Tile:moveEntrant(anEntity, newTile)
    self:removeContents(anEntity)
    newTile:addDeferredContents(anEntity)
end

There seems to be some overlap between these two methods. What happened? Which way is the “right” way? Should one of these be removed? Should one call the other?

I really thought this article would go in a different direction. It’s fascinating how often a thought-design gets overturned in the face of the actual code. Let’s look for senders of moveEntrant.

There is exactly one:

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

The flow down to that method looks like this:

function Tile:legalNeighbor(anEntity, aStepVector)
    local newTile = self:getNeighbor(aStepVector)
    return self:validateMoveTo(anEntity,newTile)
end

function Monster:basicRandomMove(tiles)
    local tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
    self:basicMoveToTile(tile)
end

function Monster:basicMoveTowardPlayer(dungeon)
    local tiles = dungeon:availableTilesCloserToPlayer(self.tile)
    self.tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
end

That last one can be folded into a call to basicRandomMove. That simplifies things. Test and commit: folded basicMoveTowardPlayer to call basicRandomMove.

Some readers may be thinking “how does calling someone who does a thing ‘simplify’ anything over just calling the thing? It adds yet another layer of calling. That’s not simpler!”

But it is. We’ve taken an idea “validate a random one of these tiles”, which occurred in two places, down to one. And we’re reduced the connections into the validate method.

That’s why I like that change. YMMV, and in your program you get to do it your way.

But I’m still on the track of something, and it’s involved in this legal and validate stuff.

Here’s a trick. I’ve burned a couple of brain cells doing and committing a quick refactoring of that one call to validate. So my brain isn’t entirely fresh on the users of validate and legal, and the smart money says that I should refresh my cache.

There are now just two callers of validateMoveTo, aside from tests.

function Tile:legalNeighbor(anEntity, aStepVector)
    local newTile = self:getNeighbor(aStepVector)
    return self:validateMoveTo(anEntity,newTile)
end

function Monster:basicRandomMove(tiles)
    local tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
    self:basicMoveToTile(tile)
end

Lets move up the legal chain:

function Player:moveBy(aStep)
    self.runner:clearTiles()
    local tile = self.tile:legalNeighbor(self,aStep)
    tile:moveEntity(self)
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)
    self:basicMoveToTile(tile)
end

When we look at the validate method:

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

There are two things going on here. (That’s a bit of a code smell, but we do need both things.)

First, we try to enter the cell, which has an important side effect of triggering events, via the Tile Arbiter, which is what opens chests and initiates combat and so on. The attemptedEntrance method also returns a tile, either the new one, if we’re allowed in, or the current tile, if we’re not. Either way, we return the tile we’re supposed to land on, and we return that, again, if we are the call from legalNeighbor, and we finally move wherever we’re told:

function Player:moveBy(aStep)
    self.runner:clearTiles()
    local tile = self.tile:legalNeighbor(self,aStep)
    tile:moveEntity(self)
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)
    self:basicMoveToTile(tile)
end

But we have this:

function Monster:basicMoveToTile(tile)
    tile:moveEntity(self)
end

If we inline that method we can remove it:

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)
    self:basicMoveToTile(tile)
end

That becomes:

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)
end

Similarly:

function Monster:basicRandomMove(tiles)
    local tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
    self:basicMoveToTile(tile)
end

Becomes:

function Monster:basicRandomMove(tiles)
    local tile = self.tile:validateMoveTo(self,tiles[math.random(1,#tiles)])
    tile:moveEntity(self)
end

There’s one more call:

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    if newTile then
        self.monster:basicMoveToTile(newTile)
        self.tile = self.monster:getTile()
    else
        self.monster:pathComplete()
    end
end

We inline:

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    if newTile then
        newTile:moveEntity(self)
        self.tile = self.monster:getTile()
    else
        self.monster:pathComplete()
    end
end

But:

function Tile:moveEntity(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

That sets the tile. Therefore:

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    if newTile then
        newTile:moveEntity(self)
    else
        self.monster:pathComplete()
    end
end

There are now no calls to the basicMoveToTile. Remove it.

Commit: refactor to remove basicMoveToTile.

Now, I want to see what other methods are setting tile directly.

Here’s one:

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.runner = runner
...

We can, and should, do this:

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    tile:moveEntity(self)
    self.runner = runner
...

A small but notable simplification, fanning in a bit more.

function Monster:moveTo(aTile)
    self.tile = aTile
end

Who’s calling this? A search makes me think that it is never called. Remove it and test. Everything seems fine. Nice.

function WayDown:init(tile,runner)
    self.runner = runner
    self.tile = tile
    self.tile:addDeferredContents(self)
end

That becomes:

function WayDown:init(tile,runner)
    self.runner = runner
    tile:moveEntity(self)
end

I think this may fail for lack of the protocol expected, as it’s the first object that isn’t a subclass of Entity that I’ve tried to move. We need to implement getTile and setTile if they’re not there.

function WayDown:setTile(tile)
    self.tile = tile
end

When I rez a PathFinder and let it move, I get this crash:

Tile:332: attempt to call a nil value (method 'getTile')
stack traceback:
	Tile:332: in method 'moveEntity'
	MonsterStrategy:92: in method 'execute'
	Monster:280: in method 'executeMoveStrategy'
	Monster:231: in method 'chooseMove'
	Monsters:75: in method 'move'
	GameRunner:371: in method 'playerTurnComplete'
	Player:253: in method 'turnComplete'
	Player:181: in method 'keyPress'
	GameRunner:321: in method 'keyPress'
	Main:38: in function 'keyboard'

That won’t be the current change. That’ll be in the pathfinder somewhere. What’s MonsterStrategy:92?

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    if newTile then
        newTile:moveEntity(self)
    else
        self.monster:pathComplete()
    end
end

Ah. That was a change or two back, and clearly I never did a decent test. We can’t move the strategy, we need to move the monster:

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    if newTile then
        newTile:moveEntity(self.monster)
    else
        self.monster:pathComplete()
    end
end

Now back to my testing of the pathfinder and waydown …

Works now. Commit: fixed problem in pathfinder strategy; WayDown uses moveEntity.

Remarks

We’re following our nose here, but sniffing out a particular scent, people who set a tile directly. I’m searching for .tile =. It seems that most of those should use our moveEntity (which should get renamed). Let’s rename it now, to moveObject.

Commit: rename moveEntity to moveObject

Back to tile-setters.

function Tile:initDetails()
    self.contents = DeferredTable()
    self.seen = false
    self:clearVisible()
    self:setTile(nil)
end

My general practice is that if a class has an accessor, the class should use the accessor. However, I am not perfect about finding and replacing direct access. I did the one above in the spirit of the general practice. I make a firm purpose of amendment.

The Tile object has a member variable called tile. That should be renamed to sprite. Do it.

The previous clever move in tile was mistaken. Should have been:

function Tile:initDetails()
    self.contents = DeferredTable()
    self.seen = false
    self:clearVisible()
    self.sprite = nil
end

So much for “general practice”. What we had there was an interesting defect. Throughout the whole system, the word tile means an instance of Tile. Except inside Tile, where it meant sprite. I cleverly confused myself. All better now.

It takes me a few more tries to find all the tile references in Tile, but I find them. (Codea search is weak, and so am I.) Commit: rename Tile.tile to Tile.sprite.

Back to searching for .tile =

Here’s an interesting one:

function HangoutMonsterStrategy:init(monster, tile)
    self.monster = monster
    self.tile = tile
    self.min = 3
    self.max = 6
end

The hangout strategy is to remain between 3 and 6 tiles away from the target tile. We have called that tile tile. Not a great name. Let’s rename it to targetTile.

function HangoutMonsterStrategy:init(monster, targetTile)
    self.monster = monster
    self.targetTile = targetTile
    self.min = 3
    self.max = 6
end

function HangoutMonsterStrategy:execute(dungeon)
    local method = self:selectMove(self.monster:manhattanDistanceFromTile(self.targetTile))
    self.monster[method](self.monster, dungeon, self.targetTile)
end

function HangoutMonsterStrategy:selectMove(range)
    if range < self.min  then
        return "basicMoveAwayFromTile"
    elseif self.min <= range  and range <= self.max then
        return "basicMoveRandomly"
    else
        return "basicMoveTowardTile"
    end
end

Test and commit: rename tile to targetTile in HangoutMonsterStrategy.

Continuing … but note that I could stop at any point, each one a bit further along toward goodness.

Ah, a new candidate:

function Loot:init(tile, kind, min, max)
    self.tile = tile
    self.kind = kind
    self.icon = self:getIcon(self.kind)
    self.min = min
    self.max = max
    if tile then tile:addDeferredContents(self) end
end

This will need a setTile, and we need to take note of the fact that we may not have a tile to move to. I think that’s fine, though I rather feel like I should explicitly set it to nil.

function Loot:init(tile, kind, min, max)
    self.tile = nil
    if tile then tile:moveObject(self) end
    self.kind = kind
    self.icon = self:getIcon(self.kind)
    self.min = min
    self.max = max
end

Test. A Loot test fails:

1: Create Health Loot -- Loot:11: attempt to call a nil value (method 'moveObject')

That test isn’t bearing any weight. I remove it entirely. Remind me to speak about that later.

Another test fails, Tiled Game. Bummer, someone was using the FakePlayer from that tab. Revert. Discover it’s a reference to FakePlayer in Loot. Remove that. Spike test fails. Revert again. Someone wants FakeTile. Revert. Remove the test, keep the fake objects.

Test. Tests are green. Rename tab to FakePlayerAndTile. Test again, commit: removed testLoot, retain FakePlayer and FakeTile

Back to the search. The revert lost the changes here:

function Loot:init(tile, kind, min, max)
    self.tile = tile
    self.kind = kind
    self.icon = self:getIcon(self.kind)
    self.min = min
    self.max = max
    if tile then tile:addDeferredContents(self) end
end

function Loot:init(tile, kind, min, max)
    self.tile = nil
    self.kind = kind
    self.icon = self:getIcon(self.kind)
    self.min = min
    self.max = max
    if tile then tile:moveObject(self) end
end

And a set method as usual:

function Loot:setTile(tile)
    self.tile = tile
end

Test. Commit: convert Loot to use moveObject.

I am tempted to break, but I’m having a sort of rhythmic fun doing these replacements.

function Key:init(tile, runner)
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.runner = runner
end

Add a setter and then:

function Key:init(tile, runner)
    self.runner = runner
    tile:moveObject(self)
end

Test. Commit: convert Key to use moveObject.

function Chest:init(tile, runner)
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.runner = runner
    self.closedPic = asset.builtin.Planet_Cute.Chest_Closed
    self.openPic = asset.builtin.Planet_Cute.Chest_Open
    self.pic = self.closedPic
end

Ditto. Test, commit: convert Chest to use moveObject.

function Spikes:init(tile, tweenDelay)
    self.delay = tweenDelay or tween.delay
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.damageTable = { down={lo=1,hi=1}, up={lo=4,hi=7}}
    self.assetTable = { down=asset.trap_down, up=asset.trap_up }
    self.verbs = {down="jab ", up="impale " } 
    self:up()
    self:toggleUpDown()
end

Again, same drill.

function Spikes:init(tile, tweenDelay)
    self.delay = tweenDelay or tween.delay
    tile:moveObject(self)
    self.damageTable = { down={lo=1,hi=1}, up={lo=4,hi=7}}
    self.assetTable = { down=asset.trap_down, up=asset.trap_up }
    self.verbs = {down="jab ", up="impale " } 
    self:up()
    self:toggleUpDown()
end

Test. Test fails. FakeTile needed improvement.

Commit: convert spikes to moveObject.

function Decor:init(tile,runner,kind)
    self.sprite = kind and DecorSprites[kind] or Decor:randomSprite()
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.runner = runner
    self.scaleX = ScaleX[math.random(1,2)]
end

Same drill. Test, commit: Decor converted to moveObject.

That’s the last one. There are no direct setters of tile member variables in the system, other than inside a setTile method. Where are all the senders of setTile?

There are a series of setTile methods used in dungeon creation:

function GameRunner:createTiles()
    self.tiles = {}
    for x = 1,self.tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,self.tileCountY+1 do
            local tile = Tile:edge(x,y, self)
            self:setTile(tile)
        end
    end
end

function GameRunner:setTile(aTile)
    self:getDungeon():setTile(aTile)
end

Inline and cache that:

function GameRunner:createTiles()
    self.tiles = {}
    local dungeon = self:getDungeon()
    for x = 1,self.tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,self.tileCountY+1 do
            local tile = Tile:edge(x,y, self)
            dungeon:setTile(tile)
        end
    end
end

But the method is poorly named. It should be defineTile.

function GameRunner:createTiles()
    self.tiles = {}
    local dungeon = self:getDungeon()
    for x = 1,self.tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,self.tileCountY+1 do
            local tile = Tile:edge(x,y, self)
            dungeon:defineTile(tile)
        end
    end
end

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    self.tiles[pos.x][pos.y] = aTile
end

function Dungeon:setHallwayTile(x,y)
    local t = self:privateGetTileXY(x,y)
    if not t:isRoom() then
        self:defineTile(Tile:room(x,y,t.runner))
    end
end

Test. Tests fail. Don’t look. Revert.

Revert Without Looking???

Yes. I was just flying along. Clearly did something wrong. Don’t care what it was. Revert, slow down, do again. Do better.

function GameRunner:createTiles()
    self.tiles = {}
    for x = 1,self.tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,self.tileCountY+1 do
            local tile = Tile:edge(x,y, self)
            self:setTile(tile)
        end
    end
end

Rename this method to defineTile in this class only.

Ah, interesting. Room:paint calls setTile.

function Room:paint(roomNumber)
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            self.runner:defineTile(self:correctTile(x,y, roomNumber))
        end
    end
end

That was the cause of the trouble, I suspect, and because I was flying along, I didn’t test soon enough to spot it.

We’re green. Commit: rename GameRunner:setTile to defineTile.

Now we can do the rename in dungeon, I think. Yes, green. Commit: rename Dungeon:setTile to defineTile.

Here’s an interesting call to setTile:

function Player:init(tile, runner)
    self.runner = runner
    self:setTile(tile)
    self:initAttributes(self:retainedAttributes())
    self:initSounds()
    self:initVerbs()
end

Can’t we use moveObject here? I think we can and should. Test, commit: use moveObject in Player:init.

At this point, the only sender of setTile in the whole system (other than tests) is this one::

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

This method enforces the invariant that only one tile may contain a given thing. We have improved the design to enforce an important invariant, and in so doing, we’ve removed quite a few duplicated calls to add and remove functions.

We do have some new duplication, however, and one other small issue. The duplication is among Decor, Loot, and so on, the inanimate objects that can be in a tile. They each have their own getTile and setTile, all the same.

And we have this:

function Monster:setTile(tile)
    self.tile = tile
end

function Player:setTile(tile)
    self.tile = tile
    self.tile:illuminate()
    self.tile:addDeferredContents(self)
end

The Player has some special stuff in its setTile, t illuminate the dungeon. And that add? That’s not necessary: we know we just did it because the only caller does it. Remove. Test. Commit: remove extra call to addDeferredContents from Player:setTile.

I’d really like to move that illuminate so that all the setTile methods are pure setters. All but this one are.

Can we move it higher up in the player?

We want to illuminate if the player moves. Where’s that clearTiles call?

function Player:moveBy(aStep)
    self.runner:clearTiles()
    local tile = self.tile:legalNeighbor(self,aStep)
    tile:moveObject(self)
end

We can move the illuminate here, but there will be an issue if we do. For example, if we move the player to room 1, we’ll hit the moveObject and the setTile, but where will the illumination change come from?

This is a bit tricky, and it has been a long morning, 0800 to 1120, so let’s wrap it up here and leave a note for later. The note says:

Change Player:setTile to pure setter. Move illumination. (Cache in “Illuminator”?)

Good enough, let’s have a Summary.

Summary

I wanted to speak about removing tests. Historically, my advice has always been to keep them. They’re mostly harmless and might find something. Other, possibly wiser souls recommend removing them if they haven’t found a problem for a long time. Other folks remove them after using them to build an object. That last idea seems weak to me, because when that object changes, the tests may become valuable again.

I think it’s always risky to remove a test, but they do get in the way, and if they require a lot of changing, they may not be carrying their weight.

I didn’t think deeply about removing the test that I removed. I looked at it, decided it wasn’t all that helpful, and nuked it. Interestingly, its Fake objects turned out to be useful elsewhere. So I kept those.

We’ll see whether we get in trouble without that test. My expectation is that we won’t.

Technical Debt

This article started out talking about technical debt, the difference between the design we have and the design we see. Today we moved to a design which I believe is unquestionably better: we have a single method that maintains the old and new tile collections, such that the rule that a given object can only be on one tile is automatically maintained and enforced. This is a very good thing.

I would argue that looking back from here, we reduced technical debt, but I would have to admit that I didn’t have a clear picture of the design I was moving toward. I did know that I wanted everyone going through that method, but I had no clear picture of the impact on the overall code.

Instead, we just sought out each place where we weren’t using that method, and changed it to use it. We just incrementally improved a bunch of similar things until there were no things left to improve.

Rather nifty. And probably a new record as well: 16 commits in less than four hours. Most of them were extremely atomic. I like the way it felt, with one exception.

The changes were mostly rote, but not entirely so. So when I hit one that wasn’t quite rote, I was moving rather swiftly and messed up the change a time or two. I suspect that’s an issue with tiny commits: one can get to running too fast and suddenly you’re doing that windmilling thing to avoid falling down and sliding on your bare chest on an alley paved with sharp cinders. Or was that just me?

So a steady pace, not too fast, lots of commits. It was good.

But still no poison.

See you next time!

D2.zip