My plan for the day is to find a place to try out my clever idea, so that we can look at it in place and decide whether to allow it. Will it stay in?

You may recall that many of the lower-level objects in the dungeon have access to the GameRunner via a member variable. I have an aversion to lower-level objects having pointers upward. It’s usually OK but if a higher level object were to get replaced, existing objects would be holding on to the wrong object. What we have is OK, but I don’t love it.

So I have the idea of using the system Bus to publish a message that includes a container object, probably just an array. Subscribers to that message can put answers into the array, so that when the publish returns, the answers are there in the array.

This isn’t exactly an everyday idea. Normally we might publish a message and replies would be sent back via another publish to which we would subscribe. But it’s not entirely odd. Many of you will have used languages and systems where you can issue an HTTP message and write code that makes it appear that the reply comes right back. The mechanisms behind the scenes deal with all the sends and replies that it takes to make that look ordinary.

So maybe it’s OK. To decide, I want to do it and look at the result.

We’ll begin by looking for objects that have a GameRunner member variable, and observing how they might use it. Then we’ll pick a likely situation and try my “clever” publish-subscribe idea, and decide whether it is too clever to be allowed to live.

Key

I picked Key to look at first, because I observe that it doesn’t ever use the runner, even though it receives it upon creation and saves it:

Key = class(DungeonObject)

function Key:init(tile, runner)
    self.runner = runner
    tile:moveObject(self)
end

function Key:draw(tiny, center)
    if tiny then return end
    sprite(asset.builtin.Planet_Cute.Key,center.x,center.y, 50,50)
end

function Key:getTile()
    return DungeonContents:getTile(self)
end

function Key:query()
    Bus:informDotted("Could I possibly be ... a key???")
end

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

It would seem that we could just remove that parameter. There are a few reasons why that might not be OK.

It is barely possible that getTile or removeContents need the runner. If they do, they are ripping it out of the object via a direct access.

Another possibility is that the superclass requires the runner. We might save it in the subclasses, because of my uncertainty about initializing superclasses.

There are at least two ways to find out. One is to remove the saving of the runner and run the tests. The other is to review the code. I’ll start with the former. I make the change, wander around, find a key, and take it. It all works. I’m pretty confident. But it seems prudent to check the superclass anyway.

A quick glance assures me that there are no references there:

DungeonObject = class()

function DungeonObject:zLevel()
    return 5
end

function DungeonObject:identify()
    return "DungeonObject"
end

If I’m going to ignore the runner in Key, I should find its creators and not bother to provide it. I do find a couple of tests that create a Key directly, but I don’t find the code that drops them around in the system. A bit more careful use of Find discovers this:

function DungeonBuilder:customizeContents()
    self:placeSpikes(15)
    self:placeLever()
    --self:placeDarkness()
    self:placeNPC()
    self:placeLoots(10)
    self:createDecor(30)
    self:createThings(Key,5)
    self:createThings(Chest,5)
    self:placeWayDown()
    self:setupMonsters()
end

function DungeonBuilder:createThings(aClass, n)
    for i = 1,n or 1 do
        local tile = self:randomRoomTileAvoidingRoomNumber(self.playerRoomNumber)
        aClass(tile,self)
    end
end

We have a general-purpose creating method that passes the runner to whomever it creates. There are two cases: either Chests require runner, or they do not. If they don’t we can simplify all the way down.

Alternately, I could do this and commit right now:

function Key:init(tile, _runner)
    tile:moveObject(self)
end

I use the underbar to indicate that the runner is ignored. Other times I might say ignoredRunner, but the underbar trick is more idiomatic Lua. Now I can commit this, which means nothing needs to be unwound if Chest doesn’t go my way. Commit: Key no longer requires runner parameter on creation.

Now about Chest. It seems not to need its runner either. Remove the save and test. That works, so I make the changes throughout:

function Chest:init(tile)
    tile:moveObject(self)
    self.closedPic = "mhide01"
    self.openPic = asset.open_chest
    self.pic = self.closedPic
    self.opened = false
end

function Key:init(tile)
    tile:moveObject(self)
end

function DungeonBuilder:createThings(aClass, n)
    for i = 1,n or 1 do
        local tile = self:randomRoomTileAvoidingRoomNumber(self.playerRoomNumber)
        aClass(tile)
    end
end

And there were several tests to change. Easy enough, just remove the second parm. Test, commit: createThings no longer passes runner to Key or Chest creation.

Retrospective

So, this is nice. Looking for places to be clever, so far all I’ve found is places that can be simpler. This is good. I sort of wonder why we ever passed the runner down, and why we didn’t remove it when it wasn’t needed, but these things happen.

I think the trick is to observe places like this where things can safely be improved, and to take the opportunity, even if it wasn’t quite what we had in mind.

Moving on …

Moving on to more references to self.runner …

There’s a reference to runner in the MonsterPlayer, part of the MusicPlayer code, but it doesn’t interest me much. And there are a number of references in Entity, Monster, and Player. Recall that Monster and Player inherit from the base class Entity (which inherits from DungeonObject).

This is somewhat nasty:

function Entity:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:getTile())
end

Why nasty? Because Entity does not initialize runner, so in principle it has no right to access it. However, Entity is an abstract class, that is, never instantiated on its own, and both Player and Monster do init runner. So it works, but it’s a bit nasty. Furthermore, I see no particular reason why Player would ever ask this question. If she does the answer is always zero. Let’s move that method down to Monster and see what happens.

I get an immediate crash:

AttributeSheet:25: attempt to call a nil value (method 'manhattanDistanceFromPlayer')
stack traceback:
	AttributeSheet:25: in method 'drawSheet'
	OperatingModes:13: in method 'drawSheet'
	AttributeSheet:16: in method 'draw'
	Player:130: in method 'drawInLargeAndSmallScale'
	OperatingModes:9: in method 'drawInLargeAndSmallScale'
	Player:112: in method 'drawExplicit'
	Player:108: in method 'draw'
	DungeonContentsCollection:113: in method 'draw'
	GameRunner:159: in method 'drawMapContents'
	GameRunner:125: in method 'draw'
	Main:102: in function 'draw'

I decide to fix that this way:

function Player:manhattanDistanceFromPlayer(_)
    return 0
end

Test. All seems well. I think that’s better. Commit: Move manhattanDistanceToPlayer to Player and Monster from Entity.

Still haven’t found any place to be clever. But let’s look at the uses we’re making of runner in Monster (and then Player if we find anything good in Monster).

The method we just moved is one candidate:

function Monster:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:getTile())
end

There are more:

function Monster:xScaleToFaceTowardPlayer()
    local dir = self.runner:playerDirection(self:getTile())
    return ((dir.x > 0) and self.facing) or -self.facing
end

function Monster:pathComplete()
    self.runner:removeMonster(self)
end

function Monster:startActionWithPlayer(aPlayer)
    self.runner:initiateCombatBetween(self,aPlayer)
end

Now if we are to do this, the point would need to be to remove the runner member entirely from Monster. Removing just some uses might be a step in that direction, but unless we think we can get rid of them all, there’s no value.

Of these, the first two need the return trick that I have in mind. The latter two do not, they are just requests or announcements and do not expect a return.

Now that I’m staring this code right in the eye, I’m not as interested in my clever trick as I was. But let’s do one to see what it looks like. Let’s convert this:

function Monster:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:getTile())
end

The code here would become roughly this:

function Monster:manhattanDistanceFromPlayer()
    local result = {}
    Bus:publish("playerManhattanDistance", self, self:getTile(), result)
    return result[1]
end

Then GameRunner would need to subscribe to that message:

    Bus:subscribe(self, self.playerDistance, "playerManhattanDistance")

And implement the new method:

function GameRunner:playerDistance(event, sender, tile, result)
    local dist = self:playerManhattanDistance(tile)
    table.insert(result,dist)
end

This might work. Test. In fact it does work. OK, now we can assess the situation.

We’ve replaced this:

function Monster:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:getTile())
end

With all this:

function Monster:manhattanDistanceFromPlayer()
    local result = {}
    Bus:publish("playerManhattanDistance", self, self:getTile(), result)
    return result[1]
end

...
    Bus:subscribe(self, self.playerDistance, "playerManhattanDistance")
...

function GameRunner:playerDistance(event, sender, tile, result)
    local dist = self:playerManhattanDistance(tile)
    table.insert(result,dist)
end

The benefit? If we do this in four separate places, we can remove the runner member variable from Monster. We haven’t changed how things work: the monster still relies on the GameRunner to do things for it. We’ve just removed the direct reference to the runner, replacing it with an indirect reference that is at least a bit harder to trace. And we’ve done that odd trick about putting a single result into a table, just to get it back to the caller.

It’s not terribly difficult to find out who subscribes to the message, a simple Find will do it. But it is still less obvious than the direct self.runner:tellMeThis.

My conclusion, in this case, is that it isn’t worth it. I am gratified to confirm that the publish-subscribe feature can be used this way, because, who knows, maybe someday it will be worth it. Today is not that day.

Revert.

Summary

I really wanted to try that trick, and finally found a place where it was at least possibly a good idea. Along the way, I found two places where I could just remove the up-reference without any other changes, and one place where moving a method down to a subclass made more sense.

Doing the trick replaced one perfectly clear line with seven lines that are undeniably less clear, although once one got used to the pattern, they wouldn’t be too awful. And possibly I could have reduced the impact a bit by folding the original GameRunner method together with the subscription receiver.

In the end, even though I really like this idea, I just don’t see the payoff. There may be a place in this system, or some future system, where this style makes sense. Here, I just don’t see it.

Does this mean that I was wrong to try it? I don’t think so. If I hadn’t tried it, my assessment of the idea would have been too optimistic, perceiving the cleverness without clearly seeing the drawbacks. Next time an opportunity to do something like this turns up, I’ll be better qualified to assess it.

So I learned a bit and improved the code a bit, and avoided making the code more obscure at some cost in change and–possibly–mistakes that I’d make along the way to removing all four of those references.

I didn’t get to be clever, but I did get to try it out successfully. A decent result, and not the one I expected. That’s always good.

See you next time!