Dungeon 302

A bit of thinking and doing with the publish feature. Small changes, much thinking.

I’ve done a questionable thing with the publish-subscribe capability of the EventBus: I’ve allowed subscribers to return values. This is uncommon: I haven’t found it in the literature anywhere. More common would be for one object to publish a event, and another object to publish a different event, which the first object could receive. Even that form of conversation hasn’t crossed my radar as a pattern.¹

Now in networked applications, we do see a request to a site, and a reply coming back later. The “later” is often hidden in the library software, making it appear that the original request call has returned with a useful result. That’s a lot like what I’m doing here, although there is no network latency or clever coroutine/multi-tasking going on. My publish calls all the subscribers and they return, with the publish accumulating the results, all in one go.

On a Slack that I belong to, I asked members about what I’m doing here, with an eye to better names, or anything, to improve the fact that, when I expect a single result, the code is:

result = Bus:publish("myQuery")[1]

That [1] at the end of the line is easy to miss, and easy to forget. So, I asked my colleagues.

GeePaw Hill expressed a lot of concern over the whole idea. I confess that I can’t do justice to his concerns. I think it’s because we’re envisioning different schemes or situations, but am not sure. One thing he said that I’ll quote here “My worry is that important business logic is getting implicit here”.

Where does this leave us? Well, we have this thing we’ve done. It works rather nicely, and allows an object that wants information to get it without needing to know what object provides it, which reduces direct code coupling. (There is still run-time coupling, and there is the implicit coupling that the query made and the implementation have the same name.)

Anyway, I like the idea, and along the way I came up with some better names.

Query

I propose to recast the EventBus code so that

• publish calls do not anticipate a return and return nil;
• query calls expect exactly one return value;
• queryAll calls expect a collection of return values.

We’ll check this, but if there are no uses for queryAll, we may not provide it, as it would constitute speculation.

We have some tests for EventBus, so let’s review them and create some new ones.

        _:test("can subscribe to a message", function()
            local listener = FakeListener("itHappened")
            Bus:publish("itHappened", info)
            _:expect(itHappened).has(listener)
        end)
        
        _:test("two subscribers each get message", function()
            local lis1 = FakeListener("itHappened")
            local lis2 = FakeListener("itHappened")
            Bus:publish("itHappened", info)
            _:expect(itHappened).has(lis1)
            _:expect(itHappened).has(lis2)
        end)
        
        _:test("can subscribe to different events", function()
            local lis1 = FakeListener("itHappened")
            local lis2 = FakeListener("somethingElseHappened")
            Bus:publish("itHappened", info)
            _:expect(itHappened, "lis1 didn't get message").has(lis1)
            _:expect(itHappened, "lis2 received wrong message").hasnt(lis2)
        end)
        
        _:test("can unsubscribe", function()
            local lis1 = FakeListener("itHappened")
            Bus:publish("itHappened", info)
            _:expect(itHappened).has(lis1)
            Bus:unsubscribeAll(lis1)
            itHappened = {}
            Bus:publish("itHappened", info)
            _:expect(itHappened, "lis1 was unsubscribed, got message").hasnt(lis1)
        end)
        
        _: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", mul)
            _:expect(answers).has(30)
            _:expect(answers).has(74)
        end)

Well. That last test is just what we don’t want, but it’s a good start. Let’s stick to our guns and demand that publish returns nil, that query returns exactly one, and, if we must, that queryAll returns multiples. It’s getting hard to resist building it, for reasons that will appear shortly.

Recast this test to use query and expect one result, first demanding that publish doesn’t return results.

        _:test("query returns result, publish does not", function()
            local answer
            local lis1 = TimesListener("provideAnswer", 15)
            local mul = 2
            answer = Bus:publish("provideAnswer", mul)
            _:expect(answer, "publish returned something").is(nil)
            answer = Bus:query("provideAnswer", mul)
            _:expect(answer).is(30)
        end)

This should fail with the “returned something” error and then for lack of a query method.

6: query returns result, publish does not publish returned something -- Actual: table: 0x285b9f9c0, Expected: nil
6: query returns result, publish does not -- EventBus:66: attempt to call a nil value (method 'query')

Perfect. I think that to make this work, we’re going to start with some duplication:

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

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

The publish has been changed just to call the method. The query has been changed to return only the first result (or nil if the table were empty).

Our current test runs, but many others fail, for example:

7: Room:announce places in ring -- Tile:247: attempt to index a nil value

We need to replace any publish that expects to use its result. It would perhaps be better to preserve the old publish behavior for one generation. I decide to fix the user calls immediately.

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

This wants to be a query:

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

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

Similarly:

function Dungeon:playerTile()
    return Bus:query("tileContaining", self:getPlayer())
end

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

Similarly.

function DungeonObject:currentTile()
    return Bus:query("tileContaining", self)
end

A couple of test lines:

            local cont34 = Bus:publish("getTileContents", 34)[1]

I think I’m good. Test:

1: DungonContents remembers object -- DungeonContentsCollection:36: attempt to index a nil value (local 'tileTable')
2: DungeonContentsCollection get tile contents -- DungeonContentsCollection:48: attempt to index a nil value

Apparently I’m not good. Two tests. The first becomes:

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

It was doing the [1] in the expect, so my search didn’t find it. And:

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

The query above was a publish...[1] before. Test. Tests are good. We have a bit of duplication to think about:

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

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

I just did the query with the same loop for two reasons. First, I copy-pasted the method. Second, I wanted the automatic return of nil if the collection is empty.

One might say “shouldn’t more than one in the subscribers be an error?” and one would not be entirely wrong, but my practice is not to throw errors that no one wants.

There is a somewhat obscure way to get an index and value from a table, the next function. We could do this, for example:

function EventBus:query(event, ...)
    local subscriber,method = next(self:subscriptions(event),nil)
    if subscriber then return method(subscriber, ...) end
end

The next function is built in to Lua and given a table and an index, returns the next index and value in the same order as for would provide them. It’ll return an index of nil if there is one … I think. I’m nearly certain that’s what it does.

Honestly, next is pretty obscure by my standards, and I think I prefer the early-out of the loop, so I go back to this:

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

There may be some fancy way to get rid of the duplication here, but I don’t see it.

Commit: EventBus:publish returns no results. query returns one.

OK, I like that better than the horrid [1] thing. Let’s reflect on our tiny change.

Reflection

Before I forget it, I just took a short walk around the house, and bopped a bit to “Baby You Can Drive My Car”, and had an evil thought.

I could change the EventBus so that if you send it a message, it publishes the message, or treats it as a query. That is, saying

result = Bus:tileContaining(something)

Would be equivalent to saying

result = Bus:query("tileContaining", self)

I think that would be evil, and I’m not going to do it. But it is tempting, just because it’s possible. Part of what holds me back is that we wouldn’t know whether to publish it or treat it as a query.

That did make me think that if we wished, we could extend Bus to allow two kinds of subscriptions, group ones, subscribing to “publish” and the query type, which could enforce there being only one subscriber.

We have no need of that, that we’re aware of, so we won’t do that either. But we could, and if we need it, we will.

We could even have two objects, Bus and Query …

Enough. Let’s think about what we have now.

We have a Bus, and it responds to publish by sending a message to everyone who has expressed interest, and it responds to query by sending the message to the first object that expressed interest, and returning the result. If respondents to publish return things, we don’t care, and if more than one person responds to a query, we don’t care, we return the first response.

The primary purpose of all this, it seems to me, is to allow an object to communicate with related objects without knowing those objects. We use this to publish messages to the information crawl, or for dungeon objects to inform other objects about what’s going on, such as a Lever being changed, or a Darkness tile being stepped upon. The Inventory uses this extensively: use of any Inventory item causes a message to be published, and whatever object needs to know about that item subscribes. So the Horn Girl subscribes to “catPersuasion”, while AnkleBiters subscribe to “dullSharpness”.

The dungeon game is a bit unlike many other OO programs, in that it comes fairly close to a multi-tasking situation, where there are a lot of loosely-connected independent objects interacting with each other. Most of the OO programs I’ve encountered are more of a top-down sort of thing, where higher-up objects use underling objects to get things done.

The result of this somewhat collaborative design is that dungeon entities would need to know a lot of other entities, or would need connections to high-level objects, in order to communicate. We have some of that, and it seems to mess things up. Objects often need to know the GameRunner or the Dungeon, to get things done. Whether they are given that object an initiation, or whether the object has global scope, it gets messy.

In particular, it has made micro-testing very difficult, because in order to test some low-level mostly standalone kind of object, we need to provide it a lot of environment, runner, dungeon, tiles, and so on. This has resulted in fewer tests, which results in testing in the game rather than relying on the micro-tests for confidence. That same interconnection is also surely slowing down development, and certainly makes refactoring more difficult.

It is my intention that the pub-sub-query capability will reduce the need for objects to know each other, allowing them to work more or less on their own. I expect this to make testing, refactoring, and general development easier.

It’s early days but my feeling so far is that it’s working. Testing via the Bus does often require use of a fake receiver / query responder, but while I am not fond of test fakes, it’s far easier than setting up a whole dungeon to test one object.

At the same time, I’m aware that wise programmers I know have questions about what I’m doing, so I’ll try to remain alert to problems arising due to this scheme. But my primary focus in refactoring, I think, will be to try to reduce explicit coupling using pub-sub-query.

If you have comments, observations, questions, hit me up. And stop by next time.