Oh Hell, let’s just do something in roughly the right direction.

I see no hope for a major rewrite, nor even a major refactoring effort, on the Dung[eon] program. This is how life goes. Even if offered the chance, I don’t feel the energy for a sustained effort. I can do little things. Can I move the product forward doing little things? Of course I can. At least, I’ll try.

One big thing we need is to break out game running from game building. Naively, we might wind up with a GameRunner class and a GameBuilder class. Those would presumably have other related classes, but I envision them as separate more coherent objects, compared to our current GameRunner, which is trying to be both.

To proceed incrementally, I can see two approaches that seem to me to be opposites. I could create a new class GameBuilder and move function over to it, or I could rename GameRunner to GameBuilder, create a new GameRunner class and move things into it, leaving only builder things behind.

Looking at GameRunner, I see that it contains quite a few member variables that reflect what’s in the dungeon. It has the dungeon itself, a list of monsters, keys, I don’t know what all.

Hmm. Maybe a good thing would be for there to be a new object, a DungeonLevel, containing all the necessary stuff to run on level of the game, and the GameRunner would receive that one object and use it to run the game, while the GameBuilder would create the thing.

I like that idea. Without packaging up the information, I’m faced with an unbounded (and somewhat unknown) bunch of member variables that have to be dealt with in our separation of these two capabilities, running and building. One thing to be concerned about would be behavior in the new DungeonLevel object. As I first think of it, it’s just a package of data. But my design sense is that usually we should question such things, data without behavior. So we’ll try to stay alert to that.

Another good thing about this idea is that I can create the class, and incrementally cause a method that makes something kept as a member variable in GameRunner to store it instead in the DungeonLevel, and GameRunner can fetch it back out. The behavior changes would be quite small.

They’d be even better if my practice were to create and use accessor methods inside classes like GameRunner, but generally I’ve not adopted that practice. We’ll see what I mean as we go.

Spoiler: This isn’t quite what we actually do! But the idea got us started. Then the code kept us going.

Let’s try it.

What Is It?

We have a member variable now, dungeonLevel, which is the integer number of the level. Seems unwise to have a class named the same as that. We could rename that, but it’s a good name.

We have a class now called Dungeon, which contains the dungeon “map”, the collection of tiles. So we probably can’t call it Dungeon. Unless …

N.B. Here is where I changed direction from a new class to using an existing one.

The class called Dungeon contains all the tiles for one level. It does a lot of the creation and setting up of the dungeon, although far from all of it.

For now, let’s try this: we’ll move more information about the dungeon definition into Dungeon. Since GameRunner already has an instance, deferring more to it should be natural. Meanwhile, we’ll look at moving building and operating out of Dungeon, since it, too, does both building and operating.

Hmm, that’s interesting, isn’t it? We have building going on in two classes (at least),GameRunner and Dungeon, and we have operating going on in both. That’s even worse than I thought. No matter, we’ll just make it better.

First Move

I think the first move should be a simple one but, ideally, significant. Let’s look at the creation of monsters, which is in GameRunner.

Oh this is interesting!

function GameRunner:setupMonsters(n)
    self.monsters = Monsters()
    self.monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    self.monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
end

The actual monster creation is done in the Monsters instance. Let’s refactor this just a bit:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
    self.monsters = monsters
end

I’m honesty not sure why I did that. I just felt it would be better. I wanted to reduce references to the member variable monsters. Now it turns out there is more than one place where we store into self.monsters, one for each kind of map we create. There are three. Let’s change to have an accessor for monsters:

function GameRunner:getMonsters()
    return self.monsters or Monsters()
end

Now to use it everywhere:

function GameRunner:createPathMonster(map, tile)
    self:getMonsters():createPathMonster(self,map,tile)
end

function GameRunner:hasMonsterNearPlayer(range)
    return self:getMonsters():hasMonsterNearPlayer(range)
end

function GameRunner:playerTurnComplete()
    if self.requestNewLevel then
        self:createLevel(12)
        self.requestNewLevel = false
    else
        self.playerCanMove = false
        self.getMonsters():move()
        self.playerCanMove = true
    end
end
 
function GameRunner:removeMonster(monster)
    self.getMonsters():remove(monster)
end

function GameRunner:startTimers()
    self.getMonsters():startAllTimers()
    self.player:startAllTimers()
end

This should all work, if I haven’t messed up.

GameRunner:443: attempt to index a nil value (local 'self')
stack traceback:
	GameRunner:443: in field 'getMonsters'
	GameRunner:569: in method 'startTimers'
	GameRunner:176: in method 'createLevel'
	Main:62: in function 'setup'

Hmm … that’s my accessor. With no self? Oh. I said . not : here:

function GameRunner:startTimers()
    self:getMonsters():startAllTimers()
    self.player:startAllTimers()
end

Did I do that elsewhere? Search for self.getMonsters …

function GameRunner:removeMonster(monster)
    self:getMonsters():remove(monster)
end

function GameRunner:playerTurnComplete()
    if self.requestNewLevel then
        self:createLevel(12)
        self.requestNewLevel = false
    else
        self.playerCanMove = false
        self:getMonsters():move()
        self.playerCanMove = true
    end
end

Jeffries standard error #34 or some such number. Now test again. Tests run, games run. Commit: convert monsters member variable in GameRunner to use accessor. Accessor defaults to empty Monsters collection.

Now I want to move the monsters member variable into Dungeon. Why? Because Dungeon is going to become my run-time info repository. Is that the right move? I don’t know. I think it is. I’m going to move things that direction until it seems wrong.

Now I will put the monsters into Dungeon and access them there:

function GameRunner:getMonsters()
    return self.dungeon:getmonsters()
end

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
    self.dungeon:setmonsters(monsters)
end

And in Dungeon:

function Dungeon:getMonsters()
    if not self.monsters then
        self.monsters = Monsters()
    end
    return self.monsters
end

function Dungeon:setMonsters(monsters)
    self.monsters = monsters
end

I expect this to work. Let’s see why I’m wrong.

Right, failed to capitalize:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

function GameRunner:getMonsters()
    return self.dungeon:getMonsters()
end

I’m starting to worry about my shift key …

We’re good. Commit: monsters are now stored in Dungeon.

Now let’s create them there. Change this:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

To:

function GameRunner:setupMonsters(n)
    self:getDungeon():setupMonsters(n)
end

function Dungeon:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self,2, self.wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

That variable n seems to be ignored, but I preserved it. this should work. Test.

Wow!

Monster:17: attempt to compare number with nil
stack traceback:
	Monster:17: in method 'getMonstersAtLevel'
	Monster:51: in method 'getRandomMonsters'
	Monsters:94: in method 'createRandomMonsters'
	Dungeon:435: in method 'setupMonsters'
	GameRunner:562: in method 'setupMonsters'
	GameRunner:166: in method 'createLevel'
	Main:62: in function 'setup'

We can’t pass self into those guys: they are expecting dungeon. But why?

function Dungeon:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.dungeon, 6, self.dungeonLevel, self.playerRoom)
    monsters:createHangoutMonsters(self.dungeon,2, self.wayDown:getTile())
    self.dungeon:setMonsters(monsters)
end

Oh sheesh, that’s just all weird isn’t it? Still, I want this to work, so let’s fix all the self refs … I have to get my head on straight. We reference back to runner not dungeon. We are dungeon. Driver error. Going too fast, probably.

function Dungeon:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.runner, 6, self.runner.dungeonLevel, self.runner.playerRoom)
    monsters:createHangoutMonsters(self.runner,2, self.runner.wayDown:getTile())
    self:setMonsters(monsters)
end

This works. It’s somewhat nasty. Still, commit: monsters now created in Dungeon class.

OK, that could have gone more smoothly. I’m not sure what a better order would have been. We do see that we’re going to want a lot more information over here before we’re done.

Right now I’d like to fix all these Demeter violations, self.runner.whatever … and what is that (n) anyway? I think it’s supposed to be how many Random monsters we make. Let’s make that be the case:

function Dungeon:setupMonsters(howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(self.runner, howManyRandom, self.runner.dungeonLevel, self.runner.playerRoom)
    monsters:createHangoutMonsters(self.runner,2, self.runner.wayDown:getTile())
    self:setMonsters(monsters)
end

But let’s pass in the other info we need directly:

function Dungeon:setupMonsters(runner, level, playerRoom, wayDown, howManyRandom)
    local monsters = Monsters()
    monsters:createRandomMonsters(runner, howManyRandom, level, playerRoom)
    monsters:createHangoutMonsters(runner,2, wayDown:getTile())
    self:setMonsters(monsters)
end

Now to fix the senders:

function GameRunner:setupMonsters(howManyRandom)
    self:getDungeon():setupMonsters(self, self.dungeonLevel, self.playerRoom, self.wayDown, howManyRandom)
end

This is a weird calling sequence, because the monsters want to know weird things when setting up. Today at least, I figure it is what it is. Tests run, game runs. Commit: pass in detailed parameters to Dungeon:setupMonsters().

Time to pull our heads out of the grit and grime.

Retro

Our larger mission is to improve the cohesion of dungeon creation and dungeon running. Have we made any progress toward that at all? I would say yes: we have moved one bit of creation out of GameRunner and over to Dungeon, which already has a cohesion problem between building and running. So we have improved GameRunner’s cohesion, at some cost in Dungeon’s cohesion. Is that better than a wash?

I say yes. Suppose that this was the last building thing in GameRunner, and that all the other building stuff (along with some running stuff) was over in Dungeon. Moving that last bit would leave GameRunner out of the building business, and would localize all the building in Dungeon. That’s clearly worth doing. Therefore every step along the way is worth doing.

At least that’s how I see it.

Do More?

I’m 2 1/2 hours in. Is it time to stop? Probably. I think there’s one more thing, if I recall. Look for accesses to self:getMonsters() in GameRunner. I think there’s another building one. Ah, here it is:

function GameRunner:createPathMonster(map, tile)
    self:getMonsters():createPathMonster(self,map,tile)
end

This one is tricky: it happens during running, when you open a PathFinder jar. But it can still delegate to dungeon at least:

function GameRunner:createPathMonster(map, tile)
    self:getDungeon():createPathMonster(self,map,tile)
end

Implement that on Dungeon:

function Dungeon:createPathMonster(runner, map, tile)
    self:getMonsters():createPathMonster(runner, map, tile)
end

Tests run, pathfinder works. Commit: path monster now created by Dungeon, not GameRunner.

OK, just one more …

Here’s another reference:

function GameRunner:hasMonsterNearPlayer(range)
    return self:getMonsters():hasMonsterNearPlayer(range)
end

This can surely be deferred to Dungeon?

function GameRunner:hasMonsterNearPlayer(range)
    return self.dungeon:hasMonsterNearPlayer(range)
end

function Dungeon:hasMonsterNearPlayer(range)
    return self:getMonsters():hasMonsterNearPlayer(range)
end

Works. Commit: GameRunner defers hasMonsterNearPlayer to Dungeon.

There’s one more that I want to think about:

function GameRunner:startTimers()
    self:getMonsters():startAllTimers()
    self.player:startAllTimers()
end

Should this move to Dungeon? It’s not clear. Is it setup, or is it the beginning of running. Anyway, I think we need to work out what Dungeon will know and what it won’t. We’re already passing a lot of odd things in, and we’d have to pass in player here. We’ll let this ride, and stop here.

Let’s sum up.

Summary

We have moved the building of monsters to Dungeon, improving the cohesion of GameRunner a bit, at a corresponding cost in Dungeon. I consider that a small gain, in that it improves GameRunner and doesn’t harm Dungeon that much. The closer we get to GameRunner not building, the more clear this will be.

I think that tomorrow–or real soon now–we should see about moving some building out of Dungeon and into some kind of Builder object, thus improving Dungeon as well. Be that as it may, we’ve made a few small steps and moved in the direction we wanted to move. With enough small steps, I posit that things will be substantially better, and we won’t have had to rewrite the world, nor even beg time from “them” to allow us a big refactoring. And those are good things.

See you next time, I hope …