Dungeon 286

The FNCS has spoken regarding the defect. I do some smoothing. Multi-signature method? Possibly. I ain’t bovvered.

The Defect

At the Friday Night Coding Slack last night (Tuesday), we discussed my shipped defect in the PathFinder. We were unable to point to some particular practice that I “should” (or “shouldn’t”) have been following that would have ensured that the change to getMap from .map would have been caught. I can think of one this morning, however.

The code in question was directly accessing a member variable, map from another object, via dungeon.map. That, I’ve been taught and also frequently learned, is almost invariably, I think I would say just plain old invariably, a bad idea. Why? Because when the object providing the map wants to change, Other People’s Code has to be changed.

Now as a rule, we would go a step further and say that rather than supply the map, the map-owner should provide services which it forwards to the map. But if one does choose to provide an object, it’s best done by calling a method, often but not always called an “accessor”.

So while the FNCS didn’t come up with this answer last night, it was probably because we were discussing my article and my description, rather than actually reviewing the old and new code. I’m sure that my excellent colleagues would have then seen the problem. I’m not blaming them at all. No, really, they’re fine. It’s all fine. Fine. Thank you.

Smoothing

I brought up another topic with the FNCS last night. I told them that the code has a method getTile, which is available in a number of objects, returning the Tile at a given position. That’s something one often needs to know, because everything in the Dungeon takes place on one or more Tiles. I mentioned that some implementations of getTile take a vector position, and some take x and y coordinates, and that this seems less than ideal. I think everyone agreed that it ought to be one way or the other, and I think we mostly agreed that it would be better to have a Position to deal with rather than x and y coordinates. And I think there was general agreement that rather than have a vec2 as the parameter, it would be better to have a class of my own, such as Position.

We had no specific reason to prefer that. Our general reason is that when we use a system type, its only methods are system methods. When we use a type (class) of our own, it can have methods that are useful to our application, and, supposing it to be implemented against some kind of system types, it can take advantage of those internally.

GeePaw Hill mentioned that he had a similar problem and that he was perfectly willing to overload methods to accept both signatures. That is, he would implement two versions of getTile, one with position and one with x and y, and the compiler would sort it out. Lua, of course, does not have method overloading, but I commented then (and now) that it would be possible to check the parameters at the time of the call and handle both cases.

Hill’s reasoning is worth mentioning here. He would provide both signatures because he wants methods to be convenient when used. Since some code will prefer to think in x and y, and some will prefer to think in a unified position object, it makes sense to have the “same” method support either.

This is a very small idea, but it has a big payoff over time. If a method is not convenient to call, almost anything we do will make our code just a bit worse, a bit harder to understand, a bit more error-prone. As Hill puts it, “Momma didn’t raise no thinkers”. As Chet puts it, “A mind is a terrible thing to waste, so use yours sparingly”.

Hill went on to say that somewhere, where the method signatures are sorted out, he’d expect convergence, where one of the lower-level methods calls the other, after aligning the variables. Somewhere, we want the division to come to a single point of contact.

You Know What …

When I started today’s article, I had in mind converging my getTile method down to one of the two calling sequences, and changing everyone to work that way, for consistency. But in the spirit of Hill’s convenience observation, let’s do the reverse.

Let’s make our getTile function accept either a vector or x and y, and, while we’re at it, everywhere it’s called, use whichever one is most convenient. That will probably improve at least some of our uses. We’ll find out.

I love it when a plan falls apart.

There is one issue that I ran across last night: there is a third method named getTile. In the DungeonContentsCollection, to find the tile of a given DungeonObject, you can say

DungeonContents:getTile(anObject)

That one should really be renamed. Let’s rename it to tileContaining and do that first.

function DungeonContentsCollection:getTile(object)
    return self.contentMap[object]
end

Rename that:

function DungeonContentsCollection:tileContaining(object)
    return self.contentMap[object]
end

Now I’ll have to search for senders of getTile and determine whether they are referring to this one. Since the DungeonContents is a global, I can probably get most of them with a search for “DungeonContents:getTile”.

There are ten files affected, because each unique dungeon object seems to know how to do this. I think it likely that we should move that method upward. For now, I’ve committed that much. “Change DungeonContents:getTile to tileContaining”.

Wow, that was bad! Not only have I not checked to see if there are calls to getTile that are this one even though they don’t say DungeonContents, I didn’t even test. Belay that commit. OK, back to where we were with 10 files needing committing. Now let’s search for :getTile` and verify each one.

Hm. I observe that all the methods I just changed were implementing DungeonObject:getTile (or its equivalent). That is, you can send getTile to any dungeon object and get back the tile it is standing on. That’s yet another meaning for that one name. Let’s hold off on that for now, but I think I’d like to fix it. But one thing at a time, for now we want to be sure that all the calls

The search convinces me that I’ve got them all. Test. I feel the need to run the game a bit, because I doubt that all those methods are tested in unit tests. We should talk about that.

A test fails!

1: DungonContents remembers object -- DungeonContentsCollection:30: attempt to call a nil value (method 'getTile')

Bril. What’s this? Ah. Test should say this:

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            _:expect(dc:tileContaining(object)).is(34)
        end)

Well, I’m confident that that’s fixed, but it doesn’t add much to my overall confidence. Test again. Game plays well. I think we’re good. Commit: Change DungeonContents:getTile to tileContaining.

Now the implementors of getTile. If they are all subclasses of DungeonObject, I can move that method up there and remove it from those guys.

The method is already implemented on Entity. Move it up:

function DungeonObject:getTile()
    return DungeonContents:tileContaining(self)
end

Now find the others that we can remove. Search is easy, can look for “:getTile()”. Remove from WayDown, Loot, Key, Chest, Spikes, Decor, and Lever. I think that’s all of them. Test again.

I see something odd. When you step on a Decor, it may deal you some damage, and it may give you something. When it has nothing to give, it says “You receive nothing”. When I step on some of the Decor in this run, I’m not seeing that message. I guess that has always happened. You’ve been given the nothing and the decor no longer has a nothing. I think it has always been that way.

OK, I declare this good. Commit: Move getTile to DungeonObject remove from all subclasses.

About That Swamp …

We were going to make the getTile that takes position or x, y accept either. We have discovered another kind of getTile that takes no parameters, and is sent to a DungeonObject to get its tile. I think that should have a different name, in the forlorn hope of reducing my confusion. I don’t have a good idea for the name. Let’s stick to our current scheme and deal with the other implementations of getTile.

I would commit a misdemeanor to have a pattern search in Codea right now, because what I really want to search for is “function :getTile()". Nothing for it but to plow through the hard way.

Here’s the first one:

function GameRunner:getTile(aPosition)
    return self:getDungeon():getTile(aPosition)
end

My plan is to change all these to accept arbitrary parameters, so that you can call them with either a vector (position) or x and y.

function GameRunner:getTile(...)
    return self:getDungeon():getTile(...)
end

That’s how you say you don’t care what you get. We’ll unwind that at the bottom, when we get there.

function Dungeon:getTile(pos)
    return self:privateGetTileXY(pos.x, pos.y)
end

function Dungeon:privateGetTileXY(x,y)
    return self:getMap():atXYZ(x,0,y) or Tile:edge(x,y, self:asTileFinder())
end

This is a legitimate unwinding. But we’d like to make the getTile agnostic.

-- "overloaded" to allow (pos) or (x,y)
function Dungeon:getTile(posOrX, y)
    if y then
        return self:privateGetTileXY(posOrX,y)
    else
        return self:privateGetTileXY(posOrX.x, posOrX.y)
    end
end

This is enough to deserve testing and its own commit. Test. We’re good. Commit: Dungeon:getTile “overloaded” to accept pos or x,y.

Now I find this:

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

Let’s make this one agnostic and forward to Dungeon: getTile:

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

Test again. Green. Commit: BuilderView getTile accepts pos or x,y.

Now:

function Tiles:getTile(x,y)
    return self.map:atXYZ(x,0,y) or Tile:edge(x,y,self)
end

This one needs to implement the overloading. It would be nice if everyone called down to this one. I’ll make a card for that.

Funnel all getTile(…) to Tiles

That aside, it needs to be like the other one:

-- "overloaded" to accept pos or x,y
function Tiles:getTile(posOrX,y)
    if y then
        return self.map:atXYZ(x,0,y) or Tile:edge(x,y,self)
    else
        local x,y = posOrX.x, posOrX.y
        return self.map:atXYZ(x,0,y) or Tile:edge(x,y,self)
    end
end

Test. Arrgh:

MapPoint:17: bad argument #1 to 'floor' (number expected, got nil)
stack traceback:
	[C]: in function 'math.floor'
	MapPoint:17: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in function <... false
    end

    setmetatable(c, mt)
    return c
end:20>
	(...tail calls...)
	BaseMap:46: in method 'atXYZ'
	Tiles:133: in function <Tiles:131>
	(...tail calls...)
	Tile:296: in method 'getSurroundingInfo'
	Tile:156: in method 'convertEdgeToWall'
	DungeonBuilder:110: in method 'convertEdgesToWalls'
	DungeonBuilder:255: in method 'defineDungeonLayout'
	DungeonBuilder:63: in method 'buildTestLevel'
	GameRunner:91: 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'

I’m not even going to look. Revert and do again.

From here:

function Tiles:getTile(x,y)
    return self.map:atXYZ(x,0,y) or Tile:edge(x,y,self)
end

To:

function Tiles:getTile(x,y)
    if y then
        return self.map:atXYZ(x,0,y) or Tile:edge(x,y,self)
    else
        error("for now")
    end
end

This ought to work. Test. It works. Rename parm:

function Tiles:getTile(posOrX,y)
    if y then
        return self.map:atXYZ(posOrX,0,y) or Tile:edge(posOrX,y,self)
    else
        error("for now")
    end
end

Test. Good. Extend:

function Tiles:getTile(posOrX,y)
    if y then
        return self.map:atXYZ(posOrX,0,y) or Tile:edge(posOrX,y,self)
    else
        local xx,yy = posOrX.x, posOrX.y
        return self.map:atXYZ(xx,0,yy) or Tile:edge(xx,yy,self)
    end
end

Test. Green. Commit: Tile:getTile accepts pos or x,y.

There is no test for the second case here, nor in other calls. We can’t be totally confident in this. Let me do an ad-hoc check.

No. I’m in mid edit. Make a card that it’s not really tested.

Agnostic getTile implementations not fully tested

Continue current train of thought. Ah, wait. I find this:

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

Let’s find senders of that and replace them with getTile. No, there are lots. First do this:

function BuilderView:getTileFromPos(pos)
    return self:getTile(pos)
end

If this works, the agnostic call works. There are lots of tests calling the “fromPos” version.

Tests pass. Change all the callers to just call getPos. Remove the method. Test. Green. Commit: remove getTileFromPos, use agnostic getTile.

Search for more getTile.

I find this:

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

Here’s an example of needing to do something inconvenient, pulling x and y out. We can now just pass in the new pos:

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

Test, of course. Good. Commit: use parm-agnostic getTile in randomRoomAvoidingCoordinates.

We find this:

function Tiles:cellIsAccessible(position)
    return self:getTile(position.x,position.y):isFloor()
end

Here again, no need to pull out the x and y. Make our job easier.

function Tiles:cellIsAccessible(position)
    return self:getTile(position):isFloor()
end

Test. This is PathFinder code. All good, including PathFinder. Commit: Tiles:cellIsAccessible uses parm-agnostic getTile.

function Tile:getTile(pos)
    return self.tileFinder:getTile(pos.x,pos.y)
end

This one should be made agnostic:

function Tile:getTile(...)
    return self.tileFinder:getTile(...)
end

Test. We’re good. Commit: Tile:getTile made agnostic to pos or x,y.

This:

function Room:centerTile()
    local cp = self:centerPos()
    return self:getTile(cp.x, cp.y)
end

Wants to be this:

function Room:centerTile()
    local cp = self:centerPos()
    return self:getTile(cp)
end

That requires this:

function Room:getTile(x,y)
    return self.tileFinder:getTile(x,y)
end

To be better as this:

function Room:getTile(...)
    return self.tileFinder:getTile(...)
end

And now this:

function Room:tileAt(x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return self:getTile(tilePos.x, tilePos.y)
end

Can be this:

function Room:tileAt(x,y)
    local tilePos = self:centerPos() + vec2(x,y)
    return self:getTile(tilePos)
end

That’s all the changes in Room, so test. We’re good. Commit: Room getTile is parm-agnostic and used accordingly.

Moving right along …

This is odd:

function Player:privateFromXY(tileX,tileY,runner)
    local tile = runner:getTile(vec2(tileX,tileY))
    return Player(tile,runner)
end

I can certainly make this change:

function Player:privateFromXY(tileX,tileY,runner)
    local tile = runner:getTile(tileX,tileY)
    return Player(tile,runner)
end

And test. Seems to work. Who’s calling this thing though? Ah, it’s a factory test method. Fine, let it be. Commit: Player:privateFromXY calls getTile(x,y) variant.

Next:

function PathMap:nextAfter(tile)
    local cell = self:getCellFromTile(tile)
    local prev = cell.parent
    if not prev then return nil end
    return self.dungeon:getTile(prev.x,prev.y)
end

Here we can use the more convenient:

function PathMap:nextAfter(tile)
    local cell = self:getCellFromTile(tile)
    local prev = cell.parent
    if not prev then return nil end
    return self.dungeon:getTile(prev)
end

Test. We’re good. Commit: PathMap uses getTile(pos) rather than (x,y).

That seems to be all there is. Let’s reflect and probably sum up.

Reflecto-Summary

We set out to change all the implementors of getTile with either a position or x and y, to accept either. It ahs gone quite smoothly. We have already found places where the code using getTile is a bit nicer because of the option to use either kind of parameter. What’s not to like?

There are objections that could be raised. Well, I can think of one:

We could imagine that a Lua programmer off the street would look at people calling getTile(x,y) and getTile(pos) and becoming confused, because that’s not a thing commonly done. But, I argue, if they just look at the implementations of getTile, they’ll quickly find the ones with ... as their parameter, and then the ones that sort out the options. Those are marked as “overloaded”, and any Lua programmer should understand the code, even if they might object to it.

Here chez Ron, we have been struggling with the multiple kinds of getTile, and I can fairly confidently predict that we’re not going to be confused by it. And, if we are, see the preceding paragraph.

That aside, I don’t see any credible objections to what has been done here.

We did discover another matter which you may not have noticed. There are two implementations of getTile that access the map.

First there’s this:

-- "overloaded" to allow (pos) or (x,y)
function Dungeon:getTile(posOrX, y)
    if y then
        return self:privateGetTileXY(posOrX,y)
    else
        return self:privateGetTileXY(posOrX.x, posOrX.y)
    end
end

function Dungeon:privateGetTileXY(x,y)
    return self:getMap():atXYZ(x,0,y) or Tile:edge(x,y, self:asTileFinder())
end

Even on its own that seems weird. Are there other callers of that? There are, mostly in tests. I think all of them are in tests. But we also have this:

function Tiles:getTile(posOrX,y)
    if y then
        return self.map:atXYZ(posOrX,0,y) or Tile:edge(posOrX,y,self)
    else
        local xx,yy = posOrX.x, posOrX.y
        return self.map:atXYZ(xx,0,yy) or Tile:edge(xx,yy,self)
    end
end

I think that should be converged down to one.

This should work:

function Dungeon:privateGetTileXY(x,y)
    local finder = self:asTileFinder()
    return finder:getTile(x,y) or Tile:edge(x,y, finder)
end

Test. We’re good. Commit: Dungeon getTile converges on Tiles:getTile. Only one method accesses the map now.

One more thing … the one implementor looks like this:

function Tiles:getTile(posOrX,y)
    if y then
        return self.map:atXYZ(posOrX,0,y) or Tile:edge(posOrX,y,self)
    else
        local xx,yy = posOrX.x, posOrX.y
        return self.map:atXYZ(xx,0,yy) or Tile:edge(xx,yy,self)
    end
end

We can do better and remove some duplication:

function Tiles:getTile(posOrX,y)
    local xx,yy
    if y then
        xx,yy = posOrX,y
    else
        xx,yy = posOrX.x, posOrX.y
    end
    return self.map:atXYZ(xx,0,yy) or Tile:edge(xx,yy,self)
end

Test. We’re good. Commit: all getTile(pos) and getTile(x,y) come down to exactly one access to the map.

Now, I for one, welcome this implementation. I note that it has the property that dungeon accesses create a TileFinder on every access, and a certain kind of person might object to that on the basis of efficiency, or perhaps temporal duplication. The first argument would not bother me at all: I trust my compiler to be able to create and destroy small objects at will. The temporal duplication argument would at least catch my attention, but I’d argue that if it actually became a problem, it would be an efficiency problem, so I’m still not concerned. Look at my face. Am I bovvered? I ain’t bovvered.

I am, however, bovvered by this two step bit here:

-- "overloaded" to allow (pos) or (x,y)
function Dungeon:getTile(posOrX, y)
    if y then
        return self:privateGetTileXY(posOrX,y)
    else
        return self:privateGetTileXY(posOrX.x, posOrX.y)
    end
end

function Dungeon:privateGetTileXY(x,y)
    local finder = self:asTileFinder()
    return finder:getTile(x,y) or Tile:edge(x,y, finder)
end

But I don’t see a nicer way. I need the x and y broken out, so as to use them in the Tile:edge default return. I’ll let it ride for now.

Overall, I’m happy with this convergence of getTile. I’d still like to get rid of the other use of the same name in DungeonContents but I’m overdue for a break, so we’ll call it a day.

We have 14 commits in two hours, less than 9 minutes per commit on the average. And we’ve gently touched ten or fifteen files, made everyone’s job just a bit easier, and come down to a single method accessing tiles from the map.

And, by the way, both my cards have been resolved. We are now using the parm-agnostic calls here and there, so they are tested, and everyhing converges down to Tiles class. Whee!

Nice. Tomorrow … maybe nothing. I have an eye exam. If they dilate me I’ll be offline until Friday. We’ll see. Try to hang tough until then.