Dungeon 114

It’s International Women’s Day here at the dungeon, so let’s see whether we can give the Princess the ability to heal.

Sometimes in these articles, I refactor something just because I see a better way to do it, and to demonstrate how I go about refactoring, ideally in very small steps, aided by tests, never breaking anything.

In a real product development, however, I prefer to refactor code only if we’re working on it anyway. There doesn’t seem to me to be much advantage to improving code that isn’t causing us any trouble. That helps us focus on the new capabilities that our product needs.

That said, my reason for refactoring on “health” was that we have some stories pending that would benefit from a better design. First, we want the player to “heal” over time, with their health rating increasing up to the level we now call “nominal”. Second, we want to implement “poison”, which causes health to slowly decline until the poison is reversed. Additionally, we want power-ups that take us above nominal to slowly decline back down to nominal.

This morning, as I was trying to decide whether it was time to get up, I had a related idea. What if instead of applying immediately, power-ups went into the player’s inventory? Then they might apply when the player touches their icon, or they might even auto-apply if needed. We’ll keep that idea in mind, but it’ll take me back to a place where I don’t want to go, working on the AttributeSheet, which is mostly just tedious and doesn’t interest me much.

I have to admit, though, that the most recent change to AttributeSheet, which was the new AttributeView class, is rather nice. Reusing that idea may make the sheet easier to extend over time. We’ll see.

Today, we just want to implement …

Healing

The idea is that when the player’s Health is below nominal, every few moves, the meaning of “few” to be decided, her Health ticks back up by a point. She’s healing from her injuries. A very real question in my mind is, how the heck are we going to do this? I can see a couple of options:

Move Counter

It would be pretty easy to put a move counter into the player, and to check it with explicit code to see if it has advanced past the number of moves between e.g. the Healing event, and it if has, tick her health up.

Tween Delay

I have developed the pattern of using tween.delay for timed events, and could certainly use it for this feature. Doing so would mean that the princess would heal even if she was just standing still, idling. That could be fine. She might run over to a corner and just wait until she feels better.

One reason to avoid the tween approach is that they tend to make testing messy, because if the entity starts tweening, things happen that are not part of the test. I have a fake tween delay function that I inject when this gets too troubling, but it’s generally a hassle. But it makes more sense, I think, if the player could just stand and wait for healing. I think we’ll do the tween.

Either way, once we have a clock running, whether move-based or time-based, there will be other things to attach to it, so we should keep that in mind. Not that we’ll generalize too far, but we should at least leave it a bit open.

I feel the need to speak about generalizing. I apologize in advance.

Generalizing

I strongly believe in writing good code that covers today’s cases, leaving generalization for the occasion when we have a need for the second or third thing. That’s not to say that I never go further than necessary, but I do feel that one wants to spend very little time preparing for the future, beyond putting things in the right places, and keeping them well structured.

That said, I know we’re going to do poison, which will be based similarly to healing. Poison is just negative healing, if you think about it. And we’re going to have other power-ups tick down, not just the Health one.

Here’s a question then. Right now, if we wanted to tick health up and down, we could put a tween into the HealthAttribute to do the job. But if we wanted strength to tick down, we have no StrengthAttribute (yet) to attach to. We’d have to do that from the level of Player.

Recall that we created a (not very) smart collection a few days back, Monsters. That class just supports a few useful actions, like telling all the monsters to move, starting all their timers, and so on.

If we had a similar object, Attributes, the player would be simplified to hold on to just one thing instead of the many it has now, including strengthPoints, keys, speedPoints, and health. We’re already heading in that direction, and this opportunity for healing and poisoning pushes us more that way.

Design Decision

OK, I’m glad we had this little chat. We’re going to push in the direction of having a single object on the player that retains all the player’s attributes, including inventory items like keys and such. I believe that in D\&D such a thing is called a Character Sheet, so let’s call ours that.

Let’s assume that in the fullness of time, strength and speed will become attributes much like Health, encompassing a current and nominal value. But let’s not start there. Instead, I think I’ll try to TDD a CharacterSheet that can hold health, strength, speed, and keys … in their present forms. We can then refactor just inside the CharacterSheet as we think of better ways to do things.

So the purpose is to build a character sheet that the player can just hold on to and forward messages to as need be. In due time, we hope to use just the sheet in things like combat.

I’ll begin with a test.

-- CharacterSheet
-- RJ 20210308


function testCharacterSheet()
    CodeaUnit.detailed = true
    
    _:describe("Character Sheet", function()
        
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("Create CharacterSheet", function()
            local cs = CharacterSheet()
            _:expect(cs).isnt(nil)
        end)
        
    end)
end

CharacterSheet = class()

function CharacterSheet:init()
end

That runs, of course. Now we need to look at how player inits itself now:

function Player:init(tile, runner)
    self.runner = runner
    self:initTile(tile)
    self:initAttributes(self:retainedAttributes())
    self:initSounds()
    self:initVerbs()
end

function Player:retainedAttributes()
    return { keys=0, _health = HealthAttribute(12,12), speedPoints = 8, strengthPoints = 10 }
end

function Player:initAttributes(attrs)
    self.attributeSheet = AttributeSheet(self,750)
    self:initRetainedAttributes(attrs)
end

function Player:initRetainedAttributes(attrs)
    for k,v in pairs(self:retainedAttributes()) do
        self[k] = attrs[k]
    end
end

This is all rather nicely contained. A credit to code-cleaning kinds of refactoring. Let’s just push all that right down like this. But wait, what’s that about attribute sheet?

AttributeSheet is the class that draws the attribute sheet. Its name should probably be AttributeView or CharacterView or something like that. We needed be concerned.

Our initRetainedAttributes function sets up four member variables in Player, keys, _health, speedPoints, and strengthPoints. Let’s anticipate that we’re just going to replace all that with a single variable, _characterSheet, and forward as needed. And monsters will make similar use of the CharacterSheet as we go forward. One thing at a time.

Let’s test this:

        _:test("Create CharacterSheet", function()
            local cs = CharacterSheet()
            _:expect(cs:numberOfKeys()).is(0)
        end)

Now, I could do the “fake it till you make it” trick here, but I don’t want to. I want to move a bit faster, but still safely.

function CharacterSheet:init(attrs)
    self:initAttributes(attrs or self:defaultAttributes())
end

function CharacterSheet:defaultAttributes()
    return { keys=0, _health = HealthAttribute(12,12), speedPoints = 8, strengthPoints = 10 }
end

function CharacterSheet:initAttributes(attrs)
    for k,v in pairs(attrs) do
        self[k] = v
    end
end

function CharacterSheet:numberOfKeys()
    return keys
end

This errors:

1: Create CharacterSheet  -- Actual: nil, Expected: 0

So much for safe. How about self.keys?

Test runs. But I notice that when it failed, I didn’t get my big red popup of the errors. How did that get lost? Ah, turns out that if the font size is too large to fit all the text, the text doesn’t display. Weird. Easily fixed.

Now, fact is, I think all the attributes are now installed in the CharacterSheet just as they once were in Player. Now let’s find all Player’s references to those attributes, and let’s TDD the methods it’ll call to fetch them.

keys is done, with the keyCount method. Entity accesses strengthPoints through the method strength. We'll plan to forward that, and similarly for speed and health`:

        _:test("Create CharacterSheet", function()
            local cs = CharacterSheet()
            _:expect(cs:numberOfKeys()).is(0)
            _:expect(cs:strength()).is(10)
            _:expect(cs:speed()).is(8)
            _:expect(cs:health()).is(12)
        end)

function CharacterSheet:health()
    return self._health:value()
end

function CharacterSheet:numberOfKeys()
    return self.keys
end

function CharacterSheet:speed()
    return self.speedPoints
end

function CharacterSheet:strength()
    return self.strengthPoints
end

So that works. I think we’ll commit: initial CharacterSheet, not plugged into anything.

Installing CharacterSheet

OK, with time out for making chai and other random activities involving cat, that took a bit more than an hour, which seems long to me. I must have been frittering somewhere. But now we want to install the thing, and we’re going to need to install it both for Player and Monster, as all the accessors are up in Entity.

Is this an argument against inheriting implementation? It might well be. In fact, I could proceed this way:

I could override the Entity methods in Player, and leave Monsters running the old way. I think that would be bad. Overriding concrete methods can only lead to confusion, it seems to me. I’ll just make it all work. Can’t really take long, can it?

It’s 0950

I’ll start here:

function Player:initRetainedAttributes(attrs)
    self.characterSheet = CharacterSheet(attrs)
end

And, since I’m doing them both at once, I’ve got to do Monster as well. Here’s the relevant init:

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    self.tile = tile
    self.tile:addDeferredContents(self)
    self.runner = runner
    self.mtEntry = mtEntry or self:randomMtEntry()
    self.dead = self.mtEntry.dead
    self.hit = self.mtEntry.hit
    self.moving = self.mtEntry.moving
    self.level = self.mtEntry.level or 1
    self.attackVerbs = self.mtEntry.attackVerbs or {"strikes at"}
    self._health = HealthAttribute(self:roll(self.mtEntry.health))
    self.strengthPoints = self:roll(self.mtEntry.strength)
    self.speedPoints = self:roll(self.mtEntry.speed)
    self.movingIndex = 1
    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
end

I see we haven’t refactored this one for neatness, as we had done in Player. Let’s at least do this much:

...
    self.attackVerbs = self.mtEntry.attackVerbs or {"strikes at"}
    self:initAttributes()
    self.movingIndex = 1
...

function Monster:initAttributes()
    self._health = HealthAttribute(self:roll(self.mtEntry.health))
    self.strengthPoints = self:roll(self.mtEntry.strength)
    self.speedPoints = self:roll(self.mtEntry.speed)
end

Now we have the init nicely isolated and we can sort it. We’ll need to make a table to init the monster, since it has random attributes:

function Monster:initAttributes()
    local health = HealthAttribute(self:roll(self.mtEntry.health))
    local strengthPoints = self:roll(self.mtEntry.strength)
    local speedPoints = self:roll(self.mtEntry.speed)
    local attrs = { _health=health, strength=strengthPoints, speed=speedPoints }
    self._characterSheet = CharacterSheet(attrs)
end

I think that’ll be right. Now we have to fix the accessors:

function Entity:healthAttribute()
    return self._characterSheet._health
end

function Entity:health()
    return self._characterSheet:health()
end

function Entity:isAlive()
    return self._characterSheet:isAlive()
end

function Entity:isDead()
    return self._characterSheet:isDead()
end

function Entity:speed()
    return self._characterSheet:speed()
end

function Entity:strength()
    return self.characterSheet:strength()
end

function Entity:willBeAlive()
    return self._characterSheet:willBeAlive()
end

function Entity:willBeDead()
    return self._characterSheet:willBeDead()
end

I see we have a few more methods needed on CharacterSheet:

function CharacterSheet:isAlive()
    return self._health:isAlive()
end

function CharacterSheet:isDead()
    return self._health:isDead()
end

function CharacterSheet:willBeAlive()
    return self._health:willBeAlive()
end

function CharacterSheet:willBeDead()
    return self._health:willBeDead()
end

And I’m concerned about this method:

function Entity:healthAttribute()
    return self._characterSheet._health
end

Where’s that used? Here:

    self:drawText("Health")
    self:drawAttribute(self.healthIcon, m:healthAttribute())

We’ll let that ride for now. Let’s see what we’ve forgotten by running the program.

Entity:60: attempt to index a nil value (field '_characterSheet')
stack traceback:
	Entity:60: in method 'isDead'
	Player:106: in method 'drawExplicit'
	GameRunner:195: in method 'drawMap'
	GameRunner:181: in method 'drawLargeMap'
	GameRunner:160: in method 'draw'
	Main:29: in function 'draw'

That seems like I’d have had that working. Some tests failed as well. Let’s fix those first. Here’s the first one:

12: monster can't enter player tile even if player is dead -- Entity:88: attempt to index a nil value (field '_characterSheet')

The test is:

        _:test("monster can't enter player tile even if player is dead", function()
            local chosenTile
            local runner = Runner
            local monsterTile = Tile:room(10,10,runner)
            local monster = Monster(monsterTile, runner)
            local playerTile = Tile:room(11,10,runner)
            local player = Player(playerTile,runner)
            runner.player = player -- needed because of monster decisions
            chosenTile = monsterTile:validateMoveTo(monster,playerTile)
            _:expect(chosenTile).is(monsterTile)
            player:die()
            chosenTile = monsterTile:validateMoveTo(monster,playerTile)
            _:expect(chosenTile).is(monsterTile)
        end)

Certainly looks like we’re not initializing _characterSheet correctly somewhere, doesn’t it?

function Player:initRetainedAttributes(attrs)
    self.characterSheet = CharacterSheet(attrs)
end

That might be it.

function Player:initRetainedAttributes(attrs)
    self._characterSheet = CharacterSheet(attrs)
end

Also this needed its under-bar:

function Entity:strength()
    return self._characterSheet:strength()
end

12: monster can't enter player tile even if player is dead -- Entity:72: attempt to call a number value (method 'speed')

function Entity:speed()
    return self._characterSheet:speed()
end

I don’t see the problem:

function CharacterSheet:speed()
    return self.speedPoints
end

The message seems to think that the character sheet has a member variable speed that is a number, as if we had somehow stored into speed rather than speedPoints.

function Monster:initAttributes()
    local health = HealthAttribute(self:roll(self.mtEntry.health))
    local strengthPoints = self:roll(self.mtEntry.strength)
    local speedPoints = self:roll(self.mtEntry.speed)
    local attrs = { _health=health, strength=strengthPoints, speed=speedPoints }
    self._characterSheet = CharacterSheet(attrs)
end

That’s wrong, should be:

function Monster:initAttributes()
    local health = HealthAttribute(self:roll(self.mtEntry.health))
    local strengthPoints = self:roll(self.mtEntry.strength)
    local speedPoints = self:roll(self.mtEntry.speed)
    local attrs = { _health=health, strengthPoints=strengthPoints, speedPoints=speedPoints }
    self._characterSheet = CharacterSheet(attrs)
end

My tests are serving me pretty well so far, failing with good reason. Running again:

12: monster can't enter player tile even if player is dead -- Entity:7: attempt to index a nil value (field '_health')

That’s a method I failed to forward:

function Entity:accumulateDamage(amount)
    self._health:accumulateDamage(amount)
end

Let’s require the character sheet to deal with that.

function Entity:accumulateDamage(amount)
    self._characterSheet:accumulateDamage(amount)
end

function CharacterSheet:accumulateDamage(damage)
    self._health:accumulateDamage(damage)
end

12: monster can't enter player tile even if player is dead -- Entity:19: attempt to index a nil value (field '_health')

I must not have been as careful as I thought. I’ll fix this and search for other references to _health.

I find die and applyAccumulatedDamage, easily dealt with. And this:

function Player:addPoints(kind, amount)
    if kind == "Health" then
        self._health:addPoints(amount)
    else
        local attr = self:pointsTable(kind)
        if attr then
            local current = self[attr]
            self[attr] = math.min(20,current + amount)
        end
    end
    self:doCrawl(kind, amount)
end

All of that if needs to be pushed down:

function Player:addPoints(kind, amount)
    self._characterSheet:addPoints(kind,amount)
    self:doCrawl(kind, amount)
end

function CharacterSheet:addpoints(kind,amount)
    if kind == "Health" then
        self._health:addPoints(amount)
    else
        local attr = self:pointsTable(kind)
        if attr then
            local current = self[attr]
            self[attr] = math.min(20,current + amount)
        end
    end
end

function CharacterSheet:pointsTable(kind)
    local t = {Strength="strengthPoints", Speed="speedPoints"}
    return t[kind]
end

We’re down to two failed tests now. It’s 1030, by the way. The tests are driving my work. It’s an easy rhythm, but a bit tedious. I expected to be done by now.

18: TileArbiter: player can step on key and receives it  -- Actual: nil, Expected: 0

This test needs fixing:

        _:test("TileArbiter: player can step on key and receives it", function()
            local runner = Runner
            local room = Room(1,1,20,20, runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local kt = Tile:room(10,10, runner)
            local key = Key(kt,runner)
            _:expect(player.keys).is(0)
            _:expect(kt.contents).has(key)
            local arb = TileArbiter(key,player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(kt)
            _:expect(player.keys).is(1)
            _:expect(kt.contents).hasnt(key)
        end)

function Player:keyCount()
    return self._characterSheet:keyCount()
end

The OK method name is keyCount, so I’ll change the CharacterSheet tests and code accordingly.

function CharacterSheet:keyCount()
    return self.keys
end

Still one test fails:

18: TileArbiter: player can step on key and receives it -- Player:203: attempt to perform arithmetic on a nil value (field 'keys')

function Player:startActionWithKey(aKey)
    self.keys = self.keys + 1
    sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
    aKey:take()
end

Ah, yes.

function Player:startActionWithKey(aKey)
    self._characterSheet:addKey(aKey)
    sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
    aKey:take()
end

I’m passing it down because someday keys might be differentiated, and I’m thinking about the future. Perhaps too much?

function CharacterSheet:addKey(aKey)
    self.keys = self.keys + 1
end

Now isn’t there some place where we consume a key?

function Player:provideWayDownKey()
    if self.keys > 0 then
        self.keys = self.keys - 1
        return true
    else
        return false
    end
end

function Player:provideWayDownKey()
    return self._characterSheet:provideWayDownKey()
end

function CharacterSheet:provideWayDownKey()
    if self.keys > 0 then
        self.keys = self.keys - 1
        return true
    else
        return false
    end
end

Run tests. They all run. Now I think I’d best check to see if monster attribute tables display correctly. I can see that the princess’s does … well but when I step on a strength power-up:

Player:64: attempt to call a nil value (method 'addPoints')
stack traceback:
	Player:64: in method 'addPoints'
	Loot:27: in method 'actionWith'
	Player:204: in local 'action'
	TileArbiter:27: in method 'moveTo'
	Tile:90: in method 'attemptedEntranceBy'
	Tile:347: in function <Tile:345>
	(...tail calls...)
	Player:168: in method 'moveBy'
	Player:120: in method 'executeKey'
	Player:162: in method 'keyPress'
	GameRunner:277: in method 'keyPress'
	Main:33: in function 'keyboard'

Typo in method, no uppercase P.

Everything worked fine until I tried to enter a WayDown, at which point this happened:

CharacterSheet:81: attempt to index a nil value (field '_health')
stack traceback:
	CharacterSheet:81: in function <CharacterSheet:80>
	(...tail calls...)
	Player:98: in method 'drawExplicit'
	GameRunner:195: in method 'drawMap'
	GameRunner:181: in method 'drawLargeMap'
	GameRunner:160: in method 'draw'
	Main:29: in function 'draw'

That makes me think that something went wrong with the CharacterSheet when we spawn into the new level. This a bit of a gumption trap, and it’s nigh on to 1100, so I am on overtime.

Let’s see what the WayDown does.

function WayDown:actionWith(aPlayer)
    if aPlayer:provideWayDownKey() then
        self.runner:createNewLevel()
    else
        self.runner:addTextToCrawl("You must have a key to go further!")
    end
end

function GameRunner:createNewLevel()
    self.requestNewLevel = true
end

function GameRunner:playerTurnComplete()
    if self.requestNewLevel then
        self:createLevel(12)
        self.requestNewLevel = false
    else
        self.playerCanMove = false
        self.monsters:move()
        self.playerCanMove = true
    end
end

function GameRunner:placePlayerInRoom1()
    local r1 = self.rooms[1]
    local tile = r1:centerTile()
    self.player = Player:cloneFrom(self.player, tile,self)
end

Ah, we clone the player. No doubt that’s gone flaky.

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then player:initAttributes(oldPlayer) end
    return player
end

This is surely wrong, but I also think that I changed the semantics of initAttributes:

function Player:initAttributes(attrs)
    self.attributeSheet = AttributeSheet(self,750)
    self:initRetainedAttributes(attrs)
end

function Player:initRetainedAttributes(attrs)
    self._characterSheet = CharacterSheet(attrs)
end

Ah, we’re not doing anything with the attrs variable at all yet. But right now it’s not an attrs, it’s a player masquerading as attributes. Let’s forward this as we should:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then player:cloneCharacterSheet(oldPlayer) end
    return player
end

function Player:cloneCharacterSheet(oldPlayer)
    self._characterSheet = CharacterSheet:cloneFrom(oldPlayer._characterSheet)
end

function CharacterSheet:cloneFrom(oldSheet)
    return oldSheet -- no change at this time
end

I’m allowing the reference to the member variable in cloneCharacterSheet, since we are a Player and we know what our members are. One could argue against this but I’m feeling rushed. I think this works. I’ll have to find a WayDown to test it.

Works as intended. I should really write a test for that method.

        _:test("Clone CharacterSheet", function()
            local cs = CharacterSheet()
            cs:addPoints("Health", 5)
            _:expect(cs:health()).is(17)
            local clone = CharacterSheet:cloneFrom(cs)
            _:expect(clone:health()).is(17)
        end)

Bump the first sheet’s health, clone it, expect a bumped health. Perfect. Let’s commit: Entities now use CharacterSheet for all access to attributes.

Let’s sum up:

Summary

Well, we haven’t done our story, healing, yet, but we have managed to encapsulate all the character attributes into a separate class, CharacterSheet, which was the task we set ourselves for the morning. It did take rather longer than I had anticipated, and we should consider why.

The good news is that almost all the things that needed changing were triggered by tests. The only one I remember that wasn’t was cloning. So the tests did what tests are supposed to do, drive out a implementation that works.

However, the process was particularly tedious because of the way test results are displayed by CodeaUnit. It displays the result of every expect in the console, whether it succeeds or fails. There are about 180 of them in my tests, and when five of them fail, you have to scroll around trying to find them in that narrow little console. So it takes a while even to find the one you want to work on.

It turns out that I can somewhat improve that by setting

    CodeaUnit.detailed = false

In any test suite where I don’t want to see all the OKs. And that’s all of them at least for now. That reduces the clutter a lot and I wish I had remembered that fact. They’re set now, for next time. Commit: set all tests to detailed = false.

Tedious or not, the process worked pretty smoothly, with only one or two real surprises. But why were there so many changes?

There were so many changes because we didn’t have the CharacterSheet abstraction, and the result was that there were lots of little methods all over who were dealing with one little element of what would finally wind up in CharacterSheet. Had I seen that object trying to be born sooner, I’d have had less to change. Even so, it was only a hour or so of work, so not what you could call a “big refactoring”.

We’re a good step closer to being able to put in healing. Unless something catches my eye¹, we’ll do that tomorrow. See you then!

D2.zip