Let’s try to finish moving InventoryItem to publish-subscribe, and maybe centralize Item creation. And look for Watch This near the end.

Well, that’s the plan. By “centralize”, I mean that InventoryItem instances are created wherever they are needed, by providing their tables literally at that point, and I’d like to get them all located together. That should make it easier to check them for correctness, and get rid of at least one case of duplication. It will also follow our style of having all the Monster definitions in one place.

But first, publish-subscribe. What Items are left that use the object-method form of notification when an Item is used?

Here’s one in Decor:

        InventoryItem{ icon="red_vase", name="Poison Antidote", object=self.player, method="curePoison" },

Decor also has these three “fake” ones, which act like nothing:

        InventoryItem{object=self.player},
        InventoryItem{object=self.player},
        InventoryItem{object=self.player},

We’ll need to look into how those even work. I suspect the lack of a “method” element makes them act like no operation, but allows for something to be there.

There are also two cases in Loot:

...
    elseif self.kind == "Antidote" then
        return InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
    end

Let’s do the curePoison first. Which reminds me of a target that I think we should have.

In publish-subscribe, when you sign up for an event, say “curePoison”, you can specify the method on yourself that you want called. I think this is unnecessary generality. Let’s move toward the rule that if the event is foo, and you sign up for event foo, your method foo will be called. That’ll reduce code complexity a bit, and save us from having to make up and remember quite so many names.

Moving right along. Our current convention, which we plan to simplify once we’re fully committed to the publish-subscribe model in InventoryItem, is that we use the attribute element of the table as the event to trigger. For example:

    if self.kind == "Pathfinder" then
        return InventoryItem{ icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" }

So we’ll change these two curePoison guys that way. I’m going to do them both at once, though I may regret that if I mess up.

        InventoryItem{ icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" },

Another thing I don’t like is providing the method here as a string rather than an actual function/method. I’ll make a card for that too. No, wait. The issue will go away when we have them all going to the Bus, because “publish” will be baked in. Never mind.

At this point, the “curePoison” should not work. I would do well to test that. As expected, the message that you’ve used the antidote comes out but not the message about having been cured of poison.

Who’s interested in this event? Well, whoever can be poisoned. The Player. We need to subscribe:

function Player:init(tile, runner)
    self.runner = runner
    self:initAttributes(self:retainedAttributes())
    self:initSounds()
    self:initVerbs()
    Bus:subscribe(self, self.spawnPathfinder, "spawnPathfinder")
    Bus:subscribe(self, self.curePoison, "curePoison")
    tile:moveObject(self)
    tile:illuminate()
end

Since our method was already curePoison, we’re good to go. Now to find one of the bloody things for a test. We must talk about this.

Well, I found about 51 Pathfinders before finding an Antidote, but as you can see here, it works:

cured message comes out

Commit: poison antidote InventoryItem uses publish-subscribe.

Sweet. Just one more, though we want to look at those no-op ones as well.

    elseif self.kind == "Antidote" then
        return InventoryItem{icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison"}
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
    end

I believe we have catered for this in yesterday’s changes to publish and subscribe. Let’s check:

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    for subscriber,method in pairs(self:subscriptions(event)) do
        method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
    end
end

Yes. We’re now sending each subscriber these arguments: self, event, senderOfEvent, and two parameters info1 and info2, which are defaulted to empty tables. (I think thats a crock, but we are here on a mission.)

How does InventoryItem use this facility?

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        Inventory:remove(self)
        self:informObjectRemoved()
        self:usedMessage()
        self.object[self.message](self.object, self.attribute, self.value1, self.value2)
    end
end

We pass in value1 and value2, which should, in this case, be the attribute e.g. “Health” and the rolled value.

Therefore:

        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}
    end

I feel a bit like I’m juggling here, having changed so many fields there, but I think that’s right. With this in place, Loot health items should have no effect. I’ll check that. (We must talk about this.)

They don’t work any more. Now to subscribe:

function Player:init(tile, runner)
    self.runner = runner
    self:initAttributes(self:retainedAttributes())
    self:initSounds()
    self:initVerbs()
    Bus:subscribe(self, self.spawnPathfinder, "spawnPathfinder")
    Bus:subscribe(self, self.curePoison, "curePoison")
    Bus:subscribe(self, self.addPoints, "addPoints")
    tile:moveObject(self)
    tile:illuminate()
end

Now it should work. Well, it doesn’t. I suspect addPoints has different expectations now. The error is:

CharacterSheet:67: attempt to index a nil value (field '?')
stack traceback:
	CharacterSheet:67: in method 'addPoints'
	Player:83: in local 'method'
	EventBus:107: in field '?'
	Inventory:271: in method 'touched'
	Inventory:205: in method 'touched'
	GameRunner:569: in method 'touched'
	Main:97: in function 'touched'

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

Well, to tell you the truth, I think that looks right. No, wait, there’s that fake sender that we send along now. Irritating, that.

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

That works:

speed added

health added

Commit: All InventoryItems now use Bus publish-subscribe.

That being the case, we can change InventoryItem to always use Bus:publish, and then I think we’ll improve the items.

That being the plan, let’s do it in another order. Presently we have those InventoryItem tables in Decor and in Loot.

Hm, I’ve started to wonder: how do Chests work? They all have a Health in them. And this code:

function Chest:open()
    if self:isOpen() then return end
    self.opened = true
    self.pic = self.openPic
    return Loot(self:getTile(), "Health", 4,10) -- returned for testing
end

This is a job for TileArbiter, I reckon.

    t[Chest][Player] = {moveTo=TileArbiter.refuseMove, action=Player.startActionWithChest}

function Player:startActionWithChest(aChest)
    aChest:open()
end

I guess the creation of that Loot is making the Health appear on that tile. Ah, yes, that finally drops down into the code we just made use Bus. So that’s OK. Back to the plan.

I’d like to have all the InventoryItem tables defined in InventoryItem. No one else should need to know anything but the name of the item table. We will have to provide separate entries for health, strength, and speed by the time we’re done.

I think we can do a bit of TDD here, to drive out the code for this.

InventoryItem Tables Centralized

We have a nice test fixture for InventoryItem, so we can write a new test:

        _:test("Pathfinder table", function()
            local t = InventoryTable:getTable("pathfinder")
            _:expect(t.attribute).is("spawnPathfinder")
        end)

I’ve decided there’s a new class, InventoryTable, that manages the input tables for Item creation. It has just the one public method, to get a table of a given name.

Let’s code, the minimum needed:

InventoryTable = class()

function InventoryTable:getTable(name)
    return { icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" }
end

OK, that’s not really the minimum, a table with just one element would have worked, but I’m here to get this job done and I fully intend to paste in all the items by the time I’m done.

Test oughta run. And it does, too. Extend test.

        _:test("Pathfinder table", function()
            local t
            t = InventoryTable:getTable("pathfinder")
            _:expect(t.attribute).is("spawnPathfinder")
            t = InventoryTable:getTable("curePoison")
            _:expect(t.attribute).is("curePoison")
        end)

Fails as expected:

7: Pathfinder table  -- Actual: spawnPathfinder, Expected: curePoison

Fix. Let’s be a bit obtuse, then talk about it:

function InventoryTable:getTable(name)
    if name == "pathfinder" then 
        return { icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" }
    else
        return { icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" }
    end
end

I expect this to work. And it does.

Now, you could make the case that we know we’re going to build a table, keyed by those names, and that we could have done it in response to this test. And you’d be right. Of course, since what we’re doing here is pretty simple, and pretty hard to get wrong, we could have just typed it in. But there’s a rhythm and a ritual to TDD, not done blindly, but done cunningly, with our mind fully engaged. The core idea is to make the simplest change possible to make the test run, and when it does run, refactor to make the code better.

In the case of what we’re doing here, the ritual isn’t of immense value, because after all building a table isn’t terribly hard, although I am pretty sure I could screw it up. But practicing the ritual is always useful, so that when the going gets tough, we’ll have it at our fingertips to go in tiny tiny simple simple steps.

So let’s do another item:

        _:test("Pathfinder table", function()
            local t
            t = InventoryTable:getTable("pathfinder")
            _:expect(t.attribute).is("spawnPathfinder")
            t = InventoryTable:getTable("curePoison")
            _:expect(t.attribute).is("curePoison")
            t = InventoryTable:getTable("rock")
            _:expect(t.attribute).is("dullSharpness")
        end)

And we’ll do it naively, sticking to our current pattern:

function InventoryTable:getTable(name)
    if name == "pathfinder" then 
        return { icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" }
    elseif name == "rock" then
        return { icon="rock", name="Rock", object=Bus, method="publish", attribute="dullSharpness", description="Mysterious Rock of Dullness", used="You have mysteriously dulled all the sharp objects near by." }
    else
        return { icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" }
    end
end

I expect this to work. And it does. Now let’s reflect on that code. We’ll pretend we don’t already know we want a table, we’ll pretend that we’re a beginner, and we have our favorite mentor looking over our shoulder and they say “I see some duplication there, do you?”

And we look and finally we say “the conditionals”? And our mentor says “Yes. And it’s just going get worse as we add items. Can you think of another way to do this?”

And because we are bright students, not rank beginners, and because we talked about tables just yesterday we say “We could use a table?” And our mentor says “Can we try it?”

And we say “OK” and after some messing about we have this:

local ItemTable = {
    pathfinder={ icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" },
    rock={ icon="rock", name="Rock", object=Bus, method="publish", attribute="dullSharpness", description="Mysterious Rock of Dullness", used="You have mysteriously dulled all the sharp objects near by." },
    curePoison={ icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" },
}

InventoryTable = class()

function InventoryTable:getTable(name)
    return ItemTable[name]
end

The test still runs. So we think maybe we’ll extend it. Or maybe we feel brave and we think we could incrementally change all the items as we go. But then we think, no, as things stand now we can finish this and be sure we’ve done no harm and commit it.

So we carry on as we were. And we realize we’re down to the “addPoints” one, and here we have a design decision to make. Loot is creating these tables dynamically:

        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}

It’s actually plugging in the icon, kind, and the number of points that the Item will provide. We could certainly save an instance of each kind, and let the use plug in a new value. Let’s do that. We may like it, or we may not.

We extend the test with three more items, because we’re feeling hot.

        _:test("Pathfinder table", function()
            local t
            t = InventoryTable:getTable("pathfinder")
            _:expect(t.attribute).is("spawnPathfinder")
            t = InventoryTable:getTable("curePoison")
            _:expect(t.attribute).is("curePoison")
            t = InventoryTable:getTable("rock")
            _:expect(t.attribute).is("dullSharpness")
            t = InventoryTable:getTable("health")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Health")
            t = InventoryTable:getTable("strength")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Strength")
            t = InventoryTable:getTable("speed")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Speed")
        end)

I guess I should check the value2 as well, since I think we should default it in case our callers forget.

            t = InventoryTable:getTable("health")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Health")
            _:expect(t.value2).is(1)
            t = InventoryTable:getTable("strength")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Strength")
            _:expect(t.value2).is(1)
            t = InventoryTable:getTable("speed")
            _:expect(t.attribute).is("addPoints")
            _:expect(t.value1).is("Speed")
            _:expect(t.value2).is(1)

This will fail a lot, but the first failure interests me:

7: Pathfinder table -- Inventory:106: attempt to index a nil value (local 't')

This is one of the reasons we do TDD. The tests sometimes tell us things we didn’t expect. This time, it’s telling me that when we give this thing a key it doesn’t know, it returns nil. We need to decide what it should do. We have those fake items that we already use, let’s see about those:

        InventoryItem{object=self.player},

Somehow those can pass right on through and nothing happens. Why?

Hm, this is a bit nasty. Here’s what happens:

function Inventory:add(item)
    self:put(self:get(item):increment())
end

function Inventory:get(item)
    return inventory[item.name] or InventoryEntry(item)
end
 
function InventoryEntry:increment()
    self.count = self.count + 1
    return self
end

function Inventory:put(entry)
    if entry.count > 0 then
        inventory[entry.name] = entry
    else
        inventory[entry.name] = nil
    end
    self:makeLinear()
end

These fake items do not have a .name field. So the fetches all try to fetch Inventory[nil], which is always nil, so they put whatever they put away at Inventory[nil]. But nil cannot be a table index, and so nothing ever goes into the table.

Wild. I think an empty table would work just as well. We’ll use that and see what happens. We also need to think about whether we’re comfortable with this rather slick but mysterious handling of an item with no name.

Anyway:

function InventoryTable:getTable(name)
    return ItemTable[name] or {}
end

Now we expect more interesting errors:

7: Pathfinder table  -- Actual: nil, Expected: addPoints
7: Pathfinder table  -- Actual: nil, Expected: Health
7: Pathfinder table  -- Actual: nil, Expected: 1

And so on, for Strength and Speed as well. So. We’re starting from this, in Loot:

InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}

We need to provide the right icon, kind, desc, value1, and the default 1 in value2. We can get all this info from Loot, which happens to know these things:

local LootIcons = {Strength="blue_pack", Health="red_vial", Speed="green_flask",
Pathfinder="blue_jar", Antidote="red_vase"}

local LootDescriptions = {Strength="Pack of Steroidal Strength Powder", Health="Potent Potion of Health", Speed="Potion of Monstrous Speed" }

We use those to set up our new table items:

local ItemTable = {
    pathfinder={ icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" },
    rock={ icon="rock", name="Rock", object=Bus, method="publish", attribute="dullSharpness", description="Mysterious Rock of Dullness", used="You have mysteriously dulled all the sharp objects near by." },
    curePoison={ icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" },
    health={icon="red_vial", name="Health", description="Potent Potion of Health", object=Bus, method="publish", attribute="addPoints", value1="Health", value2=1},
    strength={icon="blue_pack", name="Strength", description="Pack of Steroidal Strength Powder", object=Bus, method="publish", attribute="addPoints", value1="Strength", value2=1},
    speed={icon="green_flask", name="Speed", description="Spirits of Substantial Speed", object=Bus, method="publish", attribute="addPoints", value1="Speed", value2=1},
}

Tests all run. We can commit: InventoryTable can provide all necessary InventoryItem tables. Unused, initial commit.

Now let’s see how to use them. The trivial approach would be to change this:

        InventoryItem{ icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison" },

To this:

        InventoryItem(InventoryTable:getTable("curePoison"))

And that should in fact work. I’ll test it just for fun.

I’m glad I did. When I find one of those as a Loot, which is still using the old table, it works. But when I find one in Decor, I’m not getting the message out about receiving a poison antidote.

Why not? Did I somehow get the wrong item? It should be the same.

When I touch the one with no message, I get this:

Inventory:309: attempt to call a nil value (field '?')
stack traceback:
	Inventory:309: in method 'touched'
	Inventory:243: in method 'touched'
	GameRunner:569: in method 'touched'
	Main:97: in function 'touched'

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        Inventory:remove(self)
        self:informObjectRemoved()
        self:usedMessage()
        self.object[self.message](self.object, self.attribute, self.value1, self.value2) --< -- 309
    end
end

I am daunted, and I can feel gumption bleeding out of the wound in my mind. I see nothing for it but to dump out information during item creation to see what’s getting messed up. I thought I just pasted these things in: how could they be different?

After way too long, I determine the problem. Let me revert and then I’ll explain.

We, that is to say, I defined the ItemTable as a local literal. That means that it is compiled and stored before any game code runs. That means that Bus isn’t initialized. And we don’t send inform to any objects that don’t recognize it, and nil doesn’t. So we sent no inform messages.

One possible fix will be to lazy-init that table. The other would be to go ahead and change InventoryItem to assume unconditionally that it is going to talk to the Bus. That’s our plan anyway: I was just holding off until I’d converted over to using the new table.

So we’ll do it in the other order, and we’ll remove the reference to object and method in all the table entries and in InventoryItem etc:

local ItemTable = {
    pathfinder={ icon="blue_jar", name="Magic Jar", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" },
    rock={ icon="rock", name="Rock", attribute="dullSharpness", description="Mysterious Rock of Dullness", used="You have mysteriously dulled all the sharp objects near by." },
    curePoison={ icon="red_vase", name="Poison Antidote", attribute="curePoison" },
    health={icon="red_vial", name="Health", description="Potent Potion of Health", attribute="addPoints", value1="Health", value2=1},
    strength={icon="blue_pack", name="Strength", description="Pack of Steroidal Strength Powder", attribute="addPoints", value1="Strength", value2=1},
    speed={icon="green_flask", name="Speed", description="Spirits of Substantial Speed", attribute="addPoints", value1="Speed", value2=1},
}

I think that’s all of them removed from there. Removed a lot of duplication, matter of fact.

Now:

function InventoryItem:init(aTable)
    self.icon = aTable.icon
    self.name = aTable.name or self.icon
    self.object = aTable.object or self
    self.message = aTable.method or "print"
    self.description = aTable.description or self.name
    self.used = aTable.used or nil
    self.attribute = aTable.attribute or nil
    self.value1 = aTable.value or aTable.value1 or nil
    self.value2 = aTable.value2 or nil
end

We no longer need object or message/method:

function InventoryItem:init(aTable)
    self.icon = aTable.icon
    self.name = aTable.name or self.icon
    self.description = aTable.description or self.name
    self.used = aTable.used or nil
    self.attribute = aTable.attribute or nil
    self.value1 = aTable.value or aTable.value1 or nil
    self.value2 = aTable.value2 or nil
end

Now we have to change the references to object and message:

function InventoryItem:addToInventory()
    if self.icon then
        Inventory:add(self)
        self:informObjectAdded()
    elseif self.object then 
        self:informObject("You received nothing.")
    end
end

Here we’ll just change that to an else, because we’re using empty tables now.

function InventoryItem:addToInventory()
    if self.icon then
        Inventory:add(self)
        self:informObjectAdded()
    else
        self:informObject("You received nothing.")
    end
end

And:

function InventoryItem:informObject(msg)
    local implementsInform = self.object["inform"]
    if implementsInform then self.object:inform(msg) end
end

Becomes:

function InventoryItem:informObject(msg)
    Bus:inform(msg)
end

And:

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        Inventory:remove(self)
        self:informObjectRemoved()
        self:usedMessage()
        self.object[self.message](self.object, self.attribute, self.value1, self.value2)
    end
end

Tricksy Hobbitses, we’ll have to be careful here:

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        Inventory:remove(self)
        self:informObjectRemoved()
        self:usedMessage()
        Bus:publish(self.attribute, self.value1, self.value2)
    end
end

I think this must all still work, though we’re not using the table yet. A quick test assures me of this. Commit: InventoryItem now always uses Bus:publish and inform.

Now we can proceed, again, to using our new table:

        InventoryItem(InventoryTable("curePoison")),

It does work, but I forgot that two tests failed. I’ve committed two failing tests:

4: Inventory item always has target and message  -- Actual: nil, Expected: table: 0x2805b4540
4: Inventory item always has target and message  -- Actual: nil, Expected: print

Well, one test, two checks. This is no longer a requirement. Remove test. Commit: removed test no longer relevant. curePoison uses ItemTable.

Now, I don’t really like this format for the creation:

InventoryItem(InventoryTable("curePoison")),

Why should we have to do that? We should just pass in the name of the desired table, and let the object look up the table.

I can see two ways to go. One is that we can go ahead and do all these longhand, and then change the creation method and edit all these again. Or we can make the creation method accept either form, then change the items one at a time.

In this case, I’m inclined to just break the few remaining ones and then fix them. But let’s do it the more complex way. In a larger program we might want to defer changing all these entries until we were in the right modules, or for some other reason, so we might want both ways to work.

So in InventoryItem:init:

function InventoryItem:init(aTable)
    if type(aTable) == "string" then
        aTable = InventoryTable:getTable(aTable)
    end
    self.icon = aTable.icon
    self.name = aTable.name or self.icon
    self.description = aTable.description or self.name
    self.used = aTable.used or nil
    self.attribute = aTable.attribute or nil
    self.value1 = aTable.value or aTable.value1 or nil
    self.value2 = aTable.value2 or nil
end

This is about the best we can do in Lua. A more truly OO language might let us double dispatch there or something, but this is only temporary anyway.

Now I can fix the one:

        InventoryItem("curePoison"),

That should still work fine … and it does. Let’s commit that. I’m always trying to develop the habit of committing often. Commit: curePoison uses string creation in InventoryItem

Now let’s just rip thru and do them all:

function GameRunner:createDecor(n)
    local sourceItems = {
        InventoryItem("curePoison"),
        InventoryItem("pathfinder"),
        InventoryItem("rock"),
        --
        InventoryItem("nothing"),
        InventoryItem("nothing"),
        InventoryItem("nothing")
    }
    local items = {}
    for i = 1,n or 10 do
        table.insert(items, sourceItems[1 + i%#sourceItems])
    end
    Decor:createRequiredItemsInEmptyTiles(items,self)
end

Here are all the Decor ones. Let’s test those and then commit before doing the others. All seems fine, commit: Decor uses only string-indexed InventoryItems.

Now the Loot ones. I’m a bit concerned about those: I don’t think we’re setting up the value2 to anything other than 1. We have a bug in the commit queue, though we haven’t shipped it yet.

We must discuss this. But first we must fix it.

Looking at this:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "Pathfinder" then
        return InventoryItem{ icon="blue_jar", name="Magic Jar", object=Bus, method="publish", attribute="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature" }
    elseif self.kind == "Antidote" then
        return InventoryItem{icon="red_vase", name="Poison Antidote", object=Bus, method="publish", attribute="curePoison"}
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}
    end
end

I think we have no problem, yet, because we’re not yet using our new table entries. So there’s no bug that we know of, in the queue. Let’s keep it that way.

The first two are easy:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "Pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "Antidote" then
        return InventoryItem("curePoison")
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}
    end
end

Let’s test and commit. “Loot uses string creation for pathfinder and antidote”.

Now for that last one, all we have to do is cache the item and tell it to set value2. Or we could cache the table, edit it, and then create the item. The first way is better, because when we’re done, we’ll have isolated knowledge of the table and its format to the Inventory tab.

I have an idea. Let’s give a second, optional parameter to InventoryItem:init, a value to be given to value2. A bit odd, but lets us keep the object immutable, which is nice.

function InventoryItem:init(aTable, aValue2)
    if type(aTable) == "string" then
        aTable = InventoryTable:getTable(aTable)
    end
    self.icon = aTable.icon
    self.name = aTable.name or self.icon
    self.description = aTable.description or self.name
    self.used = aTable.used or nil
    self.attribute = aTable.attribute or nil
    self.value1 = aTable.value or aTable.value1 or nil
    self.value2 = aValue2 or aTable.value2 or nil
end

That’s actually fairly nice and it’s decently idiomatic Lua, as I see it.

Now, relying on that, we can change this:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "Pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "Antidote" then
        return InventoryItem("curePoison")
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}
    end
end

This is a bit tricky. I think that in Loot, self.kind may be the upper-case value of the desired field. Here’s some relevant code. From the learning level:

    local loot = Loot(lootTile, "Health", 5,5)
    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to add it to your inventory.")

And from regular levels:

local RandomLootInfo = {
    {"Strength", 4,9},
    {"Health", 4,10},
    {"Speed", 2,5 },
    {"Pathfinder", 0,0},
    {"Antidote", 0,0}
}

function GameRunner:createLoots(n)
    for i =  1, n or 1 do
        local tab = RandomLootInfo[math.random(1,#RandomLootInfo)]
        local tile = self:randomRoomTile(self.playerRoom)
        Loot(tile, tab[1], tab[2], tab[3])
    end
end

We can just change those first three, or all of them, to lower case. Let’s do that and fix Loot to expect that.

local RandomLootInfo = {
    {"strength", 4,9},
    {"health", 4,10},
    {"speed", 2,5 },
    {"pathfinder", 0,0},
    {"antidote", 0,0}
}

Better change the other one:

    local lootTile = r2:tileAt(self.dungeon, 2,2)
    local loot = Loot(lootTile, "health", 5,5)
    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to add it to your inventory.")

And then in Loot:

local LootIcons = {strength="blue_pack", health="red_vial", speed="green_flask",
pathfinder="blue_jar", antidote="red_vase"}

local LootDescriptions = {strength="Pack of Steroidal Strength Powder", health="Potent Potion of Health", speed="Potion of Monstrous Speed" }

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "antidote" then
        return InventoryItem("curePoison")
    else
        return InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=Bus, method="publish", attribute="addPoints", value1=self.kind, value2=math.random(self.min,self.max)}
    end
end

We should be fine for everything but the health, speed, and strength guys, which are still being done the old-fashioned custom-table way. Now let’s do those:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "antidote" then
        return InventoryItem("curePoison")
    else
        return InventoryItem(self.kind, math.random(self.min,self.max))
    end
end

That should work. Arrgh. Almost everything does, but when I opened a chest, which I haven’t done lately, I got this error:

Loot:55: expect userdata, got nil
stack traceback:
	[C]: in function 'sprite'
	Loot:55: in method 'draw'
	DungeonContentsCollection:113: in method 'draw'
	GameRunner:323: in method 'drawMapContents'
	GameRunner:285: in method 'draw'
	Main:87: in function 'draw'

I seem to recall that Chest creates its loot directly.

Right:

function Chest:open()
    if self:isOpen() then return end
    self.opened = true
    self.pic = self.openPic
    return Loot(self:getTile(), "Health", 4,10) -- returned for testing
end

We need to say lower-case “health” here. With that in place, substantial gameplay convinces me that almost all is well. Why “almost”? It appears that my open-field health loots are always giving me 10 points. The other guys seem to be OK.

Looking at the code, I don’t see how this can be happening. Let’s drop in a print.

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "antidote" then
        return InventoryItem("curePoison")
    else
        local rand = math.random(self.min,self.max)
        print(self.min,"<=",rand,"<=",self.max)
        return InventoryItem(self.kind, rand)
    end
end

It doesn’t seem to be happening. Maybe I just was hitting a lucky series or dice rolls. Remove the print, commit: all InventoryItems now created using table entry name.

Now we can remove the little hack from InventoryItem:

function InventoryItem:init(aString, aValue2)
    local aTable = InventoryTable:getTable(aString)
    self.icon = aTable.icon
    self.name = aTable.name or self.icon
    self.description = aTable.description or self.name
    self.used = aTable.used or nil
    self.attribute = aTable.attribute or nil
    self.value1 = aTable.value or aTable.value1 or nil
    self.value2 = aValue2 or aTable.value2 or nil
end

Let’s rename aTable:

function InventoryItem:init(aString, aValue2)
    local entry = InventoryTable:getTable(aString)
    self.icon = entry.icon
    self.name = entry.name or self.icon
    self.description = entry.description or self.name
    self.used = entry.used or nil
    self.attribute = entry.attribute or nil
    self.value1 = entry.value or entry.value1 or nil
    self.value2 = aValue2 or entry.value2 or nil
end

Commit: InventoryItem accepts only key strings for creation from InventoryTable.

Are we there yet? Yes, I believe we are. Let’s Summarizate.

Summarization

We set out to convert all the InventoryItems to use publish-subscribe through the Bus and to consolidate their definitions. We accomplished both of those things.

Oh! I just remembered something I spotted:

Watch This!

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "antidote" then
        return InventoryItem("curePoison")
    else
        local rand = math.random(self.min,self.max)
        return InventoryItem(self.kind, rand)
    end
end

There seems to be a bit of duplication here. Let’s make more duplication by changing the expected “antidote” to “curePoison”, by changing the input table:

local LootIcons = {strength="blue_pack", health="red_vial", speed="green_flask",
pathfinder="blue_jar", antidote="red_vase"}

To this:

local LootIcons = {strength="blue_pack", health="red_vial", speed="green_flask",
pathfinder="blue_jar", curePoison="red_vase"}

That requires this change:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem("pathfinder")
    elseif self.kind == "curePoison" then
        return InventoryItem("curePoison")
    else
        local rand = math.random(self.min,self.max)
        return InventoryItem(self.kind, rand)
    end
end

Wow, the duplication is getting deep in here. Let’s make it worse:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "pathfinder" then
        return InventoryItem(self.kind)
    elseif self.kind == "curePoison" then
        return InventoryItem(self.kind)
    else
        local rand = math.random(self.min,self.max)
        return InventoryItem(self.kind, rand)
    end
end

My god there’s a lot of duplication. Let’s make more!

function Loot:createInventoryItem(aPlayer)
    local rand = math.random(self.min,self.max)
    if self.kind == "pathfinder" then
        return InventoryItem(self.kind, rand)
    elseif self.kind == "curePoison" then
        return InventoryItem(self.kind, rand)
    else
        return InventoryItem(self.kind, rand)
    end
end

Encroyable! So much duplication! How can we get rid of it?

Like this:

function Loot:createInventoryItem(aPlayer)
    local rand = math.random(self.min,self.max)
    return InventoryItem(self.kind, rand)
end

And let’s inline that expression:

function Loot:createInventoryItem(aPlayer)
    return InventoryItem(self.kind, math.random(self.min,self.max))
end

We just reduced 8 lines that looked somewhat alike to one line that handles all the cases. We did it by making the lines more and more alike, until they were completely alike in all the branches, then we removed the branches.

The only “trick” to it was knowing that we can pass a value into the creation that isn’t needed, and it will do no harm. The rest was just “make these things more alike”.

It’s a good day when that opportunity arises, but it’s there more often than we probably recognize.

Continuing with the Summarizationing, we accomplished what we set out to, and more. The only glitch was that moment where I had referred to a missing Bus because the table is defined at compile time, and Bus isn’t. We resolved that easily, because we were going to make the object always use the Bus anyway.

A good morning. See you next time!

D2.zip