I said I wasn’t going to write an article today. Was I wrong? I want to just look around. And some thoughts on self improvement.

I watched an excellent TED Talk by Adam Grant, and it reminded me of some things that matter to me. Go watch it, I’ll wait.

I enjoy the way that at least a few people seem to enjoy my articles, even when I write about a silly game written in a language they don’t use. It makes me feel that what I do matters to at least a few folks. I enjoy the way digging into that increasingly complicated program engages my mind, requiring an active kind of attention that I don’t get from reading or watching whatever my dear wife has running on the TV. And I definitely enjoy the feeling of mastery that I get when the program yields under my fingers, becomes more capable or its code becomes more habitable.

So this morning, I’m going to look around in the Dung, starting near the active area of the Dungeon class, just to find code that might benefit from my touch. And, I’ll write about it, because I enjoy that, too.

Small Wins

Adam Grant mentions “small wins” in his talk. Frequent readers know that my practice, and that of my colleagues, is aimed at making mane small changes to our program, each one making it a bit better, each one adding capability or clarity, inch by inch, step by step1, improving it.

These are “Small Wins”, and they are precious. They feel good. They are tiny shots of endorphins right to the brain.

Let’s look around.

Huh. Here’s something right here:

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

This code sets the scale to either (1,1) or (-1,1), depending on whether the player is to the right or left of the monster in question. (It happens that self.facing is set to 1 or -1, depending on the graphic I found for that monster. Some face left, some face right.)

We could leave this alone. Maybe we should, it certainly isn’t causing any trouble. But, suppose we wanted to make it more clear, what could we do? What about this:

function Monster:flipTowardPlayer()
    if self:playerIsToOurRight() then
        scale(-self.facing,1)
    else
        scale(self.facing,1)
    end
end

function Monster:playerIsToOurRight()
    local dir = self.runner:playerDirection(self:getTile())
    if dir.x > 0 then
end

That might be better. We could give those two scale calls names as well:

function Monster:flipTowardPlayer()
    if self:playerIsToOurRight() then
        self:faceRight()
    else
        self:faceLeft()
    end
end

function Monster:faceLeft()
    scale(self.facing,1)
end

function Monster:faceRight()
    scale(-self.facing,1)
end

Better or worse? We’re up to 16 lines for what used to take only 4. I like the new scheme for clarity, but I don’t like it for adding the complexity of three new methods.

Still, it’s good to know that we could make it more clear this way. Is there some other way?

Well, all that really changes in that function is the sign, - or +. Could we make this work?

function Monster:flipTowardPlayer()
    scale(self:faceTowardPlayer(), 1)
end

Well, we could do this:

function Monster:faceTowardPlayer()
    local dir = self.runner:playerDirection(self:getTile())
    return ((dir.x > 0) and -self.facing) or self.facing
end

One thing this tells me is that self.facing is backward from what I wish it were. I’d think it more natural to negate the value when the x value is negative. We could change that, if it’s not too hard.

It turns out that most of our Monsters face left (or are symmetrical). The Mimic faces right. Let’s reverse the sense of facing so that facing left is -1, which makes a great deal of sense.

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    tile:moveObject(self)
    self.runner = runner
    self.mtEntry = mtEntry or self:randomMtEntry()
    self:initAnimations()
    self.level = self.mtEntry.level or 1
    self.facing = self.mtEntry.facing or -1 -- +1 = facing right
    self.attackVerbs = self.mtEntry.attackVerbs or {"strikes at"}
    self:initAttributes()
    self.swap = 0
    self.move = 0
    self.attributeSheet = AttributeSheet(self)
    self.deathSound = asset.downloaded.A_Hero_s_Quest.Monster_Die_1
    self.hurtSound = asset.downloaded.A_Hero_s_Quest.Monster_Hit_1
    self.pitch = 1
    self:startAllTimers()
end

    m = {name="Mimic", level=1, health={10,10}, speed={10,10}, strength={10,10}, facing=1, strategy=MimicMonsterStrategy, behaviorName="mimicBehavior",
        attackVerbs={"bites", "chomps", "gnaws"},
...

function Monster:faceTowardPlayer()
    local dir = self.runner:playerDirection(self:getTile())
    return ((dir.x > 0) and self.facing) or -self.facing
end

Tests work, game works. I think I’ll commit that: Reversed sense of Monster.facing for minor improvement in clarity.

Was This Worth Doing?

Maybe not, so let’s invest a bit more:

function Monster:flipTowardPlayer()
    scale(self:xScaleToFaceTowardPlayer(), 1)
end

function Monster:xScaleToFaceTowardPlayer()
    local dir = self.runner:playerDirection(self:getTile())
    return ((dir.x > 0) and self.facing) or -self.facing
end

Better? Worse? I call it better. If I didn’t, I’d revert it. As it is, I’ll commit it: renamed faceTowardPlayer to xScaleToFaceTowardPlayer.

Worth it? Who knows? It’s Saturday, we’re just exercising our mind, getting little jolts of endorphin for making things better, occupying our mind in a positive way, and maybe influencing some of our friends to think a bit.

Looking Somewhere Else

Here’s something we actually care about:

function Dungeon:neighbors(tile)
    local tPos = tile:pos()
    local offsets = self:neighborOffsets()
    return map(offsets, function(offset) return self:getTile(offset + tPos) end)
end

function Dungeon:neighborOffsets()
    return { 
        vec2(-1,-1), vec2(0,-1), vec2(1,-1),
        vec2(-1,0),              vec2(1,0),
        vec2(-1,1),  vec2(0,1),  vec2(1,1)
    }
end

This function on Dungeon is supposed to return all the cells, including corner ones, adjacent to the tile provided. The code shown here is assuming cartesian coordinates. We’ve provided a partial solution to this, but only for the four vs six case:

function Maps:directions()
    return _ms:directions()
end

function HexStrategy:directions()
    return MapDirection:hexMapDirections()
end

function CartesianStrategy:directions()
    return MapDirection:cartesianMapDirections()
end

function MapDirection:hexMapDirections()
    return{ MapDirection(1,-1,0), MapDirection(1,0,-1), MapDirection(0,1,-1),MapDirection(-1,1,0), MapDirection(-1,0,1), MapDirection(0,-1,1) }
end

function MapDirection:cartesianMapDirections()
    return {MapDirection(1,0,0), MapDirection(0,0,1), MapDirection(-1,0,0), MapDirection(0,0,-1)}
end

Maybe we need a new method, allDirections, that returns the same old Hex directions, but all eight of the Cartesians.

To support that, we would need a new method allCartesianDirections on MapDirection.

Now it turns out, no one in the game is even using the :directions method yet. I do find another user of the neighborOffsets, however:

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local start = startingTile:pos()
    local target = targetTile:pos()
    local offsets = self:neighborOffsets()
    local choices = map(offsets, function(o) return o + start end)
    local posResults = arraySelect(choices, function(o) return manhattan(o,target) == desiredDistance end)
    local tileResults = map(posResults, function(p) return self:getTile(p) end)
    local finalResults = arraySelect(tileResults, function(t) return t:isFloor() end)
    return #finalResults > 0 and finalResults or { startingTile }
end

Does that even work? It seems to me that all this does is consider tiles that are right next to the tile we’re starting at. That’s because we can only move to those tiles, and–I think–because we’re allowing Monsters to move diagonally in some cases. (To remain at a constant distance from the player, I think you might have to move diagonally.) Otherwise, why include the corners at all?

Let’s see if we can improve that method, make it a bit more clear.

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local start = startingTile:pos()
    local target = targetTile:pos()
    local offsets = self:neighborOffsets()
    local positionsToConsider = map(offsets, function(o) return o + start end)
    local positionResults = arraySelect(positionsToConsider, function(o) return manhattan(o,target) == desiredDistance end)
    local tileResults = map(positionResults, function(p) return self:getTile(p) end)
    local finalResults = arraySelect(tileResults, function(t) return t:isFloor() end)
    return #finalResults > 0 and finalResults or { startingTile }
end

I just renamed some variables here. What about those functions? Let’s see if we can make them more clear:

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local start = startingTile:pos()
    local target = targetTile:pos()
    local offsets = self:neighborOffsets()
    
    local offsetToPos = function(offset) return offet + start end
    local isRightDistance = function(pos) return manhattan(pos,target) == desiredDistance end
    local tileFromPosition = function(pos) return self:getTile(pos) end
    local tileIsFloor = function(tile) return tile:isFloor() end
    
    local positionsToConsider = map(offsets, offsetToPos)
    local positionResults = arraySelect(positionsToConsider, isRightDistance)
    local tileResults = map(positionResults, tileFromPosition)
    local finalResults = arraySelect(tileResults, tileIsFloor)
    return #finalResults > 0 and finalResults or { startingTile }
end

I’ve given each function a hopefully meaningful local name, then used it. And I grouped them together, so that the operational code could be mostly together at the bottom.

These functions are using other local variables, so they have to be in scope, and I think the locals need to be defined before the functions. I’m not sure of that: the local function access to variables in the same scope is pretty deep in the bag of tricks.

I think this is better, but it is still rather obscure. It gains in obscurity by using my specialized functions map and arraySelect, which are handy but not used enough to lift themselves from relative obscurity. On the other hand, it stands out as a large function with extra whitespace, so at least it flags itself as interesting and needing careful attention.

And it could be worse: I could nest the calls. Commit: refactor availableTilesAtDistance.

Hm. In an attempt to show that I’m not entirely evil, I tried nesting the calls, to show what evil really looks like. The test and game broke. Since I’m not evil, and this code is already obscure, I don’t really want to make it worse, so I’ll not drill down to figure out why my nesting wasn’t right. I think what happens is that I missed an upvalue that way. But it’s too weird to chase, since I’m not going to do it anyway.

OK, Was THIS Worth It?

I don’t know. I think it’s better, and I think I’m protected by my tests, and it’s Saturday and I’m just messing around. Do I wish I’d written the method this way originally? Yes, I think I do. But going back to change it, probably won’t speed us up.

So this is fun, interesting, possibly educational, but not something I’d necessarily do on a real project, unless I was working right in that area and saw these changes as desirable.

But that neighborOffsets thing. We do need to change that: it needs to defer to Maps. So … what I’d like to do is this:

function Dungeon:neighborOffsets()
    return Maps:allNeighborDirections()
end

function Maps:allNeighborDirections()
    return _ms:allNeighborDirections()
end

function HexStrategy:allNeighborDirections()
    return HexStrategy:directions()
end

function CartesianStrategy:allNeighborDirections()
    return MapDirection:allCartesianNeighborDirections()
end

function MapDirection:allCartesianNeighborDirections()
    return { 
        MapDirection(-1,0,-1), MapDirection(0,0,-1), MapDirection(1,0,-1),
        MapDirection(-1,0,0),                        MapDirection(1,0,0),
        MapDirection(-1,0,1),  MapDirection(0,0,1),  MapDirection(1,0,1)
    }
end

In fact I’ve typed that in. But I expect this to blow up massively, because I’ve got a collection of MapDirections now, not vec2 as the other Dungeon methods expect. Let’s test and see.

We get a couple of failures. I think we’ll convert from directions to offsets here, for now, and see if that lets things flow through.

function Dungeon:neighborOffsets()
    local dirs = Maps:allNeighborDirections()
    local function toOff(dir)
        return dir:toCartesianOffset()
    end
    local offsets = map(dirs, toOff)
    return offsets
end

That demands this, of course:

function MapDirection:toCartesianOffset()
    return vec2(self.x, self.z)
end

I expect this to work, and if it doesn’t, and the fix isn’t obvious, I expect to revert.

The bug is in fact obvious, the code has this:

    local offsetToPos = function(offset) return offet + start end

That should be offset, not offet. And why didn’t the tests tell me this? I am not sure. You’d think they would have failed right along on adding nil to start. Anyway, changing that makes this code work.

I did a quick check to be sure you can’t add nil and a vector, and you can’t. So I don’t see how this typo ever worked, unless the method was never called during my testing.

Never mind. I’m going to let that go and proceed. Perhaps a better person would dig down to find out why … but to do that I’d have to back out this most recent code. Sorry, I’m just not going to go there. My tests did finally find the problem, so they’re not far off.

Extensive testing (I found and killed a Death Fly) tells me all is OK. Commit: Dungeon:neighborOffsets calls down to Maps (but then converts to offsets. more work needed.)

What is that work? Well, we’re doing a lot of work here with vectors representing offsets, which are really Directions. and we need to start working with Directions and MapPoints.

So this whole section of the Dungeon code really needs to be adjusted to our new Map stuff before we can really claim to be operating in a Hex map.

I don’t think we want to do that today. At least I’m pretty sure that I don’t, and I’m pretty sure that you won’t go ahead without me, but let’s at least look and see what we’d need to do.

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local start = startingTile:pos()
    local target = targetTile:pos()
    local offsets = self:neighborOffsets()
    
    local offsetToPos = function(offset) return offset + start end
    local isRightDistance = function(pos) 
    return manhattan(pos,target) == desiredDistance end
    local tileFromPosition = function(pos) return self:getTile(pos) end
    local tileIsFloor = function(tile) return tile:isFloor() end
    
    local positionsToConsider = map(offsets, offsetToPos)
    local positionResults = arraySelect(positionsToConsider, isRightDistance)
    local tileResults = map(positionResults, tileFromPosition)
    local finalResults = arraySelect(tileResults, tileIsFloor)
    return #finalResults > 0 and finalResults or { startingTile }
end

Notice that by the time we get down to tileResults in the above, we’re dealing with tiles. Right above that, we consider an array of positions, that is, vec2 instances. We use our function manhattan:

function manhattan(v1,v2)
    local abs = math.abs
    local s = 0
    for i in ipairs(v1) do
        s = s + abs(v2[i]-v1[i])
    end
    return s
end

Suppose we changed this function to create a list of neighboring Tiles, and suppose we implemented manhattan on Tile. Or, alternatively, we could create a list of MapPoints, and implement manhattan on those. That’s a bit tricky, because we only have one kind of MapPoint, and manhattan distance is different in Hex than in Cartesian. I guess either way we are going to need to defer to Coord and ultimately down to Maps.

Let’s think what the avail function does: it returns all the neighboring Tiles at a distance d from the provided input Tile.

We could create the list of all neighbor Tiles, then winnow it down if we had Tile:manhattan(aTile). The manhattan method would send manhattan to the two coordinates, perhaps with a double dispatch to avoid irritating Demeter (wait and see, we’ll do this soon, maybe not today).

I think we should push a lot of this down to our Map and its related objects and strategies. I think that since we’re not distinguishing the type of Points by a class distinction, we’ll need to break out a specialized methods there.

To get ready for this change, we need to TDD some manhattan tests for MapPoint, and probably some trivial ones for Tile, which will just call the MapPoint ones.

We’ll definitely leave that for a later time. This is enough for a Saturday. Let’s sum up.

Sumamry

What just happened? Two things, at least.

First, we did some code improvement exercises, picking code that attracted our eye, and trying to make it a little bit better. This is good practice, in the same way that playing Czerny exercises is good practice, but it’s not necessarily the sort of thing one would do during the work day.

What? Am I saying that we have to learn on our own time, not our company’s time? No, I’m not saying quite that. Here’s what I believe about that:

  1. I believe that a wise company would provide enough slack time, and enough encouragement, so that their people would have time and energy to practice their craft, to learn new things, perhaps even to study for advancement. A company that does not invest in its employees is not wise, and probably not a good place to work.
  2. I believe that as an individual, I have a responsibility to myself to apply time and energy to learning my craft and preparing myself for better things. If my company makes that time available, and doesn’t sap my energy to the point where I can’t use the time, great. If not, I believe that I would be wise to find the time and energy, somehow. to learn what I need to learn.

I understand that for some people that is incredibly difficult. And it is also incredibly tragic, because that person is very likely to be stuck wherever they are. If no one else will take care of us, we need to take care of ourselves. And if we simply cannot, that is tragic.

It’s not possible to help someone who has no time, or no energy, to be helped. I hope and pray that few of you are so down-trodden to be unable to progress, however slowly. Of course, if you are lucky enough, and foolish enough, to read these words, you do have time.

I don’t know how to help those who haven’t. If you have an idea, do let me know.

Anyway, we made a few passes at making the code a bit better, and perhaps we did.

Then we turned our attention to something we actually need, which is to get the Dungeon class rigged up so that it is no longer thinking cartesian, and deferring everything that does so down to the Maps area, where the work can be done properly for either cartesian or hex maps.

So far that has not been difficult, but there sure is a lot of it. What has caused the code not to be as well-factored as it might be?

Primitive Obsession. There is lots of code here that is processing vec2 instances, thinking of them as directions or positions, doing arithmetic on them, binding the code closely to the capabilities–and limitations–of vec2. If I were smarter, more sensitive to this aspect of Primitive Obsession, I’d already have some kind of Point and Direction object, and our job would be closer to done.

But I am not that smart, that sensitive, that prescient to realize that working with vec2 was an issue. Arguably, until someone came up with the Hex idea, it wasn’t much of an issue.

Things here aren’t bad. I’m feeling the pain of the primitive vector, but it’s a mere pin-prick compared to what it might be. Since the functions we’re looking at tend to filter down to the one we just worked on, we may find that a few more changes fix everyone up. We can hope.

In the next few days, we’ll move from hope to knowledge. I hope.

I hope to see you then!

  1. Slowly, I turned … ↩