Today we’ll follow up an interesting idea from an idea source that is new to me. I expect that we’ll have an interesting and desirable outcome. Interesting, at least.

A few days back, I posted the Entity hierarchy on a Slack group that I belong to, asking, in essence, what’s so bad about this inheritance. No one really answered that but I got a fascinating suggestion from Beth:

I might see both the Princess and Monster classes having a Health class that they delegate taking damage to, which would end up with most of the shared logic. That would make sense if, for example, there was an effect that caused someone to take more or less damage, or if different monsters took different damage from different attack types.

Similarly, they might each have an Attack property, which could include the behavior of picking between the attack phrases. That would make something like “having different weapons” easy to implement.

But I think until you know which of those functions you might want to implement first, living with the One Big Class is fine.

I found the idea interesting but not compelling right away. I wasn’t sure whether the code really wants to be that way. But as I reflect further, I think there are signs. We have duplication, and we have feature envy, and we have primitive obsession. You’d think that even this blind pig would have found those acorns by now.

In my defense, I can only say that there are much worse problems in this code than what we’ll be looking at here. Now that I think about it, that’s not much of a defense, is it. But the fact is, I am not my code, and I feel no need to defend it, as code is a fact and needs no defense, and since I’m not the code, I’m not attacked, and need no defense either.

However, when we do find areas in our code that rather obviously could benefit from some improvement, there’s value to thinking about what in our habits may have allowed that particular thing to have happened. Often, it’s just “out of sight out of mind”, but sometimes, perhaps even usually, we did pick up on the clues, and then moved on, usually in aid of “going faster” and “we’ll clean that up later”. Now, if something’s going to be left alone in imperfect form, we may really “go faster” by leaving it in its present working but not lovely state. But “we’ll clean it up later”? That day rarely comes.

But there’s another day that often comes, the day when that not quite as good as it might be code comes up for attention. A story impacts that code. And that’s the day when we can “clean it up later”. See Refactoring - Not on the backlog! for some thoughts on that.

Signs and Portents

There were signs and portents about this code, whisperings on the wind, grumblings down in the deeper levels of the dungeon. To see what I mean, let’s look at the handling of Health.

function Entity:die()
    self.healthPoints = 0
    self.accumulatedDamage = 0
    self.alive = false
end

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self.healthPoints = self.healthPoints - amount
    self.accumulatedDamage = self.accumulatedDamage - amount
    if self.accumulatedDamage < 0 then self.accumulatedDamage = 0 end
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

function Entity:willBeDead()
    return self:isDead() or self.accumulatedDamage >= self.healthPoints
end

Here we see primitive obsession, since healthPoints and accumulatedDamage are just numbers. We see feature envy, just vaguely, such as in this pair of lines:

    self.healthPoints = self.healthPoints - amount
    self.accumulatedDamage = self.accumulatedDamage - amount

These lines must occur together (coupling!). They’re moving some amount of damage from the accumulated damage pile and subtracting it from health. That’s an idea. It’s not expressed very well in the code.

We also have duplication, such as this:

function Entity:health()
    return self.healthPoints
end

function Entity:speed()
    return self.speedPoints
end

function Entity:strength()
    return self.strengthPoints
end

And more feature envy:

function Entity:speedRoll()
    return math.random(1,self:speed())
end

Now, my faculties may be fading but I think these signs and portents are still pretty weak signal in a lot of noise. When these are the worst problems in my program, I’m going to be pretty happy. But today, we have some requirement that cause us to look more deeply at this code.

Stories

Careful readers may recall that there used to be a “healthMax” value, less than 20, representing an entities maximum possible health, and similarly for the other attributes.

(Note that “other attributes” is another sign, or maybe portent, telling us that there is an idea here that is not well expressed.)

My intention when that value existed was that as a player advanced, her ability to have more health, more strength, and so on, would increase. This would be part of leveling up. I removed that capability at the time I created the attribute sheet, mostly because I didn’t like the way the sheet looked, not what you’d call a great reason. On the other hand, the feature was pretty much unused, so deleting the code made some sense.

It also hid an important fact, which was that at that time there were two variables relating to e.g. health, health, and healthMax. And a third, magical value, 20, the max max. When we have related values, they are signals that an object is waiting to be born.

But the stories. Here are some capabilities I want in the system.

Power-ups
Change the power-ups we find to be temporary. Maybe they add 8 points to your strength, but that value slowly declines back down to your strength’s current maximum.
Healing
Change things so that while you’re just wandering, you slowly heal, your health growing back up to its current maximum.
Poison
Venomous creatures can poison the player, or otherwise cause their health, strength, and speed to decline over time.
Leveling up
Something, I’m not sure what, causes you to level up. When that happens, some amount is added to some or all of your attributes and their maxima.
More attributes / inventory
You might find a +2 Sword of Slashing, adding a +2 to the effect of all your attacks. This is a strength kicker. Your own strength hasn’t leveled up. You might find an anti-venom injector. When you’re bitten, you might apply that, or it might be automatically applied. You might find a +3 Shield of Impressive Fortitude, which subtracts 3 from the strength of some or all attacks against you. And so on

Of these ideas, the story we’ll start with is this:

The player has a maximum health, at the beginning of the game. She starts with health at that maximum. When she finds a heart (health) power-up, it adds to her current health, but then declines at some rate down to her current maximum. Similarly, when you’re just wandering, if health is low, it grows slowly back to the maximum.

Even this story can be split, and we will in fact split it as we go forward. But I think we understand the ideas, and we trust ourselves to do them bit by bit.

A Bit of Design

Let’s think a bit about what we’re going to do here. Based on Beth’s idea, we’re going to create a Health object. I foresee that there will be a Strength object and a Speed object, so that this thing will grow up into something more general, like an Attribute or Aspect, but we’re not into generalizing before we have cases, so let’s TDD up a HealthAttribute.

As is my present habit, I’m going to develop the tests and the code in the same Codea tab, because proliferation of tabs is harshing my mellow.

I don’t know yet what this thing has to do, so I’ll write the first part of the test, driving out the class shell:

        _:test("HealthAttribute", function()
            local ha = HealthAttribute()
            _:expect(ha).isnt(nil)
        end)

Not much of a test, but I’m just trying to come up to speed.

1: HealthAttribute -- Health:16: attempt to call a nil value (global 'HealthAttribute')

Not very surprising. This is sufficient to make it run:

HealthAttribute = class()

Whee. OK, what do we know? We know that it has a current value and a maximum value. And we know it’s going to do that accumulation thing. We’ll look at our real code to see what makes a decent protocol. In the end, we want to defer all kinds of things down to this attribute or to a collection of them, but it’ll take us a while to get there.

I think it takes a value and a maximum:

        _:test("HealthAttribute", function()
            local ha = HealthAttribute(5,10)
            _:expect(ha:value()).is(5)
            _:expect(ha:max()).is(10)
        end)

Not thrilling but it’s a start.

1: HealthAttribute -- Health:17: attempt to call a nil value (method 'value')

And …

function HealthAttribute:init(value, max)
    self._value = value
    self._max = max
end

function HealthAttribute:value()
    return self._value
end

function HealthAttribute:max()
    return self._max
end

I’m trying a new technique that I read in a book somewhere, starting private names with underbar. It should at least help with the fact that a nice name like value can’t appear both as a member variable and a method, since there’s only room for one key of a given name in a class.

I expect the test to run. And it does.

Now we’d better look at current usage and see where we want this object to go.

Monsters init like this:

...
    self.attackVerbs = self.mtEntry.attackVerbs or {"strikes at"}
    self.healthPoints = self:roll(self.mtEntry.health)
    self.strengthPoints = self:roll(self.mtEntry.strength)
    self.speedPoints = self:roll(self.mtEntry.speed)
...

And their health and such are basically the same as the player’s. But the player goes like this:

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, healthPoints = 12, speedPoints = 8, strengthPoints = 10 }
end

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

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

There’s some weirdness in player creation. For example:

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

When we create a new level, we create a new player by cloning the one we already have. Except that we don’t always have one:

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

So we create the new one, and if we have an old one, we copy the attributes from it. If we don’t have an old one, we have those initial attributes.

So it would appear that at the level of creation, if we said something like this, we’d be OK:

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

Of course we’d then have a HealthAttribute where we expect to have a number, so perhaps we’ll work toward this:

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

Saving the attribute in a thing called health makes sense. Or maybe we’ll even use _health. We’ll see how far this new habit goes. I’m not likely to convert everything, and there are issues of consistency. But for now, we’re just sorting out the health thingie.

Let’s see those accesses to healthPoints.

function Entity:die()
    self.healthPoints = 0
    self.accumulatedDamage = 0
    self.alive = false
end

That one is interesting. Is “alive” a property of health or not? The other two are clear. What else?

This next one is the damage taker that is called from the crawl. Damage has been accumulated in the accumulated damage variable, and we move it here to the creature, which makes its noises and such, updates the value so that it’ll show in the attribute sheet, and finally enqueues a message about being dead if one is found to be dead.

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self.healthPoints = self.healthPoints - amount
    self.accumulatedDamage = self.accumulatedDamage - amount
    if self.accumulatedDamage < 0 then self.accumulatedDamage = 0 end
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

Here’s an accessor. We’ll have to see who’s doing what, but we can clearly just forward it:

function Entity:health()
    return self.healthPoints
end

Here’s a cute one:

function Entity:willBeDead()
    return self:isDead() or self.accumulatedDamage >= self.healthPoints
end

This is used in Combat, because during the combat round we need to make decisions about whether the opponent will be dead when the system gets around to doing the damageFrom. This keeps the visuals and sounds in sync with the crawl, while a combat round has ended long ago in computer years.

function Monster:selectDamageTint()
    if self.healthPoints <= 2 then
        tint(255,0,0)
    else
        tint(255,255,0)
    end
end

Creatures are supposed to change color when damaged. I’m not sure if that’s still working or not. This can forward one way or another.

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

My, we find interesting things here. This is used by the power-ups:

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

Forwarding will work for that when the time comes. Clearly calls for a feature though:

        _:test("HealthAttribute", function()
            local ha = HealthAttribute(5,10)
            _:expect(ha:value()).is(5)
            _:expect(ha:max()).is(10)
            ha:addPoints(4)
            _:expect(ha:value()).is(9)
        end)

And …

function HealthAttribute:addPoints(points)
    self._value = self._value + points
end

But what if we add a lot of points? For sure we want them to max at 20. Magic number? I’m not sure.

        _:test("HealthAttribute", function()
            local ha = HealthAttribute(5,10)
            _:expect(ha:value()).is(5)
            _:expect(ha:max()).is(10)
            ha:addPoints(4)
            _:expect(ha:value()).is(9)
            ha:addPoints(14)
            _:expect(ha:value()).is(20)
        end)

This fails, as intended:

1: HealthAttribute  -- Actual: 23, Expected: 20

And:

function HealthAttribute:addPoints(points)
    self._value = math.min(self._value + points, 20)
end

Tests green. What else?

function Player:selectDamageTint()
    if self.healthPoints <= 2 then
        tint(255,0,0)
    else
        tint(255,255,0)
    end
end

That looks familiar, doesn’t it? We could push that up into Entity. And, because that’s the kind of person we are, we will:

I’m still not sure that feature is working but at least it’s not working in one place now.

Now let’s look at references to accumulatedDamage:

function Entity:accumulateDamage(amount)
    self.accumulatedDamage = self.accumulatedDamage + amount
end

I think we want the HealthAttribute to deal with this internally, so we need some testing.

        _:test("HealthAttribute accumulated damage", function()
            local ha <const> = HealthAttribute(5,10)
            _:expect(ha._accumulatedDamage).is(0)
            ha:accumulateDamage(7)
            _:expect(ha._accumulatedDamage).is(7)
        end)

I used the <const> feature for practice. Changed the other test too. I’m assuming, for now, that no one outside is going to need to know about accumulated damage, so my test will check the private. Coupling, but better that than providing a method, I think. I could be wrong.

This test drives out:

function HealthAttribute:init(value, max)
    self._value = value
    self._max = max
    self._accumulatedDamage = 0
end

function HealthAttribute:accumulateDamage(damage)
    self._accumulatedDamage = self._accumulatedDamage + damage
end

At this point, I want to see what happens when I plug this baby in. I can commit the class and tests first: Initial HealthAttribute. refactor damage tint upward.

I probably should do that in two commits. Deal with it.

I’m just going to slam the new class in here, and follow my nose:

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

I’ve renamed the member variable to _health, just to see how I feel about that, and now I’ll look for the methods accessing healthPoints again, to fix them up.

Ha!!! Right away I hit this one:

function Entity:die()
    self.healthPoints = 0
    self.accumulatedDamage = 0
    self.alive = false
end

Remember my talking about whether inheriting concrete methods from Entity into Monster and Player was righteous or not? The arguments against are arguments about what happens when things change. We’re faced with a bit of a dilemma here. We can start moving things down from Entity into Player, or we can bite the bullet and change Monsters to use the HealthAttribute right now. I’m going that way. I have a revert button right here.

In Monster:init, there’s this line:

    self.healthPoints = self:roll(self.mtEntry.health)

I’ll change that to this:

    self._health = HealthAttribute(self:roll(self.mtEntry.health))

This call does not include the second parameter to HealthAttribute. We can leave it nil, or do something more robust. I choose robust.

        _:test("HealthAttribute max defaults", function()
            local ha <const> = HealthAttribute(7)
            _:expect(ha:max()).is(7)
        end)

That fails, driving:

function HealthAttribute:init(value, max)
    self._value = value
    self._max = max or value
    self._accumulatedDamage = 0
end

Interesting Lua fact: in Lua, zero is true. Only nil and false are false.

Tests are green but the program explodes. Kind of expected, we need to go through references to healthPoints.

function Entity:die()
    self.healthPoints = 0
    self.accumulatedDamage = 0
    self.alive = false
end

I think we want this method in HealthAttribute:

function Entity:die()
    self._health:die()
    self.alive = false
end

And, OK, a test:

        _:test("HealthAttribute dies", function()
            local ha <const> = HealthAttribute(11,14)
            ha:accumulateDamage(4)
            ha:die()
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

And :

function HealthAttribute:die()
    self._value = 0
    self._accumulatedDamage = 0
end

I’m starting to wonder about those accessor methods. We may not need them at all. We’ll see, but if we’re going to use the new _private notion, we can use that in tests and have no accessors. Some simple searches can quickly find folks delving into the privates of others.

I expect the tests to be green. Yes. Moving right along:

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self.healthPoints = self.healthPoints - amount
    self.accumulatedDamage = self.accumulatedDamage - amount
    if self.accumulatedDamage < 0 then self.accumulatedDamage = 0 end
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

This stuff here is about applying accumulated damage, so we want to say this for the first bit:

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self._health:applyAccumulatedDamage(amount)
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

We require another test, and then I want to scream.

        _:test("HealthAttribute applies damage and dies", function()
            local ha <const> = HealthAttribute(11,14)
            ha:accumulateDamage(4)
            ha:applyAccumulatedDamage(1)
            _:expect(ha:value()).is(10)
            _:expect(ha._accumulatedDamage).is(3)
            ha:die()
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

I extended the existing one and adjusted its name.

4: HealthAttribute applies damage and dies -- Health:40: attempt to call a nil value (method 'applyAccumulatedDamage')

As expected. We code:

function HealthAttribute:applyAccumulatedDamage(amount)
    self._value = math.max(0, self._value - amount )
    self._accumulatedDamage = math.max(0, self._accumulatedDamage - amount)
end

Our new friend Beth is famous for saying “Do you have a test for that”, and I don’t have a test for those math.min calls there. I’ll do that and then I want to scream.

        _:test("HealthAttributes lower limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

This passes, of course, because my speculative math,min calls accomplish what’s shown in that test. The test also shows how the values could go negative. In principle, some ind of double attack could accumulate damage beyond the limitations of the entity under attack. So we show two attacks, and those attacks being “unwound” properly.

Subtle. Perhaps too subtle, but it’s all I’ve got just now.

Back to looking at .healthpoints references. This is dragging on too long but I don’t see a more incremental way to do. And the writing and testing is … oh, right, I wanted to scream.

Writing these tests is unquestionably slowing me down, by which I mean I feel certain that it’s slowing me down. It may or may not be. So far, I’ve not noticed them helping find problems, but they’ve certainly put things into my mind that I might not have otherwise considered, so they may be helping unnoticed. Still, I’m finding it draggy to write these tests. I’m writing them because I started with them, and now it only makes sense that if I add capability to HealthAttribute, I should reflect it in the tests, and drive it where possible.

But it makes me want to scream, or at least grumble, because it feels slow. There, I feel better now.

Moving right along … we still have this:

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self._health:applyAccumulatedDamage(amount)
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

I think we should ask the _health not what its points are, but whether the entity is dead.

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self._health:applyAccumulatedDamage(amount)
    if self._health:isDead() then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

And we test first:

        _:test("HealthAttributes lower limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:isDead()).is(true)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

5: HealthAttributes lower limits -- Health:54: attempt to call a nil value (method 'isDead')

And …

function HealthAttribute:isDead()
    return self._value == 0
end

Tests are green and the princess’s attribute sheet popped up, not quite right. We’re still hunting for references to healthPoints.

function Entity:health()
    return self.healthPoints
end

I’m going to allow this one in aid of moving forward:

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

What about this:

function Entity:selectDamageTint()
    if self.healthPoints <= 2 then
        tint(255,0,0)
    else
        tint(255,255,0)
    end
end

I think this is righteous where it is, but it should call health:

function Entity:selectDamageTint()
    if self:health() <= 2 then
        tint(255,0,0)
    else
        tint(255,255,0)
    end
end

function Entity:willBeDead()
    return self:isDead() or self.accumulatedDamage >= self.healthPoints
end

Piffle. Ask the health. We test:

        _:test("HealthAttributes lower limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            _:expect(ha:willBeDead()).is(true)
            _:expect(ha:isDead()).is(false)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:isDead()).is(true)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

5: HealthAttributes lower limits -- Health:52: attempt to call a nil value (method 'willBeDead')

I think this will do:

function HealthAttribute:willBeDead()
    return self._value - self._accumulatedDamage <= 0
end

Tests are green, use the function:

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

Just one more and it’s going to be tricky:

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

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

We’ve cleverly allowed folks to call a central method to add points. This is done here:

function FakePlayer:addPoints(kind, amount)
    _:expect(kind).is("Health")
    _:expect(amount).is(5)
end

function Loot:actionWith(aPlayer)
    self.tile:removeContents(self)
    aPlayer:addPoints(self.kind, math.random(self.min, self.max))
end

I think we’re just going to have to handle health separately for now:

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)
            self:doCrawl(kind, amount)
        end
    end
end

And I’ll remove the Health from here:

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

I think we can clean that up if and when we attribute the other values. I rather expect to be done now.

Something odd happened in this video:

fight

If you follow the narration, the ghost attacks after it is marked dead. I think I know what that is. We have the alive flag and the isDead method. Maybe isAlive too. That all needs to refer to the HealthAttribute now.

function Entity:die()
    self._health:die()
    self.alive = false
end

Remove the ref to alive.

function Entity:isAlive()
    return self.alive
end

function Entity:isDead()
    return not self:isAlive()
end

Refer to HA:

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

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

That requires a test, if you’re gonna be a fanatic about it.

        _:test("HealthAttributes lower limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            _:expect(ha:willBeDead()).is(true)
            _:expect(ha:isDead()).is(false)
            _:expect(ha:isAlive()).is(true)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:isDead()).is(true)
            _:expect(ha:isAlive()).is(false)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
        end)

function HealthAttribute:isAlive()
    return not self:isDead()
end

function HealthAttribute:isDead()
    return self._value == 0
end

I noticed that isDead is formally correct but arguably unsafe. It assumes an invariant1: _value >= 0. I think we enforce that, but it’s kind of by the way. We could bullet-proof with <= here, or we could do something about the invariant.

Now at first blush, the only subtraction in the HealthAttribute class is protected:

function HealthAttribute:applyAccumulatedDamage(amount)
    self._value = math.max(0, self._value - amount )
    self._accumulatedDamage = math.max(0, self._accumulatedDamage - amount)
end

But, in principle, someone could add negative points here:

function HealthAttribute:addPoints(points)
    self._value = math.min(self._value + points, 20)
end

If they did, they could drive _value negative. And who knows, there might be a time when we damage someone that way. OK, in for a penny, let’s write a test.

        _:test("HealthAttributes limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            _:expect(ha:willBeDead()).is(true)
            _:expect(ha:isDead()).is(false)
            _:expect(ha:isAlive()).is(true)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:isDead()).is(true)
            _:expect(ha:isAlive()).is(false)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
            ha:addPoints(-5)
            _:expect(ha:value()).is(0)
        end)

5: HealthAttributes limits  -- Actual: -5, Expected: 0

And …

function HealthAttribute:addPoints(points)
    self._value = math.max(0,math.min(self._value + points, 20))
end

Test runs.

Back to looking for the .alive flag.

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    self.alive = true
    self.tile = tile
...

Remove that.

function Monster:chooseAnimation()
    if self.alive then
        self.movingIndex = self.movingIndex + 1
        if self.movingIndex > #self.moving then self.movingIndex = 1 end
        self:setAnimationTimer()
    else
        self.sprite1 = self.dead
    end
end

Replace that:

function Monster:chooseAnimation()
    if self:isAlive() then
        self.movingIndex = self.movingIndex + 1
        if self.movingIndex > #self.moving then self.movingIndex = 1 end
        self:setAnimationTimer()
    else
        self.sprite1 = self.dead
    end
end

Should have been that way all along.

function Monster:chooseMove()
    -- return true if in range of player
    if not self.alive then return false end
    if self:distanceFromPlayer() <= 10 then
        self:moveTowardAvatar()
        return true
    else
        self:makeRandomMove()
        return false
    end
end

function Monster:chooseMove()
    -- return true if in range of player
    if not self:isDead() then return false end
    if self:distanceFromPlayer() <= 10 then
        self:moveTowardAvatar()
        return true
    else
        self:makeRandomMove()
        return false
    end
end

Further fixes:

function Monster:photo()
    if self:isAlive() then
        return self.moving[1]
    else
        return self.dead
    end
end

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

Remove that ref.

function Player:drawExplicit(tiny)
    local sx,sy
    local dx = 0
    local dy = 30
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local center = self:graphicCenter() + vec2(dx,dy)
    translate(center.x,center.y)
    if not self.alive then tint(0)    end
    sx,sy = self:setForMap(tiny)
    self:drawSprite(sx,sy)
    popStyle()
    popMatrix()
    if not tiny then
        self.attributeSheet:draw()
    end
end

Becomes:

function Player:drawExplicit(tiny)
    local sx,sy
    local dx = 0
    local dy = 30
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local center = self:graphicCenter() + vec2(dx,dy)
    translate(center.x,center.y)
    if self:isDead() then tint(0)    end
    sx,sy = self:setForMap(tiny)
    self:drawSprite(sx,sy)
    popStyle()
    popMatrix()
    if not tiny then
        self.attributeSheet:draw()
    end
end

The other references to .alive are in FakeEntity and I think they are OK. It’s a Fake. I expect things to work. I’m also wondering why I didn’t commit before.

I find an interesting: defect the dead monster follows me around. Somewhere we missed something that kept it from moving when dead.

Ha, tiny fool:

function Monster:chooseMove()
    -- return true if in range of player
    if not self:isDead() then return false end
    if self:distanceFromPlayer() <= 10 then
        self:moveTowardAvatar()
        return true
    else
        self:makeRandomMove()
        return false
    end
end

I converted to isDead but didn’t remove the not.

function Monster:chooseMove()
    -- return true if in range of player
    if self:isDead() then return false end
    if self:distanceFromPlayer() <= 10 then
        self:moveTowardAvatar()
        return true
    else
        self:makeRandomMove()
        return false
    end
end

I “should” have a test that would have detected that. But I haven’t. Let’s see how things go now:

Sheesh! I notice that my power-ups aren’t hitting the crawl. What’s up with that? “The changes I made couldn’t have affected that!”2

We did change the loots but where was the crawl stuff and where is it now?

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)
            self:doCrawl(kind, amount)
        end
    end
end

Ah. We don’t doCrawl on Health.

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

Yes, that’s all better now. Let’s commit: Health concept now uses HealthAttribute throughout.

But you know what? I wonder if the cloning works. Let’s look:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then player:initAttributes(oldPlayer) end
    return player
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.accumulatedDamage = 0
    self:initRetainedAttributes(attrs)
end

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

Yes, that’s going to work. But let’s write a test for that anyway.

No. It’s too hard to set up a decent Player, at least this morning. I’ll make a sticky note for it.

In manual testing to be sure that cloning works, which I hate because it requires me to find the WayDown, and I’m likely to get killed on the way, I notice that I’m still seeing attacks after a monster is dead.

Where do monsters attack?

I think it all comes down to this:

function GameRunner:initiateCombatBetween(attacker, defender)
    if defender:isDead() then return end
    local co = CombatRound(attacker,defender)
    self:addToCrawl(co:attack())

We’re not checking the attacker for being alive, And I think both of these should check for “will be dead” just in case.

function GameRunner:initiateCombatBetween(attacker, defender)
    if attacker:willBeDead() or defender:willBeDead() then return end
    local co = CombatRound(attacker,defender)
    self:addToCrawl(co:attack())
end

Now we already had this:

function CombatRound:attack()
    if self.attacker:willBeAlive() and self.defender:willBeAlive() then
        self:append(self:display("  "))
        local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
        self:append(self:display(msg))
        self:attemptHit()
    end
    return self:getCommandList()
end

So we shouldn’t be getting to these places anyway. Is there something wrong with the willBeAlive/Deadstuff?

I find this:

function Entity:willBeAlive()
    return not self:willBeDead()
end

But:

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

So that should be OK. But we’ll change it:

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

I don’t think that method exists, nor is it tested. I add that:

        _:test("HealthAttributes limits", function()
            local ha <const> = HealthAttribute(10)
            ha:accumulateDamage(6)
            ha:accumulateDamage(7)
            _:expect(ha:willBeDead()).is(true)
            _:expect(ha:willBeAlive()).is(false)
            _:expect(ha:isDead()).is(false)
            _:expect(ha:isAlive()).is(true)
            ha:applyAccumulatedDamage(6)
            ha:applyAccumulatedDamage(7)
            _:expect(ha:isDead()).is(true)
            _:expect(ha:isAlive()).is(false)
            _:expect(ha:value()).is(0)
            _:expect(ha._accumulatedDamage).is(0)
            ha:addPoints(-5)
            _:expect(ha:value()).is(0)
        end)

Conveniently that fails:

5: HealthAttributes limits -- Health:53: attempt to call a nil value (method 'willBeAlive')

And …

function HealthAttribute:willBeAlive()
    return not self:willBeDead()
end

function HealthAttribute:willBeDead()
    return self._value - self._accumulatedDamage <= 0
end

Tests run. But doubtless we still have the problem.

I feel sure that I just caused this problem, but I suppose it’s possible that it has been there all along and I just did’t notice.

If I don’t sort this soon, I’ll check out an old version and check. In fact, heck, let’s do that now. I commit first, of course. This is surely better if not perfect.

The defect seems not to occur in the old version. That’s curious.

The defect occurs in this morning’s first commit. That was pretty large, unfortunately, because HealthAttribute required a lot of fiddling. But I can check the diff between that and the one before, that did work.

I don’t see anything that could be affecting this. I see that we used to check the .alive flag to see if an entity was alive. That’s entirely gone now.

A judicious print statement tells me that when the princess and the monster are adjacent, and it’s the princess’s turn, and she moves toward the monster, a combat round of princess against monster is created, and immediately thereafter, a round of monster on princess.

I’ve determined that in the working version, an attack is immediately initiated as in the current version, but that it is refused by this code (now including some prints):

function CombatRound:attack()
    if self.attacker:willBeAlive() and self.defender:willBeAlive() then
        self:append(self:display("  "))
        local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
        self:append(self:display(msg))
        self:attemptHit()
    else
        print("attack refused ", self.attacker:willBeAlive(), self.defender:willBeAlive())
    end
    return self:getCommandList()
end

The attacker is flagged as false, i.e. will not be alive. Somehow our HealthAttribute isn’t reflecting that same result. Let me review the code while I have it checked out:

Oh my

function Entity:willBeAlive()
    return not self:willBeDead()
end

function Entity:willBeDead()
    return self:isDead() or self.accumulatedDamage >= self.healthPoints
end

We need to check for the isDead case in our new code, I’ll wager. Because if you are dead, you will be dead.

Let me check that back out,

This is the same, net of my debug prints:

function CombatRound:attack()
    if self.attacker:willBeAlive() and self.defender:willBeAlive() then
        self:append(self:display("  "))
        local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
        self:append(self:display(msg))
        self:attemptHit()
    end
    return self:getCommandList()
end

I’ve not looked but I’m already sure the code I’m about to look at is correct:

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

function HealthAttribute:willBeAlive()
    return not self:willBeDead()
end

function HealthAttribute:willBeDead()
    return self._value - self._accumulatedDamage <= 0
end

The not here is confusing me. I’m going to code the check directly:

function HealthAttribute:willBeAlive()
    return self._value - self._accumulatedDamage > 0
end

function HealthAttribute:willBeDead()
    return self._value - self._accumulatedDamage <= 0
end

I see no reason that that’s different.

Failure is still happening. This can only happen if accumulated damage isn’t working as I expect.

Look what I found:

function Entity:accumulateDamage(amount)
    self.accumulatedDamage = self.accumulatedDamage + amount
end

This never got forwarded. Fix:

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

And now I expect the problem will be gone. And wow, don’t I wish I had better tests at the Entity level?

Yes, upon testing, as expected, the defect is gone.

Let’s commit, then sum up. Commit: Fix defect where damage was not accumulated.

Summary

I think this went quite well up until I noticed that extra attack. And had I ever thought “the accumulated damage must not be right”, or checked it, I’d have found the problem right away. Basically a refactoring missed an item.

It turns out that I knew that accumulatedDamage was being relocated into HealthAttribute. But I forgot to remove the references. They’re still there now. I’ll remove them now and commit: remove references to member variable accumulatedDamage in Entities.

If I had decent tests for Entities and their damage, the tests would have found this, because I’d almost certainly have rolled damage against at least one Entity and checked if it was dead, nearly dead, or soon to be dead. However, I do not have decent tests for that.

This is a very important realization. I’ve been making excuses for not testing right along here. The code is obvious; the thing is too hard to test; it works and it’s not going to change; blah blah.

Fact is, it did change and it’s a righteous change, and my tests were not there to discover this defect. The result was that I spent about an hour or more, rummaging around until I arranged things so that it was obviously accumulated damage being wrong, after which I found the problem instantly.

The tests I needed couldn’t have taken an hour or more to write. To demonstrate that that’s true, I’ll write them tomorrow, I promise. In a larger system, this problem could have been harder to find and harder to notice. It was just chance that I noticed the crawl showing at attack after the monster was dead. It would have been easy to miss that and ship a weird bug. And, at least once during testing, the dead monster killed me! That would really tick off a real user. It even made me a bit sad.

Aside from that mistake, Beth’s idea seems to be playing out nicely. A lot of logic is now nicely packed inside the HealthAttribute, and I predict that we’ll see more improvements, based on this idea, in the future.

One more thing. Let’s go back and put that not back in here:

function HealthAttribute:willBeAlive()
    return self._value - self._accumulatedDamage > 0
end

That goes back to:

function HealthAttribute:willBeAlive()
    return not self:willBeDead()
end

Commit: refactor willBeAlive.

OK, a good idea, implemented fairly well by me. See you next time!

D2.zip

  1. An “invariant” is a property of some object, function, or patch of code that must remain true at all times, as seen from the outside. Here, we intend never to let the value decline below zero. ↩

  2. They did, though. ↩