‘Nothing at all’, the game says. What’s up with that? And various musings.

When cruising the dungeon as an extensive test after yesterday’s many small steps and one largish one to a simpler publish-subscribe scheme, I found that a “??” query in certain locations scrolled a message “I am a valuable nothing at all”. This morning, in one careful scan of a whole room, I was unable to duplicate the issue, but I feel sure that it’s probably still in there.

“Feel sure, probably”. Well, anyway, I think it’s a small defect but a confusing one for the users if we had any users, and we should root it out.

Let’s search for “nothing at all”, which I think is a description of a DungeonObject or something.

Yes. In the item table that defines such things, we find this:

    catPersuasion = {
        icon="gray_amulet", 
        name="Precious Amulet of Feline Persuasion", 
        description="Precious Amulet of Feline Persuasion", 
        attribute="catPersuasion"},
    nothing = {
        name="nothing",
        description = "nothing at all"},

The idea is that you can tweak the odds of things appearing by providing a list of things that you want created, including “nothing”. Like this:

function DungeonBuilder:placeDecor(n)
    local sourceItems = {
        "catPersuasion",
        "curePoison",
        "pathfinder",
        "rock",
        "health",
        "speed",
        "nothing",
        "nothing",
        "nothing"
    }
    local loots = {}
    for i = 1,n or 10 do
        local kind = sourceItems[1 + i%#sourceItems]
        local loot = Loot(nil, kind, 4,10)
        table.insert(loots, loot)
    end
    Decor:createRequiredItemsInEmptyTiles(loots,self)
end

So we are placing some Decor with real items but 1/3 of them will have “nothing”.

Ah. In further play, I think I’ve found a way to reproduce the problem. When I bumped a skeleton that happened not to contain anything, and then did the “??”, I got the message. Therefore, the Decor has rezzed a “nothing” loot. When the “??” is issued, that Loot is answering the query. It shouldn’t do that, but it should exist so that it can be given to us and we get the message “You received nothing”.

So, Loot.

function Loot:init(tile, kind, min, max)
    self.kind = kind
    self.min = min
    self.max = max
    self.item = self:createInventoryItem()
    self.desc = self.item:getDescription()
    self.message = "I am a valuable "..self.desc
    if tile then tile:moveObject(self) end
end

function Loot:getIcon(kind)
    return self.item:getIcon()
end

function Loot:query()
    Bus:informDotted(self.message)
end

A quick look into history back1 tells me that this code hasn’t changed for a long time. I think what did change was that we used to just give the Loot when the Decor was bumped, and we changed it to display the Loot, like a Chest does, because we liked the look of it.

You may not be surprised to hear that there aren’t really any tests for Loot. We could fix this bug readily without a test. All it takes is for the Loot to emit no message if it is “nothing”.

For my sins, I’ll write a test. To do it, I’ll need a fake bus, I think, to determine that the call to Bus is not made when the Loot is a nothing. I think that’s the price we pay for the indirection which EventBus allows.

Here goes:

local busMessage

local FakeBus = class()

function FakeBus:informDotted(aMessage)
    busMessage = aMessage
end

function testLoot()
    CodeaUnit.detailed = false
    
    _:describe("Loot", function()
        
        _:before(function()
            _bus = Bus
            Bus = FakeBus()
        end)
        
        _:after(function()
            Bus = _bus
        end)
        
        _:test("Loot messages", function()
            local speaking = Loot(nil, "health", 0,0)
            local silent   = Loot(nil, "nothing", 0,0)
            speaking:query()
            _:expect(busMessage).is("I am a valuable health")
            busMessage = nil
            silent:query()
            _:expect(busMessage).is(nil)
        end)
    end)
end

That might actually fail as I expect it to, which is to say expected nil got “I am a valuable nothing at all”. Test to see why I’m wrong.

I am almost correct:

1: Loot messages  -- 
Actual: I am a valuable Potent Potion of Health, 
Expected: I am a valuable health
1: Loot messages  -- 
Actual: I am a valuable nothing at all, 
Expected: nil

I’m curious where that expansive message came from. Ah, it comes from the call to getDescription in the init. Fine, we can accommodate that with a little cut and paste.

        _:test("Loot messages", function()
            local speaking = Loot(nil, "health", 0,0)
            local silent   = Loot(nil, "nothing", 0,0)
            speaking:query()
            _:expect(busMessage).is("I am a valuable Potent Potion of Health")
            busMessage = nil
            silent:query()
            _:expect(busMessage).is(nil)
        end)

Now I should just get the failure that I want:

1: Loot messages  -- 
Actual: I am a valuable nothing at all, 
Expected: nil

Now we cannot just create a nil message and pass it to informDotted, because the crawl doesn’t expect people to pass it nils. So I think that the best thing to do is simply to check for “nothing” and skip the call to the Bus. We could do some clever double dispatch or something but it seems overly complicated.

Well, I do have another idea but let’s fix the bug first.

function Loot:query()
    if self.kind ~= "nothing" then
        Bus:informDotted(self.message)
    end
end

Test runs. Commit: Fix defect where “nothing” Loot would announce itself on a query.

My “another idea” was this. Our Loot contains an InventoryItem here:

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

We could defer the messaging to the item. Might be overkill, but why are we asking the item all these questions instead of letting it speak for itself. So let’s explore InventoryItem a bit.

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

And then we have this:

function InventoryItem:getDescription()
    return self.description
end

Which is used only by Loot, and only in the init, where it is used to set up the message.

However, there is a method on loot, setMessage which is used in DungeonBuilder, to allow building Loots with special messages, for the learning level. We’ll want to cater for that.

We could simplify Loot by deferring these messages to InventoryItem, of course at some cost in InventoryItem. But it would centralize the creation of these human-readable messages a bit. That seems good.

Why does deferring to the Item seem right to me? Because Loot is exhibiting “feature envy”. It is violating the Law of Demeter. It is grabbing bits of information out of the Item and then using them to create strings and send out bus messages. When one object rips out another’s innards, that’s a sign that the second object should probably be providing a service to the first, rather than raw data.

I think the change won’t be terribly difficult

Spoiler: it turns out to be more than I was envisioning here, but still not awful. But see the bit about Learning Level below.

I don’t think I’d like to undertake this change without a test, but since we have a test, the refactoring should be quite nicely guided by the test. Neat!

Let’s try it.

Change our query:

function Loot:query()
    if self.kind ~= "nothing" then
        Bus:informDotted(self.message)
    end
end

To defer to our item:

function Loot:query()
    self.item:query()
end

This will break our test, how nice that we have it, because the item doesn’t understand query.

1: Loot messages -- 
Loot:21: attempt to call a nil value (method 'query')

So in InventoryItem:

function InventoryItem:query()
    Bus:informDotted("got here")
end

I expect both expectations in my test to fail now.

1: Loot messages  -- Actual: got here, 
Expected: I am a valuable Potent Potion of Health
1: Loot messages  -- Actual: got here, 
Expected: nil

Now make the query get closer to handling the usual case:

function InventoryItem:query()
    Bus:informDotted(self.description)
end

That will be closer on the first expectation.

1: Loot messages  -- 
Actual: Potent Potion of Health, 
Expected: I am a valuable Potent Potion of Health

We should call for the longer message, which now has to be created here.

function InventoryItem:longMessage()
    return "I am a valuable "..self.description
end

function InventoryItem:query()
    Bus:informDotted(self:longMessage())
end

This should fix the first test, unless I’m missing a period or something. Perfect:

1: Loot messages  -- 
Actual: I am a valuable nothing at all, 
Expected: nil

Now for the non-message, we’ll modify this just slightly.

function InventoryItem:query()
    if self.name ~= "nothing" then
        Bus:informDotted(self:longMessage())
    end
end

Test runs. We can delete getDescription. and the call to it in Loot, and the creation of the message in Loot. Test still runs.

However, we’ll have lost the special message from the Learning Level. I’l first verify that in game. Correct. I get the default message, not the intended longer one:

We need a test for that. Here’s a bit of the code that is trying to set the learning message:

    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.")

So we want the Loot to forward this to its item. But first, let’s write the test!

        _:test("Can override a Loot's Item's message", function()
            local overridden = Loot(nil, "health", 0,0)
            overriden:setMessage("A new message")
            overridden:query()
            _:expect(busMessage).is("A new message")
        end)

This should fail with the old standard health message.

Well it would except for the typo. That word is too hard to type.

        _:test("Can override a Loot's Item's message", function()
            local overridden = Loot(nil, "health", 0,0)
            overridden:setMessage("A new message")
            overridden:query()
            _:expect(busMessage).is("A new message")
        end)

2: Can override a Loot's Item's message  -- Actual: I am a valuable Potent Potion of Health, Expected: A new message

We change Loot’s method from:

function Loot:setMessage(message)
    self.message = message
end

To:

function Loot:setMessage(message)
    self.item:overrideMessage(message)
end

And if we test now we’ll “discover” that the item doesn’t grok that.

2: Can override a Loot's Item's message -- 
Loot:23: attempt to call a nil value (method 'overrideMessage')

Now we’ll have to be a bit more clever than we were in InventoryItem. We’ll compute the long message in init and then just use it in query, and, of course, override it in override.

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
    self.message = self:longMessage()
end

function InventoryItem:longMessage()
    return "I am a valuable "..self.description
end

function InventoryItem:overrideMessage(aString)
    self.message = aString
end

function InventoryItem:query()
    if self.name ~= "nothing" then
        Bus:informDotted(self.message)
    end
end

Tests run. Verify in game just for fun. I am totally sure it’ll work.

message

Here we see the message coming out as anticipated. That’s good, and today’s task is done. Let’s sum up.

Summary

I was sure this defect with the message was trivial, and it was. It came about because we rez the Loot, where formerly we just handed it out when you stepped on Decor.

Writing the test surely made it take longer to fix the defect, since it was a simple if to fix it. However, when we looked at Loot, we saw it ripping all kinds of information out of the InventoryItem, which already knew almost everything the Loot did, and that wasn’t good design.

I could have done it all without my new test, but I’m quite glad that I wrote it, because it kept me right on point, and meant that I didn’t have to wander around in the game looking for objects to query. I originally wrote the test just because I felt that I “should”, not the best reason ever, but I do know that tests are generally helpful.

What was quite pleasant was that the test made the fix clearly a fix, and then enabled the refactoring that pushed message creation down to InventoryItem, where it seems to fit better.

A nice little outcome.

There is an odd bug tho. Did you spot it in the picture?

See you next time!


  1. Sic. This is an obscure reference to a movie. You win a valuable nothing if you can identify the movie. ↩