Dungeon 269: Façade

Last time I had part of an idea. Today I have a larger fraction of the idea. Let’s see if we can get all the way.

At the end of our last session, I wrote a card:

Convert Room to require a TileGetter of some kind rather than a whole Dungeon instance.

What I had in mind at that time, to the best of my recollection, was “some small object holding a Map and having just enough intelligence to deliver a tile that we need”. I have since reminded myself that there is a Gang of Four pattern, Façade¹, that is custom-made for this situation. Readers who use languages that support the notion of an Interface will know another way to accomplish an equivalent thing.

Let’s not discuss it further, let’s get to it. We’ll start at Room and drill down to see where to make the change.

Finding the Spot

Room is created with a Dungeon instance and has a few references to it:

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.dungeon = dungeon
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(self.dungeon, announcement)
    end
end

function Room:announce(dungeon, messageTable)
    local tile
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(vec2(x,self.y1))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(x,self.y2))
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(vec2(self.x1,y))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(self.x2,y))
        tile:addContents(Trigger(one))
    end
end

function Room:getDungeon()
    return self.dungeon
end

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

That seems to be all we need.

First of all, this code is not particularly consistent. In the announce case, I pass in the dungeon, and in the paint case, I refer to the member variable.

Having typed that, I want to think about it a bit. Lately, during this extended period of refactoring to improve the builder-runner distinction, it seems to me that I’ve resorted a few times to creating a method like getDungeon above. When “should” I do such a thing, and when not?

Well, I “should” always do it when another object is going to reference the item from this one: an accessor, in other words. And then, given that an object has an accessor for one of its members, I think that the object itself should use that accessor. That seems to me to be a bit more consistent.

Why did I do that here, and why only in one case did I use it and in the other, pass it as a parameter? Honestly, I do not know. I would probably do well to examine my thinking on this subject, to see whether I need to improve my habits. I tend to work intuitively, so I can easily get in one mode at one time and another mode at another time, unless I have a habit or standard practice. Definitely should think on that.

Right now, however, I think the right thing is pretty clear. I don’t want to have that member variable at all. I want something smaller wherever I use it, and I see no reason to hold on to it, since it’s only used during Room creation.

So first we’ll pass it as a parameter to paint:

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

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    if paint then
        self:paint(dungeon)
    end
    if announcement then
        self:announce(self.dungeon, announcement)
    end
end

I removed the saving of the member variable, and I’m now going to remove the getDungeon method. This is a bit larger step than is necessary. Will it bite me?

It bites me. I get an error that I don’t understand. I’m not going to try to figure it out. I revert and go more slowly.

First I pass in the dungeon, but ignore it inside the paint:

...
    self.dungeon = dungeon
    if paint then
        self:paint(dungeon)
    end
...

Tests run. Use it inside:

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

Fails. Fascinating. What’s the message?

Room:139: attempt to call a nil value (method 'getTile')
stack traceback:
	Room:139: in method 'paint'
	DungeonBuilder:119: in method 'createRandomRooms'
	DungeonBuilder:190: in method 'defineDungeonLayout'
	DungeonBuilder:52: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

OK, I’ve gotta look.

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

Line 119 is that paint. Those arrows are left over from a previous article. And clearly we’re not passing in the dungeon that the room needs … and if I understand this code, we’re only optionally painting it, if it’s “allowed”.

Ah, yes. The random rooms are not painted until we check them to see if they are allowed. I can make this work by passing in self.dungeon to the paint. But I’m working to get rid of this big object. Let’s revert again and come at this another way.

First Law of Holes

The first law of holes, probably, is “When you find you’re in a hole, stop digging”. I was hoping to make the upcoming change easier, but in fact I suspect I was at least partially sidetracked by wanting to make Room’s code more consistent. Let’s go ahead and build the object we want to use in place of dungeon.

Should we TDD this thing? Sure, why not. That reminds me, however, I was supposed to be drilling down to see how all this unwinds. So …

In Room, we access only dungeon:getTile. So …

function Dungeon:getTile(pos)
    return self:privateGetTileXY(pos.x, pos.y)
end

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:getDungeon())
    end
    return self.map:atXYZ(x,0,y)
end

function BaseMap:atXYZ(x,y,z)
    return self:atPoint(self.mapType(x,z))
end

function BaseMap:atPoint(hexPoint)
    return self:atKey(hexPoint:key())
end

function BaseMap:atKey(aKey)
    return self.tab[aKey]
end

Unless I miss my guess, that’ll return a nil if we were to access a Map element that isn’t present. Therefore I can simplify:

function Dungeon:privateGetTileXY(x,y)
    return self.map:atXYZ(x,0,y) or Tile:edge(x,y, self.runner:getDungeon())
end

That works. Commit: simplify dungeon:privateGetTileXY. And yes, I did the commit first.

OK, what our Room needs is a way to get a tile given the position:

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

Well, in that code, we would be just as happy with x and y. What else? It turns out that there is a method tileAt:

function Room:tileAt(dungeon,x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return dungeon:getTile(tilePos)
end

That’s used just once, in the builder, and it’s kind of a stopgap method just to place a learning item. Let’s change that not to pass the dungeon:

...
    local r2 = self:getRooms()[2]
    local lootTile = r2:tileAt(2,2)
    local loot = Loot(lootTile, "health", 5,5)
...

function Room:tileAt(x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return self:getDungeon():getTile(tilePos)
end

Tests run. Commit: No need to pass dungeon to Room:getTile.

OK, now we’ve actually made a design decision: methods in Room can refer to the dungeon (or whatever provides them with tiles). We made that decision because it makes more sense from the outside viewpoint, to as a room for its tile without passing in a dungeon. Rooms are in dungeons.

So we should change announce and paint to correspond. Paint is OK.

...
    if announcement then
        self:announce(announcement)
    end

function Room:announce(messageTable)
    local tile
    local dungeon = self:getDungeon()
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(vec2(x,self.y1))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(x,self.y2))
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(vec2(self.x1,y))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(self.x2,y))
        tile:addContents(Trigger(one))
    end
end

Test. Commit: Room:announces uses getDungeon method.

I notice some others:

function Room:connect(dungeon, aRoom)
    if math.random(1,2) == 2 then
        self:hvCorridor(dungeon, aRoom)
    else
        self:vhCorridor(dungeon, aRoom)
    end
end

function Room:corners()
    return self.x1,self.y1,self.x2,self.y2
end

function Room:hvCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    self:horizontalCorridor(dungeon,startX,endX,startY)
    self:verticalCorridor(dungeon, endY,startY,endX)
end

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

function Room:isAllowed(dungeon)
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            local t = dungeon:getTile(vec2(x,y))
            if not t:isEdge() then return false end
        end
    end
    return true
end

function Room:verticalCorridor(dungeon, fromY, toY, x)
    local prev,curr
    local increment = 1
    if fromY>toY then
        increment = -1
    end
    prev = dungeon:privateGetTileXY(x,fromY)
    for y = fromY, toY, increment do
        curr = dungeon:privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "NS")
        prev = curr
    end
end


function Room:vhCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    self:verticalCorridor(dungeon, startY,endY,startX)
    self:horizontalCorridor(dungeon, endX,startX,endY) 
end

These will be a pain to fix but I think the right thing is to remove all the parameters and fetch our dungeon tile finding thing when we need it. Here goes:

function Room:vhCorridor(aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    self:verticalCorridor(startY,endY,startX)
    self:horizontalCorridor(endX,startX,endY) 
end

function Room:verticalCorridor(fromY, toY, x)
    local prev,curr
    local increment = 1
    if fromY>toY then
        increment = -1
    end
    prev = self:getDungeon():privateGetTileXY(x,fromY)
    for y = fromY, toY, increment do
        curr = self:getDungeon():privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "NS")
        prev = curr
    end
end

I decided to tolerate the duplicated calls to getDungeon rather than “optimize”. Moving right along …

function Room:isAllowed()
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            local t = self:getDungeon():getTile(vec2(x,y))
            if not t:isEdge() then return false end
        end
    end
    return true
end

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

Can’t test until this is done. I should have tried a smaller step. No matter, the big fool pushes on.

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

function Room:hvCorridor(aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    self:horizontalCorridor(startX,endX,startY)
    self:verticalCorridor(endY,startY,endX)
end

function Room:connect(aRoom)
    if math.random(1,2) == 2 then
        self:hvCorridor(aRoom)
    else
        self:vhCorridor(aRoom)
    end
end

And all the callers. A bit tedious. But the tests all run and the dungeon works. (Yes, I know that testing the Game tells me that I don’t entirely trust the tests.) Commit: Removed dungeon parm from corridor connection and isAllowed in Room.

Still seeking dungeon references in Room. I think I have them all.

Now I need to create this thing. And that’s a pain to test, because it is going to contain a dungeon and that’s a pain to set up. I gotta do it, though. I owe it to all my fans.

TileFinder

I’m going to call it TileFinder, at least until I think of a better name. Here’s part of what I’d like to say:

        _:before(function()
            _bus = Bus
            Bus = EventBus()
            _dc = DungeonContents
            DungeonContents = DungeonContentsCollection()
        end)
        
        _:test("Can access a tile", function()
            local runner = GameRunner()
            local builder = runner:defineDungeonBuilder()
            local dungeon = builder:buildTestLevel(5)
            _:expect(dungeon:is_a(Dungeon)).is(true)
        end)

That much runs. My luck is in. That’s way more setup than I’d like but I’m here to build something. Carrying on:

        _:test("Can access a tile", function()
            local runner = GameRunner(25,15)
            local builder = runner:defineDungeonBuilder()
            local dungeon = builder:buildTestLevel(0)
            _:expect(dungeon:is_a(Dungeon)).is(true)
            local tf = TileFinder(dungeon)
        end)

I need to tell the GameRunner how big the game should be. Now the test requires a TileFinder:

1: Can access a tile -- TileFinder:22: attempt to call a nil value (global 'TileFinder')

TileFinder = class()

Test will run. It does. Test more:

        _:test("Can access a tile", function()
            local runner = GameRunner(25,15)
            local builder = runner:defineDungeonBuilder()
            local dungeon = builder:buildTestLevel(0)
            _:expect(dungeon:is_a(Dungeon)).is(true)
            local tf = TileFinder(dungeon)
            local found = tf:getTile(5,5)
        end)

I’ve chosen to pass the x and y separately, mostly because it makes sense to the test. We’ll see what our real users want shortly. This will fail with no getTile.

1: Can access a tile -- TileFinder:23: attempt to call a nil value (method 'getTile')

Respond. Minimal code to get it to run just to prove I can do it that way:

function TileFinder:getTile()
    
end

Test runs. Please, can I make it harder now?

        _:test("Can access a tile", function()
            local runner = GameRunner(25,15)
            local builder = runner:defineDungeonBuilder()
            local dungeon = builder:buildTestLevel(0)
            _:expect(dungeon:is_a(Dungeon)).is(true)
            local tf = TileFinder(dungeon)
            local found = tf:getTile(5,5)
            local wanted = dungeon:privateGetTileXY(5,5)
            _:expect(wanted).is(found)
        end)

1: Can access a tile  -- Actual: Tile[5][5]: edge, Expected: nil

Hm backward call.

            _:expect(found).is(wanted)

1: Can access a tile  -- Actual: nil, Expected: Tile[5][5]: edge

Nice. Now I can code:

function TileFinder:init(dungeon)
    self.dungeon = dungeon
end

function TileFinder:getTile(x,y)
    return self.dungeon:privateGetTileXY(x,y)
end

Test runs. Class basically works. Let’s look back at Room and see what we actually want.

We have a bunch of calls like this:

        tile = dungeon:getTile(vec2(x,self.y1))

Also …

function Room:centerTile()
    return self:getDungeon():getTile(self:centerPos())
end
...
    prev = self:getDungeon():privateGetTileXY(fromX,y)
...

We ought to get our stuff together here and decide whether we want x,y or vec2(x,y). I’ll start by removing duplication. Wherever I see self:getDungeon():getTile( I’ll replace it with self:getTile( and create a method:

function Room:getTile(aVector)
    return self:getDungeon():getTile(aVector)
end

That works. Commit: consolidate getTile(pos) method in Room. Add TileFinder class and tests.

And consolidate the private ones similarly. Tests run, commit: consolidate Room’s privateGetTileXY calls.

Now I have these two methods and their callers, local to Room:

function Room:getTile(aVector)
    return self:getDungeon():getTile(aVector)
end

function Room:getTileXY(x,y)
    return self:getDungeon():privateGetTileXY(x,y)
end

I found that most of the getTile calls, that provide a vector, would rather not, so I change all of those to call getTileXY. I’m left with just one:

function Room:tileAt(x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return self:getTile(tilePos)
end

That’s the external call that we use in the learning level. Let’s fix it to match all the others:

function Room:tileAt(x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return self:getTileXY(tilePos.x, tilePos.y)
end

Now I can remove the getTile and test. Commit: consolidate all tile references in Room into getTileXY method..

Sweet. We’re nearly there. I’m not sure about that XY name. At some point I found it desirable to handle both a vector position and separate x and y. Let’s not break out the concept in Room. Rename getTileXY to getTile.

Commit: rename Room:getTileXY to getTile.

Now we can plug in our new TileFinder. First, I’ll do it in Room:init:

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.dungeon = TileFinder(dungeon)
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(announcement)
    end
end

function Room:getTile(x,y)
    return self:getDungeon():getTile(x,y)
end

Test, expecting things to work. Doh! Failed:

7: Room:announce places in ring -- MapPoint:17: bad argument #1 to 'floor' (number expected, got userdata)

This really dampens my squib, deflates my balloon, and quenches my spiritual fire. It also brings me down. I run the learning level and get a traceback that might help.

MapPoint:17: bad argument #1 to 'floor' (number expected, got userdata)
stack traceback:
	[C]: in function 'math.floor'
	MapPoint:17: in field 'init'
	... false
    end
...
end:20>
	(...tail calls...)
	BaseMap:46: in method 'atXYZ'
	Dungeon:298: in function <Dungeon:297>
	(...tail calls...)
	Room:49: in method 'announce'
	Room:33: in field 'init'
	... false
    end
	(...tail calls...)
	Dungeon:191: in local 'f'
	fp:105: in field 'reduce'
	Dungeon:193: in method 'createRoomsFromXYWHA'
	DungeonBuilder:152: in method 'createLearningRooms'
	DungeonBuilder:196: in method 'defineLearningLayout'
	DungeonBuilder:68: in method 'buildLearningLevel'
	GameRunner:52: in method 'createLearningLevel'
	Player:168: in field '?'
	Button:79: in method 'performCommand'
	Button:64: in method 'touchEnded'
	Main:118: in function 'touched'

Oh. Whew, I think. forgot to change announce’s calls:

function Room:announce(messageTable)
    local tile
    local dungeon = self:getDungeon()
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(vec2(x,self.y1))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(x,self.y2))
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(vec2(self.x1,y))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(self.x2,y))
        tile:addContents(Trigger(one))
    end
end

I think I can just remove all the vec2s.

function Room:announce(messageTable)
    local tile
    local dungeon = self:getDungeon()
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(x,self.y1)
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(x,self.y2)
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(self.x1,y)
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(self.x2,y)
        tile:addContents(Trigger(one))
    end
end

Tests and game run. Success! Commit: Room now uses only TileFinder, not Dungeon. Created locally, need to fix callers.

Now that we know it works, let’s change the internal variables in Room to make more sense. A handful of changes like this:

function Room:announce(messageTable)
    local tile
    local tileFinder = self:getTileFinder()
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = tileFinder:getTile(x,self.y1)
        tile:addContents(Trigger(one))
        tile = tileFinder:getTile(x,self.y2)
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = tileFinder:getTile(self.x1,y)
        tile:addContents(Trigger(one))
        tile = tileFinder:getTile(self.x2,y)
        tile:addContents(Trigger(one))
    end
end

Test. Commit: Change Room to use tileFinder names, not “dungeon”.

Now, finally, the callers need to pass in a TileFinder, not a dungeon.

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

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

function Dungeon:getTileFinder()
    return TileFinder(self)
end

And …

function Room:random(spaceX, spaceY, minSize, maxSize, tileFinder)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,tileFinder, false)
end

And something goes wrong. I’ve changed too much. I could debug, but I’ll be better off to revert and take smaller steps.

OK. We’ve changed init so that it creates a TileFinder:

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.tileFinder = TileFinder(dungeon)
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(announcement)
    end
end

How can I go smaller? Or must I? Could we be satisfied if the object itself creates the Façade? Everything is working now, so the Room object is effectively isolated from the bulk of the Dungeon methods. Still, I think it’s best if we change all the calls. Let’s do this, to allow for incremental change:

function Room:init(x,y,w,h, tileFinder, paint, announcement)
    self.tileFinder = tileFinder
    if self.tileFinder:is_a(Dungeon) then
        self.tileFinder = TileFinder(tileFinder)
    end

This should work as before. And it does. Now let’s look for all the direct creators of Room, which are probably mostly in tests.

There are a raft of them But I found them all. Commit: All direct creators of Room pass a TileFinder.

Now let’s fix this:

function Room:random(spaceX, spaceY, minSize, maxSize, dungeon)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,dungeon, false)
end

function Room:random(spaceX, spaceY, minSize, maxSize, tileFinder)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,tileFinder, false)
end

And now to find all those references. There are only two, a test and the actual random room creation. Test, commit:

Now this:

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

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

Find callers. Just a couple. Tests and game run. Commit: Room:fromXYWA calls all pass TileFinder not Dungeon.

Now I should be able to replace that if with an assert.

function Room:init(x,y,w,h, tileFinder, paint, announcement)
    assert(tileFinder:is_a(TileFinder), "Room requires a TileFinder")
    self.tileFinder = tileFinder
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(announcement)
    end
end

Test. Runs. Commit: Room asserts if not provided with TileFinder.

Now then. Let’s look at all the calls to TileFinder, because now there are a lot of them. That’s telling me something.

A lot of tests look like this:

        _:test("Horizontal corridor", function()
            local tile
            local msg
            local dungeon = runner:getDungeon()
            local room = Room(1,1,1,1, TileFinder(dungeon))
            room:horizontalCorridor(5,10, 7)
            tile = runner:getTile(vec2(4,7))
            _:expect(tile:isEdge()).is(true)
            tile = runner:getTile(vec2(11,7))
            _:expect(tile:isEdge()).is(true)
            r,x,y = checkRange(dungeon, 5,7, 10,7, Tile.isFloor)
            msg = string.format("%d %d", x, y)
            _:expect(r,msg).is(true)
        end)

I think that would be better this way:

        _:test("Horizontal corridor", function()
            local tile
            local msg
            local dungeon = runner:getDungeon()
            local room = Room(1,1,1,1, dungeon:tileFinder())
            room:horizontalCorridor(5,10, 7)
            tile = runner:getTile(vec2(4,7))
            _:expect(tile:isEdge()).is(true)
            tile = runner:getTile(vec2(11,7))
            _:expect(tile:isEdge()).is(true)
            r,x,y = checkRange(dungeon, 5,7, 10,7, Tile.isFloor)
            msg = string.format("%d %d", x, y)
            _:expect(r,msg).is(true)
        end)

No one really needs to know this class name except dungeon. Test to drive out the change.

1: Horizontal corridor -- Tests:40: attempt to call a nil value (method 'tileFinder')

function Dungeon:tileFinder()
    return TileFinder(self)
end

Tests should run. They do. Find and replace all the other lines like that one. Not too difficult, used Sublime multi-cursor. Tests run. Commit: use Dungeon:tileFinder() where appropriate.

Look for more occurrences of TileFinder being created.

A bunch look like this:

                local r = Room:random(60,60, 5,16, TileFinder(runner:getDungeon()))

Those want to be:

                local r = Room:random(60,60, 5,16, runner:tileFinder())

Search and destroy, er, fix, with new method:

function GameRunner:tileFinder()
    return self:getDungeon():tileFinder()
end

After some searching, there is but one occurrence of TileFinder(, in Dungeon:

function Dungeon:tileFinder()
    return TileFinder(self)
end

Test. Commit: Room uses TileFinder, not Dungeon. Only dungeon creates TileFinder instance.

Well. There we are. Let’s sum up.

Summary

This exercise was an interesting combination of easy and tedious. The actual change is of course trivial, as is the new TileFinder class. But there are three different ways of creating a room, and there were a great many callers, almost all of them in tests. (I suppose that’s good news, after a fashion.)

So aside from a couple of missteps where I just reverted and did again, it all went very smoothly.

We did 16 commits over about three hours, About one every 11 minutes, not too bad at all.

We have to ask whether it was all worth it. All that has been accomplished is to make a very narrow class, with just one method in it, and pass that class to Room, to allow Room to access Tiles as needed, instead of passing an instance of Dungeon. Dungeon class has somewhere between 35 and 40 methods. So we have limited Room to just the one method it currently needs, but of course it only called the method it needed. Is this a net gain or not?

I think it is, at least to my mind. We’re trying to simplify the D2 program by separating building from operating. Most of dungeon is about operating. Possibly all of it is now about operating: we’ll have to check to be sure, but in any case we’re getting closer.

I am also getting a vague feeling about something waiting to be born. A lot of what we do that involves tiles is either fetching a single tile from its coordinates, or fetching a group of neighboring tiles to a given position, and then winnowing them down to make some operational decision. I have the feeling that there’s a concept there to be broken out.

We’ll see. For this morning, I’ve had more than enough. See you next time!

Also spelled “facade”, of course. I couldn’t resist learning how to type the cedilla on my iPad. ↩