Dungeon 145
Some cleaning, I expect. And a user(!) has discovered a defect!
Yes, a user! “Ubergoober” from the Codea forum reported an interesting crash, from this code:
function Entity:speedRoll()
return math.random(1,self:speed())
end
The poor lad was poisoned, his speed had faded to zero, and the random limits were illegal.
What shall we do about this? And can it happen elsewhere? A quick search suggests that this is our only current case, though if we start rolling against other attributes we might be at risk again.
I’m wondering whether this code violates the Gentle Suggestion of Demeter.
function Entity:speed()
return self.characterSheet:speed()
end
function Entity:speedRoll()
return math.random(1,self:speed())
end
We could pass the speedRoll call down. I think not, at least not now. But what do we want the answer to be? Normally I wouldn’t expect a speed of zero, but we could change the roll to 0,speed, which wouldn’t ever fail. Let’s do that.
I have a small concern. Suppose at some future time we were to decide that you can never roll zero for speed. Might we put this bug back in? How can we prevent that?
Well, I suppose we could write a test. That would document the concern.
_:test("Cannot break entity speedroll", function()
local monster = Monster(FakeTile(), nil, nil)
monster:speedAttribute():die()
local roll = monster:speedRoll()
_:expect(roll).is(0)
end)
This fails as intended.
2: Cannot break entity speedroll -- Entity:96: bad argument #1 to 'random' (interval is empty)
Now we can fix the method:
function Entity:speedRoll()
return math.random(0,self:speed())
end
Tests run. Is the test enough to remind us, and robust enough to fail if we forget? I think it is. It will fail with the interval message if we forget. Good. Commit: fixed bug with speedRoll on poisoned princess.
OK, no matter what happens now, no one can say the day was a total loss.
What’s Next?
I have a sticky note here saying “No Double Decor in one tile”. Recall that we provide a list of tiles to the Decor creation code, thus:
function GameRunner:createDecor(n)
local sourceItems = {
InventoryItem{ icon="red_vase", name="Poison Antidote", object=self.player, method="curePoison" },
InventoryItem{ icon="blue_jar", name="Magic Jar", object=self.player, method="spawnPathfinder" },
InventoryItem{object=self.player},
InventoryItem{object=self.player},
InventoryItem{object=self.player},
}
items = {}
tiles = {}
for i = 1,n or 10 do
table.insert(tiles,self:randomRoomTile(666))
table.insert(items, sourceItems[1 + i%#sourceItems])
end
Decor:createRequiredItems(items,tiles)
end
function Decor:createRequiredItems(items, tiles, runner)
local result = {}
for i = 1, #items do
local item = items[i]
local tile = tiles[i]
table.insert(result, Decor(tile, item, runner))
end
return result
end
Let’s see what randomRoomTile does, while we’re at it. That calls down to here:
function Dungeon:randomRoomTile(roomToAvoid)
while true do
local pos = vec2(math.random(1, self.tileCountX), math.random(1,self.tileCountY))
local tile = self:getTile(pos)
if tile:getRoomNumber() ~= roomToAvoid and tile:isOpenRoom() then return tile end
end
end
function Tile:isOpenRoom()
local r
for x = -1,1 do
for y = -1,1 do
r = self:getNeighbor(vec2(x,y))
if r.kind ~= TileRoom or not r:isEmpty() then return false end
end
end
return true
end
This is a bit interesting, in that it actually checks the target tile in the middle of the loop, so it requires that the tile itself and all those around it be empty.
To tell the truth, I’m not clear in my mind this morning why I thought it was cool to pass in the rooms to the decor creation method. I think I just didn’t want to call back to the runner, but since we have the runner there anyway, we might as well fetch a room from it.
Oh, I remember, I wanted to be able to test without a runner. Meh, don’t care, let’s have a new method:
function GameRunner:createDecor(n)
local sourceItems = {
InventoryItem{ icon="red_vase", name="Poison Antidote", object=self.player, method="curePoison" },
InventoryItem{ icon="blue_jar", name="Magic Jar", object=self.player, method="spawnPathfinder" },
InventoryItem{object=self.player},
InventoryItem{object=self.player},
InventoryItem{object=self.player},
}
items = {}
for i = 1,n or 10 do
table.insert(items, sourceItems[1 + i%#sourceItems])
end
Decor:createRequiredItemsInEmptyTiles(items,self)
end
Let’s have a new method, leaving the old one in place because of the test. But the test needs to match our real use. One step at a time: we’re moving toward a better place, in small steps;
function Decor:createRequiredItemsInEmptyTiles(items, runner)
local result = {}
for i = 1, #items do
local item = items[i]
local tile = runner:randomRoomTile(666)
table.insert(result, Decor(tile, item, runner))
end
return result
end
I’m going to have to test this in the game, I think. It works as advertised, and now all the decor items occur separated from each other, since we require open cells in a 3x3 square.
Now let’s see if we can change our test, or whether we should remove it.
_:test("Decor with Inventory Items", function()
local i1 = InventoryItem{ icon="red_vase", name="Poison Antidote", method="curePoison" }
local i2 = InventoryItem{ icon="blue_jar", name="Magic Jar", method="spawnPathfinder" }
local someList= {i1,i2}
local rooms = map(someList, function(x) return FakeTile() end)
local items = Decor:createRequiredItems(someList, rooms, runner)
_:expect(items[1].item.name).is("Poison Antidote")
_:expect(items[2].item.name).is("Magic Jar")
end)
All this test does is check to be sure that we create items in order from the input list. It served to drive out the code. I’m going to remove it. Remind me to reflect when we’re done here.
I remove the test and the old method. Commit: fix problem where decor appeared too close together or on top of each other.
I noticed that Decor has a runner, but I think it doesn’t use it. Let’s check. I see no uses of it. Let’s remove it. This requires me to change all the creators.
function Decor:init(tile, item, kind)
self.sprite = kind and DecorSprites[kind] or Decor:randomSprite()
self.item = item
tile:moveObject(self)
self.scaleX = ScaleX[math.random(1,2)]
self.dangerous = math.random() > 0.5
end
Changing the creators was the work of moments, even with Codea’s lack of refactoring tools. Let’s test and commit: removed reference to Runner from Decor.
OK, a quick reflection:
Removing the test
First, removing the test. Historically, I’ve thought that tests should generally be kept. They are mostly harmless, and when they’re well-written, they won’t fail randomly, but will fail if whatever they test gets broken somehow. Some smart people disagree with this position, arguing that once an automated test has shown that something works, it is essentially useless. This is clearly false, but I’d agree that they are nearly useless, because almost never do we break something that works. But note the “almost never”.
My tests on this program are not as strong as I could wish, but they do break sometimes, and when they do, it’s usually something important. By “usually”, I suppose I mean “I remember times when it was something important”. So I don’t like to remove them.
Some of my colleagues are more willing to remove them than I am. Maybe we’ll talk about that tonight in our Zoom call. If we do, I’ll tell you what was said. Anyway, I didn’t see the value to changing that test, so I removed it.
Improving the Code
But now we have an issue. I want to improve this code:
function Decor:createRequiredItemsInEmptyTiles(items, runner)
local result = {}
for i = 1, #items do
local item = items[i]
local tile = runner:randomRoomTile(666)
table.insert(result, Decor(tile, item))
end
return result
end
The code has no test. It works now, but is it safe to change it? It’s hard to imagine what kind of “safety” there is in changing a bunch of code that isn’t under test.
Let me make the change I want to make, and then let’s look at it:
function Decor:createRequiredItemsInEmptyTiles(items, runner)
return map(items, function(item)
local tile = runner:randomRoomTile(666)
return Decor(tile,item)
end)
end
A quick run of the program tells me that this works. I could gin up a test for it, passing the function in, perhaps, but all that would do is test that map works. But map has its own test.
I don’t see how to write an informative test of this: it’s too obvious. (You may disagree. If so, please get in touch.)
I’m committing: refactor createRequiredItemsInEmptyTiles to use table map.
Now What?
We are moving toward leaving no loot lying about other than inside Decor. We also want to have some Decor be dangerous. Poisoning seems too much. How about if we just deal a little damage?
The only way we damage the player now is via CombatRound, which includes this method:
function CombatRound:applyDamage(damage)
self.defender:accumulateDamage(damage)
local op = { op="extern", receiver=self.defender, method="damageFrom", arg1=nil, arg2=damage }
self:append(op)
self:append(self:display(self.defender:name().." takes "..damage.." damage!"))
if self.defender:willBeDead() then
local op = OP("extern", self.defender, "youAreDead", self)
self:append(op)
local msg = self.defender:name().." is down!"
self:append(self:display(msg))
self.defender:playerCallback(self, "playerIsDead")
end
end
We have an example of how to use this, in Spikes:
function Spikes:actionWith(player)
local co = CombatRound(self,player)
co:appendText("Spikes ".. self.verbs[self.state]..player:name().."!")
local damage = math.random(self:damageLo(), self:damageHi())
co:applyDamage(damage)
self.tile.runner:addToCrawl(co:getCommandList())
end
Let’s mimic that (which reminds me, we need Mimics):
function Decor:actionWith(aPlayer)
if self.dangerous then
self:damage(aPlayer)
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(aPlayer)
local co = CombatRound(self,aPlayer)
co:appendText("Ow! Something in there is sharp!")
local damage = math.random(0,aPlayer:health()//2)
co:applyDamage(damage)
self.tile.runner:addToCrawl(co:getCommandList())
end
Let’s check that out.
Perfect. Right now, half of the Decor is dangerous:
function Decor:init(tile, item, kind)
self.sprite = kind and DecorSprites[kind] or Decor:randomSprite()
self.item = item
tile:moveObject(self)
self.scaleX = ScaleX[math.random(1,2)]
self.dangerous = math.random() > 0.5
end
That should do for now, they’re not deadly, just harmful. Commit: Dangerous Decor deals damage up to half health.
OK, I think that’ll do for the morning. Let’s sum up.
Summary
We fixed a defect, and added a test for the case. We improved Decor allocation to avoid clumps, and refactored the creation method down to a single call to map. And we changed Decor damage to deal health damage rather than poison the player.
I just had a neat idea. What if, instead of always damaging health, messing with decor could gas you, making you lethargic, affecting speed, or weakening you, affecting strength? That could be fun, although those attributes aren’t much used at present.
I would have done that right now, but CombatRound can only deal health damage, so we’ll make a note for this idea.
It seems to me that the only questionable thing that happened today was that I removed one test, and converted a loop to a map call without a test. Was that bad practice?
I don’t know. It was the best I had at the time. We’ll see whether we get in trouble, or don’t.
See you next time!