# Dungeon 36

More refactoring: I want cleaner code to work in. Mabe one tiny feature.

I’m on a quest to make the Tile object more valuable, while–I hope–offering a more narrow interface. There are still methods around the system that are unpacking x’s and y’s, who shouldn’t need to if information and function were better kept together (Cohesion). I’m just kind of following my nose on this process, cleaning up whatever I find.

I do have a feature in mind, a tiny one, that I tried last night on the other iPad. Faithful readers may recall that I tried reversing the ghost monster’s image. The idea was to make him always face toward the player (Princess), thus appearing to be more menacing, or at least aware of what he’s doing.

Faithful readers may recall that scaling him (-1,1) did flip him, but it also moved him one square to the right, because the sprite mode is CORNER. I tried adjusting to CENTER and that worked.

Let’s put that in now, then we’re sure to have a “feature” as part of our releases today.

```
function Monster:draw()
pushMatrix()
pushStyle()
spriteMode(CORNER)
local gx,gy = self.tile:graphicCoordinates()
translate(gx,gy)
-- scale(-1,1) -- flips him to wrong square
sprite(self.sprite1, 0,0)
translate(-gx,-gy)
popStyle()
popMatrix()
end
```

We want to change the mode to CENTER, and flip him only so that he’s facing the player. We’ll want to draw him at the center of the tile. `tile:graphicCoordinates()`

returns the corner, so we’ll want a new Tile function. Let’s try this:

```
function Monster:draw()
pushMatrix()
pushStyle()
spriteMode(CORNER)
local gx,gy = self.tile:graphicCenter()
translate(gx,gy)
self:flipTowardPlayer()
sprite(self.sprite1, 0,0)
translate(-gx,-gy)
popStyle()
popMatrix()
end
function Monster:flipTowardPlayer()
local dx = self.runner:playerDirection(self.tile)
if dx > 0 then
scale(-1,1)
end
end
```

That flip will either be perfectly right or perfectly wrong. Now we need:

```
function Tile:graphicCenter()
return (self:x()-1)*TileSize+TileSize//2,(self:y()-1)*TileSize+TileSize//2
end
```

I expect this to work. And it does.

Commit: monster faces princess.

## Refactoring Opportunities

Let’s first see what methods in Tile are still returning separate x and y values. We know that `graphicCoordinates`

and `graphicCenter`

do, but that’s the form the users actually want. We should, however, rename `graphicCoordinates`

to `graphicCorner`

I think.

Commit: rename graphicCoordinates to graphicCorner

That wasn’t too exciting. Checking further in Tile …

I find these:

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

Tile uses that first function here:

```
function Tile:directionTo(aTile)
local tx,ty = aTile:tileCoordinates()
return sign(tx-self:x()), sign(ty-self:y())
end
```

We can readily convert that to use `pos`

:

```
function Tile:directionTo(aTile)
local t = aTile:pos()
return sign(t.x-self:x()), sign(t.y-self:y())
end
```

That justifies the `x()`

and `y()`

as well. Other uses of `tileCoordinates`

:

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

That one wants to be changed for another purpose, to make the moves instead of asking to make them. We’ll let that sit for a moment and look further:

```
function Monster:proposeRandomMove()
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tx,ty = self.tile:tileCoordinates()
return tx + move.x, ty + move.y
end
```

Here again, I want to make the move, not just propose it. We’ll come back to these. What else is there?

```
function Player:tileCoordinates()
return self.tile:tileCoordinates()
end
```

I am not at all sure that this is used. I see no calls to it above. Let’s remove it and see.

I note that I forgot to set sprite mode to center in `Monster:draw`

:

```
function Monster:draw()
pushMatrix()
pushStyle()
spriteMode(CENTER)
local gx,gy = self.tile:graphicCenter()
translate(gx,gy)
self:flipTowardPlayer()
sprite(self.sprite1, 0,0)
translate(-gx,-gy)
popStyle()
popMatrix()
end
```

Commit: fixed monster centering, removing some uses of `tileCoordinates`

.

Now back to the monster motion. Let’s review how the princess does it:

```
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 `Tile:legalNeighbor`

just returns the proposed tile if it is legal and the old tile if not:

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

Let’s rename that argument:

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

I think this next bit is a bit tricky, because I have to change these three methods consistently, so that nothing breaks. It’s no big deal to do them all, and I feel sure that I could do it, but it’s a better practice, in my opinion, to do things in smaller bites. First commit: renamed argument. Now I have a clean reversion point if I need it.

Here’s where we start:

```
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
function Monster:proposeMoveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
return 0,dy
else
return dx,0
end
end
function Monster:proposeRandomMove()
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tx,ty = self.tile:tileCoordinates()
return tx + move.x, ty + move.y
end
```

The problem here is that the two branches of the if statement drop through and expect tx and ty to be provided. If I’m to change the two propose methods to “do” methods, they’ll not want to return anything. So let’s “defactor”:

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

Now I can change one branch at a time. Let’s commit: defactor `Monster:chooseMove`

, moving our save point a bit forward.

Now let’s make the random move just do it:

```
function Monster:chooseMove()
local nx,ny
local tx,ty = self.tile:tileCoordinates()
if self.runner:playerDistance(self.tile) <= 10 then
nx,ny = self:proposeMoveTowardAvatar()
self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
else
nx,ny = self:makeRandomMove()
end
self:setMotionTimer()
end
function Monster:makeRandomMove()
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
self.tile = self.tile:legalNeighbor(move)
end
```

I reckon this works. It’s a bit hard to see unless it explodes, because by the time you can see a ghost, it’s following. But I can see on the expanded map that the ghosties are moving. So far so good. Now the other:

```
function Monster:chooseMove()
local nx,ny
if self.runner:playerDistance(self.tile) <= 10 then
self:moveTowardAvatar()
else
self:makeRandomMove()
end
self:setMotionTimer()
end
```

But how should we modify the mover. With just the rename I have this:

```
function Monster:moveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
return 0,dy
else
return dx,0
end
end
```

Let’s see if we can get closer:

```
function Monster:moveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
self.tile = self.tile:legalNeighbor(vec2(0,dy))
else
self.tile = self.tile:legalNeighbor(vec2(dx,0))
end
end
```

This should work. I owe a commit. Eeek. Let’s test this. All good. Commit: refactoring to make monsters use direct movement to legal neighbor.

There are now no calls to `tileCoordinates`

. Remove it, commit: removed tileCoordinates method. We still have the x,y thing going on here though:

```
function Monster:moveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
self.tile = self.tile:legalNeighbor(vec2(0,dy))
else
self.tile = self.tile:legalNeighbor(vec2(dx,0))
end
end
```

We’re using that function a couple of times. It can surely return a vector, I’d think. Let’s see how it works:

```
function GameRunner:playerDirection(aTile)
return aTile:directionTo(self.player:getTile())
end
function Tile:directionTo(aTile)
local t = aTile:pos()
return sign(t.x-self:x()), sign(t.y-self:y())
end
```

Let’s just make that baby return a vector, and work from there.

```
function Tile:directionTo(aTile)
local t = aTile:pos()
return vec2(sign(t.x-self:x()), sign(t.y-self:y()))
end
```

Users are:

```
function Monster:flipTowardPlayer()
local dx = self.runner:playerDirection(self.tile)
if dx > 0 then
scale(-1,1)
end
end
function Monster:moveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
self.tile = self.tile:legalNeighbor(vec2(0,dy))
else
self.tile = self.tile:legalNeighbor(vec2(dx,0))
end
end
_:test("player direction", function()
local dx,dy
local runner = GameRunner()
local player = Player:privateFromXY(100,100, runner)
runner.player = player
local tile = Tile:edge(101,101)
dx,dy = runner:playerDirection(tile)
_:expect(dx).is(-1)
_:expect(dy).is(-1)
tile = Tile:edge(98,98)
dx,dy = runner:playerDirection(tile)
_:expect(dx).is(1)
_:expect(dy).is(1)
end)
```

We can fix the test this way:

```
_:test("player direction", function()
local dx,dy
local runner = GameRunner()
local player = Player:privateFromXY(100,100, runner)
runner.player = player
local tile = Tile:edge(101,101)
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(-1)
_:expect(dy).is(-1)
tile = Tile:edge(98,98)
dx,dy = runner:playerDirection(tile):unpack()
_:expect(dx).is(1)
_:expect(dy).is(1)
end)
```

We can do the same for the usages if we care to, but we can do better:

```
function Monster:flipTowardPlayer()
local dir = self.runner:playerDirection(self.tile)
if dir.x > 0 then
scale(-1,1)
end
end
```

So that’s nice. I should commit but let’s see about this one:

```
function Monster:moveTowardAvatar()
local dx,dy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
self.tile = self.tile:legalNeighbor(vec2(0,dy))
else
self.tile = self.tile:legalNeighbor(vec2(dx,0))
end
end
```

We can just unpack here, or do this:

```
function Monster:moveTowardAvatar()
local dxdy = self.runner:playerDirection(self.tile)
if math.random() < 0.5 then
self.tile = self.tile:legalNeighbor(vec2(0,dxdy.y))
else
self.tile = self.tile:legalNeighbor(vec2(dxdy.x,0))
end
end
```

I like that a bit better. Let’s test. And we’re good. Commit: playerDirection returns vec2, not a pair.

Let’s glance at Tile, but I think it’s nearly good:

It uses those `x()`

and `y()`

functions a lot, like here:

```
function Tile:graphicCorner()
return (self:x()-1)*TileSize,(self:y()-1)*TileSize
end
```

What if we were to say that this function returns a vector and if you want it otherwise, unpack it? Let’s first express it with vectors:

```
function Tile:graphicCorner()
local result = (self.position - vec2(1,1))*TileSize
return result:unpack()
end
```

Then remove the local, which I just used for convenience:

```
function Tile:graphicCorner()
return ((self.position - vec2(1,1))*TileSize):unpack()
end
```

And we can do the other … apparently not. Let’s commit: modified Tile:graphicCorner to use vector calculation.

Let me try the center one again.

```
function Tile:graphicCenter()
return (self:x()-1)*TileSize+TileSize//2,(self:y()-1)*TileSize+TileSize//2
end
```

First cut:

```
function Tile:graphicCenter()
local v11 = vec2(1,1)
local offset = v11*(TileSize//2)
local center = (self.position - v11)*TileSize
local result = center + offset
return result:unpack()
end
```

This isn’t obviously better, but it might be. We have some duplication:

```
function Tile:graphicCenter()
local v11 = vec2(1,1)
local offset = v11*(TileSize//2)
local center = (self.position - v11)*TileSize
local result = center + offset
return result:unpack()
end
function Tile:graphicCorner()
return ((self.position - vec2(1,1))*TileSize):unpack()
end
```

Let’s make these things return a vector and the callers unpack:

```
function Monster:draw()
pushMatrix()
pushStyle()
spriteMode(CENTER)
local center = self.tile:graphicCenter()
translate(center.x,center.y)
self:flipTowardPlayer()
sprite(self.sprite1, 0,0)
translate(-center.x,-center.y)
popStyle()
popMatrix()
end
```

That’ll be OK. And it’s the only use of that function so far.

```
function Tile:graphicCenter()
local v11 = vec2(1,1)
local offset = v11*(TileSize//2)
local center = (self.position - v11)*TileSize
local result = center + offset
return result
end
```

Commit: Tile:graphicCenter returns vector.

The `graphicCorner`

method is used more. We’ll change them:

```
function draw()
pushMatrix()
if CodeaUnit then showCodeaUnitTests() end
if DisplayToggle then
local center = Runner.player:graphicCorner()
focus(center, 1)
else
scale(0.25)
end
Runner:draw()
popMatrix()
end
function focus(center, zoom)
local LOWX,LOWY = maxScrollValues()
translate(clamp(LOWX, WIDTH/2-center.x, 0), clamp(LOWY, HEIGHT/2-center.y, 0))
end
function Tile:draw()
pushMatrix()
pushStyle()
spriteMode(CORNER)
tint(self:getTint())
local sp = self:getSprite()
local center = self:graphicCorner()
sprite(sp, center.x,center.y, self.runner.tileSize)
popStyle()
popMatrix()
end
function Player:draw()
local dx = -2
local dy = -3
pushMatrix()
pushStyle()
spriteMode(CORNER)
local center = self:graphicCorner()
sprite(asset.builtin.Planet_Cute.Character_Princess_Girl,center.x+dx,center.y+dy, 80,136)
popStyle()
popMatrix()
end
```

I think this does it. Those latter two methods might be made a bit cleaner as well, but that’s not our mission. Let’s not confuse ourselves (i.e. me).

We’re all good. Commit: graphicCenter and graphicCorner now return vectors.

Are there still any calls to x() and y()? Just two:

```
function Tile:__tostring()
return string.format("Tile[%d][%d]: %s", self:x(), self:y(), self.kind)
end
function Tile:directionTo(aTile)
local t = aTile:pos()
return vec2(sign(t.x-self:x()), sign(t.y-self:y()))
end
```

Let’s do this:

```
function Tile:__tostring()
return string.format("Tile[%d][%d]: %s", self.position.x, self.position.y, self.kind)
end
```

And I think we’ll do that in the other as well. Makes it a bit more intricate but lets me remove those two methods.

I do have mixed feelings about this. the rationale for those methods, and `pos`

, was to allow for internal changes to this object. I could have been right then, but my view now is that it’s stable, probably won’t change, and if it does, it is small enough to change without difficulty.

I could be wrong. But I’m gonna do it.

```
function Tile:directionTo(aTile)
local t = aTile:pos()
return vec2(sign(t.x-self.position.x), sign(t.y-self.position.y))
end
```

We could perhaps push the problem down to the sign function, but that’s for another day.

Now I think there are no references to `x()`

and `y()`

.

Ah, I’m wrong. There’s this one:

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

That’s a key method, at the core of how the tiles work. We could decode from `pos()`

, which isn’t much worse. I think that reduces clarity a bit, reduces flexibility a bit, but I’d rather get rid of the two methods.

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

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

This lets me get rid of those two methods. I might be wise to call `pos()`

inside Tile rather than refer directly to `position`

. Let’s go back to that. Done … and commit: removed x and y methods from Tile.

Time for a bite. Then I’ll sum up.

## S.U.

A small feature today gives the sole monster type the ability to face toward the player, which increases its menacing factor by at least 25 percent. It was easy to do, although I still managed to do it wrong the first time.

Some simple refactoring allowed us to remove more of the pesky x,y returns from the system, with a net simplification of code in all but one case. That case was this one:

```
function Tile:graphicCenter()
local v11 = vec2(1,1)
local offset = v11*(TileSize//2)
local center = (self.pos - v11)*TileSize
local result = center + offset
return result
end
function Tile:graphicCorner()
return (self.pos - vec2(1,1))*TileSize
end
```

The `graphicCenter`

function is a bit long. However, we have one bit of refactoring yet available to us, like this:

```
function Tile:graphicCenter()
local v11 = vec2(1,1)
local offset = v11*(TileSize//2)
local corner = self:graphicCorner()
local result = corner + offset
return result
end
```

That local variable should have been named `corner`

right along. Now, v11 is used only once, so we can do Inline Variable:

```
function Tile:graphicCenter()
local offset = vec2(1,1)*(TileSize//2)
local corner = self:graphicCorner()
local result = corner + offset
return result
end
```

We apply Inline Variable once more:

```
function Tile:graphicCenter()
local offset = vec2(1,1)*(TileSize//2)
local result = self:graphicCorner() + offset
return result
end
```

And once more:

```
function Tile:graphicCenter()
local result = self:graphicCorner() + vec2(1,1)*(TileSize//2)
return result
end
```

And once more:

```
function Tile:graphicCenter()
return self:graphicCorner() + vec2(1,1)*(TileSize//2)
end
```

Let’s do one more thing: Extract Method:

```
function Tile:graphicCenter()
return self:graphicCorner() + self:cornerToCenterOffset()
end
function Tile:cornerToCenterOffset()
return vec2(1,1)*(TileSize//2)
end
```

I think that’s a bit better. Commit: refactor Tile:graphicCenter().

So we have simplified Tile substantially. It now never returns am x,y pair, and has a narrower and more consistent interface, so it’s both smaller and simpler. Good stuff.

I’m pretty sure that we’ve removed all the returns of paired x’s and y’s from the whole system, although there still hare uses of them in calling sequences, mostly in creation. Those are probably OK, I think.

It’s time for some more capability in the game, however. By Friday’s Zoom Ensemble, I want to have some cool stuff under my belt to show off.

Here are some things we should look at:

- Adding more monsters than just pink ghosts;
- Some kind of battle capability between player and monsters;
- Some treasures, like weapons, health, and gold;
- Locks and keys;
- Scoring;
- Multiple levels;
- …

It goes on and on. Our purpose is just to have fun, and to create an extended example of how one does things like this, not to produce a real game. To do that would surely be more work than I want to put in. But for now, it’s fun.

See you next time!