I think that today I might get all the Decor specified for all the dungeon layers that are supported. That’s an estimate, and I could be wrong. (Spoiler:not wrong, and some nifty refactoring.)

Yesterday, I replaced Decor creation with a temporary method that looked like this:

function DungeonBuilder:placeDecor(ignored)
    local decor = {"Chest", "Chest", "Chest"}
    local loot = {"health(4,4)", "speed(1,1)", "catPersuasion"}
    local parsed = zip(decor,loot)
    self:fromDefs(parsed)
end

That worked as advertised, placing three Chests, each containing one of those items. There were also two Mimic chests, which placed me in grave danger, in my role as the Princess. (Adjusts tiara.) I am willing to face these dangers on behalf of you, my loyal subjects.

Based on that test and the checks we have for the functions like zip, I conclude that we have running code that can be used to define the Decor layout for any dungeon. It is not code that we can be particularly proud of, since the various zip and extend functions are just global functions, not limited to any particular namespace, nor part of any class. We do need to get that cleaned up.

But first, let’s define the Decor for at least one level, as a starting position, and then extend it to all four levels. We’ll try to proceed in small steps, and I expect that we’ll make a bit of a mess and then clean it up.

The method placeDecor does all the work for regular game levels, so our modifications should start there. I think we’ll preserve the current version, renamed:

function DungeonBuilder:placeDecorRandom(n)
    local sourceItems = {
        "catPersuasion",
        "curePoison",
        "pathfinder",
        "rock",
        "health",
        "speed",
        "nothing",
        "nothing",
        "nothing"
    }
    local loots = {}
    for i = 1,n or 10 do
        local kind = sourceItems[1 + i%#sourceItems]
        local loot = Loot(nil, kind, 4,10)
        table.insert(loots, loot)
    end
    Decor:createRequiredItemsInEmptyTiles(loots,self)
end

\What I envision is that in placeDecor we’ll see whether we have a specific Decor plan for a level, and if not, we’ll use the random placement. That will let us sneak up on getting all the layer selection figured out.

In our new placeDecor, let’s first just apply it to level one. Excuse me for a moment while I figure out how we know the dungeon level. Conveniently, that’s self.levelNumber in DungeonBuilder. So …

function DungeonBuilder:placeDecor(count)
    if self.levelNumber == 1 then
        local decor = {"Chest", "Chest", "Chest"}
        local loot = {"health(4,4)", "speed(1,1)", "catPersuasion"}
        local parsed = zip(decor,loot)
        self:fromDefs(parsed)
    else
        self:placeDecorRandom(count)
    end
end

That should create level one with three chests. I can check that quickly. (But I wish I had a test at this level …) I get a surprise when I run things:

Main:61: bad argument #1 to 'random' (number expected, got function)
stack traceback:
	[C]: in function 'math.random'
	Main:61: in function 'setup'

That will be missing () …

    local seed = math.random(os.time)

I would have sworn that I tested that. Oh well …

    local seed = math.random(os.time())

Commit that: correct missing () on randomseed. Now to test my level, again at great personal risk. My Chests are there, as are a few other monsters, which I evade without bloodshed.

OK. We have a workable scheme here. Let’s digress for a moment and ask about whether there should be an automated test here.

On the side of not needing a test, we have very complete tests for all the components, here, the zip and extend methods, and the parsing and even the fromDefs method. So there isn’t much to test here.

On the side of needing one, we have the indisputable fact that I felt the need to test so badly that I went into the Dungeon to seek out the Chests and check them, in the face of great personal danger.

The rule that we used to use on the C3 project, and that I’ve long recommended, is: If anyone thinks we need a test, we need the test.

But I am not a good person, and I really don’t want to write a test like that. I do agree to look at the tests, and I offer this one:

        _:test("Collection of Decor withOUT Syntax Error", function()
            local builder = GameRunner():defineDungeonBuilder()
            builder:buildTestLevel(10)
            local defs = {
                "Skeleton1;health(5,5);avoid(1)",
                "Box,pathfinder,avoid(1)",
                "Chest;catPersuasion;avoid(1)"
            }
            local decors = builder:fromDefs(defs)
            _:expect(#decors).is(2)
            _:expect(decors[1].kind).is("Skeleton1")
            _:expect(decors[2].kind).is("Chest")
        end)

Our code in placeDecor calls that very method, fromDefs`. I convince myself that that’s a legitimate test and that visiting the dungeon “to be sure” isn’t necessary.

You know I’m wrong, don’t you? You know that I want more confidence than the existing tests give me. You know I’m making a mistake when I do what I’m about to do …

I’m going ahead with building out the level Decor.

Building Out the Decor

The general function that places Decor randomly places 30 items. There are just a dozen rooms in the level, so that’s over two Decor per room. (We also place naked Loot, but I plan to reduce that or remove it.) I don’t mind the amount of decor, but I think we should have most of it empty, and I’m inclined to focus on Chests as usually containing Loot, while other containers are less likely to have anything of value, but still frequently enough to make them worth searching.

In the first level, let’s have all the Chests have good stuff, to build up bad habits. And I think we’ll put no Mimics on the first level, for a nice surprise later on.

Let’s try this:

function DungeonBuilder:placeDecor(count)
    if self.levelNumber == 1 then
        local decor = extendAll({5, "Chest"})
        local loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        local parsed = zip(decor,loot)
        self:fromDefs(parsed)
    else
        self:placeDecorRandom(count)
    end
end

So far, this will create 5 Chests, and each of them will have something of value. Then we’ll do this:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        self:fromDefs(parsed)
        decor = extendAll{6,"Skeleton1", 6,"Skeleton2"}
        loot = randomize(extendAll{6,"health(1,1)", 6,"nothing"})
        self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
end

Now I expect we’ll have our five valuable chests, and a lot of skeletal remains, half of which contain a small dose of health. Let’s make them a bit better: I’ll change it to 1,3. That seems good. Notice that we can create our Decor in batches to vary things. In level one, all the chests have something of value and half the other items have something, the rest nothing.

With my Level Design hat on, I think we should save the scary skeletons for level 2. I’ll change things up here and use some other items:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        self:fromDefs(parsed)
        decor = extendAll{4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"}
        loot = randomize(extendAll{6,"health(1,3)", 6,"nothing"})
        self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
end

Good enough. Let’s do the next level.

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        self:fromDefs(parsed)
        decor = extendAll{4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"}
        loot = randomize(extendAll{6,"health(1,3)", 6,"nothing"})
        self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll{5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull",
            3,"PotEmpty", 3,"Crate"}
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion"})
        self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
end

At this point I see the possibility of a problem, and thus the need for a test. What if I were to make a mistake in those tables, such as leaving out a count? What happens if I provide a Decor or Loot name that isn’t correct?

Now we could fairly assume that in a real product situation, we’d do the checks for the Decor/Loot when the input files are created. And maybe that makes sense. But as things stand here, with actual code creating these things, I think we really do need a test of some kind.

I create this rudimentary one:

        _:test("Decor creation at all levels", function()
            local runner = GameRunner()
            local builder = runner:defineDungeonBuilder()
            builder:buildTestLevel(10)
            builder.levelNumber = 1
            builder:placeDecor(30)
            _:expect(1).is(10)
            builder.levelNumber = 2
            builder:placeDecor(30)
            _:expect(2).is(20)
        end)

Those expectations are in there because the test doesn’t run, and I wanted to know which placeDecor had failed. The test says:

8: Decor creation at all levels  -- Actual: 1, Expected: 10
8: Decor creation at all levels -- Decor:463: attempt to concatenate a nil value (field '?')

I’m betting that’s somewhere near the zip and that it connotes having the wrong number of items. Here’s the code near 463:

function zip(array1, array2)
    local result = {}
    for i = 1,#array1 do
        table.insert(result, array1[i]..";"..array2[i]..";avoid(1)")
    end
    return result
end

Yes. Let’s assert equal length here.

8: Decor creation at all levels -- Decor:461: arrays don't match 17,8

Not even close. Check the code:

    elseif self.levelNumber == 2 then
        decor = extendAll{5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull",
            3,"PotEmpty", 3,"Crate"}
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion"})
        self:fromDefs(zip(decor,loot))

I forgot to add in the nothings, didn’t I?

    elseif self.levelNumber == 2 then
        decor = extendAll{5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull",
            3,"PotEmpty", 3,"Crate"}
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"})
        self:fromDefs(zip(decor,loot))

I expect my rudimentary test to run now.

It does, except for the fake expects. Let’s check some returns, and make placeDecor return something.

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed, testCount
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = #self:fromDefs(parsed)
        decor = extendAll{4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"}
        loot = randomize(extendAll{6,"health(1,3)", 6,"nothing"})
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll{5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull",
            3,"PotEmpty", 3,"Crate"}
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

And in the test:

        _:test("Decor creation at all levels", function()
            local decorCount
            local runner = GameRunner()
            local builder = runner:defineDungeonBuilder()
            builder:buildTestLevel(10)
            builder.levelNumber = 1
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 1").is(17)
            builder.levelNumber = 2
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 2").is(17)
            builder.levelNumber = 3
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 3").is(17)
            builder.levelNumber = 4
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 4").is(17)
        end)

This should fail on the level 3 and 4, at least for now.

Now I’m going to write the other two layouts. The code is kind of nasty but I’m not quite sure what to do about it yet.

After a bit of tedium I have this test:

        _:test("Decor creation at all levels", function()
            local decorCount
            local runner = GameRunner()
            local builder = runner:defineDungeonBuilder()
            builder:buildTestLevel(30)
            builder.levelNumber = 1
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 1").is(17)
            builder.levelNumber = 2
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 2").is(17)
            builder.levelNumber = 3
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 3").is(27)
            builder.levelNumber = 4
            decorCount = builder:placeDecor(30)
            _:expect(decorCount, "level 4").is(23)
        end)

And this code:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = #self:fromDefs(parsed)
        decor = extendAll{4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"}
        loot = randomize(extendAll{6,"health(1,3)", 6,"nothing"})
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll{5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull",
            3,"PotEmpty", 3,"Crate"}
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 3 then
        decor = extendAll{7,"Chest", 7,"Skeleton1", 8,"Skeleton2", 2,"BarrelEmpty",
            3,"BarrelFull"} -- 27
        loot = randomize(extendAll{4,"health(5,10)", 3,"rock", 4,"curePoison",
            16,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 4 then
        decor = extendAll{6,"Chest", 9,"Skeleton1", 8,"Skeleton2"} --23
        loot = randomize(extendAll{3,"curePoison", 10,"health(7,11)", 10,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

Seems like a lot of duplication there. Let’s extract our data into a table:

local decorTable = {
    {4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"},
    {5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull", 3,"PotEmpty", 3,"Crate"},
    {7,"Chest", 7,"Skeleton1", 8,"Skeleton2", 2,"BarrelEmpty", 3,"BarrelFull"}, -- 27
    {6,"Chest", 9,"Skeleton1", 8,"Skeleton2"} --23
}

With that done, I can do this:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = #self:fromDefs(parsed)
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll{6,"health(1,3)", 6,"nothing"})
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll{4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 3 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll{4,"health(5,10)", 3,"rock", 4,"curePoison",
            16,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 4 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll{3,"curePoison", 10,"health(7,11)", 10,"nothing"})
        testCount = #self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

And test.

That reminds me, we can commit this. Commit: Decor for all four levels is customized.

Note that I have increased the duplication in the placeDecor method. Duplication is undesirable and we want to get rid of it. And we will certainly try. But first more data:

local lootTable = {
    {6,"health(1,3)", 6,"nothing"},
    {4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"},
    {4,"health(5,10)", 3,"rock", 4,"curePoison", 16,"nothing"},
    {3,"curePoison", 10,"health(7,11)", 10,"nothing"}
}

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = #self:fromDefs(parsed)
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 3 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 4 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = #self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

Now let’s see about removing some duplication. No, wait, first I want to put in some more duplication:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = #self:fromDefs(parsed)
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 2 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 3 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    elseif self.levelNumber == 4 then
        decor = extendAll(decorTable[self.levelNumber])
        loot = randomize(extendAll(lootTable[self.levelNumber]))
        testCount = testCount + #self:fromDefs(zip(decor,loot))
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

I made all the count setting lines increment. I’m not sure why I chose to do that … I just wanted things to look more alike.

Now let’s extract some of that stuff. I begin with the last one:

    elseif self.levelNumber == 4 then
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)

And:

function DungeonBuilder:placeDecorFromTables(level, count)
    local decor = extendAll(decorTable[self.levelNumber])
    local loot = randomize(extendAll(lootTable[self.levelNumber]))
    return count + #self:fromDefs(zip(decor,loot))
end

Test. Tests pass. Repeat:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = testCount + #self:fromDefs(parsed)
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    elseif self.levelNumber == 2 then
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    elseif self.levelNumber == 3 then
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    elseif self.levelNumber == 4 then
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    else
        self:placeDecorRandom(count)
    end
    return testCount
end

Test. Tests Run.

Command decision: we’ll only allow for four levels and if the level is more than 4, we’ll repeat level 4. That’s what the main game building logic does anyway:

So we can do this:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        decor = extendAll({5, "Chest"})
        loot = extendAll({2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"})
        parsed = zip(decor,loot)
        testCount = testCount + #self:fromDefs(parsed)
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    else
        local level = math.min(self.levelNumber,4)
        testCount = self:placeDecorFromTables(level, testCount)
    end
    return testCount
end

Test. Tests pass.

I’m really glad I wrote that last test. It made these changes so much easier to check and I have utmost confidence in what I’ve done here.

I wonder what we could do about that special stuff for level 1. We really want all those chests to have just that very loot. How about this? We’ll refactor:

function DungeonBuilder:placeDecorFromTables(level, count)
    local decor = extendAll(decorTable[self.levelNumber])
    local loot = randomize(extendAll(lootTable[self.levelNumber]))
    return count + #self:fromDefs(zip(decor,loot))
end

First to this:

Oh! I just noticed that I’m still using the level number. Let’s fix that first.

function DungeonBuilder:placeDecorFromTables(level, count)
    local decor = extendAll(decorTable[level])
    local loot = randomize(extendAll(lootTable[level]))
    return count + #self:fromDefs(zip(decor,loot))
end

Test. Passes. Commit, we should be committing after each success, really. Commit: refactoring Decor placement code.

Now this:

function DungeonBuilder:placeDecorFromTables(level, count)
    local decorIn = decorTable[level]
    local lootIn = lootTable[level]
    local decor = extendAll(decorIn)
    local loot = randomize(extendAll(lootIn))
    return count + #self:fromDefs(zip(decor,loot))
end

We just extracted the parameter calculations. But now we can do this:

function DungeonBuilder:placeDecorFromTables(level, count)
    local decorIn = decorTable[level]
    local lootIn = lootTable[level]
    return self:placeZippedDecor(decorIn, lootIn, count)
end

function DungeonBuilder:placeZippedDecor(decorIn, lootIn, count)
    local decor = extendAll(decorIn)
    local loot = randomize(extendAll(lootIn))
    return count + #self:fromDefs(zip(decor,loot))
end

We extracted the new function placedZippedDecor, which we’d better test. Tests run. Commit to keep us honest: Refactor.

Now we can use that new function:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        local decorIn = {5, "Chest"}
        local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
        testCount = self:placeZippedDecor(decorIn, lootIn, testCount)
        testCount = self:placeDecorFromTables(self.levelNumber, testCount)
    else
        local level = math.min(self.levelNumber,4)
        testCount = self:placeDecorFromTables(level, testCount)
    end
    return testCount
end

That’s nearly good, but I still don’t like level 1. But wait. We can do a special addition for level one and then do the rest in what is now the else. Like this:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        local decorIn = {5, "Chest"}
        local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
        testCount = self:placeZippedDecor(decorIn, lootIn, testCount)
    end
    local level = math.min(self.levelNumber,4)
    testCount = self:placeDecorFromTables(level, testCount)
    return testCount
end

Now we see some asymmetry there. Let’s extract:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        testCount = testCount + self:placeLuckyChests(testCount)
    end
    local level = math.min(self.levelNumber,4)
    testCount = self:placeDecorFromTables(level, testCount)
    return testCount
end

function DungeonBuilder:placeLuckyChests(count)
    local decorIn = {5, "Chest"}
    local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
    return count + self:placeZippedDecor(decorIn, lootIn, count)
end

Test. Tests run. Commit: extract placeLuckyChests method.

I think this is nearly good. Let me post the whole thing so that we can admire it together:

Placing Decor

local decorTable = {
    {4,"BarrelEmpty", 4,"Crate", 4,"PotEmpty"},
    {5,"Chest", 2,"Skeleton1", 2,"Skeleton2", 2,"PotFull", 3,"PotEmpty", 3,"Crate"},
    {7,"Chest", 7,"Skeleton1", 8,"Skeleton2", 2,"BarrelEmpty", 3,"BarrelFull"}, -- 27
    {6,"Chest", 9,"Skeleton1", 8,"Skeleton2"} --23
}

local lootTable = {
    {6,"health(1,3)", 6,"nothing"},
    {4,"health(4,8)", 3,"strength(5,5)", 1,"catPersuasion", 9,"nothing"},
    {4,"health(5,10)", 3,"rock", 4,"curePoison", 16,"nothing"},
    {3,"curePoison", 10,"health(7,11)", 10,"nothing"}
}

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        testCount = testCount + self:placeLuckyChests(testCount)
    end
    local level = math.min(self.levelNumber,4)
    testCount = self:placeDecorFromTables(level, testCount)
    return testCount
end

function DungeonBuilder:placeLuckyChests(count)
    local decorIn = {5, "Chest"}
    local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
    return count + self:placeZippedDecor(decorIn, lootIn, count)
end

function DungeonBuilder:placeDecorFromTables(level, count)
    local decorIn = decorTable[level]
    local lootIn = lootTable[level]
    return self:placeZippedDecor(decorIn, lootIn, count)
end

function DungeonBuilder:placeZippedDecor(decorIn, lootIn, count)
    local decor = extendAll(decorIn)
    local loot = randomize(extendAll(lootIn))
    return count + #self:fromDefs(zip(decor,loot))
end

I think that’s definitely nearly good. I was tempted to put the lucky chests into the table somehow, but that seemed more odd than just checking the level.

What we are seeing, however, is that for the “real” story, using the feature has caused us to realize something that we didn’t know before: Level Designers will need to be able to create decor in batches, some of which are more customized than others. In a real system, we’d probably be expanding the definitions over in the Making App, but for our purposes, we do it here in DungeonBuilder.

Let me sum up, I need to crow a bit more.

Summary

Our zip and extend functions may still need some kind of packaging, but they work just fine. They are polluting the global namespace at the moment, but we could improve that, and probably should at least do that. Perhaps tomorrow. We’re done for today.

We wrote out our level decor definitions longhand, resulting in lots of duplication in the if statements. We intentionally increased duplication to make the four sections of our original method look more alike.

Then we captured and extracted that duplication, creating a method that did all that messy work, and called it, first once, then three times, then finally four times.

We were then left with the special case. We removed its duplication by extracting another tiny method from our existing placeDecorFromTables method, and using it for the special case. Finally we extracted even that code into a special method placeLuckyChests, which serves two purposes: it provides an explanatory name for the method, and reduces the visual complexity of the placeDecor method. But you know what? I think we can do better: Here’s placeDecor and placeLuckyChests:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    if self.levelNumber == 1 then
        testCount = testCount + self:placeLuckyChests(testCount)
    end
    local level = math.min(self.levelNumber,4)
    testCount = self:placeDecorFromTables(level, testCount)
    return testCount
end

function DungeonBuilder:placeLuckyChests(count)
    local decorIn = {5, "Chest"}
    local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
    return count + self:placeZippedDecor(decorIn, lootIn, count)
end

Let’s do this:

function DungeonBuilder:placeDecor(count)
    local decor,loot, parsed
    local testCount  = 0
    local level = math.min(self.levelNumber,4)
    testCount = testCount + self:placeLuckyChests(level, testCount)
    testCount = self:placeDecorFromTables(level, testCount)
    return testCount
end

function DungeonBuilder:placeLuckyChests(level, count)
    if level > 1 then return count end
    local decorIn = {5, "Chest"}
    local lootIn = {2,"health(4,10)", 2,"speed(3,5)", 1,"curePoison"}
    return count + self:placeZippedDecor(decorIn, lootIn, count)
end

Now the top method, placeDecor doesn’t manipulate level at all, it just passes it on. And the placeLuckyChests does nothing for levels greater than one.

Yes, I prefer that, and I hope that you do as well.

Are there lessons here? I think so.

The biggest one is that I was really resisting writing a test for placeDecor, but when I finally did, the test wasn’t that hard to write, and it drove out at least one assertion that was needed, and in the end the checking for the number of decor created was easy enough. (Of course, if we ever were to want to change the decor for a level, we’d have to improve that test. Maybe we’d do something like fetch the tables and count them. But that’s for another day.

For today, I ran that new test probably a dozen times or more. It was what made it possible for me to improve the placeDecor as much as I did, and according to my coding standards, it is very much improved. I would not have had the courage—or madness—to do that extensive a refactoring job without that test (and all the others in support, of course).

At the end of the day, metaphorically speaking, as it isn’t even 1100 yet, we now have a very cleanly refactored handful of methods producing fully custom levels of Decor in the dungeon, supported by some useful functions that probably need a better place to live.

Overall, a very satisfactory morning. I particularly enjoyed letting the duplication grow and grow until everything started to look oddly the same, then removing more and more until now there is essentially none at all.

See you next time!