Dungeon 266
Are we missing some layers in the Dungeon? The classes, not the places.
It seems to me that we’re well on our way to removing building-related code from GameRunner, perhaps done, anyway nearly done, but I’m a bit concerned about Dungeon class. It seems to have at least a few rather different kinds of capabilities.
There are methods for finding tiles at desired distances from other tiles, used in moving monsters around. You can check to see if a tile can be stepped on, i.e. floor. You can ask which of two tiles is further from some target tile. Most of those are used during game play.
You can ask for a random tile avoiding a given tile, or avoiding a given room. This kind of thing is used during game setup, to place objects.
Then there are methods for creating rooms, which use the Room class to do some of their work. There’s a method to connect any two rooms with straight paths from center to center, which use methods to draw a horizontal or vertical corridor.
Curiously, to connect two rooms, Dungeon does this:
roomA:connect(roomB)
That method, in Room, is:
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:hvCorridor(dungeon, aRoom)
local startX,startY = self:center()
local endX,endY = aRoom:center()
dungeon:horizontalCorridor(startX,endX,startY)
dungeon:verticalCorridor(endY,startY,endX)
end
function Room:vhCorridor(dungeon, aRoom)
local startX,startY = self:center()
local endX,endY = aRoom:center()
dungeon:verticalCorridor(startY,endY,startX)
dungeon:horizontalCorridor(endX,startX,endY)
end
Dungeon calls Room, which calls back to Dungeon. That’s amusing but strikes me as odd. Room instances are really nothing more than some coordinates, but they do paint the flooring:
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
Hm. No reason to be talking to runner there, is there? That’s a thing we should look at changing. Right now, Rooms know the runner, and it is possible that they should only know the dungeon. I’ll check …
It turns out that Room has two uses for runner. It uses it to fetch specific tiles, which it could readily do using dungeon, and when it creates a tile, it passes it the runner. We’re on a path to change that, which means that Room can be downgraded to having a Dungeon rather than a GameRunner, in due time.
Further research finds that the Tile creation is a dead end. No one calls it. Let’s remove that method and test. We’re good. Commit: remove unused Room:correctTile.
OK, now we just have these uses for runner in Room:
function Room:init(x,y,w,h, runner, paint, announcement)
if paint == nil then paint = true end
self.x1 = x
self.y1 = y
self.x2 = x + w - 1
self.y2 = y + h - 1
self.runner = runner
if paint then
self:paint()
end
if announcement then
self:announce(runner:getDungeon(), announcement)
end
end
function Room:centerTile()
return self.runner:getTile(self:centerPos())
end
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
Let’s do our accessor trick from yesterday:
function Room:getDungeon()
return self.runner:getDungeon()
end
Now I’ll convert those two runner calls to use getDungeon.
function Room:centerTile()
return self:getDungeon():getTile(self:centerPos())
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
Test. Good. Commit: added and used getDungeon method in Room, to cover access to runner instance in preparation for change.
You might be wondering why I’d do this. My personal reason is that I’m fresh on the topic right now, and I know we’ll want to change all those reverences sometime, hopefully soon, so if I centralize the actual code that needs changing, self.runner:getDungeon(), I’ll be able to change it in just one place with impunity.
There’s a small argument from duplication, in that we hav removed the duplicated code self.runner:getDungeon(), and given the concept some meaning.
Finally, some people would say that member variables should always be used with accessor methods. I don’t operate according to that rule, with the result that sometimes I do and sometimes I don’t, which is arguably not ideal. I do try, when there is an accessor, to use it always, but even there I am probably not as consistent as I should be.
Today … it’s just making a future change easier.
Where Were We?
Oh, right, we were looking at whether Dungeon has too many different responsibilities. I think we can say that it does. It has methods used only in building the dungeon, and methods used only in running the dungeon.
Now when we have an object with two capabilities like being a hair spray and a food processor, it’s usually pretty easy to see how it can be split up. I find it more difficult when the split is between building a thing and using the thing.
The Dungeon contains a map (either a Cartesian one or a Hex (ptui!) one). The map handles low-level access to the tiles in the map, basically providing nothing but x,y access, though it does create the initial “array” as well.
Possibly DungeonBuilder should be a MapBuilder. One way or the other, it seems that moving some of the creation methods over from Dungeon would be a good thing.
Let’s do a quick exploration …
function DungeonBuilder:buildLevel(roomCount)
self:dcPrepare()
self:defineDungeonLayout(roomCount)
self:placePlayer()
self:customizeContents()
self:dcFinalize()
return self.dungeon
end
It comes to my mind that the GameRunner is still creating the Dungeon and passing it to DungeonBuilder. That seems off to me.
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
return DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end
We might do well to see whether we can create it in DungeonBuilder … here’s a quick experiment.
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
local dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
local numberOfMonsters = 6
return DungeonBuilder(self,dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end
function GameRunner:createLevel(roomCount)
local builder = self:defineDungeonBuilder()
builder:buildLevel(roomCount)
self.dungeon = builder:getDungeon()
self:prepareToRun()
end
This is a hack, but it should tell me whether I’ve covered all the uses of GameRunner’s dungeon during building. I make a similar change to buildTestLevel after it explodes.
Still explodes:
GameRunner:192: attempt to get a nil dungeon
stack traceback:
[C]: in function 'assert'
GameRunner:192: in method 'getDungeon'
Tile:92: in field 'init'
... false
end
...
(...tail calls...)
Dungeon:246: in method 'createTiles'
DungeonBuilder:146: in method 'dcPrepare'
DungeonBuilder:60: in method 'buildTestLevel'
GameRunner:65: 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, the quick answer is “no, we can’t do that”. I see at least two options (and kind of only two):
I can defer this issue of creating the dungeon and passing it in, and move on to more easy changes, or I can dig in and figure out how to fix this weird temporal coupling. The former is easier and should have a good chance of slowly resolving the issue above, or at least removing brush from around it. But there’s a chance that the final change will be too difficult, or even impossible, leaving a weird connection in the system.
I claim that’s no worse than we have now, and that impossible is unlikely anyway.
Still, I’d like to understand it better. What has happened here?
We were in dcPrepare, line 146, called by createTestLevel
function DungeonBuilder:dcPrepare()
TileLock = false
self.dungeon:createTiles() -- 146
self.dungeon:clearLevel()
DungeonContents = DungeonContentsCollection()
MonsterPlayer(self.RUNNER)
Announcer:clearCache()
end
Well, first thing is, we really don’t want createTiles to be in Dungeon anyway. But it is. So fixing that might be better preparation for the hack change we just tried. But why doesn’t it work?
function Dungeon:createTiles()
Maps:cartesian()
self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
end
Right. Tiles do this:
function Tile:init(mapPoint,kind, runner)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
self.mapPoint = mapPoint
self.kind = kind
self.dungeon = runner:getDungeon()
self:initDetails()
end
So we call back to the runner and don’t find a dungeon yet. Let’s revert our hack and fix Tile up. It’ll be slightly tedious but should be easy enough.
function Tile:init(mapPoint,kind, dungeon)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
assert(dungeon:is_a(Dungeon), "Tile requires dungeon")
self.mapPoint = mapPoint
self.kind = kind
self.dungeon = dungeon
self:initDetails()
end
This code will trap all our many different tile creations. I just plan to run until I find all, or most of them. But first I’ll search out the obvious ones. First Tile(. The only direct creations are in the class methods, hall, room, wall and edge. So we can look for Tile: to find those.
We have a raft of tests like this:
_:test("player direction", function()
local dx,dy
local player = Player:privateFromXY(100,100, runner)
runner.player = player
local tile = Tile:edge(101,101,runner)
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(-1)
_:expect(dy).is(-1)
tile = Tile:edge(98,98, runner)
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(1)
_:expect(dy).is(1)
end)
That was quite tedious, there are about a thousand tests creating tiles and testing TileArbiter and such. But we are green. Commit: Tile now receives its dungeon, not its runner.
My brain says this is a good time to stop. Let’s sum up.
Summary
We’ve reduced the span of concern of Tile class from runner scope to dungeon scope. This involved a lot of changes to tests and a couple of changes to code. Tile is well tested, as is its collaborator Tile Arbiter. A smarter editor would have made it less tedious but it wasn’t that difficult even so.
We did a bit of preparation in Room, giving it a getDungeon accessor that we’ll be able to modify if and when we change Room to receive a dungeon rather than runner. I think we’ll do that.
The main reason for changing Tile to expect a dungeon was that DungeonBuilder doesn’t want to have a GameRunner instance, so that building can be separate from running. Since DungeonBuilder uses Tile (indirectly), we needed to be sure that Tile wasn’t using its runner instance for anything interesting.
And in fact it wasn’t.
The whole “mess”, if mess it is, is due to my desire not to pass a lot of different objects around, combined with a bit of laziness. If an object has the GameRunner, it can do pretty much anything, and that made a lot of changes easy. It also left mud tracked through a number of objects, and, over time, that makes everything just a bit harder to really understand, and a lot harder to tease apart into more cohesive objects.
This is not something that I do for pedagogical purposes. I do it because I’m human, lazy, fallible, and prone to mistakes, errors, and redundancy1. The good news is that this results in plenty of opportunity for me to demonstrate, again and again, that it’s not that hard to improve code that has become messy. It would be better not to let things become messy, but in most code bases, it seems to happen no matter what we do.
I think what needs to be done next is perhaps to move the room and hallway painting code over to DungeonBuilder, but we’ll decide that after we review things with fresh eyes.
For now, the evil of the day etc etc. See you next time!
-
Also repetition and saying things over and over in slightly different ways. ↩