Dungeon 262

Plotter or Pantser? Which am I? Which are you?

In writing, especially fiction writing, a “Plotter” is a person who plans their novel, creating detailed outlines and other materials, working out the shape of the story before writing it. In contrast is the “Pantser”, who “flies by the seat of their pants”, letting the characters tell the story, planning very little.

Now I am not one to believe that binary distinctions really describe the world well. In fact, I think that no finite number of distinctions is likely to work. I’m looking at you, MBTI. Living things, as most of us here happen to be, have widely varied behavior across time and space. Glib explanations like “he’s an INTJ, of course he’d say that”, or “he’s a Pantser, of course he’d make that choice” are likely to lead us down a path of error, mistakes, and blunders.

Still, the Plotter/Pantser notion can be useful. One of my many betters, Martin Fowler, has identified over 60 different refactorings. You can see the catalog at refactoring.com, and I heartily recommend the book, as it includes detailed steps for accomplishing some very complex code transformations.

A Plotter in refactoring would likely use Martin’s book, or their own personal catalog, as a basis for improving code. They would likely plan their moves to accomplish the design improvement they have in mind, and then follow that plan as closely as practical.

Naturally, in real code, the pre-planned approach sometimes runs into issues in the code that force a deviation from the plan. I imagine the same thing can happen in the outline of one’s novel, where we didn’t notice that Jason was seriously injured three chapters ago and probably cannot run wildly to Jenny when he sees her on the dock. With luck, we see the problem as we write, and fix it. But, by and large, we can probably get a well-organized book by careful outlining, and we can probably get a significant refactoring to go pretty smoothly by planning it Plotter style.

If you were looking for a Pantser in refactoring, you would need to look no further than these pages. Owing, probably, to defects in my mind, I program in a very intuitive, follow my nose fashion. I think I always did, but my experience with “Agile” has driven me further in that direction.

There are at least two reasons beyond personal inclination that drive me to work in the Pantser style.

First, much–I’d argue most–of the code we encounter is not terribly well organized. It generally doesn’t match any particular well-known pattern. Even if we wanted to move the code so that it did match some desirable pattern, our starting position will probably be decidedly non-standard.

Second, I try to work at a relatively basic level, doing things that most anyone can do. I don’t feel write looking at some mess of code and then suddenly announce “This is a job for Captain Marvel” and then do something that most people have never heard of.

Third–I was mistaken about “two”–I believe that as we refactor real code, we’ll quite often go in the wrong direction, and I want us all to experience that that’s OK, we can always revert or just start digging in a new direction. Planning less than might be desirable helps ensure that we’ll encounter trouble. Probably more than I really need …

Fourth–I was way off with that “two”–I think there is great value in building up our “intuition” about the code, the sense that these things are kind of the same and belong together, and these things are different and belong apart, or that this thing changes faster than that thing, so they probably should be separated.

That said, most of Martin’s refactorings are pretty simple, and I’d probably do better to at least mention them as we refactor. I couldn’t possibly remember over 60 names, but I could remember a few. And the names are actually mostly generated over a very small space of nouns and verbs. They’re definitely worth studying, in the on-line list, and even better in the book. And I’ll try to do better at expressing where I think I’m going.

Be that as it may, it’s fair to say that I am very much a Pantser in how I work, and that if you’re more of a Plotter, more power to you. You’ll surely benefit from a bit more planning than I do.

Let’s review our rather limited plan, in the light of the code.

Ideas

We still have three leftover idea cards, I think:

• dcPrepare: move non-Dungeon stuff back to GameRunner.
• placePlayer RUNNER callback? Ditto WayDown. Return values?
• Should Monsters know RUNNER, and avoid parameters on method calls?

I should add a card. Yesterday, when I renamed some method, I had to change some tests and working code, because some of the other dungeon builds were relying on the method, not just the code moved to DungeonBuilder. And I think I ran into that difficulty a few days prior as well.

Right now, we have most of the building of the everyday game dungeon level moved out of GameRunner. There are other dungeon builds still in there, such as createTestLevel and createLearningLevel. Rather than start moving code from Dungeon to somewhere else, maybe we’d do better to finish moving all the build stuff out of GameRunner.

There are arguments on all sides. The way things are isn’t really bothering anyone. But the job’s not done until everything is moved out. But the job is never done, so maybe moving the others out isn’t that important. But if we don’t move them now, maybe the changes we make to Dungeon will make it harder to move things later. But that seems like it’s unlikely, since we’ll have had to make them work anyway. But if we move them, we can remove duplication and make future work easier …

Time to flip a coin and see whether we like how it comes up.

What’s Easier?

Moving the smaller creates over to DungeonBuilder seems easier to me, and I am feeling that I need something relatively easy to do. I actually have a theory about that.

When faced with a largeish thing to so, we can usually see some of the steps to doing it. Is it better to start with a hard step, or an easy one? I don’t think there’s an officially right answer. Sometimes I like to start with the hard thing, on the grounds that if I do a lot of work and then can’t do the hard thing, the work will be wasted. Other times I start with an easy thing, because when it’s done, the rest of the problem will probably look simpler.

But another rule I try to follow is to scale the work I undertake to the energy and brainpower I seem to have available. As such, it behooves me to do a small thing, today, and every day.

This is another way of looking at GeePaw Hill’s “Many More Much Smaller Steps” notion. Fact is, my experience is that I always do better when I’m taking very small steps, and that when I can make things easy, it will pay off immediately, in that I’m less likely to make a mistake, and when I do make a mistake, it’ll be easier to find.

Accordingly:

Move Creation Methods to DungeonBuilder

Here’s what we have now, for learning and test levels:

function GameRunner:createLearningLevel()
    self.dungeonLevel = 99
    self:defineDungeonBuilder()
    self:dcPrepare()
    
    -- customize rooms and connections
    self:createLearningRooms()
    self:connectLearningRooms()
    
    -- paint dungeon correctly
    self.dungeon:convertEdgesToWalls()
    
    self:placePlayerInRoom1()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    
    -- prepare crawl
    self.cofloater:startCrawl()
    Announcer:sayAndSave(self:crawlMessages(self.dungeonLevel))
    self:startTimers()
    
    self:dcFinalize()
end

function GameRunner:createTestLevel(count)
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    self:defineDungeonBuilder()
    
    self:dcPrepare()
    
    -- customize for level
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
    self:dcFinalize()
end

Looking at those, it’s pretty clear which is easier to do.

I’m going to do this in a very Pantser way. I’m going to change the method in GameRunner to look very similar to this one:

function GameRunner:createLevel(count)
    self:defineDungeonBuilder()
    self.builder:buildLevel(count)
    self:prepareToRun()
end

Then I’m going to move the old method over to DungeonBuilder, and run to see what explodes. I might look around a bit over there first, to see what can be scavenged.

And in fact I’m going to do the move first, of course, because I want a method that’s just like the GameRunner one over in the builder. Here it is, with renaming:

function DungeonBuilder:buildTestLevel(roomCount)
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    self:defineDungeonBuilder()
    
    self:dcPrepare()
    
    -- customize for level
    self.dungeon:createRandomRooms(roomCount)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
    self:dcFinalize()
end

Now the caller, modeled after createLevel:

function GameRunner:createTestLevel(roomCount)
    self:defineDungeonBuilder()
    self.builder:buildTestLevel(roomCount)
    -- self:prepareToRun() -- probably not needed
end

OK back to DungeonBuilder to see what we can change to give this code a chance. I’ll model after the buildLevel there:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
end

I’ll make the first part of the new method look the same:

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    
    -- customize for level
    self.dungeon:createRandomRooms(roomCount)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
    self:dcFinalize()
end

Well this is interesting isn’t it? What does our defineDungeonLayout do now?

function DungeonBuilder:defineDungeonLayout(count)
    -- customize rooms and connections
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    
    -- paint dungeon correctly
    self.dungeon:convertEdgesToWalls()
end

Sweet. I think we can probably just call that.

function DungeonBuilder:buildTestLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:dcFinalize()
end

Test. I feel fairly good about this. Woot! The tests all run. Commit: move createTestLevel work to DungeonBuilder:buildTestLevel.

That went smoothly, The next one might be a bit more difficult.

Move LearningLevel

The learning level code includes more than we’ve looked at this morning:

function GameRunner:createLearningRooms()
    local w = 12
    local h =  8
    local a2 = { "In this room, there is a health power-up.",
        "Please walk next to it (not onto it)",
        "and press the ?? button.",
    "You'll get an informative message about whatever you're next to." }
    local t = {
        {2,2, w,h},
        {15,2, w,h, a2},
        {28,2, w,h},
        {41,2, w,h},
        {2,11, w,h},
        {15,11, w,h},
        {28,11, w,h},
        {41,11, w,h},
        {2,20, w,h},
        {15,20, w,h},
        {28,20, w,h},
        {41,20, w,h},
    }
    for i,r in ipairs(t) do
        r[1] = r[1]+24
        r[2] = r[2]+24
    end
    self.dungeon:createRoomsFromXYWHA(t, self)
    local r2 = self:getRooms()[2]
    local lootTile = r2:tileAt(self.dungeon, 2,2)
    local loot = Loot(lootTile, "health", 5,5)
    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to add it to your inventory.")
end

function GameRunner:connectLearningRooms()
    local dungeon = self:getDungeon()
    local rooms = self:getRooms()
    rooms[2]:connect(dungeon, rooms[1])
    rooms[3]:connect(dungeon, rooms[2])
    rooms[4]:connect(dungeon, rooms[3])
    rooms[8]:connect(dungeon, rooms[4])
    rooms[12]:connect(dungeon, rooms[8])
    rooms[11]:connect(dungeon, rooms[12])
    rooms[10]:connect(dungeon, rooms[11])
    rooms[9]:connect(dungeon, rooms[10])
    rooms[5]:connect(dungeon, rooms[9])
    rooms[6]:connect(dungeon, rooms[5])
    rooms[7]:connect(dungeon, rooms[6])
end

function GameRunner:createLearningLevel()
    self.dungeonLevel = 99
    self:defineDungeonBuilder()
    self:dcPrepare()
    
    -- customize rooms and connections
    self:createLearningRooms()
    self:connectLearningRooms()
    
    -- paint dungeon correctly
    self.dungeon:convertEdgesToWalls()
    
    self:placePlayerInRoom1()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    
    -- prepare crawl
    self.cofloater:startCrawl()
    Announcer:sayAndSave(self:crawlMessages(self.dungeonLevel))
    self:startTimers()
    
    self:dcFinalize()
end

Still, this doesn’t look difficult, though it’s probably just detailed enough to cause me to break something. We’ll see.

First most all those methods over to DungeonBuilder. I even remembered to change all the class names to DungeonBuilder. See how clever I am?

Now delete the two helpers from GameRunner and edit the create.

function GameRunner:createLearningLevel()
    self:defineDungeonBuilder()
    self.builder:buildLearningLevel()
    self:prepareToRun()
end

Naively, we’d do this. But I note that we set the level to 99 in the learning level. Is this important? I don’t know. Let me add a reminder to the code to look into it:

function GameRunner:createLearningLevel()
    -- learning level is level 99? deal with this?
    self:defineDungeonBuilder()
    self.builder:buildLearningLevel()
    self:prepareToRun()
end

I could make a card for this instead. Let’s try this.

OK now back to DungeonBuilder, rename the create method:

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    
    -- customize rooms and connections
    self:createLearningRooms()
    self:connectLearningRooms()
    
    -- paint dungeon correctly
    self.dungeon:convertEdgesToWalls()
    
    self:placePlayer()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    
    self:dcFinalize()
end

I’ve taken a pass through to make it look like the main buildLevel. Let’s test it.

Room:109: attempt to call a nil value (method 'getTile')
stack traceback:
	Room:109: in method 'paint'
	Room:29: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in function <... false
    end

    setmetatable(c, mt)
    return c
end:20>
	(...tail calls...)
	Dungeon:208: in local 'f'
	fp:105: in field 'reduce'
	Dungeon:210: in method 'createRoomsFromXYWHA'
	DungeonBuilder:90: in method 'createLearningRooms'
	DungeonBuilder:117: in method 'buildLearningLevel'
	GameRunner:42: in method 'createLearningLevel'
	Player:168: in field '?'
	Button:79: in method 'performCommand'
	Button:64: in method 'touchEnded'
	Main:118: in function 'touched'

With luck that’s an obvious translation issue. I didn’t even look at those helper methods. Looks like the problem is in that XYWHA method:

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner)
    end
    self.rooms = ar.reduce(roomTable, Array{}, makeRoom) -- 210
end

This is a bit scary. I’m not even sure that building the Learning Level worked before these changes.

I almost wish I were on a branch. Or had stash. But I don’t. I’ll commit, pull the previous version and test. Then I’ll decide whether to remove this commit.

OK, placing the WayDown failed, but the XYWHA stuff didn’t. So the problem we’re seeing now is a result of this morning’s changes. I’ll go back to the broken commit and carry on.

I’m sure there is a more graceful way to do that, if I had full git capability here, but I’m not sure what the right WorkingCopy plan was. Fact is, right now, the build is broken. I could remove this commit and start over but I don’t see the point. Let’s find the problem and fix it.

Room:109: attempt to call a nil value (method 'getTile')
stack traceback:
	Room:109: in method 'paint'
	Room:29: in field 'init'
	... false
    end

    setmetatable(c, mt)
    return c
end:24: in function <... false
    end

    setmetatable(c, mt)
    return c
end:20>
	(...tail calls...)
	Dungeon:208: in local 'f'
	fp:105: in field 'reduce'
	Dungeon:210: in method 'createRoomsFromXYWHA'
	DungeonBuilder:90: in method 'createLearningRooms'
	DungeonBuilder:117: in method 'buildLearningLevel'
	GameRunner:42: in method 'createLearningLevel'
	Player:168: in field '?'
	Button:79: in method 'performCommand'
	Button:64: in method 'touchEnded'
	Main:118: in function 'touched'

This rather messy report isolates what’s going on to the reduce, but there’s lots inside:

We see that this method wants runner:

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner)
    end
    self.rooms = ar.reduce(roomTable, Array{}, makeRoom)
end

But we’re calling it with self since we just moved the method over. This might be easy after all.

function DungeonBuilder:createLearningRooms()
...
    self.dungeon:createRoomsFromXYWHA(t, self.RUNNER)
    local r2 = self:getRooms()[2]
    local lootTile = r2:tileAt(self.dungeon, 2,2)
    local loot = Loot(lootTile, "health", 5,5)
    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to add it to your inventory.")
end

Test, hopefully.

DungeonBuilder:98: attempt to call a nil value (method 'getDungeon')
stack traceback:
	DungeonBuilder:98: in method 'connectLearningRooms'
	DungeonBuilder:118: in method 'buildLearningLevel'
	GameRunner:42: in method 'createLearningLevel'
	Player:168: in field '?'
	Button:79: in method 'performCommand'
	Button:64: in method 'touchEnded'
	Main:118: in function 'touched'

Sure.

function DungeonBuilder:connectLearningRooms()
    local dungeon = self:getDungeon()
    local rooms = self:getRooms()
    rooms[2]:connect(dungeon, rooms[1])
    rooms[3]:connect(dungeon, rooms[2])
    rooms[4]:connect(dungeon, rooms[3])
    rooms[8]:connect(dungeon, rooms[4])
    rooms[12]:connect(dungeon, rooms[8])
    rooms[11]:connect(dungeon, rooms[12])
    rooms[10]:connect(dungeon, rooms[11])
    rooms[9]:connect(dungeon, rooms[10])
    rooms[5]:connect(dungeon, rooms[9])
    rooms[6]:connect(dungeon, rooms[5])
    rooms[7]:connect(dungeon, rooms[6])
end

Change that first line to self.dungeon.

And the tests run and the LearningLevel works. (The WayDown is not properly placed, but that’s what that coment was about. That’s jut not done yet.)

Commit: LearningLevel creation now done in DungeonBuilder.

And Now For Something …

Now that we have all the real level building moved over, let’s see if there is any duplication that we should be concerned about

As a Pantser, I rely on detection of things like duplicated code to trigger me into improving things.

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    
    self:createLearningRooms()
    self:connectLearningRooms()
    
    self.dungeon:convertEdgesToWalls()
    self:placePlayer()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    
    self:dcFinalize()
end

Compare to the canonical `buildLevel:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
end

When we look at defineDungeonLayout we find this:

function DungeonBuilder:defineDungeonLayout(count)
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

This drives me to say this:

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    self:defineLearningLayout()
    self:placePlayer()
    
    -- customize contents
    self:placeWayDown() -- needs to be fixed room
    
    self:dcFinalize()
end

function DungeonBuilder:defineLearningLayout()
    self:createLearningRooms()
    self:connectLearningRooms()
    self.dungeon:convertEdgesToWalls()
end

And then this:

function DungeonBuilder:buildLearningLevel()
    self:dcPrepare()
    self:defineLearningLayout()
    self:placePlayer()
    self:customizeLearningContents()
    self:dcFinalize()
end

function DungeonBuilder:customizeLearningContents()
    self:placeWayDown() -- needs to be placed in fixed room
end

This is nice. The methods follow the same pattern. There is, however some duplication. Let’s commit and then decide whether to deal with it.

Test. Commit: tidy up DungeonBuilder to create duplication

Ooo, I detected a bug! When we create the Learning Level, its startup message isn’t the one about learning, it is the one that you’d get if you advanced to the next level in the regular game. Remember that 99? It was important.

Let’s fix that.

function GameRunner:createLearningLevel()
    -- learning level is level 99? deal with this?
    self:defineDungeonBuilder()
    self.builder:buildLearningLevel()
    self:prepareToRun()
end

function GameRunner:createLevel(roomCount)
    self:defineDungeonBuilder()
    self.builder:buildLevel(roomCount)
    self:prepareToRun()
end

function GameRunner:defineDungeonBuilder()
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    -- determine level
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    self.builder = DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

Let’s add an optional parameter to defineDungeonBuilder:

function GameRunner:defineDungeonBuilder(providedLevel)
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    -- determine level
    self.dungeonLevel = providedLevel or self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    self.builder = DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

Now we provide the level:

function GameRunner:createLearningLevel()
    self:defineDungeonBuilder(99)
    self.builder:buildLearningLevel()
    self:prepareToRun()
end

Test. Didn’t work. It couldn’t possibly work. Bad programmer, trying to be clever.

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 = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    self.builder = DungeonBuilder(self,self.dungeon, self.dungeonLevel, self.playerRoom, numberOfMonsters)
end

That works. Commit: Learning Level now has correct starting crawl.

I think that’s a nice stopping point. Let’s sum up.

Summary

We chose an easy path this morning, moving the remaining learning and test level creation over into DungeonBuilder. Now the only thing left is the Hex level, and I’m refusing to look at that. Truth is, it should be easy to move over, I am just resisting because I don’t like working on the Hex level.

Moving the levels over went very smoothly, There was a small glitch here and there, notably when I failed to convert a method’s use of self to self.RUNNER. That confused me, and I wondered whether it had worked before. If I hadn’t been so ready to revert, I’d have found the bug and moved on. Overall, though, I think I am too reluctant to revert, so rolling back too soon is a nice change.

If I were more adept with WorkingCopy, or had a real git client, I might have done that even more smoothly, but it went just fine. Backed up, checked, went back to head and carried on.

Referring back to Martin’s refactorings, most of what we did today was Move Method. Toward the end, we did an Extract Method to pull out the elements of the learning level creation that matched the shape of the primary level creation. Along the way in doing this whole refactoring, we’ve used Move Statements into Function and Move Statements into Callers, to get things into the classes where we want them.

A while back we did Replace Magic Literal. We have plenty of places to use that again, such as that 99 we just used.

Probably we used even more of Martin’s named refactorings. See whether you can identify other moves that I’ve made without naming them.

And stop by next time. I’m curious to find out what I do, and hope that you are as well.