Dungeon 275

Gonna refactor you a little bit more …

I’m liking the new DungeonAnalyzer, a helper object that Dungeon can create to answer important requests like “find me a hallway tile that’s open”. So far it has these methods:

DungeonAnalyzer:furthestFrom(targetTile, tile1, tile2)
DungeonAnalyzer:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
DungeonAnalyzer:randomRoomTileAvoidingRoom(roomToAvoid)

Thus far, the Analyzer only knows a TileFinder, and the X and Y dimensions of the dungeon. I suspect that we’ll want it to know a few more things, but we’ll try to keep its access very limited.

Design Issue

There’s a design issue that I want to mention and try to keep in mind. Currently, to perform these requests, an object talks to the Dungeon, which forwards the message, like this:

function Dungeon:farawayOpenRoomTile(room, target)
    return self:dungeonAnalyzer():farawayOpenRoomTile(room, target)
end

It is often seen in the wild that people will instead not implement the forwarding method on the main object, instead requesting the analyzer in the user code:

    self.dungeon:dungeonAnalyzer():farawayOpenRoomTile(...)

The motivation for this is probably a good one, to limit the breadth of the main object’s interface, giving it just the one method dungeonAnalyzer rather than a number of methods that “don’t do anything but forward anyway”.

Better minds than mine would argue that forwarding is better, because it is better for other objects to know what a given object can do than to know what it has. Here, it’s a judgment call, because you can argue that what Dungeon “can do” is “provide an analyzer”.

Done the way we’re doing it now, with forwarding, the DungeonAnalyzer is an object that helps the Dungeon do things. Done where we pass out the Analyzer, we’ve created another object that many people have to know about, and most likely those people already have to know about the Dungeon. And then they’ll have to remember or look up whether it is the Dungeon that does this thing, or the analyzer. Better, I think, for the Dungeon to keep these concerns to itself, at least for now.

I freely grant that on another day in a similar situation, I might do differently, and certainly you get to decide what you’ll do. My only recommendation is to think about it, then decide, rather than just respond reflexively to the situation.

Anyway, let’s see what else we should move. As we do that, I’m going to think about how I might record my discoveries in the code, to make the next time we show up a bit easier. Maybe a comment -- for Analyzer? or something …

What’s Left?

First I’ll look for more of the tile-finding methods in Dungeon. I think they are all done. But didn’t I put some of those methods in DungeonBuilder, thinking that they belonged there, and only a day or so ago deciding that they’d be better split off into the Analyzer? Se in DungeonBuilder …

We have some interesting and concerning cases:

function DungeonBuilder:placeSpikes(count)
    for i = 1,count or 20 do
        local tile = self:randomHallwayTile()
        Spikes(tile)
    end
end

function DungeonBuilder:randomHallwayTile()
    while true do
        local pos = vec2(math.random(1, self:tileCountX()), math.random(1,self:tileCountY()))
        local tile = self:getTileFromPos(pos)
        local up = self:getTileFromPos(pos+vec2(0,1))
        local down = self:getTileFromPos(pos-vec2(0,1))
        local left = self:getTileFromPos(pos-vec2(1,0))
        local right = self:getTileFromPos(pos+vec2(1,0))
        if tile:isOpenHallway(up,down,left,right) then return tile end
    end
end

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

function DungeonBuilder:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local avoidedRoom = nil
    if roomNumberToAvoid < #self:getRooms() then
        avoidedRoom = self:getRooms()[roomNumberToAvoid]
    end
    return self.dungeon:randomRoomTileAvoidingRoom(avoidedRoom)
end

Recall that when DungeonBuilder says self.dungeon, it’s talking to a BuilderView, a Façade that Dungeon provides, to protect its inner secrets, er, to narrow the scope of concern for calling objects. Currently the BuilderView is in flux, as we settle down which methods will reside where. But what is the relationship between Dungeon, Builder, Analyzer, and View?

We intend that Builder will build up the dungeon from scratch. Currently, it is provided a BuilderView that has a newly-initialized dungeon in it, because we have some code outside the builder that accesses the dungeon before it’s done. So in principle, I guess there’d be at least three kinds of methods in the BuilderView:

1. Accessors, mostly setters, that Builder uses to initialize the Dungeon;
2. Useful method forwarders that Builder wants to use but that aren’t really coherent with Builder’s purpose. Analyzer methods might be examples;
3. Methods forwarded to Dungeon that should, someday, be moved to Builder (or Analyzer) but haven’t been moved yet.

Does this mean that the BuilderView should have a DungeonAnalyzer as a component? I’m not sure. I do think, however, that the DungeonBuilder should talk only to the BuilderView, and leave it to the BuilderView to decide who answers which questions.

Let’s try that and see how it shapes up. We’ll move all the obvious analyzer methods to Analyzer, and do the necessary wiring to make them work. We’ll probably wind up calling through to a Dungeon forwarder that produces an Analyzer and calls it … but once we see that happening, we may be able to move the method out of Dungeon entirely, into a BuilderView that has or creates analyzers.

An important question in the relationship between these objects is who calls the various Dungeon methods that use the Analyzer. If they are only Builder methods, that will let us simplify the connections by removing them from Dungeon entirely. That would be nice.

I think these methods can all be pulled from Dungeon. That’s better than forwarding, which is better than demanding an analyzer from Dungeon. Let’s try one:

function Dungeon:farawayOpenRoomTile(room, target)
    return self:dungeonAnalyzer():farawayOpenRoomTile(room, target)
end

I’m just going to remove this and see what breaks, and whether I can quickly fix it. With a stronger IDE it would be easier to check the code, but I’ve got what I’ve got.

No surprise, DungeonBuilder gets in trouble:

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

Fix it in BuilderView:

function BuilderView:farawayOpenRoomTile(...)
    return self.dungeon:dungeonAnalyzer():farawayOpenRoomTile(...)
end

Nice. We just removed another method from Dungeon. Do a little happy dance. Tests run. Commit: remove farawayOpenRoomTile from Dungeon.

Here’s another method:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

Remove it, expecting the change to be just as easy. Fix:

function BuilderView:randomRoomTileAvoidingRoom(...)
    return self.dungeon:dungeonAnalyzer():randomRoomTileAvoidingRoom(...)
end

Not good enough:

GameRunner:327: attempt to call a nil value (method 'randomRoomTileAvoidingRoom')
stack traceback:
	GameRunner:327: in method 'randomRoomTileAvoidingRoomNumber'
	Monster:55: in method 'getRandomMonsters'
	Monsters:98: in method 'createRandomMonsters'
	DungeonBuilder:320: in method 'setupMonsters'
	DungeonBuilder:188: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

Game Runner needs it.

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local room = nil
    if roomNumberToAvoid < #self:getRooms() then
        room = self:getRooms()[roomNumberToAvoid]
    end
    return self:getDungeon():randomRoomTileAvoidingRoom(room)
end

I’m not comfortable with any of the ideas I have for this. I don’t even feel really good about GameRunner knowing the rooms.

OK, first, revert. Second, can we simplify that method bove:

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    room = self:getRooms()[roomNumberToAvoid]
    return self:getDungeon():randomRoomTileAvoidingRoom(room)
end

That should return nil when the room doesn’t exist, that’s what tables do. This works. Commit: simplify GR:randomRoomTileAvoidingRoomNumber.

Move the method to Analyzer:

No, wait. Who’s calling this? This is being called in Monster creation:

Monster:55: attempt to call a nil value (method 'randomRoomTileAvoidingRoomNumber')
stack traceback:
	Monster:55: in method 'getRandomMonsters'
	Monsters:98: in method 'createRandomMonsters'
	DungeonBuilder:317: in method 'setupMonsters'
	DungeonBuilder:188: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

Monster creation is given the GameRunner:

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

function Monsters:createRandomMonsters(runner, count, level, roomNumberToAvoid)
    self.table = Monster:getRandomMonsters(runner, count, level, roomNumberToAvoid)
end

function Monsters:createHangoutMonsters(runner, count, tile)
    local tab = Monster:getHangoutMonsters(runner, count, tile)
    for i,m in ipairs(tab) do
        table.insert(self.table, m)
    end
end

function Monster:getRandomMonsters(runner, number, level, roomNumberToAvoid)
    local t = self:getMonstersAtLevel(level)
    local result = {}
    for i = 1,number do
        local mtEntry = t[math.random(#t)]
        local tile = runner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
        local monster = Monster(tile, runner, mtEntry)
        table.insert(result, monster)
    end
    return result
end

Do monsters really need the runner? Or is it just during creation?

function Monster:xScaleToFaceTowardPlayer()
    local dir = self.runner:playerDirection(self:getTile())
    return ((dir.x > 0) and self.facing) or -self.facing
end

function Monster:pathComplete()
    self.runner:removeMonster(self)
end

function Monster:startActionWithPlayer(aPlayer)
    self.runner:initiateCombatBetween(self,aPlayer)
end

We could replace the latter two with a publish-subscribe to the Bus, at some cost in complexity. And if the player published her tile, we could just let the monsters listen for that info.

Seems like this is a long way from where we’re interested. I think we’ll just have to let that method live. But at least it’s down to a forwarder. We do have the room number method in Builder, so we can do this:

Yikes. I’ve found the bug that lets monsters appear in room 1:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

I’m surprised that doesn’t blow up, but in any case it is wrong. Is it even called? (it is.)

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

Test. Works. Commit: Fix problem allowing monsters in room 1.

That breaks my perfect record of not breaking the system over a series of more than 100 refactoring steps.

Anyway I should be able to forward the GameRunner record now:

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    return self:getDungeonAnalyzer():randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
end

function GameRunner:getDungeonAnalyzer()
    return self:getDungeon():dungeonAnalyzer()
end

Test. Won’t work, we need the method. Copy it over from DungeonBuider for now:

function DungeonAnalyzer:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local avoidedRoom = nil
    if roomNumberToAvoid < #self:getRooms() then
        avoidedRoom = self:getRooms()[roomNumberToAvoid]
    end
    return self:randomRoomTileAvoidingRoom(avoidedRoom)
end

Shorten it to the better form. We lost that in a revert or something:

function DungeonAnalyzer:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    avoidedRoom = self:getRooms()[roomNumberToAvoid]
    return self:randomRoomTileAvoidingRoom(avoidedRoom)
end

Test expecting getRooms to fail. Yes. Implement:

function DungeonAnalyzer:getRooms()
    return Dungeon:getRooms()
end

Don’t know if that exists or not. Just test to find out. Doesn’t work, fails in a way that I don’t expect.

Breakfast is called. Revert. Declare the morning to be a disaster. Wander off, shaking head … see you tomorrow …

Monday Morning 0911

Well, as they said yesterday, tomorrow is another day, and that seems to be the case.

I don’t know if I was under the weather yesterday: I didn’t think so. I’m not sure about today, either, as last evening my nose started running and I felt a ripping sore throat. Seems better this morning. No idea. Creeping mortality. Timor mortis conturbat me. Meanwhile let’s review a bit and then program.

Yesterday I only managed three trivial commits, including a fix for a released defect (!) that allowed monsters to appear in room 1. It occurs to me that I’d like to understand how that happened.

The defect was that this forwarding method:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

Was calling randomRoomTileAvoidingCoordinates instead. Since that method requires four tile coordinates, the room corners, I rather wonder how it managed to pass on through without notice. Let’s dig down::

function DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self.finder:getTile(pos.x,pos.y)
    until tile:isOpenRoom()
    return tile
end

function DungeonAnalyzer:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = randomValueAvoiding(x1,x2,maxX)
    y = randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

function randomValueAvoiding(vLow,vHigh,vMax)
    local numberToSkip = vHigh-vLow + 1
    local v = math.random(1, vMax - numberToSkip)
    if v >= vLow then
        v = v + numberToSkip
    end
    return v
end

I don’t see how that manages to fail. The randomValueAvoiding should find itself unable to subtract nil from nil.

This deserves exploration, I think, because it seems that the prior code should have traced back.

I think I’ll put the defect back in and follow the thread a bit.

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
end

Now I’ll add a print to see if we call it. Yes, we call it many times, with no errors.

Oh my. I change my print in Dungeon:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    print("in dungeon", x1,y1,x2,y2)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
end

And x1,y1,x2,y2 have values. Someone has declared them and given them values.

Extensive searching finds this:

function MapPoint:cartesianManhattanDistance(aPoint)
    x1,y1 = aPoint:x(), aPoint:z()
    x2,y2 = self:x(), self:z()
    return math.abs(x1-x2) + math.abs(y1-y2)
end

function MapPoint:hexManhattanDistance(aPoint)
    x1,y1,z1 = aPoint:x(), aPoint:y(), aPoint:z()
    x2,y2 = self:x(), self:y(), self:z()
    return (math.abs(x1-x2) + math.abs(y1-y2) + math.abs(z1-z2))/2
end

Definitely not local. Fix. Test. We print nils in my print, and we crash. Wow, that was an interesting defect. Makes one wish that Lua didn’t default things to global, but there we are.

Remove the prints, replace the correct forwarding call, test. Commit: Fix problem in MapPoint using global x1 y1 x2 y2.

Yes, might have been better to revert and then refix the MapPoint, or to revert the other tabs. Anyway we’re good now.

Let’s reflect a bit.

Reflection

We’re not entirely at Codea’s and Lua’s mercy with problems like this. I do have some code that will lock down the global definitions, refusing to add new ones, or, with a simple change, printing out that it happened but allowing it to happen. I don’t usually run it, mostly because it finds legitimate things as the system sets itself up. Let’s turn it on:

    collide = false -- Codea bug, undefined
    setmetatable(_G, {
        __newindex = function (_, n)
            print("attempt to write to undeclared variable "..n, 2)
        end,
        __index = function (_, n)
            print("attempt to read undeclared variable "..n, 2)
        end,
    })

The code doesn’t find anything. The reason is that it’s not executed until we are set up, and all the nastiness has already happened.

This is an indication that our “Making App”, our toolset, isn’t as robust as we might like. Over my time with Codea, I’ve written code to detect globals being created, to detect access to variables never defined, and to print out lists of globals, classes, methods, and so on.

None of these tools is really integrated with my daily work, and I’mm honestly not sure how I could do that readily, and, frankly, the problems I’ve encountered are rare enough that I don’t see the value. I think in a team situation working on a program like this, I might feel differently. Truth is, I’ve not encountered many problems of this kind.

That’s really a “hold my beer” statement, isn’t it? Let’s compromise here and dump the globals before and after setting up and running. Whee! I find that the global room has been created. Let’s look for it.

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    room = self:getRooms()[roomNumberToAvoid]
    return self:getDungeon():randomRoomTileAvoidingRoom(room)
end

That should be local. I don’t find any more. Commit: Turn on GlobalAnalyzer. Find missing “local” causing global room.

OK, maybe we’ve made the world a bit better. Let’s get back to the bigger picture.

Draining the Swamp

Our larger mission, if I recall, was to move dungeon building code from GameRunner into DungeonBuilder (or other places if appropriate). I’ve stumbled a bit on one case. Let’s review cases in GameRunner where we send a message to self.dungeon:

function GameRunner:getRooms()
    return self.dungeon:getRooms()
end

function GameRunner:hasMonsterNearPlayer(range)
    return self.dungeon:hasMonsterNearPlayer(range)
end

function GameRunner:getMonsters()
    return self.dungeon:getMonsters()
end

That’s not bad. Let’s see who’s using these.

function GameRunner:movePlayerToRoom1()
    local tile = self:getRooms()[1]:centerTile()
    tile:moveObject(self.player)
end

function Player:youAreDead(combatRound)
    self.runner:movePlayerToRoom1()
end

There might be a better way to do that, but I think I’ll allow it. One way would be for the Player to remember her initial tile and just move there …

function Dungeon:hasMonsterNearPlayer(range)
    return self:getMonsters():hasMonsterNearPlayer(range)
end

function Monsters:hasMonsterNearPlayer(range)
    for i,m in ipairs(self.table) do
        if m:isActive() and m:manhattanDistanceFromPlayer() <= range then return true end
    end
    return false
end

There’s a little foofaraw around lazy init for Monsters, but again, I think we can allow that.

function GameRunner:getMonsters()
    return self.dungeon:getMonsters()
end

function GameRunner:playerTurnComplete()
    if self.requestNewLevel then
        self:createLevel(12)
        self.requestNewLevel = false
    else
        self.playerCanMove = false
        self:getMonsters():move()
        self.playerCanMove = true
    end
end

And a few other such references. Seems legit.

I tentatively conclude that GameRunner-based dungeon building calls are moved over to DungeonBuilder satisfactorily. So let’s review DungeonBuilder to see whether it’s doing some back references that can be improved.

In DungeonBuilder init:

function DungeonBuilder:init(runner, dungeonView, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    self.view = dungeonView
    self.dungeon = dungeonView -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

We’re receiving only a BuilderView, not the entire Dungeon interface. I’ve tried to use references to self.view when I consider the reference to Dungeon to legitimately below there, and self.dungeon when I think it’s a candidate for removal. I’ll begin by reviewing the self.dungeon lines:

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:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local avoidedRoom = nil
    if roomNumberToAvoid < #self:getRooms() then
        avoidedRoom = self:getRooms()[roomNumberToAvoid]
    end
    return self.dungeon:randomRoomTileAvoidingRoom(avoidedRoom)
end

I believe that I have crashed and burned in the past, trying to move each of these over.

Let’s see. I guess we’d like to have this method in DungeonBuilder. We can trivially do this:

function DungeonBuilder:randomRoomTileAvoidingRoom(avoidedRoom)
    return self.dungeon:randomRoomTileAvoidingRoom(avoidedRoom)
end

I expected this to fail but I was wrong, of course. This is a pure refactoring. Let’s commit: break out DB randomRoomTileAvoidingRoom, calls dungeon. Now can we move the guts over from Dungeon?

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

Sure, why not, it’ll let us move a method out of Dungeon if this works.

function DungeonBuilder:randomRoomTileAvoidingRoom(avoidedRoom)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

This probably barfs for lack of the dungeonAnalyzer() method in DungeonBuilder.

DungeonBuilder:311: attempt to call a nil value (method 'dungeonAnalyzer')
stack traceback:
	DungeonBuilder:311: in function <DungeonBuilder:310>
	(...tail calls...)
	DungeonBuilder:253: in method 'placeLoots'
	DungeonBuilder:183: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:66: in function 'setup'

So far so good. We could do one of two things. We could just fetch a DA from the BuilderView. Or, we could arrange to pass a DA in when we create the DungeonBuilder. Let’s first do the fetch.

function DungeonBuilder:dungeonAnalyzer()
    return self.dungeon:dungeonAnalyzer()
end

This should fail with BuilderView not understanding.

DungeonBuilder:295: attempt to call a nil value (method 'dungeonAnalyzer')
stack traceback:
	DungeonBuilder:295: in method 'dungeonAnalyzer'
	DungeonBuilder:315: in function <DungeonBuilder:314>
	(...tail calls...)
	DungeonBuilder:253: in method 'placeLoots'
	DungeonBuilder:183: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:66: in function 'setup'

function BuilderView:dungeonAnalyzer()
    return self.dungeon:dungeonAnalyzer()
end

Tests run. Commit: DungeonBuilder supports randomRoomTileAvoidingRoom internally. BuilderView can return DungeonAnalyzer.

Can we now remove that method from Dungeon?

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

I have suspicions. So I put an error in it.

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    assert(false, "should not get here")
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

Dungeon:272: should not get here
stack traceback:
	[C]: in function 'assert'
	Dungeon:272: in function <Dungeon:271>
	(...tail calls...)
	Monster:55: in method 'getRandomMonsters'
	Monsters:98: in method 'createRandomMonsters'
	DungeonBuilder:328: in method 'setupMonsters'
	DungeonBuilder:188: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:66: in function 'setup'

This is coming from here:

function Monster:getRandomMonsters(runner, number, level, roomNumberToAvoid)
    local t = self:getMonstersAtLevel(level)
    local result = {}
    for i = 1,number do
        local mtEntry = t[math.random(#t)]
        local tile = runner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
        local monster = Monster(tile, runner, mtEntry)
        table.insert(result, monster)
    end
    return result
end

We’re calling all the way back to Runner for this.

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local room = self:getRooms()[roomNumberToAvoid]
    return self:getDungeon():randomRoomTileAvoidingRoom(room)
end

… Yet again, when I try to untangle this, I get confused.

I think I know what’s needed. GameRunner is calling Dungeon to do this work. We have moved the work to DungeonBuilder. Accepting the need for GameRunner to do this (for now), we need it to call the Builder. But it doesn’t have a copy, because all the dungeon creators use the builder and then drop it. Let’s try saving it:

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 = nil
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    local builderView = self.dungeon:asBuilderView()
    self.builder = DungeonBuilder(self, builderView, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    return self.builder
end

Test that … good so far. Now use it:

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local room = self:getRooms()[roomNumberToAvoid]
    return self.builder:randomRoomTileAvoidingRoom(room)
end

I am hopeful but not quite confident … seems to work. It also seems to me that we have the outer method in builder so that this should work:

function GameRunner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    return self.builder:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
end

And in fact it does. I don’t really like holding on to the builder, but it seems nearly legit. Let’s improve the init:

function GameRunner:init(testX, testY)
    self.tileSize = 64
    self.tileCountX = testX or 85 -- if these change, zoomed-out scale 
    self.tileCountY = testY or 64 -- may also need to be changed.
    self.cofloater = Floater(50,25,4)
    self.dungeonLevel = 0
    self.requestNewLevel = false
    self.playerRoom = 1
    Bus:subscribe(self, self.createNewLevel, "createNewLevel")
    Bus:subscribe(self, self.darkness, "darkness")
    self.dungeon = nil -- created in defineDungeonBuilder
    self.builder = nil -- created in defineDungeonBuilder
end

Test a bit more.

Arrgh! I’m getting monsters in room one again.

Let’s see what we can find with some prints …

Found this error:

function DungeonBuilder:randomRoomTileAvoidingRoom(avoidedRoom)
    return self:dungeonAnalyzer():randomRoomTileAvoidingRoom(roomToAvoid)
end

Might help to use the parameter. Fix. Test, extensively, lots of restarts. No monsters in Room 1. I was already confident. Commit: GameRunner retains builder; defers randomRoomTileAvoidingRoomNumber to builder. Fix defect in DungeonBuilder allowing monsters in room 1 (again).

I think we’ll call it for today. I have some ideas hatching, but we have managed six commits in three hours, which included a lot of head-scratching and reverting. Let’s sum up.

Summary

Sequence Diagrams

I want to talk a bit about what I do, versus what I know how to do. In tracking down how this deferral works, there is a perfectly useful UML diagram, the “sequence diagram” that shows the messages sent from object to object, in time order. It’s just the thing for sorting out these more complex message sequences. A few times in recent days, I’ve almost drawn one. I think it would have been enlightening and helpful. Why, then, didn’t I do it?

I have two reasons.

First, it’s a bit of a pain to draw a diagram on my iPad while I’m programing and writing an article. Either I lay the iPad down and can’t type on the Magic Keyboard, or I try to draw with the iPad mounted on the keyboard, which leaves the screen at a 45 degree angle and back over the keyboard, so that it’s hard to draw.

My second reason is a bit better. I don’t use that capability because in my small program here, the diagram would be easy to work out, and I’m trying to work as if I were in a even larger program. I suspect that in a larger program of this kind, a sequence diagram would be hard to create, and most teams would not create them. I try to work without things that readers’ teams likely wouldn’t or couldn’t do.

Still, it’s tempting to do it. Maybe I should, just to demonstrate that it’s not terribly hard. It might even be useful to look at the threads passing through GameRunner, Dungeon, DungeonBuilder, TileFinder, and BuilderView, as well as those who use them. It might be scary, but that’s a good thing to know as well.

Perhaps. We’ll see.

Today’s Progress

We found a tricky bug involving spurious globals, improved and turned on our global analyzer, and found yet another spurious global with it. Good tool work, team.

Then in a few steps, we moved more room-finding over into DungeonBuilder, and simplified the method that seems to be required in GameRunner accordingly. It took a bit longer than one could be proud of, owing partly to existing complexity, partly to a couple of coding mistakes. I had to trace with some prints, which is never a good look, but sometimes the best one can do.

That part took an hour, rather longer than I feel it deserved, but somehow the complexity of things made it take that long.

Sunday’s Lack of Progress

I have no excuse and no explanation. Many of the things I tried yielded poor results or led to confusion. Today’s discovery of a need for a live Builder may have been part of the trouble. My experience is that sometimes the bear bites you, and you may not have an explanation. Well, it bites me and I don’t. You, I don’t know what happens to you. But if you have some bear bite marks, I think it’s OK to forgive yourself.

The Future …

A lot of good stuff is moved around to better locations. I’d like to draw a picture or something, to get a better sense of it. But most of my changes have been of the form “this belongs over here”, and “now can we remove the old one?”. I try to work very locally, with global things in mind, picking improvements that only take an hour or less to accomplish, showing that we can improve our code without taking big blocks of time.

We do have a God Object problem, with GameRunner, and a demi-god in the Dungeon. That leads to having low-level objects with references back to higher levels. As a rule, I find that undesirable.

My tentative plan for relaxing some of those connections will be to use the EventBus more frequently, allowing low level objects to publish information and for whoever needs to know it to subscribe. Even if that’s one of the god-like objects, the object diagram will be simpler and more flexible.

Doing this will make some things more obscure, in the sense that some object will signal an event and that’ll be the end of the chain unless we go look to see who is subscribed, where previously we could see who it was talking to. I’m hopeful that the confusion will be minimal. Either way, we’ll find out.

Tune in same bat time, same bat channel …

D2.zip