Maybe I’ll fiddle some more with InventoryItem, Loot, and Decor. Depends what time breakfast is.

Reviewing InventoryItem, we find that the array of keyed values that creates one can contain:

  • icon: a Sprite, or possibly an actual graphical asset
  • name: a short name such as “Rock”
  • description: a longer name, such as “Mysterious Rock of Dullness”
  • object: an object upon which a method will be called when the Item is touched in inventory
  • method: the method that will be called
  • attribute: first parameter to the method
  • value: second parameter to the method
  • used: a message to be displayed after the generic “used” message.

Not all of these key-value pairs are required. The icon is needed, so that the Item can display. It’s generally a string, and will be used as the default name if none exists. The object’s init is this:

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
    self.value = aTable.value
end

I think I’ll make those last two explicitly say “or nil”. They’ll wind up nil anyway, but I think there’s value to being explicit that nil is OK. Commit: Added explicit or nil to init for clarity.

Let’s review some of the flow for an Item.

When, for example, a Decor gives an item to the Player, it starts this sequence:

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

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

function InventoryItem:informObjectAdded()
    self:informObject("You have received a "..self.description.."!")
end

So you’ll get a message with the description if the Item has one, and the name if not.

When you touch an Item in inventory, to use it, this happens:

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.value)
    end
end

function Inventory:remove(item)
    self:put(self:get(item):decrement())
end

function InventoryItem:informObjectRemoved()
    self:informObject("You have used a "..self.description.."!")
end

function InventoryItem:usedMessage()
    if self.used then
        Bus:inform(self.used)
    end
end

This will put out two messages if there is a used item in the Item’s table, such as

You have used a Mysterious Rock of Dullness.
You have mysteriously dulled all the sharp objects near by.

The first message always comes out. The second is optional.

Hmm …

I’m noticing some asymmetry here. When we add the item, the inform is triggered in Inventory:add, but when we remove it, the inform is triggered in touched. (The whole contents of touched needs to be a method, I believe.)

Be that as it may, it seems like either the Item should do the informing in both cases, or the Inventory should.

I also notice this little batch of methods:

function InventoryItem:informObjectAdded()
    self:informObject("You have received a "..self.description.."!")
end

function InventoryItem:informObjectRemoved()
    self:informObject("You have used a "..self.description.."!")
end

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

This will send those messages to the object in the Item’s table. So far we have only two such objects, the Player and the Bus. They both implement inform:

function Player:inform(message)
    Bus:inform(message)
end

function EventBus:inform(message)
    if type(message) == "string" then
        message = splitStringToTable(message)
    end
    for i,msg in ipairs(message) do
        Bus:publish("addTextToCrawl", self, {text=msg})
    end
end

So, bottom line, the messages come out. What else do we ask the object to do? We wind up sending it the method. In existing InventoryItems, we find these possibilities:

object method
player curePoison
player spawnPathfinder
Bus publish
player addPoints

Adding health or speed or strength to the player might as well be sent to player, as should curePoison. It doesn’t make quite as much sense for the player to spawn the Pathfinder, but it is spawned right next to the player, so it sort of makes sense.

I don’t see much value to sending the inform messages to the player to get them sent to the Bus. I suppose the player could, in principle, modify the message.

The newest InventoryItem, the Mysterious Rock of Dullness, just sends the message “dullSharpness” via the Bus, and anything that knows how to become dull can subscribe and deal with the message. In a full implementation, I imagine that only sharp objects within some range of the player would be dulled. Or perhaps the dullness would depend on distance, so close ones are totally dulled, but ones further away just reduce their damage capability rather than reduce it to zero.

Anyway, do we want to untangle this inform asymmetry?

Let’s do. Let’s remove the call from Inventory:add, and put it into the Item:

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

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

Test. At first all seems well, but then I notice that I’m not getting messages when I pick up loot. Things go into inventory, but no messages come out. Did I just break that?

Let’s revert and find out.

Right. Loot now does display its message, where formerly it didn’t. Better look and see how Loot works.

function Loot:addLootToInventory(aPlayer)
    local item = InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
    Inventory:add(item)
end

Right. Should tell the item to add itself.

function Loot:addLootToInventory(aPlayer)
    local item = InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
    item:addToInventory()
end

Now put that other change back. I should have stashed it or some clever Git thing like that.

Weird. Now the Magic Jar and Pathfinder thing don’t display.

function Loot:actionWith(aPlayer)
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        self:addPathfinderToInventory(aPlayer)
    elseif self.kind == "Antidote" then
        self:addAntidoteToInventory(aPlayer)
    else
        self:addLootToInventory(aPlayer)
    end
end

Ah, those other two functions didn’t get converted. Duh.

function Loot:addPathfinderToInventory(aPlayer)
    local item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    item:addToInventory()
end

function Loot:addAntidoteToInventory(aPlayer)
    local item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    item:addToInventory()
end

What we have just encountered is one of those things that goes wrong when there’s duplication. The

Inventory:add(item)

Occurred in three places and until I changed all of them, not everything worked. I wonder if there are more … no, there seem not to be.

If we had had only one call, instead of three, we’d have only had this problem once instead of thrice. Of course a more powerful editor, or a more powerful programmer, might have found this the first time, but the fact that I tolerated this tiny bit of duplication is implicated in the difficulty.

Let’s commit what we have and then see if we can improve this. Commit: move calls to inform about inventory additions and removals all into InventoryItem.

Let’s see what we have here:

function Loot:addLootToInventory(aPlayer)
    local item = InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
    item:addToInventory()
end

function Loot:addPathfinderToInventory(aPlayer)
    local item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    item:addToInventory()
end

function Loot:addAntidoteToInventory(aPlayer)
    local item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    item:addToInventory()
end

Those are called from here:

function Loot:actionWith(aPlayer)
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        self:addPathfinderToInventory(aPlayer)
    elseif self.kind == "Antidote" then
        self:addAntidoteToInventory(aPlayer)
    else
        self:addLootToInventory(aPlayer)
    end
end

To make this better, let’s make this worse. Let’s inline all those add methods:

function Loot:actionWith(aPlayer)
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        local item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
        item:addToInventory()
    elseif self.kind == "Antidote" then
        local item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
        item:addToInventory()
    else
        local item = InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
        item:addToInventory()
    end
end

This should be a pure refactoring. Testing says OK. We could commit.

Remove duplicate occurrences of local:

function Loot:actionWith(aPlayer)
    local item
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
        item:addToInventory()
    elseif self.kind == "Antidote" then
        item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
        item:addToInventory()
    else
        item = InventoryItem{icon=self.icon, name=self.kind, description=self.desc, object=aPlayer, attribute=self.kind, value=math.random(self.min,self.max), method="addPoints"}
        item:addToInventory()
    end
end

Easy enough. Could commit. Remove duplicate addToInventory:

function Loot:actionWith(aPlayer)
    local item
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    elseif self.kind == "Antidote" then
        item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    else
        item = 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
    item:addToInventory()
end

Still good. Could commit. Extract the if nest:

function Loot:actionWith(aPlayer)
    local item
    self:getTile():removeContents(self)
    item = self:createInventoryItem(self.kind)
    item:addToInventory()
end

function Loot:createInventoryItem(kind)
    if kind == "Pathfinder" then
        return InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    elseif 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
end

I kind of wish I had committed earlier. The Loot messages have stopped coming out. I’m going to back these changes out and do again.

This version works:

function Loot:actionWith(aPlayer)
    local item
    self:getTile():removeContents(self)
    if self.kind == "Pathfinder" then
        item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    elseif self.kind == "Antidote" then
        item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    else
        item = 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
    item:addToInventory()
end

I honestly don’t see what I did wrong in the extract. I guess I’ll just do it again, more slowly.

I tried again, and again it didn’t work. Ah. I see the problem at last. We need to pass the aPlayer parameter into our function.

Darn nils. OK, this works, we’re back on the rails:

function Loot:actionWith(aPlayer)
    local item
    self:getTile():removeContents(self)
    item = self:createInventoryItem(aPlayer)
    item:addToInventory()
end

function Loot:createInventoryItem(aPlayer)
    local item
    if self.kind == "Pathfinder" then
        item = InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    elseif self.kind == "Antidote" then
        item = InventoryItem{icon="red_vase", name="Poison Antidote", object=aPlayer, method="curePoison"}
    else
        item = 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
    return item
end

Remove duplication:

function Loot:createInventoryItem(aPlayer)
    if self.kind == "Pathfinder" then
        return InventoryItem{icon="blue_jar", name="Magic Jar", object=aPlayer, method="spawnPathfinder", description="Magic Jar to create a Pathfinding Cloud Creature"}
    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
end

Everything is working. Commit: refactoring Loot to remove harmful duplication.

It’s Sunday, and Fathers’ Day, and breakfast is coming right along. Let’s sum up.

Summary

There are a few lessons here, both small and large.

A smaller one is that unnoticed duplication can cause us trouble. We could “be more careful”, but that trick never works. When we change the “right” protocol to do something, and we miss a change, bad things will happen.

A larger lesson is that, at least in Lua, extracting a method is tricky, especially when it’s formatted as weirdly as our InventoryItem lines are. It was difficult to notice that aPlayer was in there, although a more clever programmer might have wondered sooner why the parameter was sent if it was never used.

A more powerful refactoring tool would have told us to pass aPlayer down, and we’d have been fine. But our refactoring tool is my brain, and it is only as powerful as it is.

We could, however, beef up InventoryItem to require the object parameter to be non-nil. I’ve made a card for that.

An even larger lesson is that we can always find code that can be improved, and if it’s code we’re actually working with, that improvement can pay off. We don’t want to be gold-plating, but it’s clear that today’s little task would have gone much faster had those Inventory:add calls not been duplicated.

Will it pay off to have removed the duplication, after finding and fixing them all? I’m not sure, but I am sure that it pays off often enough that I wish I were more regular about doing it.

And here’s a small but important lesson: if I had committed every time I said “I could commit now”, things would have gone faster. I did at least two reversions by carefully using “undo”. A quick press of the revert button would have sped me up quite a bit.

Anyway, a fun morning. See you next time!

D2.zip