Let’s look at what we’ve done and see what might be next on our quest to separate building from running the Dungeon.

It occurs to me that there actually could be “business value” to the changes we’re making here. Yes, our main purpose with the development is to produce a game, and these changes don’t really enhance the game. They will probably allow us to go faster, but while I believe that, I don’t think it’s a good sales story, which is why I recommend doing large refactorings like this in small steps.

Of course small steps are their own reward, but too often we think that a big design change means that we have to stop, disassemble the vehicle, then put it back together in some new way. That’s not usually the case, in my view, so it’s usually possible to do our design improvements incrementally.

That’s a good ability to have, and a good habit to form, because the best way to keep our design from getting tangled and difficult is to improve it a bit, every day, every session, as we go.

However. Business value. Suppose we were to think

Hey, this is a pretty good game, and we’ve invested a lot into it, and it seems like the engine could support lots of interesting dungeon games. Could we change things so that we could create teams of artists and game designers, and they could sort of “define” a game, rather than programming it?

Well, we certainly didn’t have that in mind when we started, but we could surely imagine what a game-building program might be like, and begin to build in that direction. One thing we would surely want is some easier way to define a dungeon and player and treasures and NPCs. Some way that involves less code. Some kind of tabular input or something. We’d have to think about it.

One very possible piece of the plan would be to break apart dungeon creation from dungeon running. Over there in the Game Definition Department, they define a new game, compile it with our Game Definition Tool, and plug it into the GameRunner.

If that were our mission, then the work we’re doing now would be work aimed at that product.

Why do I mention this? Well, because it popped into my head. However, it’s often true that what we think of as a technical thing can be understood to have more visible value to the business, and thus gain more support for the work that has to be done.

A Story
I worked once with a team who were writing a program that analyzed credit card transactions. It was used by the accounting people to verify that the information they had matched the information from the other side of the transactions.

One thing that needed to happen was for the previous night’s batch data to be transferred (via FTP I think) to the team’s program, running up in Accounting. They wrote themselves a technical task called “FTP”, but they didn’t like doing work that wasn’t driven by the customer.

Then someone on the team had a great idea. They said to their customer from Accounting: “Would you prefer that your people come in, press a button, and wait a half hour or so to start work, or would you prefer that everything b ready for them when they come in?”

Customer of course said “When they come in”. And the team said “Please give us a story saying ‘System is ready to go when people arrive’.”

Sometimes technical things have visible business value, and when they do, it’s well worth identifying it.

But I digress. Let’s look at what we’ve wrought, and see what might be next.

Morning Planning

I have three items written down from yesterday:

  • dcPrepare: move non-Dungeon stuff back to GameRunner.
  • placePlayer RUNNER callback? Ditto WayDown. Return values?
  • Should Monsters know RUNNER, and avoid parameters on method calls?

There are surely plenty of other issues related to this effort. The basic idea, I guess, is to offload dungeon building into separate classes from dungeon running. So clearly, we need to look at Dungeon class itself, which does much of the building, formerly at the behest of GameRunner, now under the direction of DungeonBuilder. I think that is a substantial effort, about the same as what we’ve already done, but we’ll see.

We don’t need to be perfect. We need to be better. So let’s look for some things to make better.

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

The GameRunner’s code for creating a game level is looking pretty fine, I think. But maybe it’s not entirely good. What is createNewDungeon?

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

Would that be better expressed as defineNewDungeon? Or defineDungeonBuilder? Let’s try the latter.

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

That at least expresses that we’re setting up the builder. Test. Test fails:

Tests:22: attempt to call a nil value (method 'createNewDungeon')
stack traceback:
	Tests:22: 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'

Yeah, well, need to change all the references. Fix the test. Test again:

GameRunner:159: attempt to call a nil value (method 'createNewDungeon')
stack traceback:
	GameRunner:159: 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'

OK, I do the obvious and find all the occurrences and change them all. Codea really needs better refactoring tools, or a driver who is a bit more paranoid. Tests run.

Commit: rename method createNewDungeon to defineDungeonBuilder.

That was irritating. Took some wind from my sails. But I’m reminded of something J.B.Rainsberger tweeted yesterday, to the effect that if a test fails surprisingly for three times, revert and start over. I’m going to try to take that to heart.

I had intended that to be a start that got me moving but now I need to rare back1 and start again. We were looking at this:

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

I would argue that the final three calls there should be pulled out into a separate method, like this:

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

function GameRunner:prepareToRun()
    self:createButtons()
    self:prepareCrawlAndTimers()
    self.playerCanMove = true
end

The three seem to me to be “smaller” than the other two, and they all need to be done, so they should be kept together. An improvement? YMMV but I think so.

Test. Commit: extract clarifying method prepareToRun.

Enough finger exercises. Let’s look into Dungeon and see what sorts of methods we find. Here’s a picture of what my Making app finds in Dungeon class:

dungeon has many methods

There’s clearly more building in there than we want. Let’s pick one method and think what to do about it.

function Dungeon:convertEdgesToWalls()
    for key,tile in self.map:pairs() do
        tile:convertEdgeToWall()
    end
end

This method is only used in dungeon building. After all the rooms and hallways are drawn, this method ensures that all the rooms and halls have “wall” tiles around them, not “edge” tiles. That’s what makes the muddy walls around everything:

muddy walls

We could imagine this one deferring to map. But the Map, MapStrategy, Maps, etc classes are down at the bottom, and they are tricksy. I think I’d like to leave them alone, though they do deserve some attention.

Let’s look at one more build-related Dungeon method. Ah, here’s a great one:

function Dungeon:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self.runner)
            if r:isAllowed(self) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
    return self.rooms
end

What can we observe? Well. It’s clearly building and not operating. It’s building the rooms table for the Dungeon. We can assume that Dungeon wants to know the rooms, though we might explore in that direction to learn more. Right now I don’t want to learn more about that.

Most of this logic is about rooms, and with all those ends in there, I think this method needs refactoring. It seems that parts of it might want to be moved to Room class, but I am dubious. I decide, though, to look at the paint method:

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

Ah. There’s a clue. We really don’t want the runner doing that, it’s trying to get out of the building game. What does that method do?

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

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    aTile:defineInMap(self.map)
end

function Tile:defineInMap(aMap)
    aMap:atPointPut(self.mapPoint, self)
end

function BaseMap:atPointPut(aPoint, anObject)
    local key = aPoint:key()
    assert(key, "key is nil")
    self:atKeyPut(key, anObject)
end

I didn’t want to know that. Let’s look at one more method, maybe one of the corridor ones:

function Dungeon:horizontalCorridor(fromX, toX, y)
    local prev,curr
    local increment = 1
    if fromX > toX then
        increment = -1
    end
    prev = self:privateGetTileXY(fromX,y)
    for x = fromX,toX,increment do
        curr = self:privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "EW")
        prev = curr
    end
end

function Dungeon:setHallAndDoors(prevTile,currTile, dir)
    if not currTile:isRoom() then
        currTile:convertToHall()
        if prevTile:isRoom() then
            currTile:setCanBeDoor(dir)
        end
    else -- curr is room, may need to set prev
        if prevTile:isHall() then
            prevTile:setCanBeDoor(dir)
        end
    end
    return currTile
end

This is more weird than necessary, because of the decisions we’re trying to make about where we could have doors. But all these calls to tile are primitives within Tile, I think. They might adjust the tile’s state but they are not writing cells into the dungeon. We do have the get:

function Dungeon:privateGetTileXY(x,y)
    if x<=0 or x>self.tileCountX or y<=0 or y>self.tileCountY then
        return Tile:edge(x,y, self.runner)
    end
    return self.map:atXYZ(x,0,y)
end

That’s OK, we’ll have plenty of operational use for getting tiles.

Realization

I think we’re looking at a design flaw. The Dungeon has a complete array of tiles to begin with. In most of the code above, we’re just changing tile properties, adjusting individual tiles. But in the paint method, we seem to be providing a whole new tile. Let’s look deeper.

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

function Room:correctTile(x,y)
    return Tile:room(x,y, self.runner)
end

Right. We’re creating a new tile and then jamming it into the Tile array where the original one was:

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

Is this left over from some older generation where we treated the dungeon / map as a sparse array of tiles, only where needed? I don’t think that’s the case any more. Let me verify:

function Dungeon:init(runner, tileCountX, tileCountY)
    self.runner = runner
    self.tileCountX = tileCountX or assert(false, "must provide tile count")
    self.tileCountY = tileCountY or assert(false, "must provide tile count")
    self:createTiles()
    self:clearLevel()
end

function Dungeon:createTiles()
    Maps:cartesian()
    self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
end

Tracing this down, I find what I expect:

function BaseMap:init(xWidth, zHeight, mapType, klass, ...)
    assert(mapType == cartesian or mapType == hex, "MapType must be cartesian or hex")
    self.mapType = mapType
    self:createRectangle(xWidth, zHeight, klass, ...)
end

function BaseMap:createRectangle(xWidth, zHeight, klass, ...)
    self.tab = {}
    for z = 1,zHeight do
        for x = 1,xWidth do
            local pt = self.mapType(x,z)
            local item = klass(pt, ...)
            self:atPointPut(pt,item)
        end
    end
end

When we create a map, we pass in a class (Tile in this case) and we create an instance of that class (with the item = code) and store it at each location in the map. In short, the map is filled.

Accordingly, it should be OK, and I think it would be better, if we were to set the existing tile correctly rather than replace it with a new one. That is a judgment call. In some ideal world, our tiles would be immutable, but that’s not the case in our world.

Let’s look at what we might do. What does the created tile look like?

function Tile:room(x,y, runner)
    assert(not TileLock, "Attempt to create room tile when locked")
    return Tile(Maps:point(x,y),TileRoom, runner)
end

function Tile:init(mapPoint,kind, runner)
    assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
    self.mapPoint = mapPoint
    self.kind = kind
    self.runner = runner
    self:initDetails()
end

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

Let’s try something. I believe that the only thing we’re actually changing is the kind of the tile. Let’s try doing that and see what happens.

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

That works. Let’s simplify it:

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

And in Tile:

function Tile:convertToRoom()
    self.kind = TileRoom
end

Test. Commit: Room:paint no longer creates new tiles in the room.

Let’s reflect.

RESTrospective

What just happened? We were looking into Dungeon with an eye to separating building from operating. In exploring, we found that we were using a method defineTile in Dungeon, which replaced tiles in the map with new ones. That seemed wrong, so we changed the caller to change the state of the existing tile rather than replace the whole thing.

With that working, there are no callers of defineTile in GameRunner or Dungeon. We can remove those methods. Test, commit: GameRunner and Dungeon no longer have ability to redefine a Tile in the map.

I think it’s clear that this is a righteous change and a real improvement. We’ve seen how deep down the actual tiles are, inside the GameRunner’s Dungeon’s map’s array … I don’t even know how many levels down, and here’s a direct connection from the very top to the very bottom. We’re well rid of it.

But we sort of diverted from our nominal task, improving the separation of GameRunner and Dungeon from building. And yet … we just disconnected both GameRunner and Dungeon from a deep incursion into the build process.

So that’s extra good. The improvement was worth making. If we’re going around the campground picking up beer cans and ran across a soda can, it’s better to pick it up even if we only brought beer to the picnic. We’re here to make the place better.

Took longer to write about it than to do it.

We could empty our mental buffers and reload with a fresh view of Dungeon’s building support, but in fact we’ve reached a convenient time to stop.

Is there a lesson here? I think the big lesson is something like Don’t build direct connections from your top level objects to the bottom level ones. Another likely lesson is Pay more attention to cohesion and coupling, even at the beginning.

Those would be good advice on any given day, and in terms of today’s work, we should have heeded today’s advice about 250 days ago. I never go back on my own time line, because there are some pretty nice things in my life, and you never know when a change to your past might turn your BMW into a ‘67 Ford, or your lovely wife into a harridan. The next best thing is perhaps a rule like this:

Take small chunks of time, think in terms of how the system might be better, browse around, and make small improvements in the rough direction of better.

Not exactly concise, but that’s what I’ve got today. What will I have next time? I can’t wait to find out.


  1. Yes. Archaic dialect for “rear back”. We used to talk like that. It was the style at the time, along with an onion tied to your belt. ↩