Some tiny improvements … or are they?

We’ve been trying to move dungeon building out of GameRunner and Dungeon, and into the new DungeonBuilder class. This morning, I plan to take a look at where we are, and see what might be made better.

I should mention again that on a real product effort, we would probably not have the opportunity for an extended period of code improvement as seen in these articles. We might well have in mind “move builder code from GameRunner and Dungeon to DungeonBuilder”, but we’d only grab a few minutes or an hour every now and then, moving slowly toward the improved structure. We’d try to make every step a noticeable improvement, but we’d expect to take very many small steps. That would be fine with us, because we’re all about taking Many More Much Smaller Steps anyway, following GeePaw Hill’s fine motto. And eve the work in these articles has consisted of many commits, probably 45 or 50 commits, each one leaving the program working fine and the code a bit better.

Well, almost each one. I think I intentionally did a broken commit for a few minutes.

At the top, our dungeon creation looks like this:

function GameRunner:createLearningLevel()
    self:defineDungeonBuilder(99)
    self.builder:buildLearningLevel()
    self:prepareToRun()
end

function GameRunner:createLevel(roomCount)
    self:defineDungeonBuilder()
    self.builder:buildLevel(roomCount)
    self:prepareToRun()
end

function GameRunner:createTestLevel(roomCount)
    self:defineDungeonBuilder()
    self.builder:buildTestLevel(roomCount)
    -- self:prepareToRun() -- probably not needed
end

There’s duplication here that we’d like to get rid of. And there’s a createHexLevel method that isn’t converted. I’m still trying to convince myself to defer that whole hex idea. We might learn a lot but I wouldn’t enjoy it. While our learning is important to me, my general enjoyment is more important. It’s not like I’m getting paid to do this stuff.

I’m wondering now how often GameRunner actually references the builder. I think it was in place most as a temporary measure. Let’s find out.

I find this:

function GameRunner:setupMonsters()
    self.builder:setupMonsters()
end

That’s a tag end left over from the transition. We moved things incrementally by letting DungeonBuilder and GameRunner pass messages back and forth as we moved things over. That method is no longer used. Deleted. Test, commit: remove unused GameRunner:setupMonsters.

Now the only references to a builder are there in those methods above (and a nil initialization in init). Let’s not store the builder any more:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

This method initializes two member variables, dungeon and builder, and I rather suspect the “right” thing will turn out to be having the builder return a dungeon all fully crafted. One thing at a time, though. First, change to return the builder.

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    return DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

Now fix the callers to expect that.

function GameRunner:createLearningLevel()
    local builder = self:defineDungeonBuilder(99)
    builder:buildLearningLevel()
    self:prepareToRun()
end

function GameRunner:createLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    builder:buildLevel(roomCount)
    self:prepareToRun()
end

I expect this to work. I’ll test of course, to find out why I’m mistaken. Oops, there were three:

function GameRunner:createTestLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    builder:buildTestLevel(roomCount)
    -- self:prepareToRun() -- probably not needed
end

And a test fails, I think my one and only test directed at DungeonBuilder:

1: creation no builder -- Actual: nil, Expected: nil

        _:test("creation", function()
            local runner = GameRunner()
            _:expect(runner.builder).is(nil)
            runner:defineDungeonBuilder()
            local builder = runner.builder
            _:expect(builder, "no builder").isnt(nil)
            _:expect(builder.RUNNER, "no runner").is(runner)
            _:expect(builder.dungeon, "no dungeon").isnt(nil)
        end)

That becomes:

        _:test("creation", function()
            local runner = GameRunner()
            _:expect(runner.builder).is(nil)
            local builder = runner:defineDungeonBuilder()
            _:expect(builder, "no builder").isnt(nil)
            _:expect(builder.RUNNER, "no runner").is(runner)
            _:expect(builder.dungeon, "no dungeon").isnt(nil)
        end)

Nicer anyway, didn’t have to rip the guts out of the GameRunner this way. Tests green. Game works. Commit: GameRunner no longer has DungeonBuilder member variable.

Now it seems reasonable that DungeonBuilder would return a dungeon rather than be given one.

Let’s code that by intention:

function GameRunner:createLearningLevel()
    local builder = self:defineDungeonBuilder(99)
    self.dungeon = builder:buildLearningLevel()
    self:prepareToRun()
end

function GameRunner:createLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    self.dungeon = builder:buildLevel(roomCount)
    self:prepareToRun()
end

function GameRunner:createTestLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    self.dungeon = builder:buildTestLevel(roomCount)
    -- self:prepareToRun() -- probably not needed
end

Were we to run this now I think things would explode. Let’s find out. Yes. Hundreds of tests fail. Fix the problem:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    self:defineLearningLayout()
    self:placePlayer()
    self:customizeLearningContents()
    self:dcFinalize()
    return self.dungeon
end

And don’t save the dungeon member variable here:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    local dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    return DungeonBuilder(self,dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

This ought to work. Well. That’s how much I iknow:

GameRunner:201: attempt to index a nil value
stack traceback:
	GameRunner:201: in method 'getTile'
	Tile:290: in method 'getSurroundingInfo'
	Tile:156: in method 'convertEdgeToWall'
	Dungeon:194: in method 'convertEdgesToWalls'
	DungeonBuilder:154: in method 'defineDungeonLayout'
	DungeonBuilder:61: in method 'buildTestLevel'
	GameRunner:64: in method 'createTestLevel'
	Tests:23: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:36: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:13: in function 'testDungeon2'
	[string "testDungeon2()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

I’ll take one look, then if it’s not obvious, I’ll revert. The problem is here:

        _:before(function()
            _TileLock = TileLock
            _tweenDelay = tween.delay
            tween.delay = fakeTweenDelay
            _bus = Bus
            Bus = EventBus()
            runner = GameRunner(20,20)
            runner:defineDungeonBuilder()
            runner:createTestLevel(0)
            _dc = DungeonContents
            DungeonContents = DungeonContentsCollection()
            TileLock = false
        end)

There’s a somewhat general lesson here. My tests, like this one, are tricky to set up, because I’m building up a testable nest of objects, while trying not to create an entire game to test. So the tests do odd things. This is a sign of something we should think about. But for now …

OK, I don’t see the problem. I think it is a callback to runner during creation, that requires runner to know the dungeon. Revert. Test. We’re good.

Our mission is to have DungeonBuilder return the dungeon as built. That part should work, so let’s do this:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    self:defineLearningLayout()
    self:placePlayer()
    self:customizeLearningContents()
    self:dcFinalize()
    return self.dungeon
end

That should be harmless, no one is looking. Test. Commit: dungeon building returns dungeon from DungeonBuilder methods.

Now let’s see who is talking to the runner at the behest of DungeonBuilder, and why.

...
    self.dungeon:createRoomsFromXYWHA(t, self.RUNNER)
...
    MonsterPlayer(self.RUNNER)
...

function DungeonBuilder:placePlayer()
    local r1 = self:getRooms()[self.playerRoomNumber]
    local tile = r1:centerTile()
    self.RUNNER:placePlayer(tile)
end
...

function DungeonBuilder:placeWayDown()
    local r1 = self:getRooms()[1]
    local target = r1:centerTile()
    self.wayDownTile = self.dungeon:farawayOpenRoomTile(r1, target)
    local wayDown = WayDown(self.wayDownTile)
    self.RUNNER:setWayDown(wayDown)
end
...

function DungeonBuilder:setupMonsters()
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, self.numberOfMonsters, self.levelNumber, self.playerRoomNumber)
    monsters:createHangoutMonsters(self.RUNNER, 2, self.wayDownTile)
    self.dungeon:setMonsters(monsters)
end

Now, we have the legitimate runner, so what we care about is whether any of those methods, when called, ask the runner to do something that will cause it to try to access its dungeon member variable, which is not yet set.

Why am I doing this? Two reasons, at least. First, it seems like the DungeonBuilder should build and return a Dungeon instance. Second, setting up the GameRunner member variable where it is now seems like a hidden side effect, and that’s worth improving if we can. So we’ll do a little searching.

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner)
    end
    self.rooms = ar.reduce(roomTable, Array{}, makeRoom)
end

function Room:fromXYWHA(xywhaTable, runner)
    local x,y,w,h,a = table.unpack(xywhaTable)
    return Room(x,y,w,h, runner, true, a)
end

function Room:init(x,y,w,h, runner, paint, announcement)
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.runner = runner
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(runner:getDungeon(), announcement)
    end
end

function Room:paint()
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            local existing = self.runner:getTile(vec2(x,y))
            existing:convertToRoom()
        end
    end
end

Meh. Here we call back to the runner to get the tile. That will, of course, ask the Dungeon:

function GameRunner:getTile(aPosition)
    return self:getDungeon():getTile(aPosition)
end

You’d think that the Room would know a dungeon to be in, not just a runner. I think it may actually need the runner for some purpose. Let’s find out.

A little research, more that I’d like to do, tells me that Room does not need to know the runner, but that Room creates Tile instances, and they need to know the runner because they need to be able to return the player if they have her:

function Tile:getPlayerIfPresent()
    local player = self.runner.player
    if self:contains(player) then return player else return nil end
end

I’m fairly certain that all the other references to runner in Tile could just as well refer to the dungeon instead.

One moderately hacky hack would be to change the check for contains to check for an object that is_a Player. Then we could rewire Tile to accept a Dungeon instead of a Runner on creation and then change all its methods.

This seems like too much, at least for now. We can make a card to convert Tile to receive Dungeon instead of GameRunner.

But wait, let’s see who calls that method.

function Spikes:toggleUpDown()
    local tile = self:getTile()
    if tile == nil then return end -- we're being called back from a previous dungeon.
    self.state = ({up="down", down="up"})[self.state]
    if self.stayDown then self.state = "down" end
    if self.state == "up" then
        local player = tile:getPlayerIfPresent()
        if player then self:actionWith(player) end
    end
    self.delay(2, self.toggleUpDown, self)
end

There’s only the one. Let’s try something:

function Spikes:toggleUpDown()
    local tile = self:getTile()
    if tile == nil then return end -- we're being called back from a previous dungeon.
    self.state = ({up="down", down="up"})[self.state]
    if self.stayDown then self.state = "down" end
    if self.state == "up" then
        local player = tile:getInstanceIfPresent(Player)
        if player then self:actionWith(player) end
    end
    self.delay(2, self.toggleUpDown, self)
end

function Tile:instanceOrNil(klass)
    for i,t in ipairs(self:getContents()) do
        if t:is_a(klass) then return t end
    end
    return nil    
end

I have high hopes for this. Test. Tests run. Commit: Remove Tile: getPlayerIfPresent, replace with getInstanceOrNil(Player). Tile:contains currently unused.

Now, if I’m not mistaken–and often I am mistaken–we’ve made Tile able to be created with a Dungeon member variable rather than a GameRunner. That’s a rather substantial design difference, and our time for refactoring is up for today. Let’s make a couple of cards:

Issue 31: Runner is called during dungeon build, so we can’t just return the dungeon when done. Change Room and Tile to have Dungeon, not Runner.

Issue 32: Tile references its runner member variable mostly/only to get a dungeon. Change to have only Dungeon. See Issue 31.


That’s probably enough to remind me what to do next. I might wrote more, or put notes in the code, if I thought it would be a while before anyone got to this.

Just wondering: in all the hours you and your team spend programming every week, how many hours might one or two of you be able to put into improving the code with no discernible delay to anything important? I bet it would be possible …

Let’s sum up.

Summary

We’ve made a few small improvements to the dungeon building, moving in the direction of having no building code in GameRunner, and very little in Dungeon.

We have some duplication that may need addressing:

function GameRunner:createLearningLevel()
    local builder = self:defineDungeonBuilder(99)
    builder:buildLearningLevel()
    self:prepareToRun()
end

function GameRunner:createLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    builder:buildLevel(roomCount)
    self:prepareToRun()
end

function GameRunner:createTestLevel(roomCount)
    local builder = self:defineDungeonBuilder()
    builder:buildTestLevel(roomCount)
    -- self:prepareToRun() -- probably not needed
end

And here:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
    return self.dungeon
end

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    self:defineLearningLayout()
    self:placePlayer()
    self:customizeLearningContents()
    self:dcFinalize()
    return self.dungeon
end

I’m tempted to do something clever here, like some kind of Execute Around Method, but perhaps there is a less clever way to remove this duplication. I imagine that in the fullness of time we’d like to pass in a data structure or object that defined the dungeon, ideally a text string or file. But that is not for today.

For today, a few improvements to the code. Good enough.