This one’s gonna be rambling. A design idea keeps coming back to me. I feel that the program would be much better, if …

The dungeon program’s focus, to a large degree, is on the Tile. The dungeon is a rectangular array of Tiles, each of which has a little picture of floor or wall or utter darkness. The DungeonObjects think of themselves as being on a Tile, and of a Tile having contents, the DungeonObjects that are “on” that Tile.

The Tile class contains 60 methods at last count. Their responsibilities include:

  • drawing the Tile
  • reporting Tile type, Room, Edge, etc
  • changing Tile type
  • fetching and checking neighbors
  • checking whether moves to the Tile are legal
  • supporting illumination
  • moving entities and objects in and out

Tile is the second-largest class in the dungeon with 60 methods, behind Player(61) and ahead of Monster(57) and GameRunner(56). (Those last two do inherit from Entity(20) and DungeonObject(3).)

In the early days with this program, I was quite pleased with the way things centered around Tile. As the game has grown, however, and more and more notions seemed to settle into Tile, I’ve been less pleased. It seems to me that the Tile ought to be a low-level concept, just a place for things to be, and a background to draw upon. It seems to me that it shouldn’t have much behavior at all.

My present notion for how I’d do this is vague, as are all our random design ideas. My thoughts certainly don’t encompass where each of the 60 methods of Tile ought to be done if not in Tile. Let’s talk about the core idea, to see whether it makes any sense to change the design a bit.

Instead of knowing about their Tile, as DungeonObjects do now (indirectly, through DungeonContents), I think they should know a more abstract thing. It would be a mere token to them, if they have it at all. I think the token would actually be a Coordinate, or a Place, an (x,y) pair representing a place in the dungeon.

It’s true that an x,y coordinate would identify a Tile that we could draw, but the Tile would just be a drawn thing, with no other responsibilities of note. It might have properties like whether it’s a room or a wall, and might (but perhaps should not) have a property indicating whether it is illuminated.

Most DungeonObjects don’t move, but Player and Monsters do. So they need to be able to find an adjacent place to move to, and that place needs to meet some criteria, such as being floor, not having other things already there, and so on. This process could at least begin by creating the adjacent Places, since these are each a simple addition away from the current Place. Then there could be some filtering, which is of course being done now as well.

I freely grant that I don’t see where all the different bits of behavior “should” be. It just seems clear that it shouldn’t be where it is.

Ownership

One thing that troubles me here is the question of ownership of the Tiles. As things are, the DungeonObjects are concerned about their own Tile, and about adjacent Tiles. But Tiles, when it comes down to it, are arranged in a big array, and are owned by a well-known object, Tiles. that knows how to convert positions (x y coordinates) to the actual Tile at those coordinates.

Which is another issue. There is code all over the system that thinks in terms of x and y, or a vector (x,y). The Tiles object deals with those x’s and y’s.

Digression

In browsing around to write this, I’ve found some small improvements. There was a method getTile in Dungeon that called privateGetTileXY in Dungeon. That method called getTile in Tiles. So I unwound that connection, and changed all the calls to the private method, which were almost all in tests, and removed that method.

The diff looks like this:

diff showing some small changes

Then I found an unused method and removed it:

commit diff showing removed method

Small improvements help. A bit less to worry about.

You May Be Wondering …

You may be wondering why I even care. It’s all working, it’s got pretty good tests, and there’s no speed or storage savings to be had: the objects don’t even hold their own Tile any more.

There are two main reasons why I care.

The pragmatic reason is that when a class has a broad interface (60 methods!), any object that accesses anything in that class could be accessing any of those 60 methods, which means that when I’m reading code that accesses a Tile, I don’t know right away what it might be doing, and when I’m modifying code via Tile, I have to munge my way through those 60 methods to find what I want. If things were otherwise, I might only have to check 10 or 20. That would help me go faster and make fewer errors.

The other reason is a bit more abstract. I find value in thinking about what a design might be, even when it isn’t that way. In early days, if I get a better idea, it’s usually quite straightforward to shift the design toward that better idea. Later on, like today, I don’t always see how to split things up, but thinking about it a bit sometimes lets me at least see some change that will make things better, moving a little bit in the direction that seems more desirable.

The third reason—as happens so often, I was mistaken about the “two”—is that when I’m thinking about the design, and I browse the code with design thoughts in my head, and quite often I find small improvements and make them. That happened this morning, as we see in the diffs above.

We were talking about ownership.

Ownership

The code today seems to think sometimes in terms of X and Y. If it does that, it knows too much detail about the Place, or Coordinate, or whatever we might call it. All that x y stuff should be down near the bottom, probably even lower down than Tiles and the Tile concept. (A Tile probably shouldn’t even know its coordinates, I believe. It should be given them if and when it needs them.)

One way of thinking about Tile being too broad would be to divide up its responsibilities into some groups we can think about as groups, such as “drawing”, “building”, “game play”, or some such. Then maybe we could move those methods off into other classes. But the troubling questions then are something like these:

  • If I used to own a quick way to Tile, do I now have to own a quick way to something else?
  • What objects do I have to own?
  • Which ones can be passed to me at the last minute?

One of the main irritations with the current design is the network of object ownership. We’ve been working to improve that for quite some time now, breaking out tile finders, dungeon builders, and so on. This is the sort of thing that tempts me to draw a UML diagram, or at least a rudimentary one.

Object ownership, who has a pointer to whom, who knows what about whom, is at the core of good design. It’s all about cohesion, things together that belong together, and coupling, things owning each other. We want high cohesion, and we want low coupling.

But if we break an object into two bits, and everyone who used to have access to the one now has to have both, that’s not much of a step forward. Cohesion is increased, but coupling has increased as well. Even worse is the possibility that instead of owning some lower-level object, we have to replace that with access to a higher-level one, which opens the door to needing to think about what the high level object does and what all the things accessible from it might do … and so on.

So when we cleave an object in twain, we want to do it so that the objects that had the old one generally wind up only having one of the two, not both. And we’d like the two not to know each other. That’s not always possible, I suppose, but if we can’t make it turn out that way, the change idea is much less valuable.

What Can We Do?

Untangling tangled code is difficult. You could argue that that’s why long procedural methods are a good idea, but you wouldn’t gain any traction here. But it can get tricky when the objects aren’t righr, and when we want to move things around substantially. What do we do about that?

We don’t move things around substantially. We move things around just a little bit, again and again. Many More Much Smaller Steps, as Hill puts it. I did a bit of that above, in the diffs that I showed you. I found a couple of methods that seemed similar, chased them a bit, moved things a bit, and finally eliminated one of them, and upwards of twenty calls to the eliminated one now go to the central one. Each step was easy, the tests kept me confident, and things are a bit better.

But aside from something we might call “wandering around looking for things but not knowing what things we’re looking for”, which doesn’t sound like a book I’d buy, what can we do that’s a bit more directed.

Well, thinking. The reasons I’m writing this rambling article are: for me, rambling thoughts, written down, generally help me find nearby ideas to work on, and often adjust my focus so that future work moves in a better direction. For you, if you’ve troubled yourself to scan this, to show you how open rambling thinking, especially in conversation with someone, ideally someone better than a rubber duck, might help you find ways to improve your own code.

I find it productive to step back, reflect on where we are, think about what we might do. And I find that most productive when the code is right there with me, so that I can answer the questions that come to mind.

But it’s still rambling. Can we focus in on a direction, or a step that could be taken?

I’m wondering about the DungeonContentsCollection. Let’s look and I’ll show you why.

DungeonContentsCollection

The DungeonContentsCollection keeps track of where in the dungeon all the objects are. It used to be that Tiles knew their contents. Now, that’s all in the DCC. And the DCC subscribes to a lot of Bus messages:

function DungeonContentsCollection:init()
    Bus:publish("dccDelete") -- remove any old contents
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, "drawDungeonContents", "game")
    Bus:subscribe(self, "getTileContents", "game")
    Bus:subscribe(self, "moveObjectToMe", "game")
    Bus:subscribe(self, "moveObjectToTile", "game")
    Bus:subscribe(self, "removeObject", "game")
    Bus:subscribe(self, "tileContaining", "game")
    Bus:subscribe(self, "dccDelete", "game")
end

(Recall that the “game” parameter to subscribe tells the Bus how long the subscription is to be preserved. The alternative is “level”. But DCC survives across the whole game. (I’m not entirely sure, at this moment why that is. We might want to look into that. I think it’s a timing issue involving just when it gets created.))

dccDelete

Remember what I said about browsing and noticing things? We publish, in our init, “dccDelete”. We are the only subscriber to it, and here is what we do:

function DungeonContentsCollection:dccDelete()
    Bus:unsubscribeAll(self)
end

This is rather a roundabout way to do this. What’s happening is that when we create a new DCC, we want to unsubscribe the old one before we finish creating the new one. But in the init, self is already the new one. So we are relying on the Bus to know the old one, and to call dccDelete on the old one. And, if this is the first time through, no one gets called and that’s just fine.

Frankly, that’s too clever to be allowed to live, but to fix it, I think we’d need a static variable to contain the current one. Let’s at least try it. We’re on a clean commit.

DungeonContentsCollection = class()

local existingDCC

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

We might be passing in a nil to the Bus. Let’s make sure it can deal with that. It couldn’t, so I did this:

function EventBus:unsubscribeAll(subscriber)
    if susbscriber then
        for event,subscriptions in pairs(self.events) do
            subscriptions[subscriber] = nil
        end
    end
end

That code would have tried to set subscriptions[nil] and you just can’t do that. However, even after that change, some tests break and we get a walkback. Fine, we weren’t here to fix this anyway, revert. I’ve been wanting to revert more often anyway, to improve my bad habit of debugging.

Still, that is odd. I think I’ll at least put in a comment.

function DungeonContentsCollection:init()
    Bus:publish("dccDelete") -- too cleverly removes old instance
    self.contentMap = {}
    self:dirty()
    Bus:subscribe(self, "drawDungeonContents", "game")
    Bus:subscribe(self, "getTileContents", "game")
    Bus:subscribe(self, "moveObjectToMe", "game")
    Bus:subscribe(self, "moveObjectToTile", "game")
    Bus:subscribe(self, "removeObject", "game")
    Bus:subscribe(self, "tileContaining", "game")
    Bus:subscribe(self, "dccDelete", "game")
end

function DungeonContentsCollection:__tostring()
    return "DungeonContentsColl"
end

-- called with self as old instance from Bus 
-- too clever by half, but the obvious fix doesn't.
function DungeonContentsCollection:dccDelete()
    Bus:unsubscribeAll(self)
end

Sometimes the best we can do is a comment. Remember that a comment is the code’s way of asking to be improved, per Kent Beck. Just now I don’t have an improvement.

Oh. I see what I did wrong. Let me try again.

I didn’t see so I just did my changes again and this time spelled subscriber correctly. Fix works. Revert worked. Commit: Simplify unsubscribing existing DungeonContentsCollection when a new one is created.

Lesson confirmed. I might have spotted that typo, I might not. The changes were tiny and revert and doing it again was short and easy. Then I looked back at the article and spotted susbscriber or whatever it was.

Small improvement. We were browsing the DungeonContentsCollection. Its big contribution is probably drawing the DungeonObjects, which it takes care to do in zLevel order so that the right things show on top of other things:

function DungeonContentsCollection:drawDungeonContents()
    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

There’s some caching going on there. We save self.orderTable when we create it, and we dirty it when we move things around:

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

We’re looking at this because we’re interested in Tile, and we are passed a Tile on some of our various entry points. However, there is good news here, which is that we mostly just use the Tile as a token. An exception is in draw, were we ask the tile whether it is visible and for its graphic center. Those are the only two places where we even send a message to a Tile from DCC.

We draw the whole collection as a batch, with a single call from whoever sends that Bus message. That’s in GameRunner:

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

That’s called from here:

function GameRunner:draw()
    font("Optima-BoldItalic")
    self:drawLargeMap()
    self:drawTinyMapOnTopOfLargeMap()
    self:drawMapContents()
    self:drawButtons()
    self:drawInventory()
    self:drawPlayerOnSmallMap()
    self:drawMessages()
end

We could unwind that and call DCC on each tile, if we had already decided to draw it. Let me refactor something so we can talk about it:

function DungeonContentsCollection:drawDungeonContents()
    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

This is perfect, and it could use a little improvement1. We can extract a method:

function DungeonContentsCollection:drawDungeonContents()
    pushMatrix()
    pushStyle()
    for tile,entries in pairs(self:drawingOrder()) do
        self:drawTileEntries(tile,entries)
    end
    popStyle()
    popMatrix()
end

function DungeonContentsCollection:drawTileEntries(tile,entries)
    if tile:isVisible() then
        local gc = tile:graphicCenter()
        for i,object in ipairs(entries) do
            spriteMode(CENTER)
            object:draw(false, gc)
        end
    end
end

I was thinking that the above method could be called on a tile-by-tile basis, and that’s almost but not quite true. We see that drawingOrder is … well, look at that. We don’t readily see anything:

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

Let’s refactor that for clarity. First, we do this:

function DungeonContentsCollection:drawingOrder()
    if not self.orderTable then
        self.orderTable = self:createNewOrderTable()
    end
    return self.orderTable
end

function DungeonContentsCollection:createNewOrderTable()
    local 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
    return order
end

That makes the caching effect more clear. Now this:

function DungeonContentsCollection:createNewOrderTable()
    local order = {}
    for object,tile in pairs(self.contentMap) do
        self:insertObjectInOrder(object,tile)
    end
    return order
end

function DungeonContentsCollection:insertObjectInOrder(object,tile)
    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

Test, by the way.

4: DrawingOrder -- DungeonContentsCollection:160: attempt to index a nil value (global 'order')

Not as by the way as I’d have liked. Ah, that extract needs the order table.

function DungeonContentsCollection:createNewOrderTable()
    local order = {}
    for object,tile in pairs(self.contentMap) do
        self:insertObjectInOrder(order, object,tile)
    end
    return order
end

function DungeonContentsCollection:insertObjectInOrder(order,object,tile)
    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

We’re good. Let’s make a save point. Commit: Some extract method improvement.

I think we can trust Lua. Let’s do this:

function DungeonContentsCollection:insertObjectInOrder(order,object,tile)
    local ord = order[tile] or {}
    table.insert(ord,object)
    table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
    order[tile] = ord
end

That’s fine. Even if Lua can’t notice that it’s sorting a one-element table, how long could it take? And we have no performance measurement telling us to do the “optimization”. Commit: Remove “optimization” of not calling sort on a 1 element table

I think we can do better than to pass in the order table. Let’s do this:

function DungeonContentsCollection:createNewOrderTable()
    local order = {}
    for object,tile in pairs(self.contentMap) do
        order[tile] = order[tile] or {}
        self:insertObjectInOrder(order[tile], object)
    end
    return order
end

function DungeonContentsCollection:insertObjectInOrder(orderTable,object)
    table.insert(orderTable,object)
    table.sort(orderTable, function(a,b) return a:zLevel() < b:zLevel() end)
    return orderTable
end

This is shorter by a return or two but relies on the fact that tables are passed by reference in Lua. That might be a bit obscure. In addition, this is odd:

        order[tile] = order[tile] or {}

That just ensures that we have a table to insert into. We could give it an explaining method:

function DungeonContentsCollection:createNewOrderTable()
    local order = {}
    for object,tile in pairs(self.contentMap) do
        local tileTable = self:tileTable(order, tile)
        self:insertObjectInOrder(tileTable, object)
    end
    return order
end

function DungeonContentsCollection:tileTable(order, tile)
    order[tile] = order[tile] or {}
    return order[tile]
end

I’m not loving this more than what I had. Revert to here:

function DungeonContentsCollection:drawDungeonContents()
    pushMatrix()
    pushStyle()
    for tile,entries in pairs(self:drawingOrder()) do
        self:drawTileEntries(tile,entries)
    end
    popStyle()
    popMatrix()
end

function DungeonContentsCollection:drawTileEntries(tile,entries)
    if tile:isVisible() then
        local gc = tile:graphicCenter()
        for i,object in ipairs(entries) do
            spriteMode(CENTER)
            object:draw(false, gc)
        end
    end
end

function DungeonContentsCollection:drawingOrder()
    if not self.orderTable then
        self.orderTable = self:createNewOrderTable()
    end
    return self.orderTable
end

function DungeonContentsCollection:createNewOrderTable()
    local order = {}
    for object,tile in pairs(self.contentMap) do
        self:insertObjectInOrder(order, object,tile)
    end
    return order
end

function DungeonContentsCollection:insertObjectInOrder(order,object,tile)
    local ord = order[tile] or {}
    table.insert(ord,object)
    table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
    order[tile] = ord
end

Let’s rename orderTable. It doesn’t express caching and the name is making me think of restaurant orders.

First this:

function DungeonContentsCollection:drawingOrder()
    if not self.cachedSortedObjects then
        self.cachedSortedObjects = self:createNewOrderTable()
    end
    return self.cachedSortedObjects
end

And changes to the other references to orderTable of course. Now this:

function DungeonContentsCollection:drawingOrder()
    if not self.cachedSortedObjects then
        self.cachedSortedObjects = self:createNewCache()
    end
    return self.cachedSortedObjects
end

function DungeonContentsCollection:createNewCache()
    local cache = {}
    for object,tile in pairs(self.contentMap) do
        self:insertObjectInOrder(cache, object,tile)
    end
    return cache
end

And this:

function DungeonContentsCollection:insertObjectInOrder(cache,object,tile)
    local ord = cache[tile] or {}
    table.insert(ord,object)
    table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
    cache[tile] = ord
end

That expresses cache-building better. Now one more change:

function DungeonContentsCollection:insertObjectInOrder(cache,object,tile)
    local cacheEntry = cache[tile] or {}
    table.insert(cacheEntry,object)
    table.sort(cacheEntry, function(a,b) return a:zLevel() < b:zLevel() end)
    cache[tile] = cacheEntry
end

OK, I like that better. Commit: Further refactoring for clarity.

Let’s reflect and remember what swamp we were draining.

Reflection

The small swamp that I had in mind was the drawing method for the DungeonContentsCollection. I was thinking that it might be possible to call it tile by tile and avoid the outer loop in drawDungeonContents. The refactoring makes clear what the original code doesn’t: we can’t quite do that without a bit more revamping. If we have the cache, it is indexed by tile, so we could, in principle, call in, fetch the items if any, and draw them.

It wouldn’t be difficult. However, there isn’t much advantage to doing so, as this table of contents isn’t very large, just the same size as the number of tiles with contents. We’d save the processing of tiles that aren’t visible but we have no reason to think that’s a problem.

So all we’ve accomplished is to make this code better by our local standards. Which is certainly a good thing.

The larger swamp, my concern that things that think about Tile should be thinking about other things, such as a Place, Location, or Coordinate, remains an open topic.

If I were to continue to go after it, I think that in my next thinking session, I’d like to consider Tile’s 60 methods, see whether there is a partitioning of them that makes sense, and then look to see whether there’s a place for an object that managed just one partition. For that to be useful, there’d need to be Tile users who only need that one partition. It might be that there are.

For now … big thoughts have led to small improvements. And if you’ve stuck with me this far, congratulations and thanks! Feel free to tell me what I’ve been doing wrong, or what I should be doing.


  1. cf. Shunryu Suzuki ↩