Dungeon 270
Continuing separation of building from running. Today, narrowing the scope of Tile. An idea for a better way of doing it. A good idea? I’m not sure.
Originally …
I started this series with the idea of separating the dungeon building from dungeon operation. The main take-home idea is that it has become clear that we can make even a major refactoring like this in very small steps, keeping the program in perfect operation all the time. This implies that there may never be a need to crave time from our masters to “clean up the code”, that, instead, we can improve it as we go, even to the extent of a very large summation of changes.
Of course it’s not a proof that it’s possible. There may be a change that is so pervasive and so large that it cannot be done incrementally. That I’ve never encountered such a thing doesn’t mean that it couldn’t exist. My experience suggests, however, that we would do well to assume that even large refactorings can be done incrementally until we encounter one where neither we, nor our Twitter friends can see a way to do it step by step.
And certainly, it’s “better” to keep the code so clean that we never need a large refactoring. My own experience suggests that even with the best of will, our desire to provide value, and our growing learning, will often bring us to a place where we now see what we “should” have done weeks or months ago. Voila! We need a large refactoring! And we’ll do it … bit by bit.
Lately …
I’m not sure quite what has put me on the trail of narrowing the scope of concern of objects from seeing the entire dungeon to seeing much less, resulting in the new TileFinder object, which amounts to an interface on Dungeon providing just one method, getTile. I think that as I’ve moved to separate methods out of Dungeon and put them into DungeonBuilder, I’ve had to deal with objects using a few methods of Dungeon, making each case fairly easy, but I’ve too often had to pay attention to the complexity of Dungeon, which still has around 40 methods.
It has turned out that some objects, like Room. only ever need to fetch a Tile from the Dungeon, so the new TileFinder helps with that.
I’m not entirely sure that this is a good idea, but it feels right, in that it limits what an object could do, making it easier to understand it. We’ll run with the idea for a while and see whether we like it. I think it is unlikely to do any harm, and is quite likely to make thinking about things a bit easier1.
I noticed last night, browsing the code, as one does, that Tile has a Dungeon instance and doesn’t do too much with it. Let’s see if we can narrow its concerns.
Tile
Tile class has four class creation methods, hall, room, wall, and edge. Those methods just set the “type” of the Tile, so that it will know how to draw itself. But they all take a Dungeon instance, which is saved in the dungeon member variable:
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
Tile does some argument checking, which suggests to me that at some time in the past, we did some refactoring, or implemented a defect involving passing in the wrong arguments. I don’t usually check arguments. This alerts me to be a bit extra careful as I work in here.
Let’s see what uses self.dungeon:
function Tile:getDungeon()
return self.dungeon
end
OK, we have an accessor. Does anyone use it from outside? Make a note to be alert for that. Who uses it in here?
function Tile:getNeighbor(aVector)
local newPos = self:pos() + aVector
return self:getDungeon():getTile(newPos)
end
function Tile:getSurroundingInfo()
local byte = 0
local ck = { vec2(1,1),vec2(0,1),vec2(-1,1), vec2(1,0),vec2(-1,0), vec2(1,-1),vec2(0,-1),vec2(-1,-1) }
for i,p in ipairs(ck) do
byte = byte<<1
local pos = self:pos() + p
local tile = self:getDungeon():getTile(pos)
if tile:isFloor() then
byte = byte|1
end
end
return byte
end
function Tile:illuminateLine(dx,dy)
local max = IlluminationLimit
local pts = Bresenham:drawLine(0,0,dx,dy)
for i,offset in ipairs(pts) do
local pos = self:pos() + offset
local d = self:pos():dist(pos)
if d > max then break end
local tile = self:getDungeon():getTile(pos)
tile:setVisible(d)
if tile.kind == TileWall then break end
end
end
function Tile:nearestContentsQueries()
self:getDungeon():nearestContentsQueries(self)
end
The first three could use a TileFinder. What’s this last thing? There is but one sender:
function Player:query()
self:getTile():nearestContentsQueries()
end
Player has a getDungeon method. We could replace that line with
self:getDungeon():nearestContentsQueries(self:getTile())
In fact let’s do that but mark it to come back to it.
Test. Test queries manually. Works. Let’s remove that method from Tile. All still good. Commit: Player no longer calls Tile:nearestContentQueries. Method removed.
So that’s nice. Now we should be able to create Tile instances with just a TileFinder rather than a Dungeon. Let’s change the code to reflect that:
function Tile:init(mapPoint,kind, tileFinder)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
assert(dungeon:is_a(Dungeon) or dungeon:is_a(TileFinder), "Tile requires Dungeon or TileFinder instance")
self.mapPoint = mapPoint
self.kind = kind
self.tileFinder = tileFinder
self:initDetails()
end
function Tile:getDungeon()
return self.tileFinder
end
Why have I not committed to receiving only a TileFinder right now? Because there are four class methods and many callers, and I’m anticipating a longish series of changes. In a real application the conversion might span days or weeks, as we get around to a change.
However, I’m glad we had this little chat, because I have a better idea:
function Tile:init(mapPoint,kind, dungeonOrTileFinder)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
self.mapPoint = mapPoint
self.kind = kind
self.tileFinder = dungeonOrTileFinder:getTileFinder()
self:initDetails()
end
Whatever we’re given, we’ll ask it for a TileFinder and save only that. I believe that Dungeon already knows getTileFinder. I’ll test to find out. Appears not. OK:
function Dungeon:getTileFinder()
return TileFinder(self)
end
That will nearly work, but there is another issue which we’ll encounter. Let’s pretend that it didn’t enter my mind.
Let’s pretend that I think the tests will run. I do not think that.
We get a crash in testing:
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
What is happening? TileFinder:getTile expects x and y coordinates, not vectors. This should be an easy change, we just have those three methods:
function Tile:getNeighbor(aVector)
local newPos = self:pos() + aVector
return self:getDungeon():getTile(newPos)
end
And so on. They are all passing in a vector position. Let’s do this:
function Tile:getTileFromPos(pos)
return self.tileFinder:getTile(pos.x,pos.y)
end
I chose to refer to tileFinder intentionally. I plan to remove that method. It makes it seem possible that some other object would ask a Tile to return its finder, and I don’t like that. I think I’ll like this better. So I change them all:
function Tile:getNeighbor(aVector)
local newPos = self:pos() + aVector
return self:getTile(newPos)
end
function Tile:getSurroundingInfo()
local byte = 0
local ck = { vec2(1,1),vec2(0,1),vec2(-1,1), vec2(1,0),vec2(-1,0), vec2(1,-1),vec2(0,-1),vec2(-1,-1) }
for i,p in ipairs(ck) do
byte = byte<<1
local pos = self:pos() + p
local tile = self:getTile(pos)
if tile:isFloor() then
byte = byte|1
end
end
return byte
end
function Tile:illuminateLine(dx,dy)
local max = IlluminationLimit
local pts = Bresenham:drawLine(0,0,dx,dy)
for i,offset in ipairs(pts) do
local pos = self:pos() + offset
local d = self:pos():dist(pos)
if d > max then break end
local tile = self:getTile(pos)
tile:setVisible(d)
if tile.kind == TileWall then break end
end
end
I expect my tests to run. They don’t. I called it getTileFromPos. Bag that. Call it getTile.
We’re good. Commit, then think. Commit: Tile now fetches a TileFinder from its dungeon arg.
However. Did you think I had forgotten something, or made a mistake? If anyone ever does pass in a tileFinder instead of a dungeon, as we want them to do, our Tile init will send it getTileFinder. That would crash. Unless we enhance the TileFinder test:
_: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 = dungeon:tileFinder()
local tftf = tf:getTileFinder()
_:expect(tftf).is(tf)
local found = tf:getTile(5,5)
local wanted = dungeon:privateGetTileXY(5,5)
_:expect(found).is(wanted)
end)
OK I hate those tf names. Hold your horses. Make it work.
1: Can access a tile -- TileFinder:23: attempt to call a nil value (method 'getTileFinder')
Implement:
function TileFinder:getTileFinder()
return self
end
That should run green. It does. Improve the test names.
_: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:tileFinder()
local found = aFinder:getTile(5,5)
local wanted = dungeon:privateGetTileXY(5,5)
_:expect(found).is(wanted)
end)
_:expect("TileFinder returns self from getTileFinder", 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:tileFinder()
local anotherFinder = aFinder:getTileFinder()
_:expect(anotherTileFinder).is(aFinder)
end)
There. I even wrote another test to help with clarity. We’re green. Commit: TileFinder used exclusively in Tile. TileFinder returns self from getTileFinder.
Irritating to require two sentences in the commit message, but we can ship it and now Tile is in fact limited to TileFinder’s one method.
reStrospective
Maybe that’s a better spelling. Anyway, let’s lift our heads and reflect a bit.
This line of code might seem odd to you:
function Tile:init(mapPoint,kind, dungeonOrTileFinder)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
self.mapPoint = mapPoint
self.kind = kind
--
self.tileFinder = dungeonOrTileFinder:getTileFinder()
--
self:initDetails()
end
We can think of it as a “cast”, as in some other language:
self.tileFinder = (TileFinder) dungeonOrTileFinder
A Smalltalk programmer might prefer this:
self.tileFinder = dungeonOrTileFinder:asTileFinder()
Casts in Smalltalk are typically named as something. asNumber etc. Tell the truth, I rather prefer that. I’m going to rename that method right now.
That took just moments. Commit: rename getTileFinder to asTileFinder throughout.
Anyway, as I was about to say, we want to be able to move incrementally from passing a whole Dungeon in to Tile, to passing just a TileFinder, which is all it needs. Tile itself protects us by casting whatever it’s given to a TileFinder, and TileFinder, of course, casts to itself. Now we can proceed at leisure to change Tile’s creators.
The definition of leisure is now: We’ll do them until we get tired or start making mistakes.
A quick search tells me that there are no calls to create Tile instances other than through our four class methods, so I can search out those:
However, I think there is one bizarre case, where we create our Maps. I make a note to check it. No, I’ll just check it now.
function Dungeon:createTiles()
Maps:cartesian()
self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self)
end
Here we tell the Maps what to populate the map with, i.e. Tile instances. We’re passing the dungeon. We want:
function Dungeon:createTiles()
Maps:cartesian()
self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self:asTileFinder())
end
This is a massive change, in the sense that we create nearly 10,000 Tile instances. Test. Good. Commit: Dungeon passes TileFinder instance to Maps rather than self.
Now then, that’s out of the way. Better than trying to remember. My desk is paved with yellow sticky notes.
I decide to go after the creation methods one at a time. Start with looking at all the Tile:edge lines.
function Tile:edge(x,y, tileFinder)
return Tile(Maps:point(x,y),TileEdge, tileFinder)
end
The first access I find is odd:
function Dungeon:privateGetTileXY(x,y)
return self.map:atXYZ(x,0,y) or Tile:edge(x,y, self.runner:getDungeon())
end
Why can’t that just use self? We must be trying to be sure we get the right runner. That’s scary code. We are tempted to change it. If it’s an error … it’ll be a weird one.
Let’s change it and see what breaks.
function Dungeon:privateGetTileXY(x,y)
return self.map:atXYZ(x,0,y) or Tile:edge(x,y, self:asTileFinder())
end
Test. Tests and game run. Maybe it was belt and suspenders. Maybe clumsiness. Anyway we’re good. Change the other calls to edge before committing:
_:test("player direction", function()
local dx,dy
local player = Player:privateFromXY(100,100, runner)
runner.player = player
local tile = Tile:edge(101,101,runner:getDungeon():asTileFinder())
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(-1)
_:expect(dy).is(-1)
tile = Tile:edge(98,98, runner:getDungeon():asTileFinder())
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(1)
_:expect(dy).is(1)
end)
I hate that double call. Turns out we have tileFinder method on GameRunner, having been here before.
_:test("player direction", function()
local dx,dy
local player = Player:privateFromXY(100,100, runner)
runner.player = player
local tile = Tile:edge(101,101,runner:tileFinder())
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(-1)
_:expect(dy).is(-1)
tile = Tile:edge(98,98, runner:tileFinder())
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(1)
_:expect(dy).is(1)
end)
Test. Commit: tile:edge calls all pass a TileFinder, not Dungeon.
Similarly, for Tile:hall.
In my search, I find lots of runner:getDungeon(), all of which need to be runner:tileFinder(). I’ll do them all, without regard to what kind of Tile they ask for.
That took a few minutes … there are lots of tests on Tile. I think I have them all. Let’s do this:
function Tile:init(mapPoint,kind, dungeonOrTileFinder)
assert(dungeonOrTileFinder:is_a(TileFinder), "must provide TileFinder")
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
self.mapPoint = mapPoint
self.kind = kind
self.tileFinder = dungeonOrTileFinder:asTileFinder()
self:initDetails()
end
Test. Everything works. Remove that assert, as we have the cast. Commit: All Tile creators pass a TileFinder, not a Dungeon.
Time to reflect.
ReflectionoitcelfeR
Sorry.
The Tile object is now converted to expect only a TileFinder, reducing the harm it might do substantially. It also avoids some programmer putting methods on Tile that don’t belong there just for convenience. Not that I’d ever do that.
Which reminds me of the Player:
function Player:query()
self:getDungeon():nearestContentsQueries(self:getTile())
end
That was weird. She send a nearestContentsQuery to her Tile, which sent it back to the dungeon. Let’s probe around here a bit and see what’s going on.
I’m pretty sure that query happens when the “??” button is pressed but I’m not sure how it happens. We’ll definitely probe. This won’t hurt.
Most dungeon objects understand query():
function Monster:query()
Bus:informDotted(self.behavior.query(self))
end
function WayDown:query()
Bus:informDotted("I am the Way Down! Proceed with due caution!")
end
function Loot:query()
Bus:informDotted(self.message)
end
And so on. Oddly, if we trace what happens on Player:query, the query message gets sent to pretty much everyone. How does she get called?
function Button:textButton(label, x, y, w, h)
local w = w or 64
local h = h or 64
local img = Button:createTextImage(label,w,h)
if label == "??" then label = "query" end
return Button(string.lower(label), x, y, w, h, img)
end
function Button:init(name, x,y,w,h, img)
self.name = name
self.x = x
self.y = y
self.w = w
self.h = h
self.img = img
self:setHighlight(false)
Bus:subscribe(self, self.touchBegan, "touchBegan")
end
function Button:touchEnded(pos, player)
if self:isPosMine(pos) then
self:performCommand(player)
end
end
function Button:performCommand(player)
player[self.name](player)
player:turnComplete()
end
So whatever the label is, except for ??, it is sent to the player. I think sending query is iffy because it seems we might get a recursion if we’re not careful.
Let’s change that message to “requestInfo”.
function Button:textButton(label, x, y, w, h)
local w = w or 64
local h = h or 64
local img = Button:createTextImage(label,w,h)
if label == "??" then label = "requestInfo" end
return Button(string.lower(label), x, y, w, h, img)
end
function Player:requestInfo()
self:getDungeon():nearestContentsQueries(self:getTile())
end
Test that. Wrong. We lower case all the messages. So:
function Player:requestinfo()
self:getDungeon():nearestContentsQueries(self:getTile())
end
Tests run. ?? works. Commit: rename player:query to requestinfo.
OK. Having a method query that sends query to a random collection of objects just seems potentially stack overflow to me.
Let’s continue to trace how it works.
function Dungeon:nearestContentsQueries(anyTile)
for i,neighbor in ipairs(self:neighbors(anyTile)) do
neighbor:doQueries()
end
end
function Tile:doQueries()
for k,obj in pairs(self:getContents()) do
if obj.query then
obj:query()
end
end
end
In the blurb, I mentioned an idea for a “better way”. I was referring to this situation. I was thinking that this might be a perfect opportunity for a publish-subscribe implementation rather than a direct call to the Dungeon. Why should the Player know who’s going to handle these queries?
This is arguably a general flaw in my design here. Too often we have object knowing to ask their runner or dungeon to do things. It results in object being connected upward and downward, not just downward. Something to think about more generally, but for now …
Let’s review how the Bus works. Basically it comes down to two methods:
function EventBus:subscribe(listener, method, event)
assert(type(method)=="function", "method parameter must be function")
local subscriptions = self:subscriptions(event)
subscriptions[listener] = method
end
function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
for subscriber,method in pairs(self:subscriptions(event)) do
method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
end
end
One object publishes an event, passing up to three arguments, the first of which is to be the sender itself.
Other objects can subscribe by passing in a method that expects the arguments as provided on the corresponding publish.
Let’s look at some subscribers.
function Player:init(tile, runner)
self.runner = runner
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
Bus:subscribe(self, self.spawnPathfinder, "spawnPathfinder")
Bus:subscribe(self, self.curePoison, "curePoison")
Bus:subscribe(self, self.addPoints, "addPoints")
tile:moveObject(self)
tile:illuminate()
end
function Player:addPoints(ignoredSender, kind, amount)
self.characterSheet:addPoints(kind,amount)
self:doCrawl(kind, amount)
end
This one is just right. There are publishers:
local ItemTable = {
pathfinder={ icon="blue_jar", name="Magic Jar", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" },
rock={ icon="rock", name="Rock", attribute="dullSharpness", description="Mysterious Rock of Dullness", used="You have mysteriously dulled all the sharp objects near by." },
curePoison={ icon="red_vase", name="Poison Antidote", attribute="curePoison" },
health={icon="red_vial", name="Health", description="Potent Potion of Health", attribute="addPoints", value1="Health", value2=1},
strength={icon="blue_pack", name="Strength", description="Pack of Steroidal Strength Powder", attribute="addPoints", value1="Strength", value2=1},
speed={icon="green_flask", name="Speed", description="Spirits of Substantial Speed", attribute="addPoints", value1="Speed", value2=1},
cat_persuasion = {icon="cat_amulet", name="Precious Amulet of Feline Persuasion", attribute="catPersuasion"},
testGreen={icon="green_staff"},
testSnake={icon="snake_staff"},
}
In there we can see various items that do addPoints.
So let’s try a publish-subscribe for the player query. Change this:
function Player:requestinfo()
self:getDungeon():nearestContentsQueries(self:getTile())
end
To this:
function Player:requestinfo()
Bus:publish("requestinfo", self, self:getTile())
end
At this point, query won’t work any more. No one is listening. We want dungeon to listen.
function Dungeon:init(runner, tileCountX, tileCountY)
self.runner = runner
self.tileCountX = tileCountX or assert(false, "must provide tile count")
self.tileCountY = tileCountY or assert(false, "must provide tile count")
self.map = nil -- set in createTiles
self.rooms = Array{}
Bus:subscribe(self,self.nearestContentsQueries, "requestinfo")
end
We need to change the method called:
function Dungeon:nearestContentsQueries(eventIgnored, playerIgnored, anyTile)
for i,neighbor in ipairs(self:neighbors(anyTile)) do
neighbor:doQueries()
end
end
The Bus passes the event name and the sender, in case you want them. We do not.
I think that should do it. Test. Works. Commit: Player uses Bus to execute request for info from Dungeon.
OK, let’s talk about this.
Was Publish/Subscribe Righteous Here?
We’ve replaced a direct reference from Player up to Dungeon. Dungeon knows player and probably must. That means that Player knowing Dungeon is a circular reference. It works fine, but it makes setting things up more difficult.
In the event that we want to know how queries work, we would previously have found Player calling up to dungeon and telling it exactly what to do. Player was coupled to Dungeon by a reference and a method name. Now it isn’t coupled at all, as regards query.
That does make it slightly harder to find out exactly how queries work, but quite often if we wondered at all, just seeing that it publishes an event might be enough. “OK, someone catches that event”. If we care, we search for it, find the subscribe, and that tells us the method and receiver. One more simple step.
Just today we say some code that looked like
self.runner:getDungeon():asTileFinder()
OK, we actually just wrote it today, but the runner:getDungeon() part was all over. Code like that knows too much. Not only does it know a runner, which is a big gol-durn object on its own, it also knows that the runner has a dungeon, another immense object. And it knows a specific method in that object that it doesn’t even have a real connection with.
That’s not good. I’ve done it all over, and I should be punished. Bad Ron, no biscuit.
No, I don’t believe in punishment, especially not when it’s applied to me, but in fact that’s not good design. It might be seen as expedient design, but let’s face it, “expedient” is pretty close to “mediocre” in the dictionary2.
Might publish-subscribe be a good way to disconnect these objects that are too intertwingled? It certainly worked in this case, and the more we use it, the better we’ll use it.
I think it was a righteous change.
And, of course, we also disconnected Tile from Dungeon, giving it just a TileFinder instead. And we did it in nine commits over two hours, which could have been over nine weeks if we had needed to wait, or if our system was larger, because we didn’t break anything in each of those commits.
Incremental change. Many More Much Smaller Steps. This is the way.
What is the way tomorrow? I don’t know. Can’t wait to find out.