Dungeon 202
Today I have in mind an improvement to Announcer, because I think it’ll be nice and will give us something to wonder about.
We’ll look at the code in a moment, but first let’s talk about what the Announcer and AnnouncerProxy are all about, and how they got that way.
Announcer was created so that we could put a ring of objects around the border of a room, so that when the player first enters that room, from any direction, the announcer would trigger and make a textual announcement about the room. This was originally created for the learning level, where we want to give instructions for the room as soon as the player gets there.
The Announcer had–still has–the property that once it has said its thing, it never does so again. And we used a single instance in the contents of all the ring tiles, so that when you step on one, you’ve stepped on them all, and the message only comes out once, because they’re all the same and therefore have all switched themselves off.
Then along came the requirement for “History”, which is a button you can press that will replay all the (important) Announcer messages from the beginning of the level. So the Announcer has a special method that can be sent to it to tell it to switch itself on, play its messages, and then switch itself off. Odd but easy.
In order for History to work, each Announcer, when it is created, adds itself to a “class variable”, AnnouncerCache, which is used to play back history. The operation here plays all those Announcers who have turned themselves off, because they’ve already triggered, and doesn’t play the untriggered ones.
Then we changed the way tile contents is represented, to eliminate back pointers between dungeon object and the tiles they are presently in. That implementation constrained any object to be in at most one tile. So we couldn’t put the same Announcer in every tile in the ring.
We built the AnnouncerProxy, a lightweight object that just points to and triggers an Announcer. Then we created one Announcer for the room, and put a unique AnnouncerProxy into all the tiles of the ring. Nice enough.
This all works, but I have a better idea. Let’s look at the Announcer and related code:
Announcer Code
Announcer = class(DungeonObject)
local AnnouncerCache = {}
-- class methods
function Announcer:playCache()
for i,ann in ipairs(AnnouncerCache) do
if not ann.sayMessages then
ann:repeatMessages()
if i < #AnnouncerCache then
Bus:publish("addTextToCrawl", self, {text="---"})
end
end
end
end
-- instance methods
function Announcer:__tostring()
return "Announcer"
end
function Announcer:init(messageTable)
self.messageTable = messageTable
self.sayMessages = true
table.insert(AnnouncerCache, self)
end
function Announcer:cache()
return AnnouncerCache
end
function Announcer:clearCache()
AnnouncerCache = {}
end
function Announcer:messages()
if self.sayMessages then
self.sayMessages = false
return self.messageTable
else
return {}
end
end
function Announcer:repeatMessages()
self.sayMessages = true
self:trigger()
end
function Announcer:trigger()
for i,m in ipairs(self:messages()) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
AnnouncerProxy = class(DungeonObject)
function AnnouncerProxy:init(announcer)
self.announcer = announcer
end
function AnnouncerProxy:__tostring()
return "AnnouncerProxy"
end
function AnnouncerProxy:draw(ignored, center)
end
function AnnouncerProxy:trigger()
return self.announcer:trigger()
end
function AnnouncerProxy:privateMessageTable()
return self.announcer.messageTable
end
function AnnouncerProxy:repeatMessages()
return self.announcer:repeatMessages()
end
This isn’t quite self-explanatory–and it should be–but it’s not bad. The Proxy will be sent trigger when it is stepped on. The Announcer, sent trigger, will fetch its messages and publish them. It then sets its sayMessages flag to false, so that it won’t say anything again.
The repeatMessages method sets the say flag back to true, and triggers, so that the messages will play and the flag will again be set false. (I can imagine doing that differently, with one method to set the flag and call another method that unconditionally plays, but it is what it is.)
The Proxy is in the dungeon, so it has to have a draw method, but it doesn’t draw anything. Its privateMessageTable method is there just for testing, if I recall. And I don’t think it needs the repeatMessages method at all. Let’s test that.
Right. Commit: remove unused code from Announcer.
Now, I think this is a bit intricate, but not terrible. However, I have an idea for something that will make it nicer, and that will drop out a couple of objects that may be useful in the future. I know, YAGNI, but I’m not doing this to get those objects, I’m doing this to make this code better, and doing it in a simple way that happens to produce some reusable objects.
You’ll see, as we do this, that we’re not adding “generality” here. We’re creating objects that are so simple, they’re able to be reused should we need to.
Let’s review what we need from the Announcer idea.
Announcer Requirements
- Provide an object that, if stepped upon, triggers a message.
- Allow multiple triggering objects to trigger the same message (and others to trigger other messages).
- Once a message has come out, none of its triggers will ever trigger it again.
- The History button can cause all messages that have been triggered to replay, in whatever order History happens to choose.
- Untriggered messages will not come out in History.
I think that’s the list. (If it isn’t, I’m going to come back an edit it, so it’ll be the official list.)
The Idea
The idea that first came to me was this: The AnnouncerProxy is actually a general purpose object that can point to any other object, and when it is stepped upon, the AnnouncerProxy will send trigger to that object.
That “insight” just said to me that I could use that object to trigger a door opening, or the WayDown appearing, or a monster beaming in, whatever. I resolved to rename the object to Trigger.
As we think further about it, we see that Announcer, as it is now, has these bits of function:
- Upon creation, add yourself to the cache.
- Upon demand, publish messages;
- Only publish once;
- Except, if called via
repeatMessages, publish again - Via a class message, repeat all the previously triggered cached Announcers.
It’s not easy to argue that this is a single idea, even though the object is really pretty simple. But ideally, an object encapsulates just a single idea.
My Cunning Plan
My cunning plan is to produce a number of tiny, simple objects that each do just one thing. The things will include:
- Trigger an object when I am stepped upon;
- Trigger an object just once, when I am triggered;
- Cache and trigger an object, when I am triggered;
- Play my messages when I am triggered.
These guys are all single purpose, except, arguably, for the caching one.
I think we should do some TDD on this, but these objects are so trivial that I suspect we’ll do better to TDD the overall result rather than the individual objects.
Anyway, let’s do it.
The Implementation
Here are our original tests:
_:test("Create", function()
local ann = Announcer(msgs)
_:expect(ann:messages()).is(msgs)
_:expect(#ann:messages()).is(0)
end)
_:test("Announcer cache", function()
Announcer:clearCache()
_:expect(#Announcer:cache()).is(0)
local a1 = Announcer(msgs)
local a2 = Announcer(msgs2)
_:expect(#Announcer:cache()).is(2)
end)
The first test relies on the “trick” in Announcer, where it returns an empty collection of messages if it has already returned its messages once:
function Announcer:messages()
if self.sayMessages then
self.sayMessages = false
return self.messageTable
else
return {}
end
end
One more concern, more for testing than any other purpose. The Announcer actually publishes its messages via the Bus:
function Announcer:trigger()
for i,m in ipairs(self:messages()) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
This is nice from the overall system design, but it makes testing a bit difficult, because, at least in principle, we want to check to see that the right stuff was added to the crawl.
I guess we’ll just bite the bullet and make an auxiliary testing object that will subscribe to the bus. We’ll want to save any existing bus and replace it after our tests run.
local msgs = {"This is the mysterious level called", "Soft Wind Over Beijing,",
"where nothing bad will happen to you", "unless you happen to be a princess.", "Welcome!" }
local msgs2 = { "Here is another message.", "And another." }
local _bus
function testAnnouncer()
CodeaUnit.detailed = false
_:describe("Announcer", function()
_:before(function()
_bus = Bus
Bus = EventBus()
end)
_:after(function()
Bus = _bus
end)
So far so good. Now a little test object to record messages.
-- Testing object
local BusCatcher = class()
function BusCatcher:init()
self.published = {}
Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
end
function BusCatcher:addTextToCrawl(msg)
table.insert(self.published, msg)
end
function BusCatcher:publishedMessages()
return self.published
end
I’m going to leave the old tests there to remind me of things, but only the new ones, that we’re about to write, are intended to last.
Design issue …
I’m planning to have all these little objects, so that we’re going to wind up with something like this
Trigger
v
OneShotTrigger
v
CachingTrigger
v
Announcer
There’s the question of a reasonably easy way to build such a nest. For now, I think I’ll write them out longhand, but we may well decide that we need some factory methods or some such support.
Longhand for now. This is more than enough to force some code:
_:test("Announcer for room", function()
local bus = BusCatcher()
Announcer:clearCache()
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)
end)
I could have done this one object at a time, but since I’ve been thinking about it, and since they are all “trivial”, I’ve written out the whole flow.
Plus, it’s helpful to have the whole flow. I might even do it if the objects were going to be difficult, except then I’d insert more expectations into the list, and perhaps I’d comment out some of the calls to get the test to run, then uncomment one, and so on.
Another alternative would be to test the objects one at a time, but these are so trivial I don’t want to do that.
(I’ve just realized that there’s a problem with the caching one, but we’ll fix it when we encounter it. The test will remember.)
Here’s our first error:
3: Announcer for room -- Announcer:60: attempt to call a nil value (global 'CachingTrigger')
This irritates me slightly, because I wanted to write them in the other order. Well, so be it. Anyway, we’ll encounter the problem I saw sooner. Here it is:
CachingTrigger = class()
function CachingTrigger:init(target, cache)
self.target = target
self.cache = cache
end
The CachingTrigger needs to know where to cache its things. So the test needs a collection to point to. Improve the test:
_:test("Announcer for room", function()
local bus = BusCatcher()
local cache = {}
local ann = Announcer(msgs)
local cac = CachingTrigger(ann, cache)
local one = OneShotTrigger(cac)
local tri = Trigger(one)
_:expect(#bus:publishedMessages()).is(0)
tri:trigger()
_:expect(#bus:publishedMessages()).is(#msgs)
end)
I realized that since I’m passing in the cache, I don’t need to clear the announcer’s. No one is going to touch it by the time we’re done here.
If I run the test now, I’ll get a missing OneShot, I think.
3: Announcer for room -- Announcer:61: attempt to call a nil value (global 'OneShotTrigger')
Let’s do that class. Note that I’ve not put anything in but the inits.
OneShotTrigger = class()
function OneShotTrigger:init(target)
self.target = target
end
Now I expect a fail demanding Trigger:
3: Announcer for room -- Announcer:62: attempt to call a nil value (global 'Trigger')
And I build it:
Trigger = class()
function Trigger:init(target)
self.target = target
end
Now, probably we fail looking for a trigger method:
3: Announcer for room -- Announcer:64: attempt to call a nil value (method 'trigger')
We’ll write it on Trigger and expect a fail on OneShot:
function Trigger:trigger()
return self.target:trigger()
end
I decided to return whatever results may come back from triggering. A bit of YAGNI, but I argue that Ruby would do it automatically.
3: Announcer for room -- Announcer:168: attempt to call a nil value (method 'trigger')
That’s the call we just wrote. OneShot needs trigger:
function OneShotTrigger:trigger()
return self.target:trigger()
end
Now we’ll fail there, aiming at the CachingTrigger.
3: Announcer for room -- Announcer:162: attempt to call a nil value (method 'trigger')
function CachingTrigger:trigger()
self.target:trigger()
end
I kind of thing this is going to work now. Let’s find out. It does. We got four messages back from the Bus.
Now let’s test the caching:
_:test("Announcer for room", function()
local bus = BusCatcher()
local cache = {}
local ann = Announcer(msgs)
local cac = CachingTrigger(ann, cache)
local one = OneShotTrigger(cac)
local tri = Trigger(one)
_:expect(#bus:publishedMessages()).is(0)
tri:trigger()
_:expect(#bus:publishedMessages()).is(#msgs)
_:expect(cache[1]).is(ann)
end)
The CachingTrigger is supposed to add its target to the cache, upon triggering.
(Remind me that we’re presently caching Announcers on creation and we need to undo that. Ideally with a test to force it.)
function CachingTrigger:trigger()
table.insert(self.cache, self.target)
return self.target:trigger()
end
I noticed that I forgot the return there in the first implementation. And I’m not actually testing it, which tells me that I shouldn’t require it. I’m going to remove them all.
Why? Because I have no use for it, and I have no tests for it. If I need it later, I can do it later, and I’ve just demonstrated that I can, and will, do it wrong.
I forgot to run the test before putting in the table.insert, so I commented it out and got the expected fail:
3: Announcer for room -- Actual: nil, Expected: Announcer
Let’s comment that expectation:
_:expect(cache[1],"ann s.b. cached").is(ann)
3: Announcer for room ann s.b. cached -- Actual: nil, Expected: Announcer
Now do it:
function CachingTrigger:trigger()
table.insert(self.cache, self.target)
self.target:trigger()
end
Expect the test to run. It does. Enhance for one-shot:
_:test("Announcer for room", function()
local bus = BusCatcher()
local cache = {}
local ann = Announcer(msgs)
local cac = CachingTrigger(ann, cache)
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(0)
end)
I expect the one shots to fail.
3: Announcer for room one shot should not trigger -- Actual: 5, Expected: 0
OK, I am duly surprised twice. I thought the cache should fail with two copies, and that the published messages would be 0. Ah, but what is Announcer doing?
We need to remove all the special behavior from Announcer.
function Announcer:init(messageTable)
self.messageTable = messageTable
self.sayMessages = true
table.insert(AnnouncerCache, self)
end
Remove those last two lines. Just save the message table.
function Announcer:trigger()
for i,m in ipairs(self:messages()) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
We don’t need to call our special messages method any more. If we’re triggered, we publish. It’s someone else’s job to see that we only trigger as needed.
function Announcer:trigger()
for i,m in ipairs(self.messages) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
Then there’s this:
function Announcer:repeatMessages()
self.sayMessages = true
self:trigger()
end
I want to remove this now, but I know it is used. Should we preserve this old behavior or go for it?
It’s a bit risky. If I go for it, I’ll have to revert all this good stuff. But I think there’s just one use. Heck, it belongs to me anyway. I can change it:
function Announcer:playCache()
for i,ann in ipairs(AnnouncerCache) do
if not ann.sayMessages then
ann:repeatMessages()
if i < #AnnouncerCache then
Bus:publish("addTextToCrawl", self, {text="---"})
end
end
end
end
function Announcer:playCache()
for i,ann in ipairs(AnnouncerCache) do
ann:trigger()
if i < #AnnouncerCache then
Bus:publish("addTextToCrawl", self, {text="---"})
end
end
end
I’ve removed the checking for sayMessages and called trigger instead of the repeat thing. Now I can remove that member variable and unnecessary code. The instance methods for the Announcer are now just these:
function Announcer:init(messageTable)
self.messageTable = messageTable
end
function Announcer:trigger()
for i,m in ipairs(self.messages) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
Test. I’ve finally broken those two old tests. Remove them.
I remove the creation one, that was actually testing one shot, which is no longer a feature. The other won’t run, but I leave it in because it tests whether things actually go into AnnouncerCache and I’m not sure yet whether I need to test that.
I also get a run-time failure. Let’s look at it and see what it is and what we need to test.
attempt to index a nil value
stack traceback:
[C]: in for iterator 'for iterator'
Announcer:99: in method 'trigger'
GameRunner:167: in method 'createLevel'
Main:43: in function 'setup'
Well, it helps if you use the same name for defining a member variable and using it:
function Announcer:init(messageTable)
self.messageTable = messageTable
end
function Announcer:trigger()
for i,m in ipairs(self.messages) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
function Announcer:init(messageTable)
self.messageTable = messageTable
end
function Announcer:trigger()
for i,m in ipairs(self.messageTable) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
Test.
2: Announcer for room one shot should not trigger -- Actual: 10, Expected: 0
As expected. Now we have to make the OneShot only go once. We could give it a flag and if on it. We could have it forget its target and if on that. We could give it a fake target and let it trigger that to no avail.
All those seem fancy except for the if. I do hate to use ifs, but it seems weird to do much more.
Let’s try something:
function OneShotTrigger:init(target)
self.target = target
self.triggerFunction = self.doTrigger
end
function OneShotTrigger:trigger()
self.triggerFunction(self)
end
function OneShotTrigger:doTrigger()
self.triggerFunction = self.skipTrigger
self.target:trigger()
end
function OneShotTrigger:skipTrigger()
end
Pluggable Behavior. triggerFunction can be doTrigger or skipTrigger. This is supposed to work, but I happen to know as I write this, that it does not:
2: Announcer for room one shot should not trigger -- Actual: 5, Expected: 0
And why “5” anyway? I kind of wish I had explored that before.
I’m going to print the messages in my fake object.
function BusCatcher:init()
self.published = {}
Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
end
function BusCatcher:addTextToCrawl(event, sender, info)
table.insert(self.published, info.text)
print(info.text)
end
I had the receiving sequence wrong. When I do the above I find that the number of messages in the msgs variable is in fact 5, so that “just” means that somehow they got pushed in there again.
Reviewing the test, I realize that the test is wrong:
_:test("Announcer for room", function()
local bus = BusCatcher()
local cache = {}
local ann = Announcer(msgs)
local cac = CachingTrigger(ann, cache)
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(0)
end)
I’ve not cleared my fake bus. It’s still showing the same messages as at the beginning. if the OneShot didn’t work, we’d get ten messages. Let’s break it to be sure.
2: Announcer for room one shot should not trigger -- Actual: 10, Expected: 0
OK, code good, test wrong.
My new test is running. We still have the saved one broken:
1: Announcer cache -- Actual: 0, Expected: 2
That’s this:
_:test("Announcer cache", function()
Announcer:clearCache()
_:expect(#Announcer:cache()).is(0)
local a1 = Announcer(msgs)
local a2 = Announcer(msgs2)
_:expect(#Announcer:cache()).is(2)
end)
Let’s actually turn that into a useful test. to do that we’ll need to use at least a CachingTrigger on our Announcers.
I’m glad I decided to do that. Here’s my test:
_:test("Announcer cache", function()
Announcer:clearCache()
_:expect(#Announcer:cache()).is(0)
local a1 = Announcer(msgs)
local c1 = CachingTrigger(a1)
local a2 = Announcer(msgs2)
local c2 = CachingTrigger(c2)
_:expect(#Announcer:cache()).is(0)
c1:trigger()
_:expect(#Announcer:cache()).is(1)
c2:trigger()
_:expect(#Announcer:cache()).is(2)
c2:trigger()
_:expect(#Announcer:cache()).is(2)
end)
It includes that last expectation, which is that a cached entry won’t be cached twice. Now if I one-shot them, they won’t be, but it seems to me that we probably don’t ever want them cached twice.
We can beef up the code around the table insert to check the table and then insert:
function CachingTrigger:trigger()
table.insert(self.cache, self.target)
self.target:trigger()
end
You know what? I think this isn’t the best design ever. We’re passing a naked collection around, which is always a bit of a smell.
I suspect the AnnouncerCache wants to be an object. And I suspect that the CachingTrigger isn’t a CachingTrigger at all, it’s a ForkingTrigger or a DoubleTrigger, some kind of trigger that triggers two objects.
If we do that, however, how will our new AnnouncerCache know what to cache? We’d have to pass it in, like this:
function CachingTrigger:trigger()
self.cache:trigger(self.target)
self.target:trigger()
end
But if this is a DoubleTrigger, it doesn’t know whether its two targets want the target object, or which one is wanted.
Wild Thought
I briefly thought about whether I could and should just pass the triggered objects around, and so on.
I think that’s a leap too far. Now what about the Caching guy not caching things that are already cached?
We can fix that by expecting the cache to be an object to which we send some message (not trigger), including our target. Or we can just not insert it if it’s already in there.
(It’s tempting to use the hash type table and then we could just plunk it in as many times as we wanted … but we want the cache to be in chronological order.
For now, let’s only insert if it’s not already there:
function CachingTrigger:trigger()
self:cacheIfNotPresent()
self.target:trigger()
end
function CachingTrigger:cacheIfNotPresent()
for i,t in ipairs(self.cache) do
if t == self.target then return end
end
table.insert(self.cache, self.target)
end
I think this ought to work. It doesn’t:
1: Announcer cache -- attempt to index a nil value
Ah. I didn’t provide the cache to the CachingTriggers. They should diagnose that:
function CachingTrigger:init(target, cache)
self.target = target
self.cache = assert(cache, "expected a cache")
end
Now I should get that failure from the test:
1: Announcer cache -- Announcer:137: expected a cache
Now fix the test:
_:test("Announcer cache", function()
Announcer:clearCache()
_:expect(#Announcer:cache()).is(0)
local a1 = Announcer(msgs)
local c1 = CachingTrigger(a1, Announcer:cache())
local a2 = Announcer(msgs2)
local c2 = CachingTrigger(c2, Announcer:cache())
_:expect(#Announcer:cache()).is(0)
c1:trigger()
_:expect(#Announcer:cache()).is(1)
c2:trigger()
_:expect(#Announcer:cache()).is(2)
c2:trigger()
_:expect(#Announcer:cache()).is(2)
end)
Oops, something still wrong. I love these tests.
1: Announcer cache -- Announcer:149: attempt to index a nil value (field 'target')
function CachingTrigger:trigger()
self:cacheIfNotPresent()
self.target:trigger()
end
That’s interesting. How did we get in here with no target?
function CachingTrigger:init(target, cache)
self.target = assert(target, "expected a target")
self.cache = assert(cache, "expected a cache")
end
First let’s diagnose it.
1: Announcer cache -- Announcer:136: expected a target
Someone isn’t playing by the rules here.
local a1 = Announcer(msgs)
local c1 = CachingTrigger(a1, Announcer:cache())
local a2 = Announcer(msgs2)
local c2 = CachingTrigger(c2, Announcer:cache())
The fool typed c2 meaning a2.
Tests run. I think a commit is in order: Adding Trigger, OneShotTrigger, CachingTrigger.
It’s time to remove the old AnnouncerProxy and use our new stuff.
Here’s our only code that uses it:
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
for x = self.x1+1, self.x2-1 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(x,self.y2))
tile:addContents(AnnouncerProxy(ann))
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(self.x2,y))
tile:addContents(AnnouncerProxy(ann))
end
end
This is the code that creates the ring around the room.
Let’s write out longhand what we want now:
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
Let’s remove the AnnouncerProxy to be sure and test this.
I realize that I didn’t give Trigger a draw method. That could be bad. Enhance the test just for fun.
_:text("Trigger has draw method", function()
_:expect(type(Trigger.draw)).is("function")
end)
Running that, a lot of tests break. I don’t care why, it’s too many revert and regroup.
OK, first require a draw for Trigger:
_:test("Trigger can draw", function()
_:expect(type(Trigger.draw)).is("function")
end)
Fails as expected. Implement:
function Trigger:draw()
end
Test.
It comes to me, based on the errors I saw before the revert, that the Trigger needs to inherit from DungeonObject, so that its interactions will work. AnnouncerProxy does that.
That should help with this next step, converting, again, to use Trigger in the Room ringer.
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
local cac = CachingTrigger(cac, Announcer:cache())
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
I’m not sure if that’s what I did before or not, and I don’t plan to look. I plan to test this.
A room test fails:
7: Room:announce places in ring -- Announcer:140: expected a target
That’s this:
function CachingTrigger:init(target, cache)
self.target = assert(target, "expected a target")
self.cache = assert(cache, "expected a cache")
end
Have I a typo?
Sure have:
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
local cac = CachingTrigger(cac, Announcer:cache())
local one = OneShotTrigger(cac)
Referred to cac, should have been ann. This is error-prone, we must do better.
Now the test fails legitimately:
7: Room:announce places in ring -- Tests:587: attempt to call a nil value (method 'privateMessageTable')
Let’s see that test. I think it’s doing too much:
_:test("Room:announce places in ring", function()
local r = Room(3,3,5,10, Runner)
local msgs = {"Message"}
local dungeon = Runner:getDungeon()
r:announce(dungeon, msgs)
local cont = dungeon:privateGetTileXY(3,3):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(3,7):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(7,3):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(7,12):getContents()
_:expect(#cont).is(1)
checkRing(dungeon, 3,3, 7, 12, msgs)
end)
It’s checkRing that is checking the message:
function checkRing(dungeon, x1,y1, x2,y2, msgTable)
local ann
local msgs
local t
for x = x1,x2 do
t = dungeon:privateGetTileXY(x,y1)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x,y1 "..x..","..y1).is(msgTable)
t = dungeon:privateGetTileXY(x,y2)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x,y2 "..x..","..y2).is(msgTable)
end
for y = y1,y2 do
t = dungeon:privateGetTileXY(x1,y)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x1,y "..x1..","..y).is(msgTable)
t = dungeon:privateGetTileXY(x2,y)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x2,y "..x2..","..y).is(msgTable)
end
end
Let’s just check for a trigger in each cell.
And let’s push that down a bit:
function checkRing(dungeon, x1,y1, x2,y2, msgTable)
local ann
local msgs
local t
for x = x1,x2 do
t = dungeon:privateGetTileXY(x,y1)
ann = findAnnouncerIn(t)
t = dungeon:privateGetTileXY(x,y2)
ann = findAnnouncerIn(t)
end
for y = y1,y2 do
t = dungeon:privateGetTileXY(x1,y)
ann = findAnnouncerIn(t)
t = dungeon:privateGetTileXY(x2,y)
ann = findAnnouncerIn(t)
end
end
function findAnnouncerIn(tile)
_:expect(tile:getContents()[1]:is_a(Trigger)).is(true)
end
I expect a pass here unless I messed up the testing bit.
I get a pass. The function name should be findTriggerIn.
The tests run. The system, not so much. No crash, but History seems not to be working, and I can’t get into the second room. That latter bit is TileArbiter, which needs to be changed to expect Triggers, and not to expect AnnouncerProxies.
This should suffice to get into the room and display the message:
t[Trigger] = {}
t[Trigger][Monster] = {moveTo=TileArbiter.acceptMove}
t[Trigger][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithAnnouncer}
Ah, good. I can go in, the message comes out, and the History includes that message but not the original. That’s because we didn’t ask it to cache.
That’s here:
Announcer(self:crawlMessages(self.dungeonLevel)):trigger()
That line occurs in three different level creators, the regular play one, the learning level, and the testing level, where it’s commented out, as is a lot of stuff. That duplication should be removed. But first let’s make it work.
Then we’re going to have to make it right, because there are some issues here. We’ll do a retro in a moment.
local a = Announcer(self:crawlMessages(self.dungeonLevel))
CachingTrigger(a, Announcer:cache()):trigger()
Testing again, I expect history to show the first message as well as the second room.
This works but I am a bit surprised to see that the messages come out in chronological order. I think they used to come out last to first. I think I prefer that.
Let me commit this and then check out an old version and see what it used to do, and if necessary, why.
Commit: Messages Replay in Chron Order.
Checkout Saturday’s last commit.
Ah. I see the problem. The order was accidental, because it depended on when the Announcer was created, and the rooms are laid out first.
Let’s leave it in chron order for now, and see what the users and product owner think.
Check out current.
There’s an odd issue here. When I revert or check out, I generally exit the Codea editor, just because I don’t trust all the right things to change. However, even that may not suffice, it appears. I’ve seen at least one case where the revert didn’t complete in time. WorkingCopy did pick up that the changes were still there, but I didn’t see it until weird things started to happen.
So I’m being even more careful to wait between reversions and checkouts.
I think what we have is nearly solid, but I have one concern.
We create our CachingTrigger by passing it the current value of Announcer:cache(). That will return the value of the AnnounceCache private class variable:
local AnnouncerCache = {}
function Announcer:cache()
return AnnouncerCache
end
However, we have this:
function Announcer:clearCache()
AnnouncerCache = {}
end
If we ever call that, I think it will disconnect any existing caches and further announcements won’t cache. I can test this by putting a clear into learning level.
self.cofloater:startCrawl()
Announcer:clearCache()
local a = Announcer(self:crawlMessages(self.dungeonLevel))
CachingTrigger(a, Announcer:cache()):trigger()
If things go as I suspect they will, I can go into the second room and see the announcement, but History will not include it. It should include the first room’s message.
Let’s find out.
As expected, the second message doesn’t come out in the history.
What do we actually want the CachingTrigger to do? I think we’d like it to fetch the cache table dynamically. Something like this:
function CachingTrigger:init(target, cachingObject, cachingMethod)
self.target = assert(target, "expected a target")
self.cacheObject = assert(cachingObject, "expected a cache object")
self.cacheMethod = assert(type(cachingMethod) == "function", "expected a cache-fetching method")
end
function CachingTrigger:cacheIfNotPresent()
local meth = self.cacheMethod
local tab = self.cacheObject[meth]()
for i,t in ipairs(tab()) do
if t == self.target then return end
end
table.insert(tab, self.target)
end
Things should fail at this point and we’ll let the fails guide our changes.
CachingTrigger(a, Announcer, Announcer.cache):trigger()
Changes like that look OK, but there’s one test that I can’t do that way. Let’s see if everything else works.
Ah. Can’t use the assert that way: It returns its first arg unless it’s falsey.
OK, this took me a bit to sort out. I’ve decided to pass a function to the CachingTrigger, and that the function is required to return whatever cache you are presently using.
It goes like this:
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
We execute the cachingMethod and expect a table back. We could bulletproof that further if need be.
Our calls all look like this now:
local ann = Announcer(messageTable)
local cac = CachingTrigger(ann, Announcer.cache)
local one = OneShotTrigger(cac)
In the test with the local cache, I did this:
_: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)
...
The function there can see the local cache, and returns it as needed.
Now there’s that clear that I patched in. Let’s remove that. We also have one in the dcPrepare method that prepares us for dungeon creation, but it belongs there. It ensures that each level pitches out any leftover messages from the previous levels.
We can commit: Announcer now uses a Trigger stack to support multiple instances, one-shot behavior, and message caching.
Let’s review and sum up.
The New Trigger Stack
Here’s all the code:
Announcer = class(DungeonObject)
local AnnouncerCache = {}
-- class methods
function Announcer:cache()
return AnnouncerCache
end
function Announcer:clearCache()
AnnouncerCache = {}
end
function Announcer:playCache()
for i,ann in ipairs(AnnouncerCache) do
ann:trigger()
if i < #AnnouncerCache then
Bus:publish("addTextToCrawl", self, {text="---"})
end
end
end
-- instance methods
function Announcer:init(messageTable)
self.messageTable = messageTable
end
function Announcer:trigger()
for i,m in ipairs(self.messageTable) do
Bus:publish("addTextToCrawl", self, {text=m})
end
end
CachingTrigger = 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
OneShotTrigger = class()
function OneShotTrigger:init(target)
self.target = target
self.triggerFunction = self.doTrigger
end
function OneShotTrigger:trigger()
self.triggerFunction(self)
end
function OneShotTrigger:doTrigger()
self.triggerFunction = self.skipTrigger
self.target:trigger()
end
function OneShotTrigger:skipTrigger()
end
Trigger = class(DungeonObject)
function Trigger:init(target)
self.target = target
end
function Trigger:draw()
end
function Trigger:trigger()
self.target:trigger()
end
This is 87 lines, of which about 65 are not blank or comment.
I’ve also added a fair number of test lines, and the code in use currently uses a couple of extra lines of setup. I think we’ll go after reducing that, or at least bringing stack creation inside these classes.
The code reported when we started was about 80 lines, so we have about the same amount of code, but, I claim, better organized and more flexible.
The Trigger objects, and the Announcer, are all rather simple. Most have just one operational method, and even the OneShot has just four, and I can see how to cut that to three. Shall we do that? Let’s
OneShotTrigger = class()
function OneShotTrigger:init(target)
self.target = target
self.triggerFunction = self.doTrigger
end
function OneShotTrigger:trigger()
self.triggerFunction(self)
end
function OneShotTrigger:doTrigger()
self.triggerFunction = self.skipTrigger
self.target:trigger()
end
function OneShotTrigger:skipTrigger()
end
I think this will work:
function OneShotTrigger:init(target)
self.target = target
end
function OneShotTrigger:trigger()
self.trigger = self.skipTrigger
self.target:trigger()
end
function OneShotTrigger:skipTrigger()
end
This code replaces this instance’s local trigger method with skipTrigger. It leaves other instances alone.
We should check that in a test. And arguably this is even more weird than the previous scheme.
You know what? It is. Too fancy. Let’s go simple.
function OneShotTrigger:init(target)
self.target = target
self.shouldTrigger = true
end
function OneShotTrigger:trigger()
if self.shouldTrigger then
self.shouldTrigger = false
self.target:trigger()
end
end
Look, that’s even shorter! So much for jumping through my own orifice to avoid an if.
Commit: Simplify OneShotTrigger.
What else? This method is questionable:
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
Has nothing to do with us, mostly it’s about the other guy’s table. He should be dealing with duplicates himself. But we’re using a raw table, so for now, that’s the best we can do, unless we wanted to create a table function insertIfAbsent, and we don’t want to do that right now.
It’s tempting to build some smarter classes on top of table. Wouldn’t be difficult. Maybe one day we’ll do that.
A general rule, which I often disobey, would be never to pass naked types like tables around, instead preferring objects that understand what’s in them. Good rule, but often seems like too much. Until one day you discover things like this, with outsiders managing your table for you, perhaps to your liking, and perhaps not.
We need to ask ourselves, selves, was this actually worth it?
I think that the answer, in a real product, might be “no”, but we’ll see whether we use Trigger again. If we do, we can probably say it was a good idea. If not, well, it’s arguably nicer and better factored code, but it was working just fine before we started. And it’s a bit harder to use as well, at least right where we are this minute.
We might see about improving the stack creation … or we might let it ride.
If you have a strongish opinion, a question, a wonderment, or a better idea about this little implementation, please tweet me up or email me or slack me, and let me know what you think.
See you next time!