Dungeon 299

Let’s apply some of the ideas from D3 in the curreent Dungeon program. We’ll see whether they’re a step toward a better design.

It wouldn’t be fair to say that today’s changes to D2 are based on learning from D3, but they’re definitely based on ideas that working on D3 has given me. D3 itself is just a few experiments with a broadcast style of publication, with individual objects interacting even though they do not have pointers to each other. So far the experiment has inspired me, rather than teaching me. But inspiration, combined with existing knowledge and intuition, is enough for today.

I have in mind a two-step process.

First, I plan to change the publish-subscribe EventBus so that it can return results. This is practical because EventBus sends messages immediately, so it can easily accumulate and return whatever might be returned from its subscribers.

Then, once we can return values to a publisher, I plan to use that capability to allow objects to publish to the Bus when they want something. This will reduce direct coupling to other objects, ultimately simplifying the object connection web we currently have.

At least that’s the plan. We’ll see what really happens, but I think it’ll go as described here.

Returning Values from the Bus

We’ll begin with a test.

        _:test("publishers can receive returns", function()
            
        end)

Not much of a test but it states the test’s purpose. That will help me focus on what I’m doing now, and later it should help me understand what the test is about.

The tests for EventBus include a FakeListener class:

FakeListener = class()

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

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

I think in this test we’ll want a different event, just for clarity. And let’s arrange it so that the new event can return different values, and then we’ll subscribe a couple of listeners to get a collection of values back.

That lets me write this test:

        _:test("publishers can receive returns", function()
            local answers = {}
            local lis1 = FakeListener("provideAnswer", 15)
            local lis2 = FakeListener("provideAnswer", 37)
            local mul = 2
            answers = Bus:publish("provideAnswer",mul)
            _:expect(answers).has(30)
            _:expect(answers).has(74)
        end)

Let me describe that in words. The listeners will subscribe to their first parameter. They will save the second in a member variable. When they get the published message, it will include the parameter mul. The service method will return that value times their saved value. The Bus will accumulate the values, and the expects will find them.

Ah. Not quite. In the tests so far, the FakeListener knows what method should be called. This is different from the publisher in D3, where the message on the publish is the method you are required to implement.

We’ll do best to have a new kind of listener.

            local lis1 = TimesListener("provideAnswer", 15)
            local lis2 = TimesListener("provideAnswer", 37)

Easy enough to build another fake:

TimesListener = class()

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

I think that’ll do the right thing. Of course we’re not accumulating results yet, so the expectations will both fail.

Hmm, they don’t fail quite as I had in mind:

6: publishers can receive returns -- EventBus:81: attempt to perform arithmetic on a table value (local 'multiplier')

Best look at the bus. Maybe it packs all the extra args into a table?

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    for subscriber,method in pairs(self:subscriptions(event)) do
        method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
    end
end

That looks OK, we only provide an empty table if we have no info. I’m not sure why that’s the default but we should be OK here. Oh. We’re supposed to provide the sender in the publish:

        _:test("publishers can receive returns", function()
            local answers = {}
            local lis1 = TimesListener("provideAnswer", 15)
            local lis2 = TimesListener("provideAnswer", 37)
            local mul = 2
            answers = Bus:publish("provideAnswer",nil, mul)
            _:expect(answers).has(30)
            _:expect(answers).has(74)
        end)

I added a nil sender. Now we should fail the expectations. Well, not quite:

6: publishers can receive returns -- CodeaUnit:94: bad argument #1 to 'for iterator' (table expected, got nil)

So far, the publish is returning nil. So I don’t have a table to inspect. Make publish do the right thing. It changes from this:

function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
    for subscriber,method in pairs(self:subscriptions(event)) do
        method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
    end
end

To 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

I expect the test to pass. Furthermore, it does.

Commit: Bus:publish now returns a collection of returns from the receivers.

Let’s reflect.

Reflection

Would an object be better?

The thing we’re returning here is just a table, not a real object. It’s wise whenever we use a system type to think about whether some smarter object would make more sense. In this case, I don’t see much value to an object, since different users will have different purposes for what comes back.

(I think that in a few moments we’ll find that that’s not quite true, but it’ll be ok for now.)

Do we need a more complex return?

Another concern is that we might have a desire to return some key-value information, implying that we should have a more robust and complicated return interface. Here again, we have no need. We try not to generalize beyond our need here chez Ron.

So it’s good enough for now and perhaps forever.

Let’s find a place to use it.

There’s a red flag right there. The thing we just did, making EventBus able to return values, is in itself speculative. We have no actual purpose for it. Should I have waited? Should I have found a place, written the call, and used that as my test?

Heck, I don’t know. Maybe. This is good enough. We use judgment here, we don’t blindly follow rules. Consistency is good, but so is flexibility. And we won’t have any trouble finding something to do with this.

The D3 version is cleaner.

In the D3 version, the message published, such as “provideAnswer” is the method you must implement to receive the message. In D2, you subscribe to the message and provide a method, possibly with a different name, which is to be called.

The advantage to that, I guess, is that you can have the method name make sense in your class, while the publication name makes sense to the requestor. But it adds complexity to the code, and to our understanding. If someone publishes “xyz”, to find out who deals with it, we may find a whole list of different objects and methods.

I think we over-designed the Bus. It happens. We’ll leave it for now, and perhaps forever.

Let’s get to it.

Using a Return from Publish

Our purpose in all this is to reduce direct coupling in our objects. Let’s look at our DungeonObjects to see what they know.

Oh here’s an interesting one right at the top:

function DungeonObject:currentTile()
    return DungeonContents:tileContaining(self)
end

We have this new global object, DungeonContents. It handles a number of requests and is in charge of drawing the dungeon objects in zLevel order, among other tasks.

Would it be better, we ask ourselves, self, would it be better if DungeonContents subscribed to some Bus messages? Could we reduce references to the global, perhaps eliminate them entirely?

We would need someone, global or not, to hold onto the DungeonContents lest it be collected as garbage, but reducing use of a global seems worthy. Let’s start with tileContaining.

We have pretty good tests for DungeonCollection, so let’s add a test.

We’ll want a fresh bus:

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

There’s a nice test for tileContaining already:

        _: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)
        end)

Let’s just enhance that to do a publish:

        _: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)

This should fail because of no subscribers to the message, and therefore a nil in table[1].

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

I love it when a plan comes together.

Now subscribe:

function DungeonContentsCollection:init()
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, self.busTileContaining, "tileContaining")
end

I realized that I had to provide a different method, because of Bus’s passing more arguments. No problem, or at least not a large one.

function DungeonContentsCollection:busTileContaining(_event,_sender,object)
    return self:tileContaining(object)
end

I think this does the trick. Test. Arrgh.

DungeonContentsCollection:105: attempt to index a nil value (global 'Bus')
stack traceback:
	DungeonContentsCollection:105: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in global 'DungeonContentsCollection'
	Tests:440: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:458: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:434: in function 'testManhattanDistance'
	[string "testManhattanDistance()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

Yikes. This isn’t my test, this is in testManhattanDistance. We need to set up a Bus for that test. So be it.

Ah. I discover that that test has a Bus set up but didn’t set it up in time. Just needed to reorder its before. Test again. Our test runs.

Commit: DungeonContentsCollection subscribes to “tileContaining”.

Now we can use it. Let’s find some tileContaining calls.

Our first candidate is the one we already found:

function DungeonObject:currentTile()
    return DungeonContents:tileContaining(self)
end

That becomes:

function DungeonObject:currentTile()
    local t = Bus:publish("tileContaining", self, self)
    return t[1]
end

It’s rather a pain to do the publish, since it expects that self parameter and then the actual argument. That’s the way of it, however.

Tests pass, game works. Commit: DungeonObject:currentTile uses Bus, no longer references DungeonCollection global.

Now we’re seeing what this thing looks like in place. And the table aspect is a bit troubling, just kind of odd.

But we can write this:

function DungeonObject:currentTile()
    return Bus:publish("tileContaining", self, self)[1]
end

That’s nearly good enough. I was thinking to return an object and use :first() but the [1] is probably decent. We’ll let it ride for now, see what else we might need.

Are there more calls to tileContaining?

function Dungeon:playerTile()
    return DungeonContents:tileContaining(self:getPlayer())
end

We replace:

function Dungeon:playerTile()
    return Bus:publish("tileContaining", self, self:getPlayer())[1]
end

Test a bit. We’re good. Commit: Dungeon:playerTile uses Bus publish rather than DungeonContents reference.

More … that’s all for that method. Let’s have a look at what other references there are to DungeonContents.

function GameRunner:drawMapContents()
    pushMatrix()
    self:scaleForLocalMap()
    DungeonContents:draw()
    popMatrix()
end

function Tile:addContents(aDungeonObject)
    DungeonContents:moveObjectToTile(aDungeonObject, self)
end

function Tile:getContents()
    return DungeonContents:getTileContents(self)
end

function Tile:removeContents(anEntity)
    DungeonContents:remove(anEntity)
end

That’s all there is. Let’s start with draw:

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

We can call draw here because draw takes no parameters. And to do the work:

function GameRunner:drawMapContents()
    pushMatrix()
    self:scaleForLocalMap()
    Bus:publish("drawDungeonContents")
    popMatrix()
end

Test. Works. Commit: DungeonContents:draw called via Bus, not via global.

This is going rather nicely, I think.

function Tile:addContents(aDungeonObject)
    DungeonContents:moveObjectToTile(aDungeonObject, self)
end

That becomes:

function Tile:addContents(aDungeonObject)
    Bus:publish("moveObjectToTile", self, aDungeonObject, self)
end

And we’ll have to subscribe and map the call.

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

function DungeonContentsCollection:busMoveObjectToTile(_event,_sender,object, tile)
    return self:moveObjectToTile(object,tile)
end

Test. We’re good. Commit: Tile uses Bus to moveObjectToTile, not DC global.

Now:

function Tile:getContents()
    return DungeonContents:getTileContents(self)
end

Becomes:

function Tile:getContents()
    return Bus:publish("getTileContents", self, self)
end

And:

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.busGetTileContents, "getTileContents")
end

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

Test. Nope. Return is wrong, I think.

function DungeonContentsCollection:busGetTileContents(_event, _sender, aTile)
    return self:getTileContents(aTile)[1]
end

That [1] is going to be easy to forget. Must think about that. Game works but a few tests fail:

7: Room:announce places in ring  -- Actual: 2, Expected: 1
(4 times)
15: TileArbiter: player can step on key and receives it  -- Actual: table: 0x281cc1240, Expected: table: 0x281cc0240

I’m hoping these guys just don’t have a Bus set up, but I am concerned.

The first test has a Bus already. It’s going to take some study. Let me check the TileArbiter one, it might be an easier fix. Hm, no, it has a Bus also.

Weird test. With some explanatory messages added, it’s:

        _:test("TileArbiter: player can step on key and receives it", function()
            local room = Room(1,1,21,21, runner:tileFinder())
            local pt = Tile:room(11,10,runner:tileFinder())
            local player = Player(pt,runner)
            runner.player = player
            local kt = Tile:room(10,10, runner:tileFinder())
            local key = Key(kt)
            _:expect(player:keyCount()).is(0)
            _:expect(kt:getContents(),"contents has no key").has(key)
            local arb = TileArbiter(key,kt, player,pt)
            local moveTo = arb:moveTo()
            _:expect(moveTo, "move is incorrect").is(kt)
            _:expect(player:keyCount()).is(1)
            _:expect(kt:getContents(), "key not removed from tile").hasnt(key)
        end)

The error is:

15: TileArbiter: player can step on key and receives it key not removed from tile -- Actual: table: 0x281e5fcc0, Expected: table: 0x281e5f740

The diagnostics from has and hasnt are always opaque but it seems clear that the key was not removed from the tile. “The changes we made shouldn’t have affected that.”

The first thing I notice is that I didn’t return the first element from the table that comes back, so I do this:

function Tile:getContents()
    return Bus:publish("getTileContents", self, self)[1]
end

The second thing I notice is this:

function DungeonContentsCollection:busGetTileContents(_event, _sender, aTile)
    return self:getTileContents(aTile)[1]
end

That shouldn’t have the [1]. I put it in the wrong place. Remove it. That makes the first batch of tests work OK. But this is still failing:

15: TileArbiter: player can step on key and receives it key not removed from tile -- Actual: table: 0x281ed9f80, Expected: table: 0x281ed9440

Why would that be happening?

This has taken most of the wind from my sails. It has been a long morning already, so I’m a bit tired and thought I’d just tick through these last two changes.

I’ll take one look to debug and then revert if I don’t immediately see the issue.

The TileArbiter should just send startActionWithKey to player. That’s this:

function Player:startActionWithKey(aKey)
    self.characterSheet:addKey(aKey)
    sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
    aKey:take()
end

That calls:

function Key:take()
    self:currentTile():removeContents(self)
end

We haven’t done anything about removeContents yet, have we?

function Tile:removeContents(anEntity)
    DungeonContents:remove(anEntity)
end

function DungeonContentsCollection:remove(object)
    self.contentMap[object] = nil
    self:dirty()
end

I’m going to put in some prints, but I feel that I should revert and fold my tent for the morning. I have an errand to run anyway.

OK, print doesn’t help me. Revert.

OK, back to green. Time for my errand. I’ll finish the article when i get back.

And I’m Back …

I’m refreshed from a nice top-down cruise to the pharm, so I want to get back at it. But since the last thing didn’t go quite well, I think I’ll change the other occurrence first. Let’s digress and talk about that for a moment.

A revert may be just the thing

There are two cases. One is that I made some silly mistake and the tests didn’t run. The other is that there is a conceptual or deep implementation flaw in this idea. I think the former is far more likely, because the changes to the Bus have been so trivial and not invasive.

When that happens, if the mistake isn’t instantly obvious, I find reverting to be a good idea. When I do the change a second time, I seem not to make the same mistake again. Even better is to defer doing it for a while, if there’s something else to do, so that any bad assumptions will have a moment to extinguish. That’s why I’m going to do removeContents next.

But the “big” lesson is: “When you’ve probably done something clumsy, a revert may be just the thing.”

function Tile:removeContents(anEntity)
    DungeonContents:remove(anEntity)
end

We change this to a publish:

function Tile:removeContents(anEntity)
    Bus:publish("removeObject", self, anEntity)
end

And we subscribe:

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

function DungeonContentsCollection:busRemoveObject(_event, _sender, object)
    return self:remove(object)
end

I have high hopes for this, but if it fails, that, too, will be interesting.

We’re green. Commit: Tile:remove uses Bus rather than DungeonCollection global.

Now for the last one. I’ll try to keep beginner’s mind here.

function Tile:getContents()
    return DungeonContents:getTileContents(self)
end

Convert to a Bus call:

function Tile:getContents()
    return Bus:publish("getTileContents", self, self)
end

And cater to it:

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

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

And I note that I’m not unwrapping the return, so:

function Tile:getContents()
    return Bus:publish("getTileContents", self, self)[1]
end

This really ought to work.

And it does. I wonder why it didn’t work before. But I don’t wonder much. Some typo, or something. I do some extensive gameplay just to be sure. Looking good. There are now no references to the global DungeonContents. Therefore, in GameRunner, this:

function GameRunner:dcPrepare()
    TileLock = false
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self)
    Announcer:clearCache()
end

Can become this:

function GameRunner:dcPrepare()
    TileLock = false
    self.dungeonContents = DungeonContentsCollection()
    MonsterPlayer(self)
    Announcer:clearCache()
end

Test again. I know there are tests referencing the global, we’ll go after them shortly. Commit: In Game, DungeonContents global is never addressed as global, only via Bus:publish.

Now for the other creations. In some tests, I just don’t save and restore the old DC. I am confident enough that the GameRunner will create a new one for every dungeon level we run.

In DungeonBuilder, I do this:

function DungeonBuilder:dcPrepare()
    TileLock = false
    self:createTiles()
    self:clearLevel()
    self.dungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

I’m a bit concerned about this. It might get collected when the builder is collected. There was similar code in GameRunner:dcPrepare, which I think may be redundant? Yes, that’s only used in the Hex level, which I well and truly plan to remove. Ah, but wait!

DungeonContentsCollection registers with the EventBus. Therefore the EventBus will keep it alive. We can replace all assignments of DungeonContentsCollection with simple calls.

function GameRunner:dcPrepare()
    TileLock = false
    DungeonContentsCollection()
    MonsterPlayer(self)
    Announcer:clearCache()
end

function DungeonBuilder:dcPrepare()
    TileLock = false
    self:createTiles()
    self:clearLevel()
    DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

    _:describe("DungeonAnalyzer", function()
        
        _:before(function()
            _bus = Bus
            Bus = EventBus()
            DungeonContentsCollection()
            runner = GameRunner(25,15)
            runner:createTestLevel(0)
            dungeon = runner:getDungeon()
            _TileLock = TileLock
            TileLock = false
        end)
    
    _:describe("TileFinder", function()
        
        _:before(function()
            _bus = Bus
            Bus = EventBus()
            DungeonContentsCollection()
        end)
    
    _:describe("Spikes", function()
        
        local runner
        local room
        
        _:before(function()
            DungeonContentsCollection()
        end)

I leave the actual DungeonContentsCollection tests alone. They call the base methods and test them. One more extensive test and then let’s sum up.

Tests run. Commit: all global references and global definitions of DungeonContents are removed. All in-game references are via Bus.

Summary

So. What do we think? We have removed an entire Global from the game. Or, putting it another way, after many lines of change we have removed only one global from the game.

The cost of doing this is a bit of complexity in how we do the things the former global did for us. Instead of saying

local x = DungeonContents:doX(a,b)

We now have to say:

local x = Bus:publish("doX", self, a, b)[1]

There’s no question that this is a bit slower than the direct call. It’s not going to turn up in the game, where a cycle is generally at human pace, but it’s certainly running more code. The notation is a bit obscure, but I think we’ll quickly get used to it. I am concerned about the need to do the [1] thing, but if we want to return all the replies if there’s more than one receiver of the publication, we need it. I am willing to predict, however, that at some point I’ll wind up forgetting it, or doing it when I shouldn’t.

There are things we could do. We could have a simpler publish where we don’t have to pass in that extra “self” Not much help. We could have a publishWithOneReturn method. We’d have to remember to use it, in which case we can remember the [1].

We could change the rules on pub-sub so that the method called is the same as the message, but that just improves the subscription a bit, which isn’t a big deal anyway.

I’m not sure we’ll ever have a use for multiple subscribers all returning their result. We could change it so that it only returns the first thing that comes back … or the last … which would obviate the need for the [1].

Let’s not worry about that for now. I think of it this way:

We’ve done a fairly extensive experiment testing the use of publish rather than direct messages to a well-known object. It has worked excellently, and shows promise.

There’s one more thing. DungeonContentsCollection now looks like this:

DungeonContentsCollection = class()

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

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

function DungeonContentsCollection:busMoveObjectToTile(_event,_sender,object, tile)
    return self:moveObjectToTile(object,tile)
end

function DungeonContentsCollection:busRemoveObject(_event, _sender, object)
    return self:remove(object)
end

function DungeonContentsCollection:busTileContaining(_event,_sender,object)
    return self:tileContaining(object)
end

function DungeonContentsCollection:dirty()
    self.orderTable = nil
end

function DungeonContentsCollection:draw()
    pushMatrix()
    pushStyle()
    for tile,entries in pairs(self:drawingOrder()) do
        if tile:isVisible() then
            local gc = tile:graphicCenter()
            for i,object in ipairs(entries) do
                --zLevel(object:zLevel()) -- breaks visibility??
                spriteMode(CENTER)
                object:draw(false, gc)
            end
        end
    end
    popStyle()
    popMatrix()
end

function DungeonContentsCollection:drawingOrder()
    local order -- maps tile to array sorted by object level.
    if not self.orderTable then
        order = {}
        for object,tile in pairs(self.contentMap) do
            local ord = order[tile] or {}
            table.insert(ord,object)
            if #ord > 1 then
                table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
            end
            order[tile] = ord
        end
        self.orderTable = order
    end
    return self.orderTable
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

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

function DungeonContentsCollection:remove(object)
    self.contentMap[object] = nil
    self:dirty()
end

function DungeonContentsCollection:tileContaining(object)
    return self.contentMap[object]
end

There are two related methods for everything except draw, the busXXX and the XXX method. We could now fold those together and reduce the breadth of the class. We’d have to change a couple of tests, but that would be no problem.

What this is telling me is that we would probably prefer that a class support bus methods or direct call methods, but not both. We could create a Façade for the Bus to call. That might be interesting enough to do just to see how it would look.

There’s a real possibility that EventBus is over-generalized and that we should either rare back and simplify it, or perhaps provide an alternative form of subscription that makes the call without passing in the event and sender.

We’ll leave these ideas on the table to be discovered. Time will tell us if we should improve things.

For now, I’m pleased with the outcome. D3 made me think that indirect calls through Bus might help untangle our objects, and so far, the work in D2 is confirming that it seems likely to be a bit better. I grant, however, that some folks might not like not knowing who’s going to deal with a request. For me, just knowing that someone will seems sufficient.

A fun morning, and it even went pretty much according to plan.