Let’s do some more opportunistic refactoring toward DungeonBuilder. And I have a few thoughts to think about the direction I’ve taken.

My basic view on “big refactorings”, such as “Let’s move all the dungeon creation off to a new object and get GameRunner, and even Dungeon class, out of the building business” is “No, let’s not.”

“Why do you feel this way, Ron?”

Thanks for asking. It all stems from something that happened when I was a child, probably, but more recently1, my experience in software efforts is that they are generally focused on getting certain capabilities into the system, and there are always more things desired2 than there is time for.

Accordingly, if we asked to take a week or two off for some “big refactoring”, Questions Would Be Asked, and permission would not be granted. And, of course, if we just paused development unilaterally, the Higher-Ups would descend from Olympus and sort us right out.

I conclude that we often will benefit from some “big refactoring”, and that we can’t do it as a big thing, but only as many small things. And that’s what’s been going on here. There have been probably at least 25 individual commits during the DungeonBuilder effort, and every one of them left the game working just fine. Instead of doing those commits one every ten minutes, we could have done them one ever day, or one every week. It would take longer to get a visible result, but nothing stands in the way of improving the system.

I suppose it would be possible to build a system that couldn’t be improved incrementally, though I don’t think I could do it. But let’s assume that none of us writes code quite that horrible, and assume that we can always make it better in small steps. And, with that assumption, let’s grab a few minutes, or an hour or so, every day or week, to make things better.

It’s even rather fun. You can get into a decent state of flow doing these things, and it can be a relaxing time away from whatever random feature they’re after us for. Good for software health and mental health as well.

Let’s get to it.

Issues?

The largest issue when we spread a refactoring over time is that our memory won’t be fresh on the subject and we’ll need to look around for our next steps. It can help to do that paired or in a mob, even if the work is not going to be done paired or mobbed. (Even though it would likely go better if it were.)

But the cat doesn’t want to help this morning, so I’m on my own.

If I recall where we are, we have moved the game’s main level creation method, createLevel over to DungeonBuilder, and have it calling back outside itself where it needs to. What we have not done is to deal with the other level creations much at all. There’s createTestLevel or something like that, and createLearningLevel.

My thinking was that the createLevel was the most complex of the builds, and that it would serve as an example that the other methods could copy. I still believe that, but I’m a bit concerned that the current breakout of the build will make that a bit odd. I’m going to try to notice where that might be true and comment on it, and I might even try to improve what’s there. But that would be speculative, unless we change direction and do a smaller builder. That doesn’t seem tasty: I’d really prefer to get one done.

Nonetheless, there’s a design tension there, in my mind if not in the code.

An additional design issue is hanging over us. We’ll encounter it, I’m sure, so if I had forgotten it, it would arise anyway. The issue is that the GameRunner has the Player object, and the Player’s creation is tricky. With all the creation moved over to the builder, it’s not clear how that particular interaction should be managed.

But no sense borrowing trouble: we’ll encounter it soon enough. Just stuff that’s on my mind.

Let’s see what’s in the code.

The Code Is Reality

All speculation aside, the code is the thing. Where are we? The DungeonBuilder is too long to dump here in the article: we don’t need to take it all in. Its top levels look like this:

function DungeonBuilder:init(runner, dungeon)
    self.runner = runner
    self.dungeon = dungeon
    self.playerRoom = 1 -- could this change? If so, make param.
end

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self.runner:buildLevelA(roomCount)
    self:defineDungeonLayout(roomCount)
    self:buildLevelC(roomCount)
end

function DungeonBuilder:buildLevelC()
    self:placePlayer()
    self:customizeContents()
    self.runner:customizeContents()
    self.runner:prepareCrawlAndTimers()
    self.runner:dcFinalize()
end

function DungeonBuilder:customizeContents()
    self:placeSpikes(15)
    self:placeLever()
    --self:placeDarkness()
    self:placeNPC()
    self:placeLoots(10)
    self:createDecor(30)
    self:createThings(Key,5)
    self:createThings(Chest,5)
end

At this level, we can quickly see where we have issues: the references to self.runner reflect places where we’ve not yet successfully moved building-related functionality over to Builder. Well, mostly reflect that. Some may be desirable. At this point, they’re all certainly flags to attract our attention.

Speaking of that … I wish those lines were more readily visible. I’m going to do something odd: I’m going to rename the runner member variable to be RUNNER, to make the troubling lines show up better. I grant that that’s weird, but I think I’ll like it. We’ll see.

That was quickly done. Test. One test fails:

1: creation no runner -- Actual: nil, Expected: GameRunner

Basically our only direct test on DungeonBuilder:

        _:test("creation", function()
            local runner = GameRunner()
            _:expect(runner.builder).is(nil)
            runner:createNewDungeon()
            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)

Should read:

            _:expect(builder.RUNNER, "no runner").is(runner)

Tests run, game runs. Commit: rename runner member var to RUNNER for visibility in DungeonBuilder.

Here’s what we have to choose from for something to do:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self.RUNNER:buildLevelA(roomCount)
    self:defineDungeonLayout(roomCount)
    self:buildLevelC(roomCount)
end

function DungeonBuilder:buildLevelC()
    self:placePlayer()
    self:customizeContents()
    self.RUNNER:customizeContents()
    self.RUNNER:prepareCrawlAndTimers()
    self.RUNNER:dcFinalize()
end

(I should mention that I don’t like the names like buildLevelC. A B and C were meant just to be sections of the build. They have no useful meaning and the names are confusing. “Please take me to level C.” “There is no level C.”

Now we should quickly review those runner methods to see which one to work on next:

function GameRunner:buildLevelA(count)
    -- determine level
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
end

This just sets the dungeon level. I’m pretty sure that Monster creation is conditioned by the dungeon level, so I decide to check that. A quick glance at DungeonBuilder tells me that it’s not building the monsters.

function GameRunner:customizeContents()
    self:placeWayDown()
    self:setupMonsters(6) -- must follow waydown
    self:createButtons()
end

Sure enough, that’s still in GameRunner. Let’s follow this trail a bit longer. We might find a quick improvement.

function GameRunner:placeWayDown()
    local r1 = self:getRooms()[1]
    local target = r1:centerTile()
    local tile = self:getDungeon():farawayOpenRoomTile(r1, target)
    self.wayDown = WayDown(tile)
end

We have a similar issue to the one we have with Player: GameRunner wants to know where the WayDown is. It’s used to place some monsters, but it also gets used, I imagine, when we rez a Pathfinding cloud to lead us to the WayDown.

I’m going to try an idea. It’s tempting to do something sophisticated like allow callbacks from DungeonBuilder methods, but let’s try just accepting coupling back from DungeonBuilder to the runner. Maybe it won’t be nasty.

I’l move the WayDown creation over to DungeonBuilder.

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

And …

function GameRunner:setWayDown(wayDown)
    self.wayDown = wayDown
end

And remove the call from GameRunner and add it here:

function DungeonBuilder:customizeContents()
    self:placeSpikes(15)
    self:placeLever()
    --self:placeDarkness()
    self:placeNPC()
    self:placeLoots(10)
    self:createDecor(30)
    self:createThings(Key,5)
    self:createThings(Chest,5)
    self:placeWayDown()
end

I expect this to work. Test and discover … that you forgot to change the class name when you moved the method:

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

I need a refactoring browser for Codea. Test runs, game works, pathfinder works. Commit: move WayDown creation to DungeonBuilder, provide WayDown object back to GameRunner.

Next?

I noticed when doing the WayDown that we do seem to be calling back with the Player:

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

function GameRunner:placePlayer(tile)
    self.player = Player:cloneFrom(self.player, tile,self)
end

So that’s already done. I forgot that I did that. Heck, it was way back two days ago.

“Why didn’t you edit out that part of the article?”

I didn’t remove that “mistake” because when we do these refactorings incrementally over time, we will forget what we have done and rediscover it. It’s all part of the game: we work on the code as it it, not as we believe it is.

“Isn’t there danger that someone will do the thing twice, breaking the system?”

Maybe there’s some danger of that. However, if we are careful to remove duplication as we work, there will be only one place where, say, Player is placed, so we are just about guaranteed to find it and learn what the situation actually is. We do need to be careful. I know of no way to avoid forgetting, nor of avoiding all mistakes. I just code and test as carefully as I can.

Speaking of Tests

When I started with DungeonBuilder, I spoke of tests, and did test-drive the original implementation of the class. But after that, I’ve just moved capability from GameRunner into DungeonBuilder, and it’s entirely possible that I’ve made a mistake. I can think of one kind that I might not even notice.

Suppose that I had started moving, say, Decor, and removed the call from GameRunner and then become distracted and failed to move it to DungeonBuilder. Decor might just go missing, and no tests would break. There are no tests for “dungeon quality”. And building without various components is implicitly allowed. I’ve even shown that in methods like this:

function GameRunner:createLearningLevel()
    self.dungeonLevel = 99
    self:createNewDungeon()
    self:dcPrepare()
    
    -- customize rooms and connections
    self:createLearningRooms()
    self:connectLearningRooms()
    
    -- paint dungeon correctly
    self.dungeon:convertEdgesToWalls()
    
    self:placePlayerInRoom1()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    --self:placeSpikes(5)
    --self:placeLever()
    --self:setupMonsters(6)
    --self.keys = self:createThings(Key,5)
    --self:createThings(Chest,5)
    --self:createLoots(10)
    --self:createDecor(30)
    --self:createButtons()
    
    -- prepare crawl
    self.cofloater:startCrawl()
    Announcer:sayAndSave(self:crawlMessages(self.dungeonLevel))
    self:startTimers()
    
    self:dcFinalize()
end

There I just commented out the various setups that I didn’t want. I let them there as reminders for things I might want to do when enhancing the learning level, which is a work in progress, to be enhanced when the Customer (me) tells the Programmers (me) to do so.

I don’t see a way to write a decent test for this kind of mistake. Perhaps you do. If so, let me know. I’d prefer not to write two separate dungeon definitions and then use the one to build and the other to test. That seems tedious.

Anyway, what’s next? Let’s look again at customizeContents: it seems like it was nearly done.

Carrying On

function GameRunner:customizeContents()
    self:setupMonsters(6) -- must follow waydown
    self:createButtons()
end

Seems like Buttons don’t belong in DungeonBuilder at all, and setting up monsters does. Let’s look at that: I anticipate one tiny issue.

function GameRunner:setupMonsters(howManyRandom)
    self:getDungeon():setupMonsters(self, self.dungeonLevel, self.playerRoom, self.wayDown, howManyRandom)
end

function Dungeon:setupMonsters(runner, level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(runner, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(runner,2, wayDown:getTile())
    self:setMonsters(monsters)
end

function Dungeon:setMonsters(monsters)
    self.monsters = monsters
end

GameRunner is hardly involved here at all. All it does is provide information that’s used to decide where to put monsters, which monsters to select (level), and what tiles to avoid.

Certainly we could provide the level number when we create the DungeonBuilder. We could remember the player room (which is always room 1, a rule that we might need to change someday, but today is not that day, and that day may never come). We could remember the wayDown.

This is a bit intricate, because of all the parameters.

Make the Change Easy, then Make the Easy Change

The above phrase is from Kent Beck, my old master in the trade3.

We’ll make this change easier by saving the information we need, bit by bit. I’ll save the dungeon level for last.

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

I think we only need the tile, so in the spirit of not extending my coupling too far, I’ll just save the tile. Now I think we need the player room.

(I’m starting to wish that I’d started this top down instead of bottom up. I’m putting in things because I think I’m going to need them. That’s not bright.)

In fact, since it’s not bright, I’ll revert and go in what I think is a better direction. One big advantage to lots of tiny commits.

Is There Another “Make the Change Easy”?

I had thought that having the DungeonBuilder “already know” the things needed to place the monsters would make it easier to move the method over, especially since it bounces now from GameRunner to Dungeon. Let’s look again and see what might be easy and get us going.

function GameRunner:setupMonsters(howManyRandom)
    self:getDungeon():setupMonsters(self, self.dungeonLevel, self.playerRoom, self.wayDown, howManyRandom)
end

function Dungeon:setupMonsters(runner, level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(runner, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(runner,2, wayDown:getTile())
    self:setMonsters(monsters)
end

function Dungeon:setMonsters(monsters)
    self.monsters = monsters
end

All the work is done in the Monsters class. I’m thinking of a few steps here. First, change the Dungeon method to call DungeonBuilder, with all the parameters. Then, look around and see how to take Dungeon out of the loop. Then … I don’t know yet.

No. We can do better. Let’s move this entire method over to DungeonBuilder, and let GameRunner call it there for now, like this:

function GameRunner:setupMonsters(howManyRandom)
    self.builder:setupMonsters(self, self.dungeonLevel, self.playerRoom, self.wayDown, howManyRandom)
end

DungeonBuilder:178: attempt to call a nil value (method 'setMonsters')
stack traceback:
	DungeonBuilder:178: in method 'setupMonsters'
	GameRunner:481: in method 'setupMonsters'
	GameRunner:153: in method 'customizeContents'
	DungeonBuilder:59: in method 'buildLevelC'
	DungeonBuilder:53: in method 'buildLevel'
	GameRunner:139: in method 'createLevel'
	Main:64: in function 'setup'

I really need a ‘move method” refactoring tool. I wish I were smart enough to write one.

function DungeonBuilder:setupMonsters(runner, level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(runner, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(runner,2, wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

Tests and game runs. Commit: moved Dungeon:setMonsters to DungeonBuilder.

That was easy. Does it make the next changes easier? It does: We can now go through this method and its caller and arrange to need fewer parameters, until we need none.

function DungeonBuilder:setupMonsters(level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

function GameRunner:setupMonsters(howManyRandom)
    self.builder:setupMonsters(self.dungeonLevel, self.playerRoom, self.wayDown, howManyRandom)
end

Removed runner. Test. Commit: removed runner parameter from setUpMonsters.

I think that level and howManyRandom will become creation parameters for the builder. We need to deal with wayDown and playerRoom. I’ll do those first. One at a time: Many More Much Smaller Steps.

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

We can save the room here:

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

And now remove that parameter:

function DungeonBuilder:setupMonsters(level, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

function GameRunner:setupMonsters(howManyRandom)
    self.builder:setupMonsters(self.dungeonLevel, self.wayDown, howManyRandom)
end

Test.

DungeonBuilder:168: attempt to compare table with number
stack traceback:
	DungeonBuilder:168: in method 'randomRoomTile'
	DungeonBuilder:125: in method 'placeLoots'
	DungeonBuilder:69: in method 'customizeContents'
	DungeonBuilder:58: in method 'buildLevelC'
	DungeonBuilder:53: in method 'buildLevel'
	GameRunner:139: in method 'createLevel'
	Main:64: in function 'setup'

Huh? Oh. Didn’t correctly do the method.

function DungeonBuilder:setupMonsters(level, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, self.playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

Same error. I’ll allow myself one look at randomRoomTile but if that’s not obvious, I’m going to revert. “The changes I made couldn’t have affected that.”

I don’t see it. Revert. Do again. Maybe do something different.

function DungeonBuilder:setupMonsters(level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

Let’s do the wayDown this time. Maybe whatever I did wrong will fade from memory.

Our code wants the wayDown tile not the wayDown. Again, we’ll just save that:

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(level, playerRoom, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, self.wayDownTile)
    self.dungeon:setMonsters(monsters)
end

function GameRunner:setupMonsters(howManyRandom)
    self.builder:setupMonsters(self.dungeonLevel, self.playerRoom, howManyRandom)
end

I expect this to work: It does. Commit: remove wayDown parameter from setupMonsters.

OK, we’re after playerRoom. Something broke before, so I’ll be extra careful. First let’s see what it actually is.

It’s a NUMBER!?!? It’s the named magic number 1. Super. And we don’t even honor it:

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

Let’s fix that as we pass by.

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

We should pass it as a construction parameter, but we’re on another thread. Even this is a distraction but let’s test commit and move on. Commit: reference playerNumber in placePlayer.

function DungeonBuilder:setupMonsters(level, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.RUNNER, howManyRandom, level, self.RUNNER.playerRoom)
    monsters:createHangoutMonsters(self.RUNNER, 2, self.wayDownTile)
    self.dungeon:setMonsters(monsters)
end

function GameRunner:setupMonsters(howManyRandom)
    self.builder:setupMonsters(self.dungeonLevel, howManyRandom)
end

Here I just referenced the value remotely and removed the parameter. We’re getting close.

I think we want to create DungeonBuilder knowing the level number it is building, and how many random monsters to build, and the room in which to place the player. Let’s do that now:

function DungeonBuilder:init(runner, dungeon, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    self.dungeon = dungeon
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

function GameRunner:createNewDungeon()
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    -- determine level
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    self.builder = DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

This felt a little ragged. I’m going to test and see what I’ve forgotten or done wrong.

DungeonBuilder:170: attempt to compare nil with number
stack traceback:
	DungeonBuilder:170: in method 'randomRoomTile'
	DungeonBuilder:127: in method 'placeLoots'
	DungeonBuilder:71: in method 'customizeContents'
	DungeonBuilder:60: in method 'buildLevelC'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:139: in method 'createLevel'
	Main:64: in function 'setup'

function DungeonBuilder:placeLoots(n)
    for i =  1, n or 1 do
        local tab = RandomLootInfo[math.random(1,#RandomLootInfo)]
        local tile = self:randomRoomTile(self.playerRoom)
        Loot(tile, tab[1], tab[2], tab[3])
    end
end

I renamed to playerRoomNumber. Let’s have a bit more care.

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

function DungeonBuilder:createThings(aClass, n)
    for i = 1,n or 1 do
        local tile = self:randomRoomTile(self.playerRoomNumber)
        aClass(tile,self)
    end
end

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

Test. Feeling good about this now. Works. Commit: passed level number, player room number, and number of monsters to DungeonBuilder init.

Now setupMonsters needs no parameters:

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

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

Test, feeling good. Feelings dashed:

Monster:53: bad 'for' limit (number expected, got nil)
stack traceback:
	Monster:53: in method 'getRandomMonsters'
	Monsters:94: in method 'createRandomMonsters'
	DungeonBuilder:178: in method 'setupMonsters'
	GameRunner:482: in method 'setupMonsters'
	GameRunner:150: in method 'customizeContents'
	DungeonBuilder:61: in method 'buildLevelC'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:139: in method 'createLevel'
	Main:64: in function 'setup'

Did I refer to the wrong thing? Yes, it’s nunberOfMonsters. Sheesh.

Test. OK. Commit: setupMonsters requires no parameters, only DungeonBuilder member variables set at creation.

Now we can remove the call to setupMonsters from GameRunner and move it to the builder:

function GameRunner:customizeContents()
    self:setupMonsters(6) -- remove
    self:createButtons()
end

~~~lua
function DungeonBuilder:customizeContents()
    self:placeSpikes(15)
    self:placeLever()
    --self:placeDarkness()
    self:placeNPC()
    self:placeLoots(10)
    self:createDecor(30)
    self:createThings(Key,5)
    self:createThings(Chest,5)
    self:placeWayDown()
    self:setupMonsters()
end

Test. We’re good. Commit: moved setupMonsters entirely into DungeonBuilder.

Now let’s normalize out the creation of buttons which is here:

function GameRunner:customizeContents() -- remove
    self:createButtons()
end

function GameRunner:createLevel(count)
    self:createNewDungeon()
    self.builder:buildLevel(count)
    self:createButtons()
end

And remove the call to customizeContents.

function DungeonBuilder:buildLevelC()
    self:placePlayer()
    self:customizeContents()
    -- self.RUNNER:customizeContents() -- remove
    self.RUNNER:prepareCrawlAndTimers()
    self.RUNNER:dcFinalize()
end

Test. Works. Commit: buttons created in GameRunner, customize contents completely moved to DungeonBuilder.

Time to look around.

RESTrospective

OK, time to breathe. Look around, see what’s going on in the room, where’s the cat, that sort of thing …

I’d like to assess where everything is now and see what needs changing. I think we can probably now identify things that belong entirely to GameRunner or Dungeon, such as the buttons, and make some near-final determinations of what goes where.

function DungeonBuilder:init(runner, dungeon, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    self.dungeon = dungeon
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self.RUNNER:buildLevelA(roomCount)
    self:defineDungeonLayout(roomCount)
    self:buildLevelC(roomCount)
end

function DungeonBuilder:buildLevelC()
    self:placePlayer()
    self:customizeContents()
    self.RUNNER:prepareCrawlAndTimers()
    self.RUNNER:dcFinalize()
end

function DungeonBuilder:dcPrepare()
    TileLock = false
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

The fact that dcPrepare can be done here give me hope for dcFinalize, but I’ll save that for a moment. What’s in buildLevelA? Nearly funny:

function GameRunner:buildLevelA(count)
end

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

We can stop calling A and I think we already stopped calling C. Right. Remove both. Commit: removed unnecessary buildLevel methods from GameRunner.

Now dcFinalize?

function GameRunner:dcFinalize()
    self.playerCanMove = true
    TileLock = true
end

We can accept the TileLock on our end, but need to set the playerCanMove flag in gameRunner.

function DungeonBuilder:buildLevelC()
    self:placePlayer()
    self:customizeContents()
    self.RUNNER:prepareCrawlAndTimers()
    self:dcFinalize()
end

I need to leave dcPrepare and dcFinalize in GameRunner for now, as they are used by the other level creation methods. Let’s inline that levelC thing:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self.RUNNER:prepareCrawlAndTimers()
    self:dcFinalize()
end

This is good. Commit: dcFinalize supported in DungeonBuilder, still needed by non-standard levels in GameRunner.

Now there’s that stuff about the prepareCrawl. That doesn’t belong here at all. Move that line back to where we start this thing off:

function GameRunner:createLevel(count)
    self:createNewDungeon()
    self.builder:buildLevel(count)
    self:createButtons()
    self:prepareCrawlAndTimers()
    self.playerCanMove = true
end

Test, commit: move crawl and timer startup back to GameRunner.

That code above needs to be a bit more communicative, but we’ll leave that while we check around for uses of RUNNER in DungeonBuilder.

function DungeonBuilder:dcPrepare()
    TileLock = false
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

These are mostly not the concern of a Dungeon builder. I made a card to move them, because the day is coming to a close here.

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

This is a legitimate callback, I think, unless we wanted to do something fancier. I’ll make a card.

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

Another callback. Maybe we should return this information!

The other two uses of RUNNER are passing it to methods on Monster that need it. We might want to init Monsters with the gameRunner. I’ll make a card for that, too.

All these concerns can be left for another day. I think we’re nearly good here.

Let’s sum up.

Summary

Thirteen commits between 0900 and 1130. 150/13 is about 11 1/2 minutes between commits. Not too bad. Would be shorter if I weren’t writing you these love notes.

Lessons along the way:

  • Small commits make it easier to revert in case of a misstep.
  • Reverting is usually faster than debugging, even when steps are small.
  • If you start out to make something easy and it isn’t, try something else.

I think we have most of dungeon construction moved over to DungeonBuilder, for the main createLevel method. I think we should change things so that building the level returns information to the caller, with information in it like the WayDown and Player info we’re jamming back now.

I think we’ll find that we can smooth things out a bit with some better names in both GameRunner and DungeonBuilder.

I think we need to better isolate the non-Dungeon aspects of setting up for a level from the dungeon building aspects.

And, of course, we need to convert the other creation methods to use our new DungeonBuilder.

And we should look now at Dungeon and see whether there are things to do there to separate building from running. Perhaps we should move the Tiles into DungeonBuilder and then create a DungeonRunner, more lightweight class, for run time?

But GameRunner has now lost a lot of weight, and since DungeonBuilder is smaller, building is easier to understand.

So with something like 40 or 50 small commits, we’ve made the system a better place to live.

This is the way.


  1. At his age, more recently is rather a vague concept. He’s been programming longer than most programmers have been alive. ↩

  2. There is today, and always has been and ever will be, a population of wise people who will tell us that product desirability isn’t about lots of features, and so on. Somehow, whatever trickles down to most programmers is more to do than they’ll ever accomplish in the time allowed. ↩

  3. Old? He’s much younger than Ron. I suppose Ron meant “revered” rather than “ancient”. ↩