I think we’ll look at some of the methods that analyze the dungeon as part of building. I’m hoping for a good idea to pop up. TL;DR: Ron is out of kilter, copes, then stops before doing damage.

The cat goddess has accepted my food offering, I have chai and a banana, my Mac is downloading new updates very slowly, and SXM is playing Good Day Sunshine. It’s overcast, not sunny, and about -1C. It must be time to do some refactoring.

I am of course quite confident that “large” refactorings can always be done in small steps. However, I have to admit that with over 100 small committable steps in this exercise, I am impressed with how well it has gone. Slowly, we’ve moved code from Dungeon to Builder, and narrowed the scope of concern of Tile and Room substantially, all without breaking the system. Most of the work has been quite simple in nature.

We find something that Dungeon is doing that seems to belong in building, we move it over with light edits. Possibly we defer a few of that method’s calls back to Dungeon. Bit by bit, we are withdrawing the tentacles that DungeonBuilder has into Dungeon, drawing small chunks of code over. It’s not quite hypnotically simple, but it has been rather straightforward in most cases.

Along the way, we’ve created a couple of Façades on top of Dungeon, limiting access to just the necessary, rather than facing other objects with the whole Dungeon class. This makes understanding those objects easier, reduces the chance of messing up the design, and might even help us move other capabilities into better objects. That last has not happened yet, but the year is young.

I think I’ll get to the methods that search the nascent dungeon for places to put things, but we’ll start with a view of DungeonBuilder, to see what it seems to want.

DungeonBuilder

I search for self.dungeon: in DungeonBuilder. Lines containing that string will be sending messages to the dungeon. Let’s see what we find:

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

Ah. First one is an example of the things I thought we might do today. There are methods on Dungeon that search the tiles to find locations of interest. Here, we want to place spikes in a hallway, so that they can’t be avoided. (Unless we happen to find a wide hallway. Nothing is perfect, at least not around here.)

Let’s look at a few more references, before we choose.

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

That’s all there is. No wonder that I thought we might work on these methods today. Currently, of course, all these method are in the BuilderView, forwarding to the Dungeon:

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

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

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

Let’s now review what the real methods do, and pick an easy one to move over.

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoiding(x1,y1, x2,y2)
end

Uh oh. This method accepts a parameter, and our forwarding method does not. This means that it is possible that something could turn up improperly in the first room (that’s the only room we ever avoid, I think). Even though we’re here to replace this method, let’s fix the BuilderView. I think I’ll just have the convention that all its methods have the … argument list.

Test, commit: ensure BuilderView passes all parameters.

That kind of gave the lie to my saying that we’d doe 100 commits and not broken the system. We briefly introduced a defect where something could appear in the main room that should not. Bad Ron, no biscuit.

Huh. That sort of bump in the road unsettles me. Focus.

Moving RandomRoomTile()

Copy the method and paste into DungeonBuilder, to see it in context. Well, no. We have a partial implementation already:

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

The Dungeon version is this:

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoiding(x1,y1, x2,y2)
end

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

So it’s really randomRoomAvoiding that we need to move over, with a one line change to our existing method.

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

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

function DungeonBuilder: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

I moved over randomPositionAvoiding, which is really a method that belongs neither to Dungeon nor DungeonBuilder, but it needs to live somewhere. Maybe we need a Utilities class or something. Anyway, test this to see why I’m mistaken.

DungeonBuilder:296: attempt to compare table with number
stack traceback:
	DungeonBuilder:296: in function <DungeonBuilder:294>
	(...tail calls...)
	DungeonBuilder:253: in method 'placeLoots'
	DungeonBuilder:183: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

That didn’t take long to break.

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

The error is on the compare. We have a nil parameter. But that’s supposed to work.

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoiding(x1,y1, x2,y2)
end

I need to do that. Also it’s good that this broke as it did because otherwise it would have recurred forever. I have done something weird with the naming of these methods.

I feel a bit of confusion. The thing to do is revert and go again.

Again

Starting over, I see what has confused me. Here’s the method in DungeonBuilder:

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

This method, called randomRoomTile, takes a room number, and calls a method of the same name that takes a room as its parameter. I think someone picked a poor name, probably thinking to improve things. They were mistaken.

This has made things weird. And I’m sort of simulating what would happen if we were doing this refactoring over a long period. I’ve looked back into prior articles (history) to see if I can figure out why this was done, and I can’t find (remember) anything useful. Nothing for it but to sort it out.

I want the DungeonBuilder:randomRoomNumber to have the same signature as in Dungeon. Then we can move the functionality over fairly readily. So this method’s name is wrong.

I change this method’s name and carefully call it from inside DungeonBuilder:

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

Test. Crash:

Decor:74: attempt to call a nil value (method 'randomRoomTile')
stack traceback:
	Decor:74: in local 'f'
	fp:71: in function <fp:68>
	(...tail calls...)
	DungeonBuilder:240: in method 'createDecor'
	DungeonBuilder:184: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

That’s this:

    Decor:createRequiredItemsInEmptyTiles(items,self)

function Decor:createRequiredItemsInEmptyTiles(items, runner)
    return ar.map(items, function(item)
        local tile = runner:randomRoomTile(666)
        return Decor(tile,item)
    end)
end

OK that can be the avoiding room number one OK, but it still feels rather broken to me. Oh, it’s calling through Runner. He needs to change to call my new method. I was ignoring everything but Dungeon and Builder.

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

Arrgh. How did this ever work?

At this point I realize that my mind is fuzzy this morning, perhaps a residual from my tooth extraction or just perhaps not my most clever morning. Maybe I’m losing it, if I ever had it. I’m going to revert again and expand my considerations a bit.

Yet Again

What’s with the reverts? Well, I find that when I’ve started to make a series of changes that I think make sense, and then hit a problem from outside the area I’ve been thinking about, it’s telling me that I don’t understand the situation well enough. The problem is wider than I realize.

And I’ve just changed the code from working to not working. While, if not confused, at least insufficiently informed. So I want to get back to a stable place to stand. Yes, I could surely make this work. But with the changes spread around as they look to be, I want to start with a better picture.

So I revert. Start again:

And maybe this time, start somewhere else. It’s all good. Let’s see about changing this one:

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

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

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

Let’s see if we can move this one over. First put the method into DungeonBuilder …

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

We need to fix those accesses to the counts: we don’t have those as member variables in Builder.

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

I don’t have those methods yet, I’ve been calling to the view. Let’s cover them:

function DungeonBuilder:tileCountX()
    return self.view:tileCountX()
end

function DungeonBuilder:tileCountY()
    return self.view:tileCountY()
end

This should run, since we aren’t calling the new methods yet. I want to commit, to save the tileCount methods. Commit: Added tileCountX() and Y() to DungeonBuilder.

Now the getTile calls. We have no such method in DungeonBuilder. Nor do we have it in the view. We do have a getTile in TileFinder and it takes X and Y coordinates. I really wish that Jeffries had been more consistent with the naming of these methods.

My choices include adding a suitable method to DungeonBuilder and BuilderView, or adding just to BuilderView and calling it, or making BuilderView into a subclass of TileFinder and inherit the method. I think that last would be a waste of inheritance. I decide this:

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

function BuilderView:getTileFromPos(pos)
    return self:getTile(pos.x, pos.y)
end

Remind me to talk about pushing this decision much further down. But not now. Back in DungeonBuilder:

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:getTileFromPos(pos)
    return self.view:getTileFromPos(pos)
end

Now I think I can call this and have a chance that it works.

Tests and game run. Commit: randomHallwayTile implemented and used in DungeomBuilder.

Now let’s see if we can remove that method from Dungeon. Yes. Commit: remove randomHallwayTile from Dungeon.

Break.

ReStrospective

It’s clear to me that I need to pull my head out. I’m not at my best. But with this small success under my belt, I think I’ll try another move, if it’s easy. Choices are:

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

When making a hard change, Kent Beck tells us “make the change easy, then make the easy change”. What made the change hard in the second case above was the use of methods with the same name but different meaning and signatures. Let’s make the change easier by giving the methods in question, randomRoomTile more meaningful names.

I choose randomRoomTileAvoidingRoomNumber and randomRoomTileAvoidingRoom. But … do those names exist? They might.

We’re in luck. We do have randomRoomTileAvoiding(x1,x2,y1,y2). I’ll rename that first, to randomRoomTileAvoidingCoordinates. Test. Commit: rename randomRoomTileAvoiding to randomRoomTileAvoidingCoordinates.

Inch by inch, especially when not firing on all cylinders.

Now the other renames, avoidingRoonNumber or Room. The changes are extensive. I’ll spare you the code pasting. Just when I think I’m done, I find one failure:

DungeonBuilder:315: attempt to call a nil value (method 'randomRoomTileAvoidingRoom')
stack traceback:
	DungeonBuilder:315: in method 'randomRoomTileAvoidingRoonNumber'
	DungeonBuilder:253: in method 'placeLoots'
	DungeonBuilder:183: in method 'customizeContents'
	DungeonBuilder:55: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

Failed to build that forwarder. Test.

Monster:55: attempt to call a nil value (method 'randomRoomTileAvoidingRoom')
stack traceback:
	Monster:55: in method 'getRandomMonsters'
	Monsters:94: 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'

Fixing that I get this:

1: Create the monsters -- Monster:55: attempt to call a nil value (method 'randomRoomTileAvoidingRoomNumber')

That turns out to be a missing method on a test double. OK the renaming is done. Commit: renaming methods ….avoidingRoom[Number} for consistency.

Despite having made the change easy, I think I’m done for the day. Let’s sum up.

Summary

Pretty unproductive day. Fortunately I’m in touch enough to realize that I feel off, and to proceed with caution. And caution tells me not to attempt the next move, even though I am sure it’s simple. Certainly simpler than it was before the rename. But I feel muzzy and it’s best not to screw things up.

Tomorrow, I hope, will be a better day. It will also be a weekend day. Anyway, next time, we’ll hope for a better day.