Well, with that musing out of the way, what shall we program today?

Let’s have a look at the cards. Yes, I actually converted my yellow sticky-notes to cards:

picture of cards with notes on them

Here are some interesting ones, and a few uninteresting as well, so you can get a sense of what I write down:

  • More flexible descriptions of Loot / InventoryItem. See article 201.
  • Duplication of description of “Magic Jar”. See “spawnPathfinder” in Loot and Decor.
  • Advance the Learning Level.
  • “Interface” for e.g. all dungeon objects, “must have query or other required methods.
  • Doors (wall pop up / go down?)
  • Floor switch tile (drawing of the pushbutton tile)
  • Announcing tools need enhancement?
  • Can the test using runCrawl be simplified? Coroutine still there?
  • Use Announcer for Initial Dungeon message. (This one is done, yay, tear up card.)
  • AdjustedSprite not same as other sprites. Eliminate “duplication”.
  • Poison from - what sources - needs improvement
  • Story - Game needs one - Purpose - Side Quests

And so on. I have the luxury, of course, or prioritizing whatever I want to do, and leaving things I don’t want to do until forever. In a real product we’d not have that luxury. But we’re here to have fun and to learn. Yay, us!

I think we should start with a review of yesterday’s work, which was substantial. I’m sure we’re left now with a more difficult way of creating our triggered announcers, and there may be improvements to the actual code as well.

Recall that we have a series of tiny objects that result in interesting behavior, namely a bunch of tiles such that if you step on any one of them, a common message comes out once and never again, even if you step on others, unless you press the History button, in which case all such messages for the level come out, in chronological order.

The objects that do that go like this:

Trigger -> OneShotTrigger -> CachingTrigger -> Announcer

These can be fit together in other ways. You could put a OneShotTrigger in a cell, and it would trigger whatever it was pointed at, just once. And so on.

But to use these, we currently write code like this:

function Room:announce(dungeon, messageTable)
    local tile
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann, Announcer.cache)
    local one = OneShotTrigger(cac)
... use Trigger(one) each time you want to put the
same trigger in another place.

So that’s not entirely easy to do. And it’s Very Easy to get wrong: I’ve already done it two or three times, typically by referring not to the thing just created but to the thing I’m about to create, like:

    local cac = CachingTrigger(ann, Announcer.cache)
    local one = OneShotTrigger(one)

That works poorly, to say the least.

We can make that less likely with some asserts.

function CachingTrigger:init(target,cachingMethod)
    self.target = assert(target, "expected a target")
    assert(type(cachingMethod) == "function", "expected a cache-fetching function")
    self.cacheMethod = cachingMethod
end

I don’t usually do that sort of thing. I generally feel that if I have solid tests, a problem like accidentally failing to provide parameters won’t happen. But here it seemed valuable, because errors were happening, and because these objects are intended to be used over and over, in new situations, which means that we’ll probably make the same old mistakes again.

Let’s add a few, just to see how they look:

function Announcer:init(messageTable)
    self.messageTable = assert(messageTable, "must provide message table")
end

function OneShotTrigger:init(target)
    self.target = assert(target, "OneShot must have a target")
    self.shouldTrigger = true
end

function Trigger:init(target)
    self.target = assert(target, "Trigger must have a target")
end

I think I’ll improve that first one:

function Announcer:init(messageTable)
    self.messageTable = assert(messageTable, "Announcer must have message table")
end

Test. Test runs. Test tells me that I don’t like this message in the Learning Level:

    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to receive its benefit.")

First commit the init changes: added asserts to inits in the Trigger objects.

The message above is wrong, should be:

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

Commit: improve learning level health powerup message to refer to inventory

Small Improvements, Little Jolts of Joy

As I wrote about in this morning’s other article, I like to get little jolts of satisfaction by doing something good and getting it into the product. Noticing the problem with that message, I could have written a story, or entered something into Jira. Maybe someday someone would fix it. Or I could just fix it. It took two minutes from the previous commit, counting the time to say I was going to do it and what I did.

I think that’s what we want to do, what we want our team to do, what we want all teams to do: make things better, moment by moment, in small steps. Hill calls it “ENOF”, Easiest Nearest Owie First. He’s not wrong.

Now what about those constructors? There are at least two cases:

  1. We want to reuse a stack, probably topped by OneShot, in multiple locations, by creating a new Trigger wrapping the stack. We have to create a new one, because an object can only be in one location in the Dungeon. So we probably want a constructor that goes OneShot->Caching->Announcer(messages).
  2. We may want a unique stack all the way down, with Trigger on top.
  3. We now see that we don’t know what we want, with one exception, only what we may want.

If we could write this, things wouldn’t be so bad:

local one = OneShotTrigger(CachingTrigger(Announcer(msgs)))

That’s at least reasonably nested. But we can’t write that, because we have to provide the cache method to call. Let’s inline this code to see what we get:

    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann, Announcer.cache)
    local one = OneShotTrigger(cac)

    local one = OneShotTrigger(CachingTrigger(Announcer(messageTable), Announcer.cache))

That’s correct, but I don’t think I could write it that way. The Explaining Variable Names help me get it right. It would be easier if the caching could be included somehow.

Maybe it can be.

What if the rule was that the caching trigger told its target to cache itself? If you want to use a CachingTrigger, you have to implement cacheYourself() or saveYourself() or the like.

Let’s try that. Here’s the class:

function CachingTrigger:init(target,cachingMethod)
    self.target = assert(target, "expected a target")
    assert(type(cachingMethod) == "function", "expected a cache-fetching function")
    self.cacheMethod = cachingMethod
end

function CachingTrigger:cacheIfNotPresent()
    local tab = self.cacheMethod()
    for i,t in ipairs(tab) do
        if t == self.target then return end
    end
    table.insert(tab, self.target)
end

function CachingTrigger:trigger()
    self:cacheIfNotPresent()
    self.target:trigger()
end

Is what I’m about to do a refactoring or not? I don’t know. It’s not going to change the top level behavior. But it is going to change this class’s behavior a bit. Let’s do it.

CachingTrigger = class()

function CachingTrigger:init(target,cachingMethod)
    self.target = assert(target, "expected a target")
end

function CachingTrigger:trigger()
    self.target:trigger()
    self.target:cacheYourself()
end

Testing, I expect a fail looking for cacheYourself on Announcer.

1: Announcer cache -- Announcer:125: attempt to call a nil value (method 'cacheYourself')
2: Announcer for room -- Announcer:125: attempt to call a nil value (method 'cacheYourself')

So far so good. Now:

function Announcer:cacheYourself()
    for i,ann in ipairs(self:cache()) do
        if ann == self then return end
    end
    table.insert(self:cache(), self)
end

Tests don’t quite run:

2: Announcer for room ann s.b. cached -- Actual: nil, Expected: table: 0x28294c600

That’s going to require a change, because there’s a clever trick in this test:

        _:test("Announcer for room", function()
            local bus = BusCatcher()
            local cache = {}
            local ann = Announcer(msgs)
            local cac = CachingTrigger(ann, function() return cache end)
            local one = OneShotTrigger(cac)
            local tri = Trigger(one)
            _:expect(#bus:publishedMessages()).is(0)
            tri:trigger()
            _:expect(#bus:publishedMessages()).is(#msgs)
            _:expect(cache[1],"ann s.b. cached").is(ann)
            cache = {}
            tri:trigger()
            _:expect(#cache, "one shot should not cache again").is(0)
            _:expect(#bus:publishedMessages(),"one shot should not trigger").is(#msgs) -- still
        end)

Note the function up where we create cac. That’s local to the test and it means that the test can find cached messages in its local variable, cache. We also need to remember to remove those second parameters on our uses of CachingTrigger of course.

We are clearing the real Announcer cache elsewhere in these tests. Let’s put a clear in both the before and after, to be sure we don’t contaminate the real cache.

        _:before(function()
            _bus = Bus
            Bus = EventBus()
            Announcer:clearCache()
        end)
        
        _:after(function()
            Bus = _bus
            Announcer:clearCache()
        end)

Now we can just go ahead and look at the real cache. We’re in the same tab and could access it locally but we can ask for it instead. That’ll make the test continue to work if we move it to another tab, as we surely would in a real product.

        _:test("Announcer for room", function()
            local bus = BusCatcher()
            local cache = Announcer:cache()
            local ann = Announcer(msgs)
            local cac = CachingTrigger(ann)
            local one = OneShotTrigger(cac)
            local tri = Trigger(one)
            _:expect(#bus:publishedMessages()).is(0)
            tri:trigger()
            _:expect(#bus:publishedMessages()).is(#msgs)
            _:expect(cache[1],"ann s.b. cached").is(ann)
            cache = {}
            tri:trigger()
            _:expect(#cache, "one shot should not cache again").is(0)
            _:expect(#bus:publishedMessages(),"one shot should not trigger").is(#msgs) -- still
        end)

Let’s add a warning, since we’re changing the protocol:

function CachingTrigger:init(target,ignored)
    self.target = assert(target, "expected a target")
    if ignored then print("Unexpected second arg to CachingTrigger()") end
end

I bet a few of these come out. They do. I almost wish I had used an assert, to get the walkback. But I can quickly search out the perps.

    self.cofloater:startCrawl()
    local a = Announcer(self:crawlMessages(self.dungeonLevel))
    CachingTrigger(a):trigger()
    self:startTimers()
    (duplicated oh no)

function Room:announce(dungeon, messageTable)
    local tile
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    local one = OneShotTrigger(cac)
    for x = self.x1+1, self.x2-1 do
...

And the tests. Commit: Caching now handled by cached object, e.g. Announcer.

This is better. I mentioned yesterday that the caching code in CachedTrigger suffered from feature envy and belonged on the cached object. Today we fixed that problem because it was actually surfaced at the using level, by the need to provide a redundant parameter that the Announcer already knew.

Now let’s see whether we still want a creation or factory method somewhere.

We have two main uses of this stuff now:

    self.cofloater:startCrawl()
    local a = Announcer(self:crawlMessages(self.dungeonLevel))
    CachingTrigger(a):trigger()
    self:startTimers()

That just wants to display a cached message table. And there’s this:

function Room:announce(dungeon, messageTable)
    local tile
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    local one = OneShotTrigger(cac)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(vec2(x,self.y1))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(x,self.y2))
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(vec2(self.x1,y))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(self.x2,y))
        tile:addContents(Trigger(one))
    end
end

Here we want a one-shot-caching-announcer, which we intend to reuse by wrapping it in new triggers.

Let’s do some Announcer factory methods:

    self.cofloater:startCrawl()
    Announcer:announceAndSave(self.crawlMessages(self.dungeonLevel))
    self:startTimers()

And this:

function Announcer:announceAndSave(messageTable)
    CachingTrigger(Announcer(messageTable)):trigger()
end

Curiously, that doesn’t work. Walkback is:

GameRunner:404: attempt to concatenate a nil value (local 'level')
stack traceback:
	GameRunner:404: in field 'crawlMessages'
	GameRunner:129: in method 'createLearningLevel'
	Player:161: in field '?'
	Button:59: in method 'performCommand'
	Button:54: in method 'touched'
	GameRunner:552: in method 'touched'
	Main:97: in function 'touched'

Do I have a typo? That message says that this line found a nil in level:

    local m = msgs[level] or {"Unexpected level "..level}

And it was concatenating. So it was in the or clause. Let’s improve that slightly:

Ack. I’ve messed this up too much thrashing around. Revert, do over.

Here’s the code I want to improve:

    self.cofloater:startCrawl()
    local a = Announcer(self:crawlMessages(self.dungeonLevel))
    CachingTrigger(a):trigger()
    self:startTimers()

I want a factory method Announcer:sayAndSave(). Can I TDD that? Seems like I should, doesn’t it?

        _:test("Announcer:sayAndSave", function()
            local bus = BusCatcher()
            local cache = Announcer:cache()
            local msgs = {"Here are the messages."}
            Announcer:sayAndSave(msgs)
            _:expect(#bus:publishedMessages()).is(#msgs)
            _:expect(cache[1]:is_a(Announcer), "expect announcer").is(true)
        end)

4: Announcer:sayAndSave -- Announcer:85: attempt to call a nil value (method 'sayAndSave')

As expected. Now, written out longhand because I know I messed up earlier:

function Announcer:sayAndSave(messageTable)
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    cac:trigger()
end

Test. Passes. Make it harder:

        _:test("Announcer:sayAndSave", function()
            local bus = BusCatcher()
            local cache = Announcer:cache()
            local msgs = {"Here are the messages."}
            Announcer:sayAndSave(msgs)
            _:expect(#bus:publishedMessages()).is(#msgs)
            _:expect(bus:publishedMessages()[1]).is(msgs[1])
            _:expect(cache[1]:is_a(Announcer), "expect announcer").is(true)
        end)

Checking the actual message. Test. Passes.

Use the new method here:

    self.cofloater:startCrawl()
    local a = Announcer(self:crawlMessages(self.dungeonLevel))
    CachingTrigger(a):trigger()
    self:startTimers()

    self.cofloater:startCrawl()
    Announcer:sayAndSave((self:crawlMessages(self.dungeonLevel)))
    self:startTimers()

I really expect this to work. Whew, it does. Do same in the other location (duplication, Will Robinson, warning warning).

Just noticed and fixed the redundant parens in the above call. Still works. Now for the other location, same code.

Works. I have no idea what was wrong before. My only regret is not reverting sooner.

Commit: use new Announcer:sayAndSave in dungeon creation.

Now the other usage:

function Room:announce(dungeon, messageTable)
    local tile
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    local one = OneShotTrigger(cac)

I reckon I’d better TDD this as well, but golly it’s so trivial. Still, once burned etc.

        _:test("Announcer:cachingOneShot", function()
            local msgs = {"The messages", "are here"}
            local stack = Announcer:cachingOneShot(msgs)
            _:expect(stack:is_a(OneShotTrigger)).is(true)
            _:expect(stack.target:is_a(CachingTrigger)).is(true)
            local ann = stack.target.target
            _:expect(ann:is_a(Announcer)).is(true)
            _:expect(ann.messageTable).is(msgs)

I’m really only concerned whether the structure is correct, so that’s what I tested. Let’s see if we can make it pass.

function Announcer:cachingOneShot(messageTable)
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    local one = OneShotTrigger(cac)
    return one
end

Test.

Runs.

Use the new method. This:

function Room:announce(dungeon, messageTable)
    local tile
    local ann = Announcer(messageTable)
    local cac = CachingTrigger(ann)
    local one = OneShotTrigger(cac)

Becomes this:

function Room:announce(dungeon, messageTable)
    local tile
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do

Test. Tests run, Learning Level works. Commit: use new Announcer:cachingOneShot in Room:announce.

Now then. Let’s look back:

I’s Lookin Behind Us Now, Into History Back

A few days ago we created some new objects to manage announcements, caching, and triggering things only once. Yesterday, we improved those objects in ways that appealed to at least one of us, making each one simpler.

Today, we found a way to make them a bit simpler and better designed, by moving caching responsibility into the object needing to be cached, with the trigger just telling it when to do it.

Then, because creating these stacks of objects was a bit error-prone, we came up with two “factory” methods on Announcer, one to immediately trigger messages into the history, and one to give us an object that we can use multiple times, by wrapping it in one more Trigger.

We’ve almost, but not quite, removed knowledge of the trigger stack from the system. It’s almost, but not quite, embedded inside the Announcer concept. (That may not be the place for it, since someday, and that day may never come, we may call upon these objects to do some other service for us. Until then, accept this as justice.)

One way or another, our factory methods need to live on some class, according to our coding standard, and they’ll need to know, or be given, other classes to build into the bottom of the stack. We might find that they belong in Trigger or in some new place. That’s for some other day.

For today, the system is a tiny bit better, and we’ve had several jolts of joy. That’s good.

And one jolt of WTF. That happens, too, and it’s good if we can learn from it, and good if we can recover from it. Today, your author learned to revert sooner when the steps are small, and that, in the end, the computer will do what he tells it. He just has to tell it the right stuff.

See you next time!

D2.zip