OK, with discussion of ease out of the way, let’s get some ease up in this Decor.

Much as I’d rather not, let’s take a look at where we are on Decor, and why what we’re doing is, and isn’t, easy, and what we’re going to do about it.

Here are the tests from yesterday:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local decorKind = Decor:kindFrom(def)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot:fromDef(def)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)
        
        _:test("Invalid Decor kind", function()
            local decorKind = Decor:kindFrom("skeleton2,foo,bar")
            _:expect(decorKind).is("noSuchKind")
        end)

Obviously we don’t have very robust parsing going on here, since we switched the first test to use semicolon separators, so that we could have the “(5,10)” bit to provide min and max values for things like “health”. I think I’ll leave the second test wrong, so that it’ll fail and surprise me when we beef up the parsing.

It’s time to figure out what we want to do with the tile definition, where so far I’ve just written “r1”. Before we do that, however, I think we should move these tests over to DungeonBuilder, which will be the object that uses this capability, since it does all the work of building up dungeons.

The move goes smoothly, as you’d expect given how simple these tests are. Let’s review how they work. If I recall, we’re doing the parsing twice and in different classes.

Yes, we see that right in the test:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local decorKind = Decor:kindFrom(def)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot:fromDef(def)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)

We have decor and Loot parsing the “def”:

function Decor:kindFrom(aString)
    local parsed = {}
    for w in string.gmatch(aString, "%w+") do
        table.insert(parsed,w)
    end
    return self:legalKind(parsed[1])
end

function Decor:legalKind(proposedKind)
    return self:legalKinds()[proposedKind] or "noSuchKind"
end

function Decor:legalKinds()
    return {
        Skeleton1="Skeleton1",
        Skeleton2="Skeleton2",
        Chest="Chest",
        BarrelEmpty="BarrelEmpty",
        BarrelClosed="BarrelClosed",
        BarrelFull="BarrelFull",
        Crate="Crate",
        PotEmpty="PotEmpty",
        PotFull="PotFull",
    }
end

function Loot:fromDef(defString)
    -- kind,loot(min,max),tile
    local pat = "[^;]+"
    local parsed = {}
    for w in string.gmatch(defString,pat) do
        table.insert(parsed,w)
    end
    local min, max = 0,0
    local minMaxPat = "%w+"
    local iter = string.gmatch(parsed[2],minMaxPat)
    local kind = iter()
    local min = tonumber(iter() or 0)
    local max = tonumber(iter() or 0)
    return Loot(nil, kind, min, max)
end

The Loot version is the most current, but in any case we really should be parsing this control table and then sending each class just the info it needs to do the job.

Let’s refactor a bit, to do that. I’m not sure where the parsing should go. Certainly not in all the detail classes, though I think Loot can deal with the “()” bit. But maybe the parsing doesn’t really belong in DungeonBuilder either. It seems kind of low-level. For now, we’ll do it in Builder and then see how it looks.

So we recast the test:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            local decorKind = Decor:legalKind(parsed.decor)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot:fromString(parsed.loot)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)

We were talking about ease in today’s first article. This test exhibits ease. I just imagine a new object Parse and method decorDefinition that returns a Parse instance that, rather clearly, returns decor and loot strings. Then we pass those in to Decor and Loot for them to decode the info and return suitable instances. We can use the legalKind method directly, I think, and we’ll need to provide fromString in Loot. I’m not sure about the names. We’ll see what happens.

Test should fail on Parse.

3: Decor from definition string -- DungeonBuilder:45: 
attempt to index a nil value (global 'Parse')

We’ll write that right inside the Builder tab, as it is in essence a private class at this point.

local Parse = class()

function Parse:decorDefinition(aString)
    local pat = "[^;]+"
    local iter = string.gmatch(aString,pat)
    return self(iter(),iter(),iter())
end

function Parse:init(decor,loot,room)
    self.decor = decor
    self.loot = loot
    self.room = room
end

This will work, down to fromString:

3: Decor from definition string -- DungeonBuilder:62: attempt to call a nil value (method 'fromString')

As expected. We can talk about Parse in a moment, I need to make this test run. I edit down the fromDef to make fromString:

function Loot:fromString(defString)
    -- loot(min,max)
    local min, max = 0,0
    local minMaxPat = "%w+"
    local iter = string.gmatch(defString,minMaxPat)
    local kind = iter()
    local min = tonumber(iter() or 0)
    local max = tonumber(iter() or 0)
    return Loot(nil, kind, min, max)
end

I expect my test to pass, and it does. I sort of expected the other one to fail but it didn’t. Let’s have a look at both:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            local decorKind = Decor:legalKind(parsed.decor)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot:fromString(parsed.loot)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)
        
        _:test("Invalid Decor kind", function()
            local decorKind = Decor:kindFrom("skeleton2,foo,bar")
            _:expect(decorKind).is("noSuchKind")
        end)

Ah. Should remove kindFrom, as we no longer plan to use it. Now it fails:

4: Invalid Decor kind -- DungeonBuilder:70: attempt to call a nil value (method 'kindFrom')

And we improve it:

        _:test("Invalid Decor kind", function()
            local def = "skeleton2;foo;bar"
            local parsed = Parse:decorDefinition(def)
            local decorKind = Decor:legalKind(parsed.decor)
            _:expect(decorKind).is("noSuchKind")
        end)

Now, truth be told, I’m not sure I want it to return “noSuchKind”. I want these tables always to produce legal Decor, so that the game runs as well as it can even in the face of errors. My general practice is not to throw exceptions nor return things that won’t work, where I can avoid it. But for now, we’ll let this ride. Perhaps we should configure a valid Decor, perhaps we should return some kind of “NullObject” decor that will be ignored. Out of scope for now, I think.

Similarly, we might want to consider what to do with input strings that don’t have enough semicolons or that contain other errors. I’m sure there is a countable infinity of possible wrong input strings. However, other than making sure that the code doesn’t crash, I truly don’t care. It is the job of the caller to be sure that their input is good, and the job of this code not to explode, but to carry on even when the input isn’t good.

For now, of course, we’ve got nothing. Let’s invent a new thing, an InvalidParse object. In our tests:

        _:test("Invalid Parse", function()
            local def = "skel,foo,bar"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(false)
        end)

I also added checks for valid in the other tests, checking that it’s true. The implementation is simple:

local InvalidParse = class()

function InvalidParse:init()
    self.valid = false
    self.decor = "invalidKind"
    self.loot = "invalidLoot"
    self.room = "invalidRoom"
end

local Parse = class()

function Parse:decorDefinition(aString)
    local pat = "[^;]+"
    local iter = string.gmatch(aString,pat)
    local parse = {}
    for w in iter do
        table.insert(parse,w)
    end
    if #parse ~= 3 then return InvalidParse() end
    return self(parse[1], parse[2], parse[3])
end

function Parse:init(decor,loot,room)
    self.valid = true
    self.decor = decor
    self.loot = loot
    self.room = room
end

Tests run. I’m not sure just what we’ll do with the InvalidParse guy, but now we have it.

Heads Up

OK, time to lift our head up from the muck and think about what we should do next. I reckon we should work on the room generation. And thinking about that makes me consider something else …

For the room definition, I know that we have one room definition that we’ll surely want, called something like randomRoomTileAvoidingRoom(1). So we’ll devise some simple code language to put in the Decor definition that will tell us which room-finding function to call.

But that makes me think that we’ll probably want a parameter in the room part of the definition, so we can put in the (1) or whatever room we might want to mention. And that makes me think that we probably should parse the definition string entirely in Parse, rather than do part of it in Loot (and, if we were to follow that pattern, part of it in some other class).

So let’s refactor to that design before we proceed.

I am tempted to generalize this to have some kind of ParsedComponent object with a name,max, min kind of thing but I’m going to resist that, and stay ad-hoc for now.

Even as I type that, I realize that I’m not going to like it, but let’s go ahead and see why. The problem is in the Loot line of the test:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot:fromString(parsed.loot)

Since we’ll have parsed the definition in Parse, if the Parse instance is “flat”, we will just go ahead and call the Loot constructor. But we’d have to say something like

Loot(nil, parsed.lootName, parsed.lootMin, parsed.lootMax)

We’re not going to like that but it’s simple enough, so we’ll go with it:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor)
            _:expect(decorKind).is("Skeleton1")
            local loot = Loot(nil, parsed.lootName, parsed.lootMin, parsed.lootMax)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)

This won’t work, surely, because all those loot things are nil. Test just to see how it explodes.

3: Decor from definition string -- Inventory:383: attempt to concatenate a nil value (field 'description')

Wow, that’s deep in there. Loot, like most of my objects, just assumes that its caller isn’t a complete fool. This is sometimes the case.

function Loot:init(tile, kind, min, max)
    self.kind = kind
    self.min = min
    self.max = max
    self.item = self:createInventoryItem()
    if tile then tile:moveObject(self) end
end

We could do some nil checking here. Normally I wouldn’t, so let’s not. Let’s just make it work, in Parse.

function Parse:decorDefinition(aString)
    local pat = "[^;]+"
    local iter = string.gmatch(aString,pat)
    local parse = {}
    for w in iter do
        table.insert(parse,w)
    end
    if #parse ~= 3 then return InvalidParse() end
    kind,min,max = self:kindMinMax(parse[2])
    return self(parse[1], kind,min,max, parse[3])
end

function Parse:kindMinMax(aString)
    -- kind(min,max) parens optional default 0
    local iter = string.gmatch(aString, "%w+")
    return iter(), tonumber(iter() or 0), tonumber(iter() or 0)
end

I needed to change the test to refer to lootKind, not lootName. And the test runs. And I’ve deleted the loot from string method.

But seriously, as we do this, do we really want things like lootKind, lootMin, and lootMax in there? Shouldn’t we do another little structured object to hold the kind,min,max?

On a given day, I might skip that. But I think that in the interest of future ease, we should improve this, and frankly it’ll be easy now, so it’ll feel good to do it better.

I typed this in before writing the test. I apologize, but I ain’t gonna lie.

local ParseElement = class()

function ParseElement:init(kind,min,max)
    self.kind = kind
    self.min = min or 0
    self.max = max or 0
end

My plan is to create one of these for every field in the definition, however many there are. I think it’ll be cleaner that way, but I admit I’ve got some YAGNI concerns here, since right now only the middle of the string needs it. We’ll see.

The test becomes this:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor.kind)
            _:expect(decorKind).is("Skeleton1")
            local lootElement = parsed.loot
            local loot = Loot(nil, lootElement.kind, lootElement.min, lootElement.max)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)

It won’t run:

3: Decor from definition string  -- Actual: noSuchKind, Expected: Skeleton1
3: Decor from definition string -- DungeonBuilder:96: attempt to index a nil value (local 'lootElement')

Make the parser do the new things. The changes were a bit more extensive than I’d like, but the test led me right through it:

function Parse:decorDefinition(aString)
    local pat = "[^;]+"
    local iter = string.gmatch(aString,pat)
    local parse = {}
    for w in iter do
        table.insert(parse, self:parseElement(w))
    end
    if #parse ~= 3 then return InvalidParse() end
    return self(parse[1], parse[2], parse[3])
end

function Parse:parseElement(aString)
    -- kind(min,max) parens optional default 0
    local iter = string.gmatch(aString, "%w+")
    return ParseElement(iter(), tonumber(iter() or 0), tonumber(iter() or 0))
end

We’re at a solid point – and have been pretty solid right along. Well past time to commit: Improving tests for new Decor building.. Added Parse, InvalidParse, and ParseElement helper classes.

Let’s reflect and sum up.

ReflectoSummary

Today went very smoothly. I think that reflecting on and writing about ease helped me focus on keeping it smooth. I’m sure it helped guide me to what seems to be a simple yet useful breakdown of classes. Of course it helped a lot that the tests for Decor are nicely isolated and don’t require a lot of setup of dungeons and gamerunners and whatever all. That is partly a result of our refactoring from a while back, where we untangled some of the connections between all the bottom level objects and the top ones.

I think the insertion of the little ParseElement object was good. Possibly we shouldn’t have allowed all the elements of the parsed string to have parameters, but it made the code simpler and is surely harmless if they do not have parameters, as is the case at least for today.

Of course, it’s possible that someday we’ll have more parameters than two, or wish to name them something other than min and max. I think someday might come tomorrow or Thursday, but we’ll burn that boat when we come to it.

If I were to give myself any advice from today’s efforts, it would be to be quicker to notice when something is difficult, and to take the time to figure out why it’s difficult and what can be done about it.

That said, what’s been going on over the past half-dozen days is the creation of a little “language” for specifying dungeon contents, and it’s reasonable to expect some trial and error. And, in my strong view, it’s better to design these things in the presence of the code than in the head, in diagrams, or otherwise “on paper”. It’s in the using of a language that we learn what we like or don’t like. Often things that look sweet are sour, things that look awkward are graceful. And, in the trying, we can sweeten the sour and smooth out the awkward.

A good day. Join me for the next!