Dungeon 271

My side quest to move dungeon building out of Dungeon class continues. What monster shall I slay today? Let’s find out. (Stress level too high. A clue.)

I’ve done some side-side quests, narrowing the scope of objects Tile and Room. Those have provided some value in terms of simplicity, which believe me there is not enough of in this world nor in this program. And I’ve moved a fair amount of Dungeon code into DungeonBuilder. DungeonBuilder is 279 lines right now, including only 30 lines for its one test. (It is largely tested by existing tests, so that’s not as awful as it sounds.)

In a sense, further changes are easy to find. When DungeonBuilder cannot yet do something, it calls its instance of Dungeon to do the work, like this:

function DungeonBuilder:dcPrepare()
    TileLock = false
    self.dungeon:createTiles()
    self.dungeon:clearLevel()
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

Occurrences of self.dungeon:xxx in DungeonBuilder are easy to find, and when we manage to move xxx over, we’ve removed some building code from Dungeon and moved it to the Builder. When there are no such references left … well, we may not be entirely done, but Dungeon will be free of building code, which is the avowed purpose of this side quest.

So far, there are over 100 commits in this side quest. Each one of those left us with a running program, with one exception where I messed up the commits and decided to leave a broken commit in rather than revert out a lot of good changes. That interval lasted just a few minutes.

We have been working on this since at least March 7th, and it’s the 30th today. With 100 commits, we could have wofrked a few minutes every day for the last three months instead of three weeks, and been improving the design just a bit every day.

Incrementally doing large refactorings is definitely a possibility. And it’s better by far than not improving the space we work in.

Now let’s be clear. If I were doing these changes one per day there would be overhead that I do not experience. Commits have been taking me between ten and fifteen minutes. If I were to do just one thing (and not write an article about it), I could imagine that it might take me a bit longer. On the other hand, if I had someone to pair with, that would help. In any case, I’m not claiming that a few hours focus on refactoring is no better than a few minutes per day. But it should be clear that even with a half hour, we can make things a bit better, and move in the direction of an important large change.

Anyway, let’s get to it for today.

What Shall We Do Today, Brain?

Since I found them as a random example above, let’s look at the dungeon references in dcPrepare. If I recall, they’re probably a bit tricky.

function DungeonBuilder:dcPrepare()
    TileLock = false
    self.dungeon:createTiles()
    self.dungeon:clearLevel()
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

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

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

Well, that’s not so bad, is it? There is a design issue to think about, however. Let me make a change and we can discuss the issue with an example in front of us.

function DungeonBuilder:dcPrepare()
    TileLock = false
    self.dungeon:createTiles()
    self:clearLevel() -- <===
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

function DungeonBuilder:clearLevel()
    for key, tile in self.dungeon.map:pairs() do
        tile:convertToEdge()
    end
end

I do expect this to work. Let’s test. Works fine, got this nice dungeon:

Let’s commit, since it works: DungeonBuilder has its own clearLevel. I remove the version in Dungeon, test again, commit again: Remove clearLevel from Dungeon

So, what’s the design issue? As written, the clear level method is grabbing a member variable, ‘map`, out of the Dungeon:

function DungeonBuilder:clearLevel()
    for key, tile in self.dungeon.map:pairs() do
        tile:convertToEdge()
    end
end

Now in general, grabbing member variables out of the objects we hold is a violation of the “Law of Demeter”. We can argue that this object is special, because it is a DungeonBuilder and therefore can be allowed to know a lot of detail about what it takes to build a dungeon. Even so, it seems that the less DungeonBuilder knows about intimate details of the Dungeon, the better. Taking that as guidance, what might we do to improve this code?

One simple approach would be to provide accessors, getMap in this case, which would allow the Dungeon to protect its actual members. It could even, in principle, return some transformation of its real map.

An objection to this idea is that it “just adds more methods to Dungeon, and it’s already too big, and besides, the method would just invite other people to grab the map”. Yes. We could name the method something like builderOnlyGetMap, which might discourage most people from using it. What else might we do?

Maybe we could take a page from our TileFinder object, which covers Dungeon and provides a single method that can get a tile given its coordinates. We could build a BuilderAccess object and pass that in to the builder rather than the whole Dungeon (although we’d have to provide for calls through to Dungeon as we move references over). The BuilderAccess would have a “view” of the dungeon, with just the methods it needs to pull information out of Dungeon and provide things to be put back in.

That object adds a bit of complexity to the universe, as we’d have a more complex object diagram, but at the same time, we’d “add” simplicity, because the new BuilderAccess thing would say exactly what can and cannot be done to the Dungeon by the builder.

This object would be a kind of Adapter, or a kind of Façade. So it’s certainly not unheard of.

Having thought of it, I rather like it. Right now, DungeonBuilder can put its fingers anywhere in the Dungeon object. With the BuilderAccess in place, its potential impact would be limited.

There are some concerns. First of all, the sooner we do this the better, and there are already some accesses that need to be covered. So this is another side-side quest. A lesser concern is that I really don’t want to write TDD tests for a trivial object like this. So I won’t. The existing tests will almost certainly find any problems.

And there is a place to put it. I can put it in the TileFinder tab, and even write tests for it there if need be.

OK, side quest accepted.

Builder Access Object

What is the name of this object? I’ve been calling it BuilderAccess. It’s actually a “kind of” dungeon, isn’t it? We could call it BuilderDungeon, but we have a class DungeonBuilder already, so that name would sure confuse me in the distant future, like at about 1130 hours today.

We’ll stick with BuilderAccess for now. There will be only one of them, I should think.

Forgive me for this, I’m just going to type it in and let the existing tests find its problems. The fundamental design decision is that when the Dungeon creates the DungeonBuilder, it will pass in the BuilderAccess rather than self:

OK, wrong: it’s the GameRunner who actually creates the DungeonBuilder, like this:

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

Fine. Instead of passing in the Dungeon, we’ll do this. (I’ve decided to rename the thing to BuilderView.)

    return DungeonBuilder(self,self.dungeon:asBuilderView(), self.dungeonLevel, self.playerRoom, numberOfMonsters)

That will drive out the method:

GameRunner:107: attempt to call a nil value (method 'asBuilderView')

function Dungeon:asBuilderView()
    return DungeonBuilderView(self)
end

That will drive out the class:

Dungeon:133: attempt to call a nil value (global 'DungeonBuilderView')

DungeonBuilderView = class()

That will drive out some method, I have no idea what.

DungeonBuilder:187: attempt to call a nil value (method 'createTiles')

OK, createTiles.

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

function DungeonBuilderView:createTiles(...)
    return self.dungeon:createTiles(...)
end

Whatever args there are, I’ll just pass them along. I think that usage of ... is righteous. Test, see what happens. Hm, same error?

DungeonBuilder:187: attempt to call a nil value (method 'createTiles')

Need to be more careful, look harder. Here’s 187:

    self.dungeon:createTiles()

Hm the dungeon member variable is a GameRunner. That’s not right. Ah. I was calling it DungeonBuilderView and then mistakenly typed DungeonBuilder in the method definitions. Changed:

BuilderView = class()

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

function BuilderView:__tostring()
    return "DungeonBuilderView"
end

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

Now we’ll get a new error, I’m sure.

DungeonBuilder:76: attempt to index a nil value (field 'map')

Responding:

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

This is “easy”, but it is a bit large, in that I can’t really commit a working version until the BuilderView is replete. Shouldn’t take long but it’s not what I’d call micro-steps. Continuing:

DungeonBuilder:76: attempt to index a function value (field 'map')

Ah. I was accessing it directly, wasn’t I? Change:

function DungeonBuilder:clearLevel()
    for key, tile in self.dungeon:map():pairs() do
        tile:convertToEdge()
    end
end

DungeonBuilder:207: attempt to call a nil value (method 'getRooms')

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

DungeonBuilder:106: attempt to index a function value (field 'map')

Another line needs converting to a call:

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

DungeonBuilder:120: attempt to call a nil value (method 'tileFinder')

That’s interesting. The line is this:

            r = Room:random(self.dungeon.tileCountX,self.dungeon.tileCountY, 5,14, self.dungeon:tileFinder())

But aren’t we supposed to be saying asTileFinder? Yes. There was a method tileFinder left over.

This changes:

            r = Room:random(self.dungeon.tileCountX,self.dungeon.tileCountY, 5,14, self.dungeon:asTileFinder())

And this is added:

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

The first thing I discover is a lot of people calling tileFinder, so I’ll fix those. Runner has tileFinder also. I’m not sure that should be asTileFinder, so I left it.

This is taking long enough to make me nervous. We’ll press on a bit more.

With the asTileFinder mess sorted, I get a couple of tests failing and a traceback:

1: Can access a tile  -- Actual: false, Expected: true
1: Can access a tile -- TileFinder:24: attempt to call a nil value (method 'privateGetTileXY')
Room:16: attempt to perform arithmetic on a nil value (local 'spaceX')

I can feel the blade swinging over me, dropping ever lower … I don’t see a more incremental way to go, and I do think we’re getting close. Let’s look at the test that’s failing.

        _: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 aFinder = dungeon:asTileFinder()
            local found = aFinder:getTile(5,5)
            local wanted = dungeon:privateGetTileXY(5,5)
            _:expect(found).is(wanted)
        end)

Hmm … it seems that buildTestLevel is supposed to return the dungeon. But we don’t have one. Let’s check those build methods:

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
    return self.dungeon
end

Right. We need to do this:

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
    return self.dungeon:asFullDungeon()
end

And that needs to go into the other build methods. And to implement:

function BuilderView:asFullDungeon()
    return self.dungeon
end

Test … we still get the traceback:

Room:16: attempt to perform arithmetic on a nil value (local 'spaceX')

            r = Room:random(self.dungeon.tileCountX,self.dungeon.tileCountY, 5,14, self.dungeon:asTileFinder())

Right. Our BuilderView doesn’t know those counts. Make them methods:

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

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

Test. Oh this is interesting:

TileFinder:71: attempt to index a nil value (local 'self')
stack traceback:
	TileFinder:71: in field 'tileCountX'
	DungeonBuilder:120: in method 'createRandomRooms'
	DungeonBuilder:195: in method 'defineDungeonLayout'
	DungeonBuilder:52: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

What is this? No self? Oh. Dot instead of colon. Should be:

            r = Room:random(self.dungeon:tileCountX(),self.dungeon:tileCountY(), 5,14, self.dungeon:asTileFinder())

DungeonBuilder:260: attempt to call a nil value (method 'randomHallwayTile')

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

Right.

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

DungeonBuilder:285: attempt to call a nil value (method 'randomRoomTile')

Similarly:

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

DungeonBuilder:268: attempt to call a nil value (method 'farawayOpenRoomTile')

And

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

We’re just ticking through these. I’m feeling more relaxed, but it has been a while, almost 90 minutes since my last commit. Test.

Didn’t take long to make me feel unrelaxed:

Tile:189: attempt to index a nil value (local 'aTile')

function Tile:distanceFrom(aTile)
    return aTile:distanceFromMapPoint(self.mapPoint)
end

This guy was called with a nil for aTile, from …

Oh, whew. That method we just wrote has arguments:

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

We should just do that for all of them, perhaps. Or be explicit, that would be nice. I’m trying to go to fast, probably.

New error:

DungeonBuilder:292: attempt to call a nil value (method 'setMonsters')

function BuilderView:setMonsters(...)
    self.dungeon:setMonsters(...)
end

We must be getting close. I wish I had had this view idea sooner …

The tests all run. No traceback. Check the game. Main game runs. Creating Learning Level traces back. I’m not surprised, that’s why I tried it:

DungeonBuilder:157: attempt to call a nil value (method 'createRoomsFromXYWHA')

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

Everything works! Whee! Commit: DungeonBuilder now works through BuilderView, not dungeon.

We changed 26 lines and added about 60. And we can ship it.

I’ve been at it for a couple of hours, with and hour and three quarters spent with no commits. That’s too long. Let’s reflect and sum up.

Reflection

That was stressful. I was worrying that I’d hit something that I couldn’t fix. That’s not a sensible fear, since anything can be done by forwarding, but it hearkened back to other long efforts without intervening commits, where things could, and therefore did, go wrong.

What could have made this more incremental?

I think the easiest thing would have been to put in a feature toggle. A switch set to pass in the regular dungeon to the builder when the game is in production mode, and to pass in the view when in dev mode. That would have let me commit each time I added a method, and I could have worked for ages without ever breaking the system.

The smartest thing would have been to have foreseen the concern, the breath of the interface, and devised the BuilderView right at the beginning. Then each change would have added or removed a method or two in the View, and we’‘d have had the same incremental growth that we saw, just going through the view instead of direct to Dungeon. Each step would have taken about one line of code more, and the use of the View might even have made it easier to think about things.

I am not allowed to go back on my own timeline, so being smarter in the past is simply not an option for me. I do recall thinking about a sort of “builder packet” that would be filled in with parameters for the builder, and a similar return packet that could be sent back and used by the dungeon to fill things in as it wished. That seemed more trouble than it was worth, so I didn’t do it. But we do see some evidence that it could be a good idea.

Notably, the access to TileCountX and TileCountY should be some kind of parameters to the Builder. It might still be the thing. Perhaps our BuilderView turns out to be created with some parameters built in instead of referred to back in Dungeon.

We’ll try to stay alert for that.

Summing up, though, this side quest seems to be done. I hope the cat is happy. And I hope someone out there finds value here. I know that I am, it is keeping me from watching political programs and reading Twitter.

See you next time!

D2.zip