Dungeon 307

Some thoughts on something GeePaw said. And I think we’d better complete the fixing of the publish situation. What is the best we know how?

In last Tuesday’s Friday Night Coding meeting, GeePaw Hill said something that I want to reflect on. He said that even when he knows some code is temporary, he tries always to write it the best way he knows how.

That best, if I’m not mistaken, and that would be the way to bet, includes both micro-testing, and high quality code. We don’t need to define high quality here, or anywhere. We all have our own image of that, and for our purposes that’s good enough. It will surely include meaningful names, and with any luck, low coupling, high cohesion, and so on.

The deep point here is that, of us all, Hill is probably the most simultaneously aware that code is temporary, and that we will therefore see it again, and that it therefore needs to be in good order, exactly because it is temporary. It will need replacement and revision. As such, we’ll need to be able to understand it, and it will need to be easy to replace.

Again, we don’t need to go into great detail here, but let’s agree that if we’re going to replace code, it’s helpful if all the users of that code will be easy to identify, and the changes that they need to make will be easy to make. So even our temporary code needs to have a sensible interface, and it needs to be close enough to the right interface to allow us to plug in a better version without the need to revise all its many callers in some complicated way.

One last thought, the matter of testing. GeePaw uses, even I use my TDD microtesting practice because it makes me go faster. It doesn’t always feel faster, because it has a rather gentle rhythm, but when I look at the time from starting to having something working, it is generally faster. There are surely many reasons, but the primary one, for me, is that when I do not do my up front testing, I have occasional long periods of manual testing and difficult debugging, and these are much more rare when I test up front. So even if the cycle from starting to thinking I’m done were shorter without my TDD tests—and I would not bet that they are shorter—I still go faster because I eliminate long debugging cycles almost entirely.

Why are you writing this, Ron?

I’m writing this because I believe that I do not always “write it the best way I know”. I believe that, often, I rush to be done, with or without tests, and leave a bit of a mess. I believe that hurts me, and that over time, it hurts a lot. So I’m writing this to remind myself to write things the best way I know, even things that aren’t going to last, even things where I’m in a hurry.

We’ll see how that goes. Watch me. Call me out if in some article, you see me cutting corners or falling short of what the code, and I, and especially you deserve.

The Bus Situation

Yesterday I put in a fix for the issue with the Bus holding on to otherwise dead objects, fixing the fix from the day before that broke the system entirely. (And I didn’t even notice!)

That fix involved a special method to “flush” the Bus, removing all the Dungeon Objects and Buttons.

function EventBus:flush()
    local subs = self:allSubscribers()
    for _i,sub in ipairs(subs) do
        if sub:is_a(DungeonObject) or sub:is_a(Button) then
            self:unsubscribeAll(sub)
        end
    end
end

We were aware, yesterday, that the Dungeon object is held onto over this operation, and therefore there will be an instance of Dungeon on the Bus for each level in the game. That seems to be mostly harmless, and in fact yesterday I created 1000 levels without a memory overflow, so we’re not in any immediate danger. However, it’s not right, and we knew that. The fix was a temporary patch to get the system back up.

Now we need to figure out, and do, something more robust:

Scopes

Based on something that Ryan Latta said, even if it isn’t what he meant, I think we’ll go with the notion of “scopes” in the Bus. Roughly, we’ll have subscribers name the “scope” of their interest. Initial scopes will be “game” and “level”. We will extend our notion of flushing to flush subscribers of a given scope, and we’ll use that to flush the “level” subscribers before creating a new level.

My current plan is just that the scope amounts to a token, identifying the scope of interest of the subscriber. I do not propose to provide nesting, automatic flushing, or any such thing. Those might be useful for some situations, but we have no need of them yet, so we won’t do them yet.

This change means that all calls to subscribe probably need to provide a scope. I considered a different approach, such that we’d tell the Bus:

Bus:scope("level")

And it would flush the old “level” subscribers, and then all future subscribers would automatically assigned to “level”, but that seems to me to be too spooky. Better to have all our publish calls be explicit, I think. I do think we’ll have a call like scope that does the flushing, but it won’t do anything clever.

Let’s get started. I see two necessary steps:

The “Plan”

The Dungeon needs to be part of the “level” scope, because we create a new one for each level. However, it is presently created in GameRunner, and passed to the DungeonBuilder, who fills it in. The code is here:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    local builderView = self.dungeon:asBuilderView()
    self.builder = DungeonBuilder(self, builderView, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    return self.builder
end

Later, in DungeonBuilder, we have this:

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

So, if the Dungeon were to subscribe at the “level” scope, as it should, it would already be subscribed when we get to dcPrepare, and its subscriptions would be lost.

So one task before us is to untangle the dungeon creation / subscription / flushing so that it happens correctly.

And the other task, of course, is to implement our new “level” notion in EventBus and use it.

It seems that we may have to untangle Dungeon before changing EventBus, but I think that’s clearly not the case: we’ve already changed EventBus in a very similar way to what we’ll do now, and we are already living with the Dungeon building up extra copies.

I think untangling Dungeon creation will be tricky. Accordingly, I wonder if we can find a short-term improvement to get rid of duplicate dungeons in the Bus.

OK. I’m settling on a plan. We’ll implement the scope notion on EventBus, and we’ll set dungeon objects and Buttons to “level” and we’ll set everything else, including Dungeon, to “game”. That will preserve the bug, which has been harmless, and will get most of the world running correctly on Bus scopes.

Then, after that, we’ll sort out the Dungeon. We might undertake a short term fix, or we might get to the long term one directly.

Now we must digress.

Digression: Is this “the best we know”?

We’re talking here about preserving a known bug, avoiding improving the order of things the code does, and perhaps even patching in a short term fix.

Is this “the best we know how to do”?

Well, I admit I’m really tempted to mount my high horse and “do it right”, but the commitment here is to do even temporary changes in the best way we know. So we can make a short-term decision, and then program it well.

So there is no contradiction.

Still, it would be better if we could untangle Dungeon first. I think it’ll be hard but I feel that I owe it to the universe to at least try it.

Step 1: Create Dungeon in Builder

Hm. It turns out that the only creation of the Dungeon is here:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    local builderView = self.dungeon:asBuilderView()
    self.builder = DungeonBuilder(self, builderView, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    return self.builder
end

Can that be true? I think it is. In tests that need the dungeon, we seem to be calling getDungeon on the Runner, so this should be the only thing needing a change. It does get tricky but let’s try it. Consider this a spike to learn something. We’ll just modify the code in place.

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self, nil, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    self.dungeon = builder:getDungeon()
    return self.builder
end

I’ve removed the dungeon creation and the creation of the builderView. They’ll have to move to DungeonBuilder:

function DungeonBuilder:init(runner, _NilDungeonView, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    local dungeon = Dungeon(runner, runner.tileCountX, runner.tileCountY)
    self.view = dungeon:asBuilderView()
    self.dungeon = dungeonView -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

We’ll need getDungeon:

function DungeonBuilder:getDungeon()
    return self.view:asFullDungeon()
end

That seems like it might work. Let’s test to see how far off we are.

GameRunner:123: attempt to index a nil value (global 'builder')
stack traceback:
	GameRunner:123: in method 'defineDungeonBuilder'
	Tests:22: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:33: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:13: in function 'testDungeon2'
	[string "testDungeon2()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

Ah. Used a temp that I shouldn’t have:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self, nil, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    self.dungeon = builder:getDungeon()
    return self.builder
end

This becomes:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self, nil, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    self.dungeon = self.builder:getDungeon()
    return self.builder
end

Test. A test fails:

1: creation no dungeon -- Actual: nil, Expected: nil

And a crash:

DungeonBuilder:258: attempt to index a nil value (field 'dungeon')
stack traceback:
	DungeonBuilder:258: in method 'dungeonAnalyzer'
	DungeonBuilder:353: in function <DungeonBuilder:352>
	(...tail calls...)
	DungeonBuilder:300: in method 'placeLoots'
	DungeonBuilder:220: in method 'customizeContents'
	DungeonBuilder:74: in method 'buildLevel'
	GameRunner:76: in method 'createLevel'
	Main:69: in function 'setup'

The DB code is:

function DungeonBuilder:dungeonAnalyzer()
    return self.dungeon:dungeonAnalyzer()
end

Oh, same kind of problem in DB init. This:

function DungeonBuilder:init(runner, _NilDungeonView, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    local dungeon = Dungeon(runner, runner.tileCountX, runner.tileCountY)
    self.view = dungeon:asBuilderView()
    self.dungeon = dungeonView -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

Should be this:

function DungeonBuilder:init(runner, _NilDungeonView, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    local dungeon = Dungeon(runner, runner.tileCountX, runner.tileCountY)
    self.view = dungeon:asBuilderView()
    self.dungeon = self.view -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

Test. Everything works. That wasn’t so bad.

The best we know how?

Now that we’ve shown we can do it, let’s do it the best we know how. In particular, we should clean up the calling sequence for DungeonBuilder, not to include a dungeonView, and to include the size desired.

We could work from where we are now, or revert and do over. In this case, I think we can get to good code faster from here.

Let’s see who all create DungeonBuilders. Only GameRunner. We do all our testing through it. Terribly complex tests but good for now. So we just have to fix this. We pass in the tileCounts …

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self, self.tileCountX, self.tileCountY, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    self.dungeon = self.builder:getDungeon()
    return self.builder
end

And we modify DB to expect that sequence:

function DungeonBuilder:init(runner, tileCountX, tileCountY, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    local dungeon = Dungeon(runner, tileCountX, tileCountY)
    self.view = dungeon:asBuilderView()
    self.dungeon = self.view -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

This should be equivalent. We’ll test of course. We’re good. Commit: Dungeon now created only by DungeonBuilder.

That pleases me, we’ve already made the campground a bit nicer.

Step 2: Adding Scope Idea

Now let’s move on to adding the notion of scope to the EventBus.

We’ll start from this existing test:

        _:test("flush all DungeonObjects, leave others", function()
            local o1, o2
            local d1, d2
            o1 = Other()
            o2 = Other()
            d1 = DungObj()
            d2 = DungObj()
            b1 = Button("x",0,0,10,10)
            local subs = Bus:allSubscribers()
            _:expect(#subs).is(5)
            Bus:flush()
            subs = Bus:allSubscribers()
            _:expect(#subs).is(2)
            _:expect(subs).has(o1)
            _:expect(subs).has(o2)
        end)

This is my big chance to figure out just how this feature will be used. My best idea so far is this:

1. All calls to subscribe will include a new parameter, “scope”. We may want to limit it to “game” and “level” in the code.
2. The flush function will require a scope parameter, and will flush all those, and only those subscribers who have signed up with that scope.

In the existing test above, the subscribe is in the objects:

local Other = class()

function Other:init()
    Bus:subscribe(self, "someMessage")
    Bus:subscribe(self, "anotherMessage")
end

function Other:someMessage()
end

function Other:anotherMessage()
end

local DungObj = class(DungeonObject)

function DungObj:init()
    Bus:subscribe(self, "someMessage")
    Bus:subscribe(self, "anotherMessage")
end

function DungObj:someMessage()
end

function DungObj:anotherMessage()
end

Instead, let’s do the subs visibly, and with better class names.

As I work, I get to thinking: should it be possible for an object to subscribe at both levels? The rule now is that if an object is selected for flushing, all subscriptions are cleared. We could, in principle, remove only the subscriptions at the level we’re flushing.

I think no, that’s more weird than useful. Back to crafting the test.

        _:test("flush 'level' scope only", function()
            local o1Game, o2Both, o3Lev
            o1Game = TestSubscriber()
            Bus:subscribe(o1Game, "someMessage", "game")
            Bus:subscribe(o1Game, "anotherMessage", "game")
            o2Both = TestSubscriber()
            Bus:subscribe(o2Both, "someMessage", "game")
            Bus:subscribe(o2Both, "anotherMessage", "level")
            o3Lev = TestSubscriber()
            Bus:subscribe(o3Lev, "someMessage", "level")
            Bus:subscribe(o3Lev, "anotherMessage", "level")
            local preFlush = Bus:allSubscribers()
            _:expect(#preFlush).is(3)
            Bus:flush("level")
            local postFlush = Bus:allSubscribers()
            _:expect(#postFlush).is(1)
            _:expect(postFlush[1]).is(o1Game)
        end)

I think that’s what we’ve agreed to. I’ll turn off the other flush test but keep it for reference for a bit.

This test will fail for want of TestSubscriber. It does. Remove one of those other classes and make the other into TestSubscriber:

local TestSubscriber = class()

function TestSubscriber:someMessage()
end

function TestSubscriber:anotherMessage()
end

That’s going to require me to comment out or delete the other test. Delete it. We have no fear, we have REVERT.

Test fails:

7: flush 'level' scope only  -- Actual: 3, Expected: 1

Better comments in the test, please:

        _:test("flush 'level' scope only", function()
            local o1Game, o2Both, o3Lev
            o1Game = TestSubscriber()
            Bus:subscribe(o1Game, "someMessage", "game")
            Bus:subscribe(o1Game, "anotherMessage", "game")
            o2Both = TestSubscriber()
            Bus:subscribe(o2Both, "someMessage", "game")
            Bus:subscribe(o2Both, "anotherMessage", "level")
            o3Lev = TestSubscriber()
            Bus:subscribe(o3Lev, "someMessage", "level")
            Bus:subscribe(o3Lev, "anotherMessage", "level")
            local preFlush = Bus:allSubscribers()
            _:expect(#preFlush, "preFlush should be 3").is(3)
            Bus:flush("level")
            local postFlush = Bus:allSubscribers()
            _:expect(#postFlush, "postFlush should be 1").is(1)
            _:expect(postFlush[1], "only o1Game should survive").is(o1Game)
        end)

Test for the record:

7: flush 'level' scope only postFlush should be 1 -- Actual: 3, Expected: 1

I kind of expected the check for o1Game etc to fail but since we got them all back, it could be first. Anyway we have work to do.

I think that in order to keep the game working, we need to allow scope to be missing until we fix all the subscribes in the system. Let’s default it to … game, I guess.

Ah. Time for a bit of design here. How are we going to represent scope? Presently we have 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

function EventBus:subscriptions(event)
    local subscriptions = self.events[event]
    if not subscriptions then
        subscriptions = {}
        self.events[event] = subscriptions
    end
    return subscriptions
end

That is, we have a table from event (a name) to a table (of subscribers) and the table of subscribers has as its key the object subscribed, and as value, the method to be called. The method is checked in subscribe and then cached.

I like that the key to the subscription is the object. But we need some way to save both the scope and the method (or we could refetch the method given the event name, but no). Let’s create a little object to save, containing both level and method. We’ll see how it looks when we’ve done it.

I write the initial version of that presumptively. Should probably have written the test line first, but that’s coming right up.

local Subscription = class()

function Subscription:init(scope, method)
    self.scope = scope
    self.method = method
end

However this goes, I think I’m going to break a lot of tests. We’ll see …

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

Test to see what blows up. Millions of tests. We’d better make publish work with the new scheme:

function EventBus:publish(event, ...)
    for subscriber,subscription in pairs(self:subscriptions(event)) do
        subscription.method(subscriber, ...)
    end
end

We’re fetching the method directly, but the object belongs to us. I’ll allow it for now. We also need this:

function EventBus:query(event, ...)
    for subscriber,subscription in pairs(self:subscriptions(event)) do
        return subscription.method(subscriber, ...) -- first one
    end
end

Test. I think it might mostly work now. Still lots of things explode. I think we need a default for scope:

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

Why do I think that? Because otherwise it’d be nil and I don’t think we want to be passing nils all around, they tend to make things unhappy. Test again without much optimism.

Yeah, well, still 20 or 30 test are failing.

I am not sure why these things are failing. One very real option is the dreaded REVERT but I want to give myself a fair chance, so I look to see why the EventBus tests themselves are failing.

2: can subscribe to a message  -- Actual: table: 0x2808e2800, Expected: table: 0x2808e2300
3: two subscribers each get message  -- Actual: table: 0x2808e3000, Expected: table: 0x2808e2540
3: two subscribers each get message  -- Actual: table: 0x2808e3000, Expected: table: 0x2808e1680
4: can subscribe to different events lis1 didn't get message -- Actual: table: 0x2808e3380, Expected: table: 0x2808e3900

And so on.

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

Hm, what does FakeListener do?

FakeListener = class()

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

function FakeListener:itHappened(info)
    if info.info == 58008 then
        itHappened[self] = self
    end
end

So we didn’t get to itHappened.

This seemed so simple. Should I revert and go again? No.

The bug is a typo of “subsciption” for “subscription”. Test. Everything passes except my test:

7: flush 'level' scope only postFlush should be 1 -- Actual: 3, Expected: 1

Now to make flush work.

function EventBus:flush(scope)
    local subs = self:allSubscribers()
    for _i,sub in ipairs(subs) do
        if sub.scope == scope then
            self:unsubscribeAll(sub)
        end
    end
end

I expect good news here. But the ghosts will be back until we use levels. I am disappoint:

7: flush 'level' scope only postFlush should be 1 -- Actual: 3, Expected: 1
7: flush 'level' scope only only o1Game should survive -- Actual: table: 0x280b0e540, Expected: table: 0x280b0f780

But why, mama, why???

Oh. That flush needs to be rewritten. We need to do it longhand, we’re not checking on class, but on specific subscriptions.

function EventBus:flush(scope)
    for event, subs in pairs(self.events) do
        for subscriber, subscription in pairs(subs) do
            if subscription.scope == scope then
                self:unsubscribeAll(sub)
            end
        end
    end
end

Test.

7: flush 'level' scope only -- EventBus:278: table index is nil

Hmn, what’s that? Bad call to unsub:

function EventBus:flush(scope)
    for event, subs in pairs(self.events) do
        for subscriber, subscription in pairs(subs) do
            if subscription.scope == scope then
                self:unsubscribeAll(subscriber)
            end
        end
    end
end

That should do it. Yes, tests are green.

We really need a safety commit, but if we do, we can’t ship, because the ghost bug will be back.

If this were a real product situation, I’d keep the old flush message and call it when scope is nil in the new one, thus running the old class-based flush. Here, I don’t feel the need for that. I’ll commit to lock in these changes and move to use the new facility.

It pops into my mind that we need to be careful, because although Dungeon is created in DungeonBuilder, we’re probably not doing the Bus flush in the right place. If we do it in dcPrepare, where it is now, we’ll flush the Dungeon that we created in init.

Let’s do that first. We’ll move the Bus:flush() out of dcPrepare and do it in DungeonBuilder:init:

function DungeonBuilder:init(runner, tileCountX, tileCountY, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    Bus:flush("level")
    local dungeon = Dungeon(runner, tileCountX, tileCountY)
    self.view = dungeon:asBuilderView()
    self.dungeon = self.view -- used to mark things needing improvement
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

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

Test this. Should all work pretty well even though we’re not done. Yes. Now the real work, all the subscribes need to be updated. I’ll spare you the list.

OK, that’s done. Test. Enter game and create a second level, dump the Bus. No duplicates. I think we are good to go.

Commit: All subscribers provide scope. Scope “level” is flushed when a new level is created.

Now let’s go back to EventBus and make sure that scope is always provided and is always either “game” or “level”. I put level “XXX” in some tests, so those should break and give me a chance to think more clearly about them.

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

Test. Improve the message:

    assert(scope=="game" or scope=="level", "scope must be game or level, not "..tostring(scope))

Test again. All the errors are like this:

2: can subscribe to a message -- EventBus:260: scope must be game or level, not XXX

So I can search for those with “XXX” and fix them. I think I can safely set them all to “game” but it’s better to set them to “level”, because if I do, any that slip out of testing will all get flushed in the first flush.

We’re green. Commit: Bus:publish only allows game and level as scopes.

Now let’s see whether this code is “the best we know how”.

function EventBus:allSubscribers()
    local subs = {}
    for event,subTable in pairs(self.events) do
        for sub, _true in pairs(subTable) do
            subs[sub] = true
        end
    end
    local result = {}
    for sub, _true in pairs(subs) do
        table.insert(result,sub)
    end
    return result
end

That could be more clear if it used Composed Method:

function EventBus:allSubscribers()
    local subs = self:subscribersAsSet()
    return self:setToArray(subs)
end

function EventBus:subscribersAsSet()
    local subs = {}
    for event,subTable in pairs(self.events) do
        for sub, _true in pairs(subTable) do
            subs[sub] = true
        end
    end
    return subs
end

function EventBus:setToArray(set)
    local result = {}
    for element, _true in pairs(set) do
        table.insert(result,element)
    end
    return result
end

Test. Commit: apply Composed Method to EventBus:allSubscribers.

We might consider whether we could do these more nicely with our array and table operators, but for now this will hold. We’re here to clean up the whole camp here.

I do take a quick look at the fp functions that we have for operations like filter and nothing looks terribly helpful here. We might dig further into that but if we can get down to clear Lua code here, we’ll be happy.

function EventBus:subscribe(listener, event, scope)
    assert(scope=="game" or scope=="level", "scope must be game or level, not "..tostring(scope))
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscription = Subscription(scope,method)
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = subscription
end

This could be more clear. Let’s try this first:

function EventBus:subscribe(listener, event, scope)
    assert(scope=="game" or scope=="level", "scope must be game or level, not "..tostring(scope))
    local method = listener[event]
    assert(type(method)=="function", "event "..event.." must be implemented")
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = Subscription(scope,method)
end

I think that’s better but we still have that rigmarole up top. Try further:

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

function EventBus:validate(listener, event, scope)
    local method = listener[event]
    assert(scope=="game" or scope=="level", "scope must be game or level, not "..tostring(scope))
    assert(type(method)=="function", "event "..event.." must be implemented")
    return method
end

That’s better, I think, and works fine. But we can do this:

function EventBus:subscribe(listener, event, scope)
    local subscription = self:validate(listener, event, scope)
    local eventSubscriptions = self:subscriptions(event)
    eventSubscriptions[listener] = subscription
end

And then …

function EventBus:validate(listener, event, scope)
    local method = listener[event]
    assert(scope=="game" or scope=="level", "scope must be game or level, not "..tostring(scope))
    assert(type(method)=="function", "event "..event.." must be implemented")
    return Subscription(scope, method)
end

One more change, rename the validator:

function EventBus:subscribe(listener, event, scope)
    local subscription = self:validSubscription(listener, event, scope)
    local eventSubscriptions = self:subscriptions(event)
    eventSubscriptions[listener] = subscription
end

function EventBus:validSubscription(listener, event, scope)
    local method = listener[event]
    assert(scope=="game" or scope=="level", 
        "scope must be game or level, not "..tostring(scope))
    assert(type(method)=="function", 
        "event "..event.." must be implemented")
    return Subscription(scope, method)
end

I like that, and hope that you do.

I’m calling this good, and we’ll commit: Improve code in EventBus with Composed Method and renaming.

Let’s sum up:

Summary

I think we have done a good thing today. We’ve improved how the EventBus helps us with subscriptions of varying durations. We’ve improved the overall program structure a bit, with the Dungeon being created in the DungeonBuilder rather than passed in. We were able to be driven by one new test and otherwise by our existing tests. With one brief exception, your faithful author was not confused.

I did make one “card”:

DCCollection unsub is weird given level exists

The DungeonContentsCollection is inherently weird, as it is a class that acts like an instance. It is doing the right thing about subscriptions, but we could and perhaps should bring it in line. I’m content with the situation, DCC was out of scope of our current effort.

Things went well and this has been good. And …

The best we know how?

I think the final question here is “did we write it the best we know how”?

We’d have to interrogate Hill to see what he does, but my take is that we’ve written it the best we know how, given the time it makes sense to invest. We could make the code somewhat better, perhaps, if we:

• Improve the FP array functions to include ways to make sets and arrays, and then use them.
• Create a helper object or some other means to split the now-private methods like validSubscription from the few public methods of EventBus.
• Seek out and improve every short name like subs and make it better.

I’d have to say that I have rarely, if ever, written code that I could not improve in some way, perhaps only a very small way. If I interpret “the best I know how” that strictly, I’d never be done.

I’ll have to think further about what my commitment needs to be: I would prefer to hold myself to a standard I can live with. But I freely state that I feel that the code I produce here in the Dungeon could benefit from a bit more care than I’ve been giving it.

I’m not trying to get to perfect publication-ready code, code that all my betters and peers would gaze upon in wonder, finally leaving the profession because they have seen, at last, the perfect code, and know that they, on their best day ever, could never approach such a pinnacle of quality.

I’m trying to get to the kind of code that we see every day, code written by someone with a headache or a sick cat or a lot of pressures we don’t even know about. Code that exists in the real world of imperfect people, imperfect specs, and far less time than we need.

But … at the same time … I’m trying to show us all, including myself, that even in that real world, we can get to code we can be proud of, code that has a high probability of doing what we want, code that we can deal with readily when we encounter it again.

Decent code, where there might be ideas for improvement, but none of the ideas start with WHAT WAS THIS ^&#@$@! THINKING???.

I think I can do a bit better than I’ve been doing at that. I think that even under pressure, we should not race so fast that we’re leaning forward and sure to fall down. We should trust that our tests and decent clean code will be the fastest way to get where we’re going.

I think I’ve done OK at that … and that I can do better.

“Each of you is perfect the way you are … and you can use a little improvement.”
– Shunryū Suzuki