Untangling the knots. Let’s see if we can clean up the coupling between GameRunner, tile, and their friends.

I’m starting this article Wednesday afternoon, though I expect most of it will be done tomorrow. But I have a few moments and I wanted to try something.

Earlier today, I ran into a bit of trouble creating a new dungeon in the now-removed DungeonMaker class, because every Tile knows its room number. That’s set up rather late on, using Tile:paint, which gets called by the GameRunner, passing in a room number.

The room number is used in just one situation, selecting a random tile that is not in a given room. We use that, for example, to ensure that no monsters are spawned in the first room of the dungeon, where the princess spawns.

So if our purpose is to find a room tile that isn’t in a given room, one way is to have every tile know its room number, in case anyone asks. But at some small expense, we could avoid a given room by telling the functions that avoid rooms to avoid a given rectangle of tiles. And the Room objects know their rectangles. We don’t use them for much of anything, but we certainly could.

I’m thinking that if we made this change, the creation of the dungeon would be just a bit less complicated, and that would be a good thing as we move to try to separate creation from operating the game. I can see two ways to go.

The easy but inefficient way would be to change Room:getRoomNumber to search the list of rooms to see what room the tile’s coordinates are in. There are only a dozen rooms, typically, so it would be pretty quick. A more efficient but perhaps more complicated way would be to change the functions that now try to avoid a room and make them avoid a rectangle instead.

``````function Dungeon:randomRoomTile(roomToAvoid)
while true do
local pos = vec2(math.random(1, self.tileCountX), math.random(1,self.tileCountY))
local tile = self:getTile(pos)
if tile:getRoomNumber() ~= roomToAvoid and tile:isOpenRoom() then return tile end
end
end
``````

If this is the only usage, we can easily push the rectangle down, it’s in the room. So …

``````function Dungeon:randomRoomTile(roomToAvoid)
local x1,y1, x2,y2 = 0,0,0,0
if roomToAvoid then
x1,y1, x2,y2 = roomToAvoid:corners()
end
local x,y
while true do
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
local pos = vec2(x,y)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

I’m allowing for someone calling this method with no room, if they don’t care to avoid anything. I get the room’s corners, and roll individual coordinates outside the room bounds. Check for openRoom and done.

This required a change here:

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

This takes room number and sends the room down to the dungeon. And I had to change this:

``````function GameRunner:placeWayDown()
local r1 = self.rooms[1]
local target = r1:centerTile()
local tile = self:getDungeon():farawayOpenRoomTile(r1, target)
self.wayDown = WayDown(tile)
end

function Dungeon:farawayOpenRoomTile(room, target)
local candidate
local result = self:randomRoomTile(room)
for i = 1,20 do
candidate = self:randomRoomTile(room)
result = self:furthestFrom(target, result, candidate)
end
return result
end
``````

We used to pass in the princess’s tile and imply the room number from that. Can’t do that any more, requiring the above changes.

Now we can go about removing the roomNumber member variable in Tile, but that’s going to break some tests.

``````function Tile:room(x,y, runner)
assert(not TileLock, "Attempt to create room tile when locked")
return Tile(x,y,TileRoom, runner)
end

function Tile:wall(x,y, runner)
assert(not TileLock, "Attempt to create wall tile when locked")
return Tile(x,y,TileWall, runner)
end

function Tile:edge(x,y, runner)
return Tile(x,y,TileEdge, runner)
end

function Tile:init(x,y,kind, runner)
if runner ~= Runner then
print(runner, " should be ", Runner)
error("Invariant Failed: Tile must receive Runner")
end
self.position = vec2(x,y)
self.kind = kind
self.runner = runner
self:initDetails()
end
``````

And now all the callers of those creation methods:

``````function Room:correctTile(x,y)
return Tile:room(x,y, self.runner)
end
``````
``````function Room:paint()
for x = self.x1,self.x2 do
for y = self.y1,self.y2 do
self.runner:defineTile(self:correctTile(x,y))
end
end
end
``````

This seems to be everyone referring to roomNumber. Now for a million tests to break. Ah, excellent. Just one:

``````2: Get a random room -- Dungeon:242: attempt to index a number value (local 'roomToAvoid')
``````
``````        _:test("Get a random room", function()
local tile = dungeon:randomRoomTile(1)
_:expect(tile:getRoomNumber()).isnt(1)
_:expect(tile:isOpenRoom()).is(true)
end)
``````

I’m just going to delete that test. So sue me. Or tweet me up if I don’t justify it. Tests run. I notice decor in the starting room. Is that supposed to happen? Yes, it is. We create decor in all rooms not numbered 666.

I have looked up on this and it is good. Commit: Tiles no longer have room number.

Let’s retro and then I’m for supper.

## Retro

The change we wanted was to get rid of the room number in Tile, which narrows the Tile’s interface a bit and will, I believe, help us as we refactor to tease apart dungeon creation from dungeon operation. Even if it doesn’t, it’s one less member variable to worry about in creating and working with Tiles.

I did remove one test, which checked to see whether we could find a room tile that wasn’t in room 1. That might be the only test for whether that function works., and we did just change it.

OK, now I feel badly. Let’s go back and look at that changed code and see what we can do to test it.

``````function Dungeon:randomRoomTile(roomToAvoid)
local x1,y1, x2,y2 = 0,0,0,0
if roomToAvoid then
x1,y1, x2,y2 = roomToAvoid:corners()
end
local x,y
while true do
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
local pos = vec2(x,y)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

OK, I’m going to refactor this method, like 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
while true do
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
local pos = vec2(x,y)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

Test this with a run. No actual test yet. Now refactor again, starting here:

``````function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
local x,y
while true do
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
local pos = vec2(x,y)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

Let’s get a random position:

``````function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
local x,y
while true do
local pos = self:randomPositionAvoiding(x1,y1, x2,y2)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2)
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
return vec2(x,y)
end
``````

And now we have a function that we can test all we wish.

But, you know, I’d rather pass in those high water marks as well.

``````function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
local x,y
while true do
local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCounntY)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
repeat
x = math.random(1, maxX)
until x < x1 or x > x2
repeat
y = math.random(1, maxY)
until y < y1 or y > y2
return vec2(x,y)
end
``````

And, you know what, I’d like to avoid that duplication. And I’m glad I looked because I forgot to make x and y local.

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

function Dungeon:randomValueAvoiding(v1,v2,vMax)
local v
repeat
v = math.random(1, vMax)
until v < v1 or v > v2
end
``````

Nasty typo above, `self.tileCounntY`. Fixed.

Hm that loops. Revert to here:

``````function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
local x,y
repeat
x = math.random(1, maxX)
until x < x1 or x > x2
repeat
y = math.random(1, maxY)
until y < y1 or y > y2
return vec2(x,y)
end
``````

Extract again:

``````function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
local x,y
x = self:randomValueAvoiding(x1,x2,maxX)
repeat
y = math.random(1, maxY)
until y < y1 or y > y2
return vec2(x,y)
end

function Dungeon:randomValueAvoiding(x1,x2,maxX)
local x
repeat
x = math.random(1, maxX)
until x < x1 or x > x2
return x
end
``````

I’m pretty sure that last time I forgot that final `return`. Test. Works. Use extracted method again:

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

Test. Works. Rename variables:

``````function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
local v
repeat
v = math.random(1, vMax)
until v < vLow or v > vHigh
return v
end
``````

Test. Works. I’m called to supper, still no test. But all the others run. Commit: refactored randomRoomTile to death to improve testability.

I’ll retro and work on the test tomorrow. Tonight, pasta with Italian sausage and other yummies.

## Thursday 0833

So, looking back, I set out to make a small improvement, keeping a room number in each tile of the dungeon. This value was used to avoid putting things in specific rooms, generally room 1, where the player starts out.

The basic scheme was to pass in, not the room number to avoid, but the room’s corners, so that the avoidance code could avoid the rectangle instead. That went fairly smoothly.

I did find it necessary to delete one test that was checking room number in the test. It was also perhaps the only test that actually checked to see if we were really skipping the room requested. During a brief retrospective, I realized that that test was arguably important, and that we didn’t really have any tests to determine whether the right rectangle was being skipped. So I accepted the mission of testing that code.

It wasn’t easy to test. It looked like this:

``````function Dungeon:randomRoomTile(roomToAvoid)
local x1,y1, x2,y2 = 0,0,0,0
if roomToAvoid then
x1,y1, x2,y2 = roomToAvoid:corners()
end
local x,y
while true do
repeat
x = math.random(1, self.tileCountX)
until x < x1 or x > x2
repeat
y = math.random(1, self.tileCountY)
until y < y1 or y > y2
local pos = vec2(x,y)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

It wasn’t quite that nasty when I started, but it was close. How does one test a method like that? Only with difficulty. Think about how much setup it needs. Yucch. The method does at least these things:

• Set up coordinates to avoid
• Compute an x coordinate
• Loop if it’s not outside the x range
• Compute a y coordinate
• Loop if it’s not outside the y range
• Get the tile at those coordinates
• Check to see if the tile is OK
• Loop way back if it isn’t

We prefer methods that either call other methods to get several things done, or do one thing. So we refactored, using Extract Method and a small amount of recasting, down to 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
while true do
local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end

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

function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
local v
repeat
v = math.random(1, vMax)
until v < vLow or v > vHigh
return v
end
``````

This is nearly clean. I do see one change I’d like to make this morning, changing this:

``````function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
local x,y
while true do
local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
local tile = self:getTile(pos)
if tile:isOpenRoom() then return tile end
end
end
``````

To this:

``````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
``````

I find that a bit nicer. Now all this stuff about avoiding the room comes down to this one function, called twice:

``````function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
local v
repeat
v = math.random(1, vMax)
until v < vLow or v > vHigh
return v
end
``````

Now we see that this surely works, and we can clearly write tests for it. However, last night I was wondering whether we really had to try multiple times to find a random number outside an interval. A bit of searching on the Internet found an idea which I plan to adapt here.

Yes, experienced programmers use the internet to look up how to do things. Sometimes we grab what we find. Sometimes we grab the idea and build on it. But we do look. It’s called research. In the olden days, we had to read books and magazines, and keep files of things we wanted to remember. Now the Internet remembers for us.

Anyway …

``````function Dungeon: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
``````

Now if you’re like me, that code almost obviously works. Plus or minus one. Now we have a good reason to write a test for it. Here goes.

``````        _:test("randomValueAvoiding", function()
local r, ok
for try = 1,100 do
r = dungeon:randomValueAvoiding(3,5,10)
ok = (0 < r and r < 3) or (5 < r and r < 11)
_:expect(ok).is(true)
end
end)
``````

That seems like a good test. And it passes. Voila!

## Mission Accomplished

We set out to remove the room number concept from the Tile , and it turns out that we removed it from the Dungeon object as well. And we have a reasonable test to assure us that it still all works.

You may be wondering–I know that I was–whether this random number scheme could be extended to avoiding multiple ranges. I convinced myself before rolling out of bed that it can be, with a series of checks and adjustments, one interval at a time. I don’t really want to do it: I think it would involve merging intervals and such. If it became necessary, I might go back to the loop until you find one form.

But that’s for a day that may never come. Today we have a nice little scheme that calls down and down until it basically returns a random number. Nice. We like that.

### Does that last function belong on Dungeon?

No, it does not. It’s a generally useful random number function. We have a couple of other general functions, over in Main:

``````-- Generally Useful Functions

function clamp(lo,val,hi)
if lo <= hi then
return math.max(math.min(val,hi), lo)
else
return math.max(math.min(val,lo), hi)
end
end

function sign(x)
return (x < 0 and -1) or (x == 0 and 0) or (x > 0 and 1)
end
``````

Let’s move it over there.

``````        _:test("randomValueAvoiding", function()
local r, ok
for try = 1,100 do
r = randomValueAvoiding(3,5,10)
ok = (0 < r and r < 3) or (5 < r and r < 11)
_:expect(ok).is(true)
end
end)

function Dungeon: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
``````

All continues to work. I think we’re done here.

## Summary

Is there a lot of benefit to these changes? I’d have to say no … but there is some, in that we’ve removed one of the odd things that tie the GameRunner, Dungeon, Tile and other objects into a somewhat nasty tangle.

But we have a bigger picture to consider. We’re starting to work on designed dungeons, layouts that can be fully defined by the human designers (me, with designer hat on (and trust me, it’s a lovely one)), rather than be created randomly.

We ran into this bump in working on designed dungeons. It was the last straw in an attempt to create a DungeonMaker class all in one go. And two things have come out of that bump.

First, we removed the bump. Since it got in the way once already, we can be pretty sure that it would do so again. I like to focus refactoring on code that I’d be working on anyway, when I’m thinking like a product developer. Going off and refactoring random things that aren’t really being worked on is arguably a waste of time when we’re focused on delivering the most capability we can in the shortest time we can. Refactoring things that are in the way, on the other hand, speeds us up, at least if we are intelligent about it.

Of course, in these articles, sometimes I refactor things because I want to write about how we refactor things. I can always justify diverting from the product frame of mind, because the learning and practice frame of mind is more valuable to me.

To some extent, I’d suggest that it’s valuable to everyone, but when you’re working for a living, you have to be judicious.

The second thing to come out of the DungeonMaker spike was a commitment to move in smaller steps. DungeonMaker seemed to be too big a step. It looks like we’ll need to move more incrementally to untangle the mess that is GameRunner and its collaborators.

And we’re not without progress on that task. We have this new formulation of the learning level code:

``````function GameRunner:createLearningRooms()
local w = 12
local h =  8
local t = {
{2,2, w,h},
{15,2, w,h},
{28,2, w,h},
{41,2, w,h},
{2,11, w,h},
{15,11, w,h},
{28,11, w,h},
{41,11, w,h},
{2,20, w,h},
{15,20, w,h},
{28,20, w,h},
{41,20, w,h},
}
self.rooms = self:makeRoomsFromXYWH(t)
end

function GameRunner:makeRoomsFromXYWH(tab)
return map(tab, function(e)
return Room(e[1],e[2],e[3],e[4], self, true)
end)
end
``````

This may look cryptic, but I think it opens an interesting door. It has separated out a tabular representation of some rooms from the code that creates them. When we build a textual language, or, if we were totally bonkers, a graphical tool for defining a dungeon (wouldn’t that be fun?), producing a control table to drive some general building routines will be just the thing to allow us to “compile” a dungeon definition.

All that’s for future days. Today, we have a bit of improvement to our initial cut at designed dungeons. And that makes us happy.

See you next time!

D2.zip