Dungeon 146
Deleting tests, not writing tests. A moral issue? Hardly. Also an interesting discovery about InventoryItems.
I mentioned in yesterday’s article that I’d ask the Friday Night Zoom Ensemble about deleting tests. It turns out that Chet and Bryan had had a little debate about that in a developer course they were teaching, where Bryan wanted to delete a test and Chet did not. So we were all pretty fresh on the subject.
We nearly agreed that when you write a test to figure out how some language feature works, you would probably be OK deleting it. Chet demurred, saying that if he forgot once, he’d forget again. Bryan imagined a whole bunch of tests all figuring out the same thing, because you’d never find the one you wrote before.
After that, we felt there wasn’t even that much of a general rule. We didn’t talk about redundant tests, but I’m pretty sure we’d have agreed to remove all but one of them. But imagining a test that actually checked some behavior in the system, we seemed to agree that there’s usually not much benefit to removing it, and there is a small but significant risk that if we remove it, something will break and slip into the release.
We didn’t discuss this, but suppose we write some generally useful capability, like my map and select functions, which do have tests. Now if we were to “lock” those functions, perhaps by putting them into a library and leaving the source out of our app, then we could certainly remove the tests from our app. But we’d probably keep them with the source for the library. Another obvious case but not on point.
Yesterday, I removed a test because I was removing the code it had driven out. Yes, that’s OK. But then I did a new version of the code, with a different calling sequence, and that’s still in the system. And there is no test for it.
Why? Because at the time I wrote the new code, it seemed to me that writing the test would be difficult, tedious, require lots of setup or the use of a number of test doubles. Why would it be like that? Because that part of the Dungeon system is too highly connected and it is therefore hard to test things in isolation.
But if that area is highly connected, it is more subject to mistakes. It needs more tests, not fewer. Why did I not write the test? Aww, Ronnie1, it was too haaard for poor wittle you?”
Yes, that was why. GeePaw Hill had a thing called “The Money Premise”, which I hope he has renamed. The point is, we do TDD because it lets us produce working code faster. If it didn’t do that, we wouldn’t do it. But TDD doesn’t always let us produce working code faster. Sometimes, and yesterday was one of those days, the creation of the test is so difficult, so time-consuming, that carefully writing the code we need is in fact the faster way to working code. In that instance.
So, when we’re as good (or at least as decent) at TDD as we are, we generally go faster with TDD and we generally use it. When we believe we won’t go faster, we don’t. Sometimes we’re right. Sometimes we’re horribly wrong. But we use the best judgment we have to find the balance.
Did I find the right balance yesterday? I don’t know. I did my best to find it.
But what about “professionalism”?
The notion of balance took us to a discussion of some big names in our business who present ideas like TDD as a kind of moral imperative. You can’t call yourself a professional, or a craftsone, or who knows, a human being, unless you use these practices right here which were handed down from the mountain on SOLID stone tablets from the gods.
We in the Ensemble do not buy that. There are no hard-and-fast rules that we’re aware of, such that if you don’t follow those rules you’re worse than the dust beneath our sandals.
We do these things because they let us do better work faster. We do feel good when we do better work faster. We do appreciate the joy of doing our work, our craft, well. We do not draw an authoritarian moral imperative. Here’s what we do. If it seems interesting, practice until you feel you’re good enough to decide about using these ideas. Use them if you wish.
That’s my report.
Inventory Items
Last night I was fiddling with Inventory and Decor. You recall that the current Inventory may show multiple copies of items if you happen to find more than one. We want it, instead, to show one icon for the thing, and if there’s more than one, a count.
So last night, I was toying with converting the current Inventory array into a hash table, which I would treat as a Bag (a set that allows multiple copies of its elements), represented by Item -> count.
The experiment nearly worked … but it shouldn’t have. An InventoryItem is a an object, a Lua table by another name. Tables are equal if they are identical, and not otherwise. So once I got my brain on line I was surprised to find that my bag worked, keeping all the items that were the same together.
Then I realized why. Here’s our Decor creation:
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
We start with five inventory items, two of which are interesting. Then we copy those items until we have n of them. We don’t duplicate the items, or create a new one each time. Instead we put the same items into the items table again and again.
That means that every Poison Antidote in the dungeon is the same object. Not a separate instance, the identical object.
It turns out that this seems to be harmless. However, if we ever do create two Poison Antidotes that are not identical, our inventory may wind up showing the two separately, unless I do something more clever with my Bag.
Now I think it is possible to give tables a function to decide whether they are equal. And I think, if our InventoryItems defined that function, they could be compared on their name rather than their identity.
So let’s look into that. A review of the Lua documentation tells me that if you define a “metamethod” __eq, it will be used to test objects for equality. Let’s test that, and let’s test it in a new project.
_:test("EQ", function()
local i1 = InventoryItem("fred")
local i2 = InventoryItem("fred")
local i3 = InventoryItem("wilma")
_:expect(i1, "wilma").isnt(i3)
_:expect(i1,"two freds").is(i2)
end)
InventoryItem = class()
function InventoryItem:init(name)
self.name = name
end
We want i1 and i2 to be equal because they have the same name. Test fails:
2: EQ two freds -- Actual: table: 0x282f38540, Expected: table: 0x282f38100
We implement this:
function InventoryItem:__eq(item)
return self.name == item.name
end
The test runs. Voila. We can enhance our real InventoryItem to check for equality by name, which will make our new bag idea independent of whether the items are unique or not.
Speaking of tests, I’m not going to delete this new tiny project, at least not for a while. And in a real product situation, I’d probably add the test to the real InventoryItem tests. I might even do that anyway, since we have tests.
On the other hand, Bryan would probably remove the little project, having learned what we set out to learn. Judgment call. Find your own balance.
Over in the Dung project, I look at the tests for InventoryItem and decide that we might as well put in a test for equality. Easily done, we just wrote one. Paste it in, edit to match the real object:
_:test("Inventory Item equality", function()
local i1 = InventoryItem{name="fred"}
local i2 = InventoryItem{name="fred"}
local i3 = InventoryItem{name="wilma"}
_:expect(i1, "wilma").isnt(i3)
_:expect(i1,"two freds").is(i2)
end)
It fails, of course. We implement __eq. Test passes.
Now let’s see about converting Inventory to a Bag structure.
There are at least two ways to go. One is to build an actual Bag class and use it. The other is to build into Inventory, whatever we need that class to do to allow us to display a single icon, with a count, for items we have more than one of.
I don’t expect to have a lot of use for Bags, and creating a general structure of that kind requires one to “do it right”, so let’s go with the second approach and enhance Inventory to do what we want.
Inventory
We had better review the whole class. Fortunately, it’s not very large:
Inventory = class()
local inventory = {}
local ItemWidth = 64
function Inventory:init()
assert(false, "do not instantiate inventory")
end
function Inventory:clear()
inventory = {}
end
function Inventory:add(item)
table.insert(inventory,item)
item:informObjectAdded()
end
function Inventory:count()
return #inventory
end
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
for i,item in ipairs(inventory) do
item:draw(self:drawingPos(i, self:count()))
end
popStyle()
popMatrix()
end
function Inventory:drawingPos(i, count)
return vec2(ItemWidth//2 + self:xPos(i,count), HEIGHT-ItemWidth//2)
end
function Inventory:remove(anItem)
item:informObjectRemoved() local index = 0
for i,thing in ipairs(inventory) do
if thing==item then index = i end
end
if index > 0 then table.remove(inventory,index) end
end
function Inventory:touched(aTouch)
for i,item in ipairs(inventory) do
item:touched(aTouch, self:drawingPos(i, self:count()))
end
end
function Inventory:xPos(itemNr, count)
local center = WIDTH//2
local adjustment = (count+1)/2
return center + (itemNr - adjustment)*ItemWidth
end
We could do this in at least two ways:
We could keep the linear array, but check it first for having an item, and if it does, keep the count in a separate table.
We could keep a hash table it item->count.
The latter makes more sense to me. I don’t think the tests are very robust on inventory. There’s really only one:
_:test("add items to inventory", function()
local i1 = InventoryItem{icon="green_staff"}
local i2 = InventoryItem{icon="snake_staff"}
_:expect(Inventory:count()).is(0)
Inventory:add(i1)
_:expect(Inventory:count()).is(1)
Inventory:add(i2)
_:expect(Inventory:count()).is(2)
end)
What do we use count for? We use it in the calculation for where on the screen the items go. So it needs to reflect the number of unique names in the set, not the total number of items we own. OK, then we should enhance that test this way:
_:test("add items to inventory", function()
local i1 = InventoryItem{icon="green_staff"}
local i2 = InventoryItem{icon="snake_staff"}
_:expect(Inventory:count()).is(0)
Inventory:add(i1)
_:expect(Inventory:count()).is(1)
Inventory:add(i2)
_:expect(Inventory:count()).is(2)
local i3 = InventoryItem{icon="green_staff"}
Inventory:add(i3)
_:expect(Inventory:count()).is(2)
end)
As one might imagine, that fails:
1: add items to inventory -- Actual: 3, Expected: 2
Now let’s convert to a hash-map style:
function Inventory:add(item)
inventory[item] = (inventory[item] or 0) + 1
item:informObjectAdded()
end
function Inventory:count()
count = 0
for i,item in pairs(inventory) do
count = count + 1
end
return count
end
I think this should work but I note that I didn’t really check subscripting in my EQ project. Run the test. I expect it to pass, and for the system not to work because none of the other methods are converted.
The test does not run. I’m going back to the EQ project
I didn’t really check how my inventory items would behave in a table, I just checked them for equality. That doesn’t change how the table stores them. Is there a way to do that?
Meh. If there is, it is seriously deep in the bag of Lua tricks. The name trick may be useful but I can’t just stuff the items into my inventory without regard to equality. But I do want to keep the count somewhere.
This is more tricky than I thought. I’m going to work this out in my EQ project.
What Are We Trying To Do?
I only have a vague understanding of what I want to accomplish. Looking at what the existing code does, with the new notion of only displaying one item of a given name, might help.
The purpose of the inventory is to display all the things you have, and to allow you, by touching on one, to invoke its behavior. The current rule is that using the item consumes it.
Items are the same based on their name. We want to display only one item of a given name, and the count of those that we have if we have more than one. We may at some future time display the name also.
The current scheme stores each item as if it were unique, so that there can be many Magic Jars in there.
I was thinking that maybe I could brute force it in the display loop, asking whether we had already displayed a given item and not displaying it again. But we’d also have to count them.
I worried for a moment about duplicates. Would we ever want two magic jars that look alike, have the same name, but are different in some important way? If that were the case, and there’s only one icon displayed, which one are you choosing. I conclude, no, that won’t happen. They’ll have to be slightly different and separately displayed, somehow.
For display I’d like to have a linear array of item/count. If the array isn’t linear I can probably calculate the display position on the fly.
For storage, I can’t just toss them into a hash table, because they don’t hash on name, they hash on identity.
But if we hash on name, where do we keep the count? We’d have to have the hash table point from name to InventoryItem, because that’s where the actions live.
We could have a structure pointed to by the name, like {item=theItem, count=theNumber}
Or we could have two tables. Let’s do the structure.
I’ll try this on for size:
_:test("Inventory structure", function()
local i1 = InventoryItem("fred")
local i2 = InventoryItem("fred")
local i3 = InventoryItem("wilma")
Inventory:clear()
Inventory:add(i1)
Inventory:add(i2)
Inventory:add(i3)
local fredCount = Inventory:count(i1)
_:expect(fredCount).is(2)
local wilmaCount = Inventory:count(i3)
_:expect(wilmaCount).is(1)
end)
This demands something like this:
function Inventory:add(item)
local entry = inventory[item.name]
if entry then
entry.count = entry.count + 1
else
inventory[item.name] = {item=entry, count=1}
end
end
function Inventory:count(item)
local entry = inventory[item.name]
if entry then return entry.count else return 0 end
end
I’m not deeply proud of this but it might work, and in fact it does. Let’s refactor a touch:
function Inventory:get(item)
local entry = inventory[item.name]
return entry or { item=item, count=0 }
end
function Inventory:put(entry)
inventory[entry.item.name] = entry
end
function Inventory:add(item)
local entry = self:get(item)
entry.count = entry.count + 1
self:put(entry)
end
function Inventory:count(item)
return self:get(item).count
end
This is arguably an example of the NullObject pattern, in that when it can’t find the item it returns an object that will perform as one.
The tests run.
I’d best do remove while I’m here. And since I’m using count to mean total count of unique names, maybe this count method should be renamed countItems. I decide to name it howMany(item):
_:test("Inventory structure", function()
local i1 = InventoryItem("fred")
local i2 = InventoryItem("fred")
local i3 = InventoryItem("wilma")
Inventory:clear()
Inventory:add(i1)
Inventory:add(i2)
Inventory:add(i3)
local fredCount = Inventory:howMany(i1)
_:expect(fredCount).is(2)
local wilmaCount = Inventory:howMany(i3)
_:expect(wilmaCount).is(1)
end)
function Inventory:howMany(item)
return self:get(item).count
end
I don’t have a test for zero. I’ll do that while I test remove.
_:test("Inventory structure", function()
local i1 = InventoryItem("fred")
local i2 = InventoryItem("fred")
local i3 = InventoryItem("wilma")
Inventory:clear()
Inventory:add(i1)
Inventory:add(i2)
Inventory:add(i3)
local fredCount = Inventory:howMany(i1)
_:expect(fredCount).is(2)
local wilmaCount = Inventory:howMany(i3)
_:expect(wilmaCount).is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1)).is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1)).is(0)
end)
This demands a remove:
function Inventory:remove(item)
local entry = self:get(item)
entry.count = entry.count -1
self:put(entry)
end
And an enhancement to put:
function Inventory:put(entry)
if entry.count > 0 then
inventory[entry.item.name] = entry
else
inventory[entry.item.name] = nil
end
end
Tests still run.
Now we still haven’t dealt with the touching and drawing, but we’ll save that for inside the real program.
I think I’d be happier if there was a tiny helper object instead of that raw table.
I start with this:
InventoryEntry = class()
function InventoryEntry:init(item)
self.item = item
self.name = item.name
self.count = 0
end
And plug it in:
function Inventory:get(item)
local entry = inventory[item.name]
return entry or InventoryEntry(item)
end
function Inventory:put(entry)
if entry.count > 0 then
inventory[entry.name] = entry
else
inventory[entry.name] = nil
end
end
That works. But here’s why I wanted to do that:
function Inventory:add(item)
self:put(self:get(item):increment())
end
function Inventory:remove(item)
self:put(self:get(item):decrement())
end
function InventoryEntry:increment()
self.count = self.count + 1
return self
end
function InventoryEntry:decrement()
self.count = math.max(0, self.count - 1)
return self
end
Tests still run. The helper object has made my main code a bit nicer.
This:
function Inventory:get(item)
local entry = inventory[item.name]
return entry or InventoryEntry(item)
end
Can be inlined to this:
function Inventory:get(item)
return inventory[item.name] or InventoryEntry(item)
end
I think I’d like to TDD the count function, since I think I’m going to need it. However, we want to move these tests and objects over, so maybe now is the time to do that. Then we can see what the context for count is.
It takes me a little while to integrate the code. I’d have done better to mimic the existing rules more carefully in the test, or to have moved the classes back and forth instead of trying to merge. No harm done.
Tests are running. But I’m sure touch and draw are not.
One detail is that I’ve left out the inform that I used to have in there.
function Inventory:add(item)
self:put(self:get(item):increment())
item:informObjectAdded()
end
OK. now I get the message about receiving a whatever, but (no surprise) the inventory doesn’t display. Let’s check draw:
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
for i,item in ipairs(inventory) do
item:draw(self:drawingPos(i, self:count()))
end
popStyle()
popMatrix()
end
Note that we need count to be the total number we’re going to draw, which is fine, but that we really can’t go by pairs.
We could change this to iterate over the inventory using pairs, and we already have a count that should work. Or we could produce the list that this code wants, an array of items.
Let’s do that:
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
for i,item in ipairs(self:items()) do
item:draw(self:drawingPos(i, self:count()))
end
popStyle()
popMatrix()
end
We’ll produce the items:
function Inventory:items()
local items = {}
for name,entry in pairs(inventory) do
table.insert(items, entry.item)
end
return items
end
This is a bit nasty because it’s going to happen a zillion times a second, but we’ll let it ride for now, and try to make it work, then make it right.
The display works as intended. Now what about touch:
function Inventory:touched(aTouch)
for i,item in ipairs(inventory) do
item:touched(aTouch, self:drawingPos(i, self:count()))
end
end
We can make that work the same way, horrid though it is.
function Inventory:touched(aTouch)
for i,item in ipairs(self:items()) do
item:touched(aTouch, self:drawingPos(i, self:count()))
end
end
I’m not sure what this will do and I’m getting hangry so I’m just going to run it.
Voila! When I touch an item of which I have only one, it takes effect and the icon vanishes from the inventory display. If I have more than one, it takes effect and the icon stays until I do it again.
This is working as intended. Now let’s make it more efficient, even though we do not have a performance measurement to tell us that we need to do that.
Inventory = class()
local inventory = {}
local linear = {}
local ItemWidth = 64
function Inventory:clear()
inventory = {}
linear = {}
end
function Inventory:put(entry)
if entry.count > 0 then
inventory[entry.name] = entry
else
inventory[entry.name] = nil
end
self:makeLinear()
end
function Inventory:makeLinear()
linear = {}
for name,entry in pairs(inventory) do
table.insert(linear, entry.item)
end
end
I think this does the trick. I am feeling the lack of a test, however. First, I’ll run the program. Program runs as intended.
But let’s do a test.
_:test("Inventory structure", function()
local i1 = InventoryItem{name="fred"}
local i2 = InventoryItem{name="fred"}
local i3 = InventoryItem{name="wilma"}
Inventory:clear()
Inventory:add(i1)
Inventory:add(i2)
Inventory:add(i3)
local fredCount = Inventory:howMany(i1)
_:expect(fredCount, "freds").is(2)
local items = Inventory:items()
_:expect(#items).is(2)
local wilmaCount = Inventory:howMany(i3)
_:expect(wilmaCount, "wilmas").is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1),"fewer freds").is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1),"no freds").is(0)
_:expect(#Inventory:items()).is(1)
end)
I’ve added two checks of items() to the above test. I’ve not checked to see if the caching works. If it doesn’t, the table, linear, should be different on two consecutive calls to items().
_:test("Inventory structure", function()
local i1 = InventoryItem{name="fred"}
local i2 = InventoryItem{name="fred"}
local i3 = InventoryItem{name="wilma"}
Inventory:clear()
Inventory:add(i1)
Inventory:add(i2)
Inventory:add(i3)
local fredCount = Inventory:howMany(i1)
_:expect(fredCount, "freds").is(2)
local items = Inventory:items()
_:expect(#items).is(2)
_:expect(Inventory:items()).is(Inventory:items()) -- <--
local wilmaCount = Inventory:howMany(i3)
_:expect(wilmaCount, "wilmas").is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1),"fewer freds").is(1)
Inventory:remove(i1)
_:expect(Inventory:howMany(i1),"no freds").is(0)
_:expect(#Inventory:items()).is(1)
end)
I added a new expect after the check for items equalling 2.
This is good and is working. One last check. I like it. Commit: Inventory now shows only one icon even if you have more than one of the item. Count not shown yet.
Now let’s do the count.
Oops. I got this far:
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
textMode(CENTER)
for i,item in ipairs(self:items()) do
local pos = self:drawingPos(i, self:count())
item:draw(pos)
if item.count then oops() end
end
popStyle()
popMatrix()
end
Then I realized that I don’t have access to the count of items from here. I need for linear to be made of entries, so that both the item and the count are available. Murr.
Change linear:
function Inventory:makeLinear()
linear = {}
for name,entry in pairs(inventory) do
table.insert(linear, entry)
end
end
Change draw:
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
textMode(CENTER)
for i,entry in ipairs(self:entries()) do
local pos = self:drawingPos(i, self:count())
entry.item:draw(pos)
end
popStyle()
popMatrix()
end
And touched:
function Inventory:touched(aTouch)
for i,entry in ipairs(self:entries()) do
entry.item:touched(aTouch, self:drawingPos(i, self:count()))
end
end
Test. It’s still good. Not surprised. Did have to change all the code to call entries instead of items of course. Now back to the text:
After some futzing, I settle on this:
function Inventory:draw()
pushMatrix()
pushStyle()
rectMode(CENTER)
spriteMode(CENTER)
textMode(CENTER)
for i,entry in ipairs(self:entries()) do
local pos = self:drawingPos(i, self:count())
entry.item:draw(pos)
if entry.count > 1 then
fill(255)
fontSize(36)
text(entry.count, pos.x+20,pos.y-15)
end
end
popStyle()
popMatrix()
end
That looks fairly good:
(I displayed counts of one for that picture, to save time testing. In the released version count only displays if two or more, as you can see from the code above.)
Commit: inventory displays count.
Let’s Sum Up
We modified our Inventory to behave like a bag, with items and counts thereof. We even did that with TDD. I had a little trouble integrating the code, because I wasn’t worrying about matching the existing code, but discovering how to do it. But then I pasted it into the real program, and it didn’t line up perfectly. Just a matter of minutes to line it up.
I think we have a solid capability here, and a charming new feature, an inventory that shows and uses the count.
In an early future day, we’ll want to remove all the loose things lying about and put all the good stuff into Decor. Unless we decide to leave some good stuff out. But I think not.
See you next time! Feel free to tweet me with questions or comments.
-
Never call me Ronnie. I will cut you. ↩