Dungeon 35

Too many notes. Just refactoring today, with reasons why.

The dungeon program is just less than 800 lines, including blank lines and comments. (There actually are some comments, a moderate discourse in the Main tab with early notes on the program.) There are only seven tabs, and only five classes, Tile, GameRunner, Room, Monster and Player. A small program by almost any standard.

I use these small programs to explore how I would program whether the program is large or small. Large programs start small and grow over time. If they grow well, they become good large programs, and if they grow poorly, they become horror shows.

To my taste, all the classes in even a very large program would be small ones, rarely exceeding a dozen methods or a hundred lines of code. Doubtless there’d be exceptions, but most objects should be small, simple, and focused on a single issue. If a single object is large, our minds can’t take it in, and it’s hard to work with. When it’s small and simple, we make fewer mistakes working with it.

Therefore, the lessons we learn in these small demonstration programs apply to larger scale work. At least, that’s my story, and I’m sticking to it.

And this program needs work.

Too Many Notes

The program is working nicely, and I’m really interested in putting in some treasures and setting up some kind of decent game play. But while it works, I believe the program isn’t right, and I want to make it more nearly right. If I don’t, new features will be harder to do.

Now, I’m not saying that between now and the last feature, I’ll go faster if I do the refactoring. If this were a large program, I’d be more certain of that. Here, the stakes are low. I’m going to do this refactoring, not because it’s the fastest way to the end–though it may be–but because it needs doing and I expect that we’ll learn some things by observing.

Tile

I believe that the Tile object is both overly complex, and does not carry as much weight as it should. Too much code for too little benefit. I’m not able to say just now why I think it’s not carrying its weight. It’s more of an intuitive feeling. As we move forward, I’m hoping it will become more clear.

Of course, if it could carry the weight that it does with less complexity, that would be a win. If it can carry more weight with less complexity, that’s even better.

Let’s get started.

Conceptually, the whole game is based on tiles. There is a rectangular array of square tiles. There are three tile “types”, edge, wall, and room. Room tiles are places where the game is played, wall tiles surround the game space, and edge tiles are just black areas where nothing exists, filling up the big rectangle of the game.

So everything happens on tiles, room tiles to be specific. The player and monsters are “on” tiles, they move to new tiles. When doors and treasures and traps are added, they will be “on” tiles as well. When something moves, it moves from one tile to another.

All of this has grow up a bit randomly, as needed. I put capabilities into the system where they seemed to belong, and used names that made sense at the time. I created naming conventions (like ending method names in XY) more or less randomly, and haven’t stuck to them consistently.

In short, it’s a bit of a mess. We’re here to tidy it up.

One thing that I believe will help is to stop passing x,y pairs all around. My plan when I started this article was to give the tile a coordinate pair object, probably a vec2 or possibly a hand-rolled value object like that, but with the ability to add useful methods. The vec2 object does some useful things like add vectors and compute distances.

I think we’ll still do that, starting with vec2. But while there are XY methods, and methods returning x,y pairs all over, I think we can defer all that down to tile and leave the rest of the system using only tiles.

At least that’s where I think we’re going. Where we really go remains to be seen. We’ll find our way somewhere.

Remind me to commit often. We may have some bad ideas to revert coming up. Here goes

Position

I’ll start with a new member variable in Tile, its position. I will abbreviate this to “pos” because the term is meaningful to our team (me) from long use.

function Tile:init(x,y,kind, runner)
    self.x = x
    self.y = y
    self.kind = kind
    self.runner = runner
    self.sprites = {room=asset.builtin.Blocks.Brick_Grey, wall=asset.builtin.Blocks.Dirt, edge=asset.builtin.Blocks.Greystone}
end

We could keep x and y, or remove them. We’ll have to change more if we remove them, but we’ll be better off in the long run. In the interim, we will need them, because there are X Y people all over, and I’ll provide them with methods, not with direct access. Like this:

function Tile:init(x,y,kind, runner)
    self.pos = vec2(x,y)
    self.kind = kind
    self.runner = runner
    self.sprites = {room=asset.builtin.Blocks.Brick_Grey, wall=asset.builtin.Blocks.Dirt, edge=asset.builtin.Blocks.Greystone}
end

function Tile:x()
    return self.pos.x
end

function Tile:y()
    return self.pos.y
end

Now there are lots of places in this object that refer to self.x or y. Those could be replaced with self.pos.x but I want to have them refer to the method because I think it may be easier to find them later. This may be overkill, but it’s the same amount of work either way. I’ll go change them all right now. Here’s the first one:

function Tile:tileCoordinates()
    return self:x(),self:y()
end

With a decent replace function, or a refactoring browser, I could do this instantly. Almost tempting to pass this over to Sublime on my Mac. In fact, yes, why not?

Wow, that was easy. I should do that more often. It went like this:

• Select whole tab contents on iPad
• On Mac, create new tab in Sublime
• Paste
• Select a self.x,
• Repeat Command-D to select them alll
• Type “self:x()”
• Repeat for y
• Copy all
• Paste back to Codea

Took longer to write it up than to do it.

We should be working perfectly … unless there’s an outsider looking at a tile’s .x or .y directly. That shouldn’t be the case, but I might have done it. We’ll find out. Test.

GameRunner:162: attempt to index a nil value (field '?')
stack traceback:
	GameRunner:162: in method 'setTile'
	GameRunner:15: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in global 'GameRunner'
	Main:29: in function 'setup'
GameRunner:162: attempt to index a nil value (field '?')
stack traceback:
	GameRunner:162: in method 'setTile'
	GameRunner:15: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in global 'GameRunner'
	Main:29: in function 'setup'

Well then. That didn’t work. Have I made a typo? Operative line above is GameRunner:162. Aha, that devil:

function GameRunner:setTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    self.tiles[aTile.x][aTile.y] = aTile
end

Our main table of tiles is indeed by x and y. We’ll fix this:

function GameRunner:setTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    self.tiles[aTile:x()][aTile:y()] = aTile
end

This makes me fear that we’ll have more such references. I’ll do a quick search to try to find some. But I don’t. Test again. They pass, the game plays. Commit: convert Tile to pos rather than x y.

Monster Mash

A couple of days ago, I converted the Princess to move herself between tiles rather than propose moves to the GameRunner and get called back. I tried to do the same to Monster without much success. I propose to do that now, because deferring this as we did with the princess will, I think, simplify things and centralize a bit more on the Tile.

Princess does it this way:

PlayerSteps = {a=vec2(-1,0), w=vec2(0,1), s=vec2(0,-1), d=vec2(1,0)}

function Player:keyPress(key)
    local step = PlayerSteps[key]
    if step then
        self.tile = self.tile:legalNeighbor(step)
    end
end

The legalNeighbor function in Tile is this:

function Tile:legalNeighbor(aVector)
    local newTile = self:getNeighbor(aVector)
    if newTile:isRoom() then
        return newTile
    else
        return self
    end
end

function Tile:getNeighbor(aVector)
    local newPos = self:coordinateVector() + aVector
    return self.runner:getTileFromVector(newPos)
end

And coordinateVector probably could use some improvement:

function Tile:coordinateVector()
    return vec2(self:x(),self:y())
end

Well, first:

function Tile:coordinateVector()
    return self.pos
end

That’s nice. But let’s see about renaming that method to something a bit less long and type revealing. I’d like to call it pos but I’d have to rename the member variable. There’s only this one usage at present, though I’m sure there will be more.

I’m going to rename it pos at the cost of renaming the variable. There are only a few references to it anyway.

Done. Renamed the member variable to position, changed all four references to it. Now what we just looked at looks like this:

function Tile:getNeighbor(aVector)
    local newPos = self:pos() + aVector
    return self.runner:getTileFromVector(newPos)
end

I like it. I don’t like getTileFromVector. Too focused on type. This is a pos. Let’s see what other getTile things runner knows.

function GameRunner:getTile(x,y)
    if x<=0 or x>self.tileCountX or y<=0 or y>self.tileCountY then
        return Tile:edge(x,y, self)
    end
    return self.tiles[x][y]
end

function GameRunner:getTileFromVector(aVector)
    return self:getTile(aVector.x, aVector.y)
end

I wonder if anyone uses getTile other than right here. There are quite a few. Let me explain what I am pushing toward.

I want the notion of a “position”, which happens to be a vec2 but might become a more clever object, to be the core object from which we can get the x and y when we need them. (And I want us to need them only rarely.) So a method like getTile, in this new world, would accept a pos as its parameter. The method presently named getTile would be named something else, maybe getTileXY. It’s pretty low level operation. But right now, it’s also pretty common.

I’m going to gut this through. We can commit our previous change, and should have: Add pos method, rename pos member variable.

Now I can safely do this slightly long refactoring. I’ll rename getTile to getTileXY and change all the references. Time me: it’s 0947.

Done at 0950. But did I get them all? Test. Yes, game works. Commit: rename GameRunner:getTile getTileXY

Now, I wanted to rename this:

function GameRunner:getTileFromVector(aVector)
    return self:getTileXY(aVector.x, aVector.y)
end

I wanted to name that getTile. There is, however, a method getTile on Player:

function Player:getTile()
    return self.tile
end

That may lead to confusion, but I think the runner name is better as getTile(aPosition), so here goes:

function GameRunner:getTile(aPosition)
    return self:getTileXY(aPosition.x, aPosition.y)
end

We have to find all the vector guys and change them.

There’s just ours:

function Tile:getNeighbor(aVector)
    local newPos = self:pos() + aVector
    return self.runner:getTileFromVector(newPos)
end

And that becomes:

function Tile:getNeighbor(aVector)
    local newPos = self:pos() + aVector
    return self.runner:getTile(newPos)
end

Test. All good. Commit: refactoring Tile getNeighbor.

What else. There’s this:

function Tile:distanceXY(x,y)
    local dx = self:x()-x
    local dy = self:y()-y
    return math.sqrt(dx*dx+dy*dy)
end

Let’s make one that works on … hell, let’s make it work on tiles. No one should need to be calling in with a position anyway. Who uses this function? We’ll find out. We should be asking for the distance between tiles anyway. No one should be thinking about positions or x’s or y’s.

So I’ll provide this:

function Tile:distance(aTile)
    return self:pos():dist(aTile:pos())
end

See, that’s how using the objects correctly works. We just ask the positions for their distance, which they happen to know because they are vectors. But I want all these calls to distanceXY to go away. I’ll search them out:

function GameRunner:playerDistanceXY(x,y)
    return self.player:distanceXY(x,y)
end

function Player:distanceXY(x,y)
    return self.tile:distanceXY(x,y)
end

function Tile:getTint()
    if not DisplayToggle then return color(255)  end
    local d = self.runner:playerDistanceXY(self:x(),self:y())
    local t = math.max(255-d*255/7,0)
    return color(t,t,t,255)
end

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistanceXY(tx,ty) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
    else
        nx,ny = self:proposeRandomMove()
    end
    self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    self:setMotionTimer()
end

Hm. We have to replace playerDistanceXY and move on from there. I’m finding this tedious, owing to Codea’s weak search capability. Let’s put an assert in the function and trap users one by one.

function Tile:distanceXY(x,y)
    assert(false, "fix me")
    local dx = self:x()-x
    local dy = self:y()-y
    return math.sqrt(dx*dx+dy*dy)
end

Tile:56: fix me
stack traceback:
	[C]: in function 'assert'
	Tile:56: in function <Tile:55>
	(...tail calls...)
	Tile:85: in method 'getTint'
	Tile:66: in method 'draw'
	GameRunner:87: in method 'draw'
	Main:45: in function 'draw'

That’s this:

function Tile:getTint()
    if not DisplayToggle then return color(255)  end
    local d = self.runner:playerDistanceXY(self:x(),self:y())
    local t = math.max(255-d*255/7,0)
    return color(t,t,t,255)
end

We demand that runner accept a tile version of playerDistance:

function Tile:getTint()
    if not DisplayToggle then return color(255)  end
    local d = self.runner:playerDistance(self)
    local t = math.max(255-d*255/7,0)
    return color(t,t,t,255)
end


function GameRunner:playerDistance(aTile)
    return self.player:distance(aTile)
end
-- replacing:

function GameRunner:playerDistanceXY(x,y)
    return self.player:distanceXY(x,y)
end

function Player:distance(aTile)
    return aTile:distance(self.tile)
end

-- replacing:

function Player:distanceXY(x,y)
    return self.tile:distanceXY(x,y)
end

Next error:

Monster:25: attempt to call a nil value (method 'playerDistanceXY')
stack traceback:
	Monster:25: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistanceXY(tx,ty) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
    else
        nx,ny = self:proposeRandomMove()
    end
    self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    self:setMotionTimer()
end

Now we can just pass our tile in:

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistance(self.tile) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
    else
        nx,ny = self:proposeRandomMove()
    end
    self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    self:setMotionTimer()
end

Everything works. We can remove the method with the assert in it:

function Tile:distanceXY(x,y)
    assert(false, "fix me")
    local dx = self:x()-x
    local dy = self:y()-y
    return math.sqrt(dx*dx+dy*dy)
end

Commit: refactoring to use tiles in player distance.

What else? I can search for any more XY methods, and for uses of x() and y() on tile. First the XY methods.

There are quite a few usages of this one:

function GameRunner:getTileXY(x,y)
    if x<=0 or x>self.tileCountX or y<=0 or y>self.tileCountY then
        return Tile:edge(x,y, self)
    end
    return self.tiles[x][y]
end

This is really kind of the bottom of the tile storage hierarchy, where we finally come down to the actual data structure. It should be a private method. I’m going to rename it and see who breaks. I know some tests will, and I’ll let them use the private. But who else?

Here’s one:

function Tile:hasRoomNeighbor()
    local ck = { {1,0},{1,1},{0,1},{-1,1}, {-1,0},{-1,-1},{0,-1},{1,-1}}
    for i,p in ipairs(ck) do
        local tx,ty = self:x() +p[1], self:y() + p[2]
        local tile = self.runner:getTileXY(tx,ty)
        if tile:isRoom() then return true end
    end
    return false
end

We have the ability to use pos here just fine:

function Tile:hasRoomNeighbor()
    local ck = { vec2(1,0),vec2(1,1),vec2(0,1),vec2(-1,1), vec2(-1,0),vec2(-1,-1),vec2(0,-1),vec2(1,-1)}
    for i,p in ipairs(ck) do
        local pos = self:pos() + p
        local tile = self.runner:getTile(pos)
        if tile:isRoom() then return true end
    end
    return false
end

I think that does the job just fine. Let’s test and see what breaks.

Player:14: attempt to call a nil value (method 'getTileXY')
stack traceback:
	Player:14: in method 'fromXY'
	GameRunner:42: in method 'createRandomRooms'
	Main:30: in function 'setup'

That’s from this:

function Player:fromXY(tileX,tileY,runner)
    local tile = runner:getTileXY(tileX,tileY)
    return Player(tile,runner)
end

But why should we create it this way?

    local r1 = self.rooms[1]
    local rcx,rcy = r1:center()
    self.player = Player:fromXY(rcx,rcy,self)

Give her the tile, that’s all she wants. We can get rid of the fromXY method entirely, this way:

    local r1 = self.rooms[1]
    local rcx,rcy = r1:center()
    local tile = self:getTile(vec2(rcx,rcy))
    self.player = Player(tile,self)

This should work … but not yet …

GameRunner:156: attempt to call a nil value (method 'getTileXY')
stack traceback:
	GameRunner:156: in method 'randomRoomTile'
	GameRunner:66: in method 'createMonsters'
	GameRunner:44: in method 'createRandomRooms'
	Main:30: in function 'setup'

function GameRunner:randomRoomTile()
    while true do
        tx,ty = math.random(1, self.tileCountX), math.random(1,self.tileCountY)
        local tile = self:getTileXY(tx,ty)
        if tile.kind == "room" then return tile end
    end
end

This becomes …

function GameRunner:randomRoomTile()
    while true do
        local pos = vec2(math.random(1, self.tileCountX), math.random(1,self.tileCountY))
        local tile = self:getTile(pos)
        if tile.kind == "room" then return tile end
    end
end

GameRunner:135: attempt to call a nil value (method 'getTileXY')
stack traceback:
	GameRunner:135: in method 'moveMeIfPossible'
	Monster:30: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

Starting to think search would be better. Anyway, this one is:

function GameRunner:moveMeIfPossible(mover, x,y)
    local tile = self:getTileXY(x,y)
    if tile.kind == "room" then
        mover:moveTo(x,y)
    end
end

We can fix him quickly. Normally I’d hunt down his caller as well, but for now:

function GameRunner:moveMeIfPossible(mover, x,y)
    local tile = self:getTile(vec2(x,y))
    if tile.kind == "room" then
        mover:moveTo(x,y)
    end
end

Monster:48: attempt to call a nil value (method 'getTileXY')
stack traceback:
	Monster:48: in method 'moveTo'
	GameRunner:137: in method 'moveMeIfPossible'
	Monster:30: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

function Monster:moveTo(newX,newY)
    self.tile = self.runner:getTileXY(newX,newY)
end

But why didn’t GameRunner move us to the tile: he has it:

function Monster:moveTo(aTile)
    self.tile = aTile
end

function GameRunner:moveMeIfPossible(mover, x,y)
    local tile = self:getTile(vec2(x,y))
    if tile.kind == "room" then
        mover:moveTo(tile)
    end
end

I think we’re close to working. We are, except for some tests who need to call the private method for now.

Ah the error is coming from this method, which I planned to remove:

function Player:fromXY(tileX,tileY,runner)
    local tile = runner:getTileXY(tileX,tileY)
    return Player(tile,runner)
end

The tests want to use players in given positions. Let’s allow it but make this method private as well.

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

One test still fails:

7: distance from avatar -- Tests:110: attempt to call a nil value (method 'playerDistanceXY')

Fixed:

        _:test("distance from avatar", function()
            local x,y = 100,100
            local runner = GameRunner()
            local player = Player:privateFromXY(104,104, runner)
            runner.player = player
            local d = runner:playerDistance(vec2(x,y))
            _:expect(d).is(5.6,0.1)
        end)
        

Hm that still errors:

7: distance from avatar -- Player:24: attempt to call a nil value (method 'distance')

function Player:distance(aTile)
    return aTile:distance(self.tile)
end

Oh. We’re supposed to call playerDistance with a tile, not a pos. Fix the test some more:

Tests green game runs. Commit: more refactoring, moving to tile focus.

Sunday brekkers is nearly ready. We’ll stop here. I’ll begin summing up:

Summing Up

We’ve removed (almost?) all the extraneous uses of XY methods, and many of the uses of x,y pairs. But not all of them. We’re replacing most of those x’s and y’s with references to tiles, which are getting closer to being the only object that knows about x’s and y’s at all … and it just barely knows, focusing primarily on position, presently a vec2.

The code is less complex through less use of explicit x’s and y’s, and through more consistent usage of tiles as the main information-carrying element in the game.

There will be more improvements, but we’re already quite improved in my view. I think we’ll close for the day. I can smell bacon in my future.

See you next time!

D2.zip