Dungeon 300: Remove Generality

Colin Chapman of Lotus said that to add speed, you must ‘add lightness’. Can we add some simplicity up in this dungeon?

Sometimes we can add simplicity to our code. This usually makes the code smaller, as separate cases collapse into few. Sometimes we want to add generality. I think that generality comes in two forms.

In the best case, it’s the same as adding simplicity: multiple cases collapse into fewer cases, ideally one. Everything is simple and clear. There is another kind of thing that we often call generality, where we add complexity to the code, but allow it to handle more situations. Often things are complicated and sometimes obscure. I have come to favor the first type of generality and to disfavor the second. I’m inclined to call the second kind “fake generality”.

There’s nothing not to like about the first kind. It’s simple, and it handles whatever we throw at it. The second kind, however, is often done speculatively. “As long as we’re doing it for this case, let’s add these other cases. We’re gonna need them.” And it’s generally done by adding code. We add weight, not lightness.

The YAGNI principle, for which I assume blame and credit in equal measure, goes to this notion. Adding things because we think we’re going to need them is, in my very thoughtful view, not a good thing.

Why do I bring this up? Because I think that the publish-subscribe feature of the D2 dungeon program is overly general in the second way: it provides generality that we do not need and becomes more complicated in doing so.

Let’s explore.

EventBus

The Bus in D2 has two key methods, publish and subscribe. They look like this:

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
        table.insert(results,result)
    end
    return results
end

function EventBus:subscribe(listener, method, event)
    assert(type(method)=="function", "method parameter must be function")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

We could go back and read about why I wrote EventBus this way. Probably I had some good reasons for some of it. Some of it isn’t very good Lua code, and I’m not sure quite why it is the way it is.

In doing the publish-subscribe in D3ngeon, I took a simpler approach. The key difference is this:

In D2, when we subscribe, we pass in a listener object that wants to receive the publication, plus an event string representing the event, and a method (a function) to call on the listener when the event is published. In D3, the event string and the method name are the same. This means that every subscription is simpler by one parameter. The “cost” is that you can’t subscribe to “whatsNew” and dispatch it off to “userDesiresInfo”. You have to receive it in your “whatsNew” method. Hardly a big problem, but D3 does disallow that flexibility.

A second issue is in publish. D2’s call to the subscriber passes in the event string, the sender, and two optional additional arguments. D3 doesn’t need to pass the event string, since it’s the same as the method called, and it allows any number of optional arguments. If one of them is the sender, as it usually will be, that’s fine. If not, that’s OK too.

Let’s compare the code. Here’s how D3 does it, in the simpler QQ form:

function Queue:publish(methodName,...)
    for i,thing in ipairs(self.things) do
        local method = thing[methodName]
        assert(method, "Receiver does not support method '"..methodName.."'.")
        method(thing, ...)
    end
end

function Queue:subscribe(receiver)
    table.insert(self.things, receiver)
end

Now, in fairness, the way that D3 works is that you can publish anything, and all the subscribers are subscribed to all messages, and can ignore them if they wish. But the general form of a publication in D3 is just

Q:publish("giveCandy", player)

And everyone who implements the giveCandy method will see a call to their method:

function Witch:giveCandy(supplicant)
   supplicant:addCandy(poisonApple)
end

For today’s work in removing [fake] generality, I plan to make two changes.

Modify EventBus to accept arbitrary parameters in publish and pass them, whatever they are, on to subscribers.
Modify EventBus to use the message published as the method name to call in the subscriber.

I plan to maintain the subscription to specific messages, which is a nice feature that D3 does not have. My expectation is that when we’re done with these changes, both EventBus and its users will be simpler.

Let’s find out. Which one shall we do first?

Here, again, is our current subscribe:

function EventBus:subscribe(listener, method, event)
    assert(type(method)=="function", "method parameter must be function")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

We propose to remove the method parameter. Our subscriptions are broken out by event, which is good. We indicate that listener subscribes by putting them into that (keyed) table, with the value being the method that they want to have called. We’re going to use that method in publish:

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
        table.insert(results,result)
    end
    return results
end

We loop over the subscribers, pulling out each subscriber and method and calling the method.

Hey. Let’s switch directions. We haven’t changed anything yet. Let’s do this change:

Accept Arbitrary Parameters

We can make the switch to arbitrary parameters locally. Change this:

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
        table.insert(results,result)
    end
    return results
end

To this:

function EventBus:publish(event, ...)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, ...)
        table.insert(results,result)
    end
    return results
end

There is an issue here, which is that when there are no optionalInfo values in the first case, we pass empty tables, and now we’ll be passing nils.

I don’t recall why we did that. It seems oddly specific. Let’s first run the tests, some will surely break checking for this. If the tests are any good.

Well ain’t that a kick in the tail. No tests fail and the game runs just fine. I’m going to commit: EventBus:publish accepts arbitrary parameters, does not assume. Does not create default {} either.

I’m a little concerned about that, but I expect that we’ll be looking at every subscribe and its corresponding code, so if there is a situation that is relying on those empty tables, we’ll find it.

Now, we are about to require that every subscriber implement exactly the published string as a method. Some of those strings currently do not look like a method. So let’s first rename any such strings. I’ll search for publish first.

Here’s the first one, and it gives me something to keep in mind:

function touched(aTouch)
    CodeaUnit_Lock = false
    if aTouch.state == BEGAN then
        Bus:publish("touchBegan", nil, aTouch.pos, capture)
    elseif TouchCaptured and aTouch.state == ENDED then
        TouchCaptured:touchEnded(aTouch.pos, currentRunner.player)
        TouchCaptured:setHighlight(false)
        TouchCaptured = nil
    end
end

We’re passing a nil into the publish, because the existing rules are that that’s supposed to be the sender. The new rules are, the parameters have to match between published event and the receiver’s method of the same name. We’re not going to prejudge what they are. That means that we may want to go through matching pubs and subs and simplify the parameter lists.

For now, however, we’re just looking for events that aren’t good method names.

function InventoryItem:touchEnded(aPos, player, myPos)
    if self:isPosMine(aPos,myPos) then
        Inventory:remove(self)
        self:informObjectRemoved()
        self:usedMessage()
        Bus:publish(self.attribute, self.value1, self.value2)
    else
        self:informObjectTouched()
    end
end

This one is interesting in that it is going to publish an inventory item’s attribute. We’ll have to be sure that those are all legitimate methods. I make a sticky note for that.

My EventBus tests all use events with spaces in them, like this one:

        _:test("can subscribe to a message", function()
            local listener = FakeListener("it happened")
            Bus:publish("it happened", 7734, info)
            _:expect(itHappened).has(listener)
        end)

We’ll need to fix those. Might as well do it now. I’ll replace “it happened” with “itHappened”. I move the tab over to Sublime Text to do that job.

Tests all run. Commit: rename “it happened” event to “itHappened” in tests.

Now this:

function Lever:nextPosition()
    self.position = self.position + 1
    if self.position > 4 then self.position = 1 end
    Bus:publish("Lever", self, {name=self.name, position=self.position})
end

I generally use lower case names in methods. But this will work. We’ll save it for the final pass.

OK. All the names are without spaces. Let’s look back at the big picture. We are about to require that the subscriber implement a method matching the event name, rather than dispatch to a method of another name.

There’s an associated concern, which is that now, we pass in the event name on the call:

function EventBus:publish(event, ...)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, ...)
        table.insert(results,result)
    end
    return results
end

The intention of that was to allow a subscriber to receive multiple events in a single method and then dispatch internally. I doubt that anyone is doing that, and if they are, they should stop.

Let’s now go through al the subscribe calls and ensure that they use the same method name as event name. I happen to know that some do not.

function Dungeon:init(runner, tileCountX, tileCountY)
    self.runner = runner
    self.tileCountX = tileCountX or assert(false, "must provide tile count")
    self.tileCountY = tileCountY or assert(false, "must provide tile count")
    self.rooms = Array{}
    self.tiles = Tiles()
    Bus:subscribe(self,self.nearestContentsQueries, "requestinfo")
end

At this stage, we want to pass self.requestInfo as the method, and we need to be sure that we don’t already have a method of that name before we change this code.

    Bus:subscribe(self,self.requestinfo, "requestinfo")

function Dungeon:requestinfo(eventIgnored, playerIgnored, anyTile)
    for i,neighbor in ipairs(self:neighbors(anyTile)) do
        neighbor:doQueries()
    end
end

Good to go. Test. Good. Commit: rename method to requestInfo` to match subscribe.

Moving right along …

function Monster:initSubscriptions()
    if self:name() == "Ankle Biter" then
        Bus:subscribe(self, self.becomeDull, "dullSharpness")
    end
end

I wonder why I even did this.

function Monster:initSubscriptions()
    if self:name() == "Ankle Biter" then
        Bus:subscribe(self, self.dullSharpness, "dullSharpness")
    end
end

function Monster:dullSharpness()
    self.damageRange = {0,0}
end

Test, commit: AnkleBiter now uses dullSharpness method to match event.

Now I find this:

function Provider:init(default)
    self.default = OP:display(default or "default")
    self.items = {}
    Bus:subscribe(self,self.privAddTextToCrawl, "addTextToCrawl")
    Bus:subscribe(self,self.privAddItemsFromBus, "addItemsToCrawl")
end

function Provider:privAddItemsFromBus(event, sender, info)
    self:privAddItems(info)
end

Did I just rename those to avoid temptation, or are the plain methods in there? The former. I can just rename with impunity.

Test. We’re good. Commit: Adjust Provider subscription method names to match event names.

function Spikes:init(tile, tweenDelay)
    self.delay = tweenDelay or tween.delay
    tile:moveObject(self)
    self.damageTable = { down={lo=1,hi=1}, up={lo=4,hi=7}}
    self.assetTable = { down=asset.trap_down, up=asset.trap_up }
    self.verbs = {down="jab ", up="impale " } 
    self:up()
    self.stayDown = false
    self:toggleUpDown()
    if Bus then
        Bus:subscribe(self, self.leverHandler, "Lever")
    end
end

OK, we’re here now, so let’s fix this properly.

function Spikes:init(tile, tweenDelay)
    self.delay = tweenDelay or tween.delay
    tile:moveObject(self)
    self.damageTable = { down={lo=1,hi=1}, up={lo=4,hi=7}}
    self.assetTable = { down=asset.trap_down, up=asset.trap_up }
    self.verbs = {down="jab ", up="impale " } 
    self:up()
    self.stayDown = false
    self:toggleUpDown()
    if Bus then
        Bus:subscribe(self, self.lever, "lever")
    end
end

function Spikes:lever(event, sender, info)
    self.stayDown = false
    local pos = info.position or 1
    if pos == 4 then
        self.stayDown = true
    end
end

And the sender:

function Lever:nextPosition()
    self.position = self.position + 1
    if self.position > 4 then self.position = 1 end
    Bus:publish("lever", self, {name=self.name, position=self.position})
end

I’ve changed it to use the lower-case “l”, but left the bizarre table there. We’ll clean that up later. I make a sticky for it: “Lever publish: remove table?”

Here’s one in EventBus testing:

function TimesListener:init(event,multiplicand)
    Bus:subscribe(self, self.multiply, event)
    self.multiplicand = multiplicand
end

function TimesListener:multiply(event, sender, multiplier)
    return multiplier*self.multiplicand
end

The published event is “provideAnswer”. Let’s just change these.

Tests run. Let’s check further in the tests, clean up any ,.. no … commit now, you fool!

Commit: Lever uses ‘lever’ event and method. EventBus provideAnswer ditto.

I had forgotten to commit the Lever. Could have done two commits. I know that many of you would have. That’s not my practice in general. Now continuing to look at subscribes.

Ah. This one is interesting:

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.busTileContaining, "tileContaining")
    Bus:subscribe(self, self.draw, "drawDungeonContents")
    Bus:subscribe(self, self.busMoveObjectToTile, "moveObjectToTile")
    Bus:subscribe(self, self.busRemoveObject, "removeObject")
    Bus:subscribe(self, self.busGetTileContents, "getTileContents")
end

I vaguely recall from yesterday that we did this because the calling sequence from publish didn’t match direct calling sequences to the DungeonContents. I believe that we have removed all of those direct calls, so that we can coalesce these methods. We’ll have some revision to do later, of course, when we change the calling sequence to subscribers. If we do.

I could do this in a couple of ways. I could actually merge each method up into the busX one, and rename it to X. Or, I could rename the bus ones to be the real names, and rename the called ones to something else.

Folding them up is better. We’ll do that in two steps:

function DungeonContentsCollection:busGetTileContents(_event, _sender, tile)
    return self:getTileContents(tile)
end

function DungeonContentsCollection:getTileContents(desiredTile)
    local result = {}
    for object,tile in pairs(self.contentMap) do
        if tile == desiredTile then
            table.insert(result,object)
        end
    end
    return result
end

I see a third way. Call the lower one and change its parameter list to match the top one. That’s better.

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.busTileContaining, "tileContaining")
    Bus:subscribe(self, self.draw, "drawDungeonContents")
    Bus:subscribe(self, self.busMoveObjectToTile, "moveObjectToTile")
    Bus:subscribe(self, self.busRemoveObject, "removeObject")
    Bus:subscribe(self, self.getTileContents, "getTileContents")
end

function DungeonContentsCollection:getTileContents(_event, _sender, desiredTile)
    local result = {}
    for object,tile in pairs(self.contentMap) do
        if tile == desiredTile then
            table.insert(result,object)
        end
    end
    return result
end

That’s one step. The corresponding bus method is deleted. Test. Ah. Tests fail because we’re using direct calls in the tests. For example:

2: DungeonContentsCollection get tile contents object -- Actual: table: 0x281f99080, Expected: table: 0x281f98fc0

The test is:

        _:test("DungeonContentsCollection get tile contents", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local obj2 = { 2,3, 4}
            dc:moveObjectToTile(obj2, 45)
            local obj3 = {3,4,5}
            dc:moveObjectToTile(obj3, 34)
            local cont34 = dc:getTileContents(34)
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
        end)

I can change the test thus, only changing the call to getContents:

            local cont34 = dc:getTileContents(nil, nil, 34)

That should fix that test. And it does. There’s one more and I fix it. Tests run. Commit: getTileContents method replaces busGetTileContents.

It’s very tempting to do this whole tab all at once. I think I will succumb to the temptation, but let me point out that we could stop right now, if time had run out, and everything would work just fine. It’s just that the full refactoring isn’t complete. That’s why we do “big refactorings” only as a series of small ones.

But let’s go after the whole tab and see if we regret it.

I do tileContaining and test, expecting trouble.

1: DungonContents remembers object  -- Actual: nil, Expected: 34

Fix the test line:

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            _:expect(dc:tileContaining(object)).is(34)
            local tileTable = Bus:publish("tileContaining", nil, object)
            _:expect(tileTable[1]).is(34)
        end)

The call to dc is redundant now. Remove it entirely.

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            local tileTable = Bus:publish("tileContaining", nil, object)
            _:expect(tileTable[1]).is(34)
        end)

Why not change the tests just to do publish throughout? That won’t break any tests and then when we reduce the other remaining methods, the tests will continue to run.

        _:test("DungeonContentsCollection get tile contents", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local obj2 = { 2,3, 4}
            dc:moveObjectToTile(obj2, 45)
            local obj3 = {3,4,5}
            dc:moveObjectToTile(obj3, 34)
            local cont34 = dc:getTileContents(nil, nil, 34)
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
        end)

That becomes this:

No. I didn’t run the previous test and it doesn’t work. Grr. I wish I had an automatic test runner. I would revert but I see that I removed the moveObject thing.

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local tileTable = Bus:publish("tileContaining", nil, object)
            _:expect(tileTable[1]).is(34)
        end)

That should get me back on track. Test. Yes. And commit, stop trying to “save time” by taking larger steps. Commit: Refactoring EventBus tests to use only publish, in process.

Moving along, I was converting these tests to use Bus:publish throughout.

        _:test("DungeonContentsCollection get tile contents", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            Bus:publish("moveObjectToTile", nil, object, tile)
            local obj2 = { 2,3, 4}
            Bus:publish("moveObjectToTile", nil, obj2,45)
            local obj3 = {3,4,5}
            Bus:publish("moveObjectToTile", nil, obj3,34)
            local cont34 = Bus:publish("getTileContents", nil, 34)[1]
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
        end)

I have to admit that the [1] caused me trouble, as I predicted it would. But that’s for another day. The test above runs. Commit: continuing refactoring DungeonContents tests to use publish.

        _:test("DC can remove", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            Bus:publish("moveObjectToTile", nil, object, tile)
            local obj2 = { 2,3, 4}
            Bus:publish("moveObjectToTile", nil, obj2,45)
            local obj3 = {3,4,5}
            Bus:publish("moveObjectToTile", nil, obj3,34)
            local cont34 = Bus:publish("getTileContents", nil, 34)[1]
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
            Bus:publish("removeObject", nil, object)
            cont34 = Bus:publish("getTileContents", nil, 34)[1]
            _:expect(cont34).hasnt(object)
        end)

Tests run. Commit: Complete refactoring DungeonContents tests to use Bus.

OK. Let’s take a break and reflect a bit.

Reflection

I’ve been beavering away so hard that I haven’t finished my morning banana, nor had even one sip of my iced chai latte. And the ice has melted. Need more ice, brb.

The steps taken so far, in the DungeonContents code, have been very tiny, and indeed all the changes in preparation for improving pub-sub have been very small. It’s tempting to think that I could save time by taking bigger steps or committing less frequently.

Committing amounts to typing a sentence and clicking a button. So we can’t make a case that much savings comes from fewer commits, and we can be sure that if anything goes wrong while we have a lot of changes hanging we’ll either have to redo them after a big revert, or we’ll have a tedious search for the bug when a revert might have been better. And, finally, with these tiny steps I can stop any time for a tech support call or to search out some ice, or to implement six features and get back to this a week from Thursday.

Of course, coming back to it a week from Thursday, I’ll need to refresh my mind a bit but the point is that we can take a break from a refactoring done in small steps, and we cannot if we commit to getting it all done.

Anyway it seems to be going well so far. Let’s get back to finding subscribes without matching names. We must be nearly done with that phase.

I do find an interesting Fake object:

function FakeEventBus:subscribe(object, method, event)
    self.checkMessage = "Thank you!"
    self.subscriber = object
    _:expect(method).is(NPC.catPersuasion)
end

Oddly, that’s checking to be sure that the NPC subscribes her catPersuasion method. We’re not going to be using methods, so I’ll remove that check. Test, commit: Remove method check from NPC tests.

OK, the phase of making sure all the method names match the event names is done.

Stop Using the Method Parameter

We want to change subscribe not to use the method paremeter, and, ultimately, not to require it. But we can do it in two phases, so we will. First, make changes to use the event name to find the method. Change this:

function EventBus:subscribe(listener, method, event)
    assert(type(method)=="function", "method parameter must be function")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

To this:

function EventBus:subscribe(listener, method, event)
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = true
end

This just ensures that we have a keyed table, subscriptions with each subscriber in there. Now to change publish. At this point, however, we’re not checking to be sure that the subscription is valid, having removed the assert. Instead, let’s do this:

function EventBus:subscribe(listener, _method, event)
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = true
end

Hm. Having done that, we can save the method in the subscription after all. Why not?

function EventBus:subscribe(listener, _ignored, event)
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

This should just work. However:

EventBus:136: event drawDungeonContents must be implemented

I must have missed a mis-matched subscribe. Sure did:

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.tileContaining, "tileContaining")
    Bus:subscribe(self, self.draw, "drawDungeonContents")
    Bus:subscribe(self, self.busMoveObjectToTile, "moveObjectToTile")
    Bus:subscribe(self, self.busRemoveObject, "removeObject")
    Bus:subscribe(self, self.getTileContents, "getTileContents")
end

Fix that. Test. And

EventBus:136: event removeObject must be implemented

I’m glad I put that assert back. Find that one.

Oh silly me, I’ve not finished folding the DungeonContents methods:

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.tileContaining, "tileContaining")
    Bus:subscribe(self, self.drawDungeonContents, "drawDungeonContents")
    Bus:subscribe(self, self.busMoveObjectToTile, "moveObjectToTile")
    Bus:subscribe(self, self.busRemoveObject, "removeObject")
    Bus:subscribe(self, self.getTileContents, "getTileContents")
end

The changes are:

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.tileContaining, "tileContaining")
    Bus:subscribe(self, self.drawDungeonContents, "drawDungeonContents")
    Bus:subscribe(self, self.moveObjectToTile, "moveObjectToTile")
    Bus:subscribe(self, self.removeObject, "removeObject")
    Bus:subscribe(self, self.getTileContents, "getTileContents")
end

function DungeonContentsCollection:removeObject(_event, _sender, object)
    self.contentMap[object] = nil
    self:dirty()
end

function DungeonContentsCollection:moveObjectToTile(_event, _sender, object,tile)
    --requires DungeonObject to be defined way forward in tabs.
    if object then 
        self.contentMap[object] = tile 
        self:dirty()
    end
end

Test. Tests fail:

2: can subscribe to a message -- EventBus:136: event itHappened must be implemented

Hm. I made my change too soon. But it’ll soon sort out. I could revert and wait, but I think we’re OK moving forward. You might disagree if you were here …

function FakeListener:init(event)
    Bus:subscribe(self, self.itHappened, event)
    self.event = event
end

function FakeListener:itHappened(event, sender, info)
    if sender == 7734 and info.info == 58008 then
        itHappened[self] = self
    end
end

This will fix most of them …

4: can subscribe to different events -- EventBus:136: event something else happened must be implemented

        _:test("can subscribe to different events", function()
            local lis1 = FakeListener("itHappened")
            local lis2 = FakeListener("somethingElseHappened")
            Bus:publish("itHappened", 7734, info)
            _:expect(itHappened, "lis1 didn't get message").has(lis1)
            _:expect(itHappened, "lis2 received wrong message").hasnt(lis2)
        end)

That fixes that one. But there are more. I’m feeing my stack get too full. Let’s see if we can bull through …

1: DungonContents remembers object  -- Actual: nil, Expected: 34
4: DrawingOrder -- DungeonContentsCollection:85: attempt to get length of a nil value (local 'entry')

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local tileTable = Bus:publish("tileContaining", nil, object)
            _:expect(tileTable[1]).is(34)
        end)

This just needs conversion to use the Bus. I wasn’t as careful as I might have been there … or decided to wait until I did that method and then jumped the queue. This one is fixed this way:

        _:test("DungonContents remembers object", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            Bus:publish("moveObjectToTile", nil, object, tile)
            local tileTable = Bus:publish("tileContaining", nil, object)
            _:expect(tileTable[1]).is(34)
        end)

But what about this:

4: DrawingOrder -- DungeonContentsCollection:85: attempt to get length of a nil value (local 'entry')

Ah. Also failed to convert:

        _:test("DrawingOrder", function()
            local dc, ord
            dc = DungeonContentsCollection()
            local do3 = DO(3)
            local do2 = DO(2)
            local do1 = DO(1)
            dc:moveObjectToTile(do3,39)
            dc:moveObjectToTile(DO(3),99)
            dc:moveObjectToTile(do2,39)
            dc:moveObjectToTile(DO(3),98)
            dc:moveObjectToTile(do1,39)
            ord = dc:drawingOrder()
            local entry = ord[39]
            _:expect(#entry).is(3)
            _:expect(entry).has(do1)
            _:expect(entry).has(do2)
            _:expect(entry).has(do3)
            local level = -1
            for i,dungeonObject in ipairs(entry) do
                _:expect(dungeonObject:zLevel() > level, "level not increasing").is(true)
                level = dungeonObject:zLevel()
            end
        end)

I find that test less than clear, but I’m here to convert it, not to explain it.

        _:test("DrawingOrder", function()
            local dc, ord
            dc = DungeonContentsCollection()
            local do3 = DO(3)
            local do2 = DO(2)
            local do1 = DO(1)            
            Bus:publish("moveObjectToTile", nil, do3,39)
            Bus:publish("moveObjectToTile", nil, DO(3),99)
            Bus:publish("moveObjectToTile", nil, do2,39)
            Bus:publish("moveObjectToTile", nil, DO(3),98)
            Bus:publish("moveObjectToTile", nil, do1,39)
            ord = dc:drawingOrder()
            local entry = ord[39]
            _:expect(#entry).is(3)
            _:expect(entry).has(do1)
            _:expect(entry).has(do2)
            _:expect(entry).has(do3)
            local level = -1
            for i,dungeonObject in ipairs(entry) do
                _:expect(dungeonObject:zLevel() > level, "level not increasing").is(true)
                level = dungeonObject:zLevel()
            end
        end)

Tests are good. Commit: EventBus looks up method equal to event name. Tests modified accordingly.

Where Are We?

We’re working the change:

Modify EventBus to use the message published as the method name to call in the subscriber.

That’s done, but now our subscribers are all providing a method that we no longer want. So we want to change the subscribe protocol from this:

function EventBus:subscribe(listener, _ignored, event)
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

To this:

function EventBus:subscribe(listener, event)
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

Now we have merely to edit all the calls to subscribe to remove the method parameter.

Could we do that in multiple steps if we wanted to? Yes … what I’d do would be to check in subscribe to see which kind of call was being made and act accordingly. At some future time we could issue a warning, and then finally, when it’s all done, remove the parameter.

I think we’ll just change them all now, if you don’t mind.

I’ll show you once what I’m doing, for clarity, then I’ll spare you all the code dumps. From this:

function GameRunner:init(testX, testY)
    self.tileSize = 64
    self.tileCountX = testX or 85 -- if these change, zoomed-out scale 
    self.tileCountY = testY or 64 -- may also need to be changed.
    self.cofloater = Floater(50,25,4)
    self.dungeonLevel = 0
    self.requestNewLevel = false
    self.playerRoom = 1
    Bus:subscribe(self, self.createNewLevel, "createNewLevel")
    Bus:subscribe(self, self.darkness, "darkness")
    self.dungeon = nil -- created in defineDungeonBuilder
    self.builder = nil -- created in defineDungeonBuilder
end

To this:

function GameRunner:init(testX, testY)
    self.tileSize = 64
    self.tileCountX = testX or 85 -- if these change, zoomed-out scale 
    self.tileCountY = testY or 64 -- may also need to be changed.
    self.cofloater = Floater(50,25,4)
    self.dungeonLevel = 0
    self.requestNewLevel = false
    self.playerRoom = 1
    Bus:subscribe(self, "createNewLevel")
    Bus:subscribe(self, "darkness")
    self.dungeon = nil -- created in defineDungeonBuilder
    self.builder = nil -- created in defineDungeonBuilder
end

Tests run. Games run. Commit: Modify EventBus to select method equal to event name. Extensive one-line changes to simplify callers.

In fact I changed a few lines in each of twelve tabs. It’s easy to see that in a larger system we might have elected to do this in several steps. On the other hand, with a smart enough IDE, it would have been trivial to do.

What’s Next?

Let’s review publish and subscribe yet again.

function EventBus:publish(event, ...)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, event, ...)
        table.insert(results,result)
    end
    return results
end

We are passing the event on down to all the subscribers. Now that the event name is the same as the method name called, that’s redundant. We’d like to change the code to this:

function EventBus:publish(event, ...)
    local results = {}
    for subscriber,method in pairs(self:subscriptions(event)) do
        local result = method(subscriber, ...)
        table.insert(results,result)
    end
    return results
end

To make this change, however, we have to find all the methods that are called via publish. Not the calls to subscribe, but the methods implied by those calls. Here’s an example of what needs to be done to make this change work. Given this:

function Dungeon:init(runner, tileCountX, tileCountY)
    self.runner = runner
    self.tileCountX = tileCountX or assert(false, "must provide tile count")
    self.tileCountY = tileCountY or assert(false, "must provide tile count")
    self.rooms = Array{}
    self.tiles = Tiles()
    Bus:subscribe(self, "requestinfo")
end

We need to change this:

function Dungeon:requestinfo(eventIgnored, playerIgnored, anyTile)
    for i,neighbor in ipairs(self:neighbors(anyTile)) do
        neighbor:doQueries()
    end
end

We want to remove both those parameters, really. Where are the publishers? Are they sending in the caller? We don’t want them if so.

function Player:requestinfo()
    Bus:publish("requestinfo", self, self:currentTile())
end

So we need to change the publish and the receiving method to correspond. I think the best way to go through this is by searching for “publish” and then finding the corresponding receiving methods. We’ll start with this one.

function Player:requestinfo()
    Bus:publish("requestinfo", self:currentTile())
end

And this:

function Dungeon:requestinfo(eventIgnored, playerIgnored, anyTile)
    for i,neighbor in ipairs(self:neighbors(anyTile)) do
        neighbor:doQueries()
    end
end

Becomes this:

function Dungeon:requestinfo(anyTile)
    for i,neighbor in ipairs(self:neighbors(anyTile)) do
        neighbor:doQueries()
    end
end

This is going to be tedious and error-prone. But it simplifies the code every time we do it. Is there a way to do it incrementally? I’m not thinking of one.

Let’s gut it out and see how long it takes. It’s 1218.

It’s not 1238 and I think I have them all. Also I think I’m probably wrong, and I’m a bit worried about it. Let’s test a bit.

One test fails, and I think it should.

1: Callback Test -- Button:88: attempt to index a nil value (local 'pos')

Yes, we’re faking en event:

        _:test("Callback Test", function()
            local callbackButton = nil
            local b = Button("foo", 100,100, 20,20)
            local t = {pos=vec2(105,105), state=BEGAN}
            local tEnd = {pos=vec2(200,200), state=ENDED}
            _:expect(b:isPosMine(t.pos)).is(true)
            b:touchBegan(event, sender, t.pos, function(button)
                callbackButton = button
            end)
            _:expect(callbackButton).is(b)
        end)

        _:test("Callback Test", function()
            local callbackButton = nil
            local b = Button("foo", 100,100, 20,20)
            local t = {pos=vec2(105,105), state=BEGAN}
            local tEnd = {pos=vec2(200,200), state=ENDED}
            _:expect(b:isPosMine(t.pos)).is(true)
            b:touchBegan(t.pos, function(button)
                callbackButton = button
            end)
            _:expect(callbackButton).is(b)
        end)

That should fix that. Test. Touch the Lever. Crash:

Spikes:70: attempt to index a nil value (local 'info')
stack traceback:
	Spikes:70: in local 'method'
	EventBus:132: in method 'publish'
	Lever:28: in method 'nextPosition'
	Player:280: in local 'action'
	TileArbiter:37: in method 'moveTo'
	Tile:129: in method 'attemptedEntranceBy'
	Tile:442: in function <Tile:440>
	(...tail calls...)
	Player:216: in method 'moveBy'
	Player:152: in method 'executeKey'
	Player:206: in method 'keyPress'
	GameRunner:243: in method 'keyPress'
	Main:107: in function 'keyboard'

I missed this method, now fixed:

function Spikes:lever(info)
    self.stayDown = false
    local pos = info.position or 1
    if pos == 4 then
        self.stayDown = true
    end
end

Test … Trying to use the curePoison, I get this crash:

attempt to index a nil value
stack traceback:
	[C]: in for iterator 'for iterator'
	Tests:505: in function 'manhattan'
	Inventory:381: in function <Inventory:380>
	(...tail calls...)
	Inventory:275: in local 'method'
	EventBus:132: in method 'publish'
	Main:118: in function 'touched'

Here again I just missed one:

function Inventory:touchBegan(pos,callback)
    local count = self:count()
    for i, entry in pairs(self:entries()) do
        local entryPos = self:drawingPos(i,count)
        if entry:isPosMine(pos, entryPos) then
            callback(entry)
        end
    end
end

Better now. Test. Everything seems fine. I confess to less than perfect confidence. And I did notice something interesting, when testing the ?? query function, I got occasional messages “I am a valuable nothing at all”. I’ve not seen that before, but I think it can happen if we were to create a Loot with nothing in it. I’ll make a sticky to chase it.

I tested inventory objects extensively in game and they seem to add points as they should so I’ll tear up that sticky.

I’m ready to commit, but do feel a bit of concern. Let’s commit and explore my concerns. Commit: Change all subscribers to no longer expect event name and sender, which are no longer automatically provided.

That was a 15 file commit. No wonder I’m concerned.

Concerns

Of course there’s a simple common reason for feeling concern over a commit: our tests are not providing the confidence that they should.

In this case, there should be a test for every object that subscribes to an event, verifying that its method is present and that it accepts the parameters expected. That’s not the case, and I can assure you that I’m not going to fix it today. I do, however, make a sticky card for it.

As a stopgap, I can only inspect, and the inspection is both tedious and error-prone. Well, “all” I have to do is go through each subscribe and be sure the corresponding method doesn’t expect event and sender. And, I suppose, double check each publish to be sure we’re not passing anything that looks like sender.

OK, I’ll do that. Time is 1303.

It’s 1307 and I’ve checked all the subscribes. It’s 1309 and I’ve done all the publishes. Not too bad, but if there were hundreds, that would be bad.

On the other hand, in a language that checks calling sequences, the compiler would warn us.

Anyway, my confidence is pretty high now, but I grant that tests would be better. One of the reasons we’re doing these improvements is to reduce coupling and make testing easier. Perhaps one day we’ll actually add some tests.

Let’s sum up.

Summary

We set out today to “remove generality”, by taking out the explicit event and sender that had to be passed in and dealt with in publish-subscribe. The pub-sub code is a bit simpler, and the calling code, publish, invariably has one fewer parameters, while the receiving method invariably has two fewer parameters, which were never being used in even one case.

The changes to do this were small but frequent. There were 14 commits over 3 1/2 hours, touching about 45 non-unique tabs, counting each tab each time it occurred in a commit.

The last phase, changing all the callers, took the longest and I wish I had been able to see a way to do that one incrementally. If I had, I’d also have greater confidence that the system isn’t broken, since we’d have found a way to make the old style and new style coexist. Perhaps a special subclass of EventBus with the new protocol would have been good, then you’d say NewBus:whatever when you swapped things over. Or maybe the other way around, OldBus and Bus.

Be that as it may, I didn’t think of it nor do it, so I had to take a much larger step than was comfortable, but I seem to have gotten away with it this time.

We’ve simplified the system across the board by removing uneeded and awkward fake generality. It’s a good day when that happens.

See you next time!

EventBus

Change EventBus Subscribe

Accept Arbitrary Parameters

Reflection

Stop Using the Method Parameter

Where Are We?

What’s Next?

Concerns

Summary