Dungeon 294

We need to talk. We need to code, as well. And we need to test. A nice result, a better commitment.

Yesterday, my work felt quite ragged. I atteribute most of that feeling to my Internet connection repeatedly failing in a way that neither I nor Spectrum seemed to understand. Without Internet, who would answer all my wife’s questions? Who would I have to zoom with? This was a potential disaster of planetary proportions.

But having a ragged day inspired me to think a bit about what was going on, how I was reacting, how my feelings affected my work. I find that the more I think about how I work, the better my work seems to go. The fact that I’m far from perfect now just tells me how far I had to go.

Seriously, I don’t think it is given to us to be perfect, but we can certainly all be a bit better. So what might make me better?

Tests

I’m a pretty decent programmer (I claim), and I tend to write very short methods, and that generally means that I make few logic errors, and when I do, they’re usually easy to find. That’s mostly good news, but the bad news is that I can often get away without doing TDD or even writing post-hoc tests.

In the case of the Dung program, this has led me to test things in game. It usually takes very little time, and it’s kind of relaxing after concentrating on code to play the game a bit. The fact that I spent probably five minutes yesterday to find a Mimic and test something can be ignored. Or can it?

When the in-game test works, I can return to the code and do the next thing. But when it doesn’t, it’s not always easy to figure out why. And five minutes in the game is long enough to flush one’s short term thinking that created the bug, which sometimes leads me to a series of print statements and more in-game testing, to get a trace of what happened.

But there is a darker message here. I absolutely would write TDD tests if they were easy to write. Often, they are not, and I can tell you exactly why they are hard to write:

Many of my tests have very complex set up!

Let’s think about yesterday’s situation as an example, and then generalize far beyond that case.

Player, Decor, Loot Interaction

The behavior I wanted involved the Player encountering a bit of Decor possibly containing some Loot. I wanted the Player to bump against the tile once, to trigger the Decor to do its damage, if any, and to display the Loot, if any. They should not be able to enter the tile. I wanted them to bump a second time, to receive the Loot. Again, they should not be allowed in. A third time and thereafter, I wanted them to be able to step on and over the Decor.

Why did I want that? Well, I like the visual effect of bumping and not getting in, but I want Decor to be able to be traversed in general, so that it can’t accidentally block doors or hallways. But good or bad reason, it was the behavior that I wanted.

The code I wrote to do that didn’t work. I’ll explain why, because it relates strongly to why the test would be hard to write.

A Decor had a state “open”. It started false, and when the player bumped the Decor, “actionWith” took place, managed by a TileArbiter, the object that decides whether a given Tile can be entered. The TileArbiter goes through the contents of the target tile, checking each resident to see whether it wants to be involved in “action”, and whether it will permit entry. As currently written, if any Tile resident allows entry, you can enter.

I thought that I wanted the Decor to refuse entry if it was not open, and allow entry if it was open. I thought that during opening, it would create a Loot, so that next time around, the Loot could refuse entry until it had given its gift, then the Loot would be gone, the Decor would still be open, and entry would be permitted.

That didn’t work. Let me describe why.

First, the resident’s interaction with the player takes place before the question is asked about entry being allowed. This meant that by the time the TileArbiter asked the Decor whether it would allow entry, it would be open, and would say yes. Given that one yes is enough, that would let you in, and you’d be standing on top of the Loot.

Second, I had been thinking, vaguely, that since the Loot would have been created, the Loot would refuse entry, and it would still be OK. (I freely grant that I wasn’t thinking about this in detail, just kind of “how it works, seems ok”.) But no! The Loot is created after the loop over Tile residents starts, so it doesn’t get picked up on the first time through. That may or may not be a design flaw, but TileArbiter has worked fine so far. And anyway, if we had rejected entry from the Decor as planned, everything would have been fine.

But the Decor was open by the time it was asked about entry, and so it was all “Sure, why not”.

Now this is all intricate enough to make a test the right thing to do, but I rationalized not doing it, because I felt that doing it right would be too hard. Why?

Well, we’d need a Tile, and it would need contents, which would mean that we needed to set up the DungeonContentsCollection, and probably a Dungeon for the Tile to live in, then we’d need a Decor and a Loot, and then we’d need a Player to try to enter, and then we’d have to write a test that tried three times to enter and checked all the state each time.

Now as a Detroit School¹ TDDer, I prefer to test against the real objects. A London Schooler might create a number of test doubles for the case, and would be in a position to inject them in wherever they needed to go. Even then, I think it would be a pretty intricate setup, and one would be sorely tempted not to do it. Doing it with the real objects … yucch. Especially feeling rattled, I just wasn’t up to it.

But here’s the larger issue: my design is causing these tests to be hard to write. If tests are hard to write, that’s a knock against the design. So we need to think about what’s going wrong.

What is It About the Design?

I’m sure there are quite a few issues here. I’ll see which ones I can identify just now.

GameRunner and Dungeon

GameRunner and Dungeon classes are both pretty much “God classes”. They know too much and they control too much. We’ve been working to tease apart these classes, moving Dungeon creation to a new class, and using the new classes like Tiles, which is a “tile finder” that contains and accesses the Tiles, via the Map …

Even with these changes, GameRunner and Dungeon know a lot, and the GameRunner is often the only object that something in the dungeon knows about, so it gets asked all the interesting questions.

As large and important classes, GameRunner and Dungeon are difficult to set up, and setting up testing versions is particularly fraught, so tests often wind up building essentially an entire game instance just to test a Tile or something.

Surprising Connections

The game’s structure seems mostly like a hierarchy, with a GameRunner having a Dungeon, which has Tiles, which have residents like Players, Monsters, and DungeonObjects. But when the Player, who is resident in some Tile, wants to move East, finding the Tile that is directly East is a big deal. It involves telling the GameRunner to reset what Tiles can be seen, asking the DungeonContents to find the Tile containing the Player (who does not really know her own tile), asking the Tile for the “legal neighbor” in that direction, then moving the Player to the returned Tile (again invoking DungeonContents) and then asking the Tile to illuminate, which invokes Bresenham to draw lines of illumination …

All this came about with the best of will, and the best thinking that I had available at the time. But the upshot is that all these objects make assumptions about their environment, and have connections that need to be in place for things to work at all.

Now, describing the bit above, we can almost see a better way to have done it. Why not wait until the move is accomplished—since it might not be—and then trigger an event to do the redrawing? Probably that triggering could be done somewhere near wherever we actually record the Player’s position. But even if that would be a better way, it’s not the way we have now, and we’re at the mercy of the existing code. If we create a Player and try to move it to a Tile containing a Decor … all this stuff has to happen, and that means we need to set up real or fake objects for all that action.

It’s just hard. Why? As Jerry Weinberg would have put it: “Because it got that way”.

We’re Here Now

It is of course tempting to say that we wouldn’t be having these problems had we only “done it right”. But we did it as rightly as we could see at the time, and berating ourselves about the past is a waste of a good shout. We can’t redo the past. This isn’t a game with save points.

What do we do now? Well, we deal with the world as we find it. We can make some resolutions and some decisions about how to proceed.

In the future …

I can certainly resolve to treat this experience as evidence that I need to be far more sensitive to my code’s resistance to testing, and quicker to get it back to easy to test. And I recognize that if that takes me much time at all, the pressure to build features will push against my commitment to do this refactoring. And, since I do know other ways to keep the code working … I’m not sure this commitment will make much difference. And anyway, I don’t know when, if ever, I’m going to get to start over on some new program.

Right now …

If I had a team here, or even one pair partner, this would be easier. We could commit to each other that when something is hard to test, we’ll spend a bit of time figuring out how to make it easier to test, and either do that, or, as a team, decide how else to build the necessary feature safely. With multiple minds coming up with ideas, and putting energy into the system, we would probably come up with some good ways to test some things.

As things stand, I’m on my own here, and when I’m inclined to just git ‘er done rather than test, there’s no little angel on my shoulder to get me to try harder. I’m trying as hard as I can, at every moment in time, even if it doesn’t look to you as if I am. I give what I’ve got in the moment. We all do.

Let’s think about the test that I did write. It’s a bit of a crock, but not entirely so.

        -- Decor requires three tries to enter,
        -- once to materialize the Loot
        -- once to fetch the loot without entering
        -- once to be allowed to enter thereafter.
        _:test("Decor open state", function()
            local tile = FakeTile()
            local decor = Decor(tile,item)
            decor.currentTile = function()
                return tile
            end
            decor.danger = decor.doNothing
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"3").is(true)
        end)

This is really here just to test these methods:

function Decor:actionWith(aPlayer)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        self.sprite = self:openSprite(self.kind)
        self:currentTile():moveObject(self.loot)
        local round = CombatRound(self,aPlayer)
        self.danger(self,round)
    end
end

function Decor:isOpen()
    return self.actionCount > 2
end

To do this testing, I had to monkey patch the class’s currentTile method to return a FakeTile (to avoid having to set up a DungeonCollection (and Dungeon, probably). And I had to monkey patch the danger function to doNothing, because danger is set up randomly in the Decor’s init, and I didn’t want to trigger a CombatRound, which would trigger a publish …. aieee.

Let’s think about what we want, what we really really want. We have a Decor that is not open. We know that it will receive an actionWith message. We know that it needs to do some rigmarole about changing its appearance, possibly doing damage, creating a Loot, and so on, in the actionWith, on the first time through. Then after that first actionWith, it will be asked whether it is open and we want it to return false. After all that. If it gets another actionWith, we want it to take no visible action, but then the next time it is asked whether it is open, it must return true, and do so thereafter.

So all we really want to test is whether the sequence of three actionWith, isOpen returns false, false, true.

So let’s see. Let’s change the test to what we’d like to write:

        -- Decor requires three tries to enter,
        -- once to materialize the Loot
        -- once to fetch the loot without entering
        -- once to be allowed to enter thereafter.
        _:test("Decor open state", function()
            local noTile = nil
            local decor = Decor(noTile,item)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"3").is(true)
        end)

Now, run the test.

4: Decor open state -- Decor:98: attempt to index a nil value (local 'tile')

function Decor:init(tile, loot, kind)
    self.kind = kind or Decor:randomKind()
    self.sprite = DecorSprites[self.kind]
    if not self.sprite then
        self.kind = "Skeleton2"
        self.sprite = DecorSprites[self.kind]
    end
    self.loot = loot
    tile:moveObject(self) -- line 98
    self.scaleX = ScaleX[math.random(1,2)]
    local dt = {self.doNothing, self.doNothing, self.castLethargy, self.castWeakness}
    self.danger = dt[math.random(1,#dt)]
    self.actionCount = 0
end

We could condition that move by an if, maybe commenting that we generally allow DungeonObjects to exist without tiles. We could put the FakeTile back in the test. We could even put the FakeTile into Decor’s init.

Let’s put the FakeTile back into the test. Maybe that’s not too high a price.

        _:test("Decor open state", function()
            local noTile = FakeTile()
            local decor = Decor(noTile,item)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(nil)
            _:expect(decor:isOpen(),"3").is(true)
        end)

4: Decor open state -- Decor:113: attempt to index a nil value

function Decor:actionWith(aPlayer)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        self.sprite = self:openSprite(self.kind)
-- line 113 is below:
        self:currentTile():moveObject(self.loot)
        local round = CombatRound(self,aPlayer)
        self.danger(self,round)
    end
end

This is the code we’re trying to test (and therefore, if TDDing, to write). But we’re only concerned with the logic, not the rigmarole. So let’s refactor, as we probably should have anyway:

function Decor:actionWith(aPlayer)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        self:openUp()
    end
end

function Decor:openUp(aPlayer)
    self.sprite = self:openSprite(self.kind)
    self:currentTile():moveObject(self.loot)
    local round = CombatRound(self,aPlayer)
    self.danger(self,round)
end

That doesn’t make the error go away, of course, but now we can do this:

function Decor:actionWith(aPlayer)
    self:playerAction(aPlayer, self.openUp)
end

function Decor:playerAction(aPlayer, method)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        method(self,aPlayer)
    end
end

function Decor:openUp(aPlayer)
    self.sprite = self:openSprite(self.kind)
    self:currentTile():moveObject(self.loot)
    local round = CombatRound(self,aPlayer)
    self.danger(self,round)
end

And if we change the test to call playerAction …

        _:test("Decor open state", function()
            local noAction = function() end
            local noTile = FakeTile()
            local decor = Decor(noTile,item)
            decor:playerAction(nil, noAction)
            _:expect(decor:isOpen(),"1").is(false)
            decor:playerAction(nil, noAction)
            _:expect(decor:isOpen(),"2").is(false)
            decor:playerAction(nil, noAction)
            _:expect(decor:isOpen(),"3").is(true)
        end)

The test passes. No more monkey patching, but the cost is that the actual code is a bit more tricky, And probably we should indicate that playerAction is private?

I think I’ll do some renaming, see if I like that better:

function Decor:actionWith(aPlayer)
    self:actionLogic(aPlayer, self.actionOperation)
end

-- pluggable action for testing
function Decor:actionLogic(aPlayer, actionMethod)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        actionMethod(self,aPlayer)
    end
end

function Decor:actionOperation(aPlayer)
    self.sprite = self:openSprite(self.kind)
    self:currentTile():moveObject(self.loot)
    local round = CombatRound(self,aPlayer)
    self.danger(self,round)
end

I have to change the test to call actionLogic, of course.

So this is interesting. By coding the test more simply, we’ve forced the code to accommodate the easier testing, at the cost of becoming a bit more obscure.

Let’s refactor the actionOperation a bit.

function Decor:actionOperation(aPlayer)
    self:changeAppearance()
    self:showLoot()
    self:applyDamage(aPlayer)
end

function Decor:applyDamage(aPlayer)
    local round = CombatRound(self,aPlayer)
    self.danger(self,round)
end

function Decor:changeAppearance()
    self.sprite = self:openSprite(self.kind)
end

function Decor:showLoot()
    self:currentTile():moveObject(self.loot)
end

This is just Composed Method applied to actionOperation. Tests still happy, of course. Let’s commit while we’re thinking about it: Improve testability for Decor opening logic. Mild refactoring.

Let’s look back and sum up. We might be done for the morning.

Summary

Yesterday’s feelings led me to a deeper consideration of my process, in particular my tendency not to test. That has led me to recognize that my objects are in some cases too large, too powerful, and too connected, to make testing easy.

Today’s experience was that with a bit of effort, I could write a fairly simple test for the behavior I needed. I’m going to try to take that to heart. When faced with a thing that “should” be TDD’d, I’ll try to write the test I’d like to write, not the test that I think I have to write, with all the messy setup, and then make the code help me make that test work.

You can watch me over the next while and see if I manage to live up to that rather weak “I’ll try” commitment.

What I had to do today doesn’t entirely please me. I’d prefer to be able to check the three-step logic and then plug it in to the Decor object. As it stands, it’s rather entangled, with that pluggable operation, which I replaced with my noAction empty function. Less significant but still a pain, I rather wish I could do without the FakeTile, but I hesitate to let things be created without tiles … although Loots can be that way. But it seems error-prone, since things aren’t really right if they haven’t a Tile to live on.

Maybe … maybe there should be a tiny “State” object that manages the Decor state and tells Decor what to do? We could TDD that and then use it.

I’ll try that next time. This time, I’ve managed to make testing this one thing a bit easier, and maybe given myself a way of thinking that will get more tests done. I’ll try, when I’m tempted to “just code” something and then test in-game, to do some TDD instead, following my new rule:

Write the easy test you would like to write, not the difficult test that the code seems to demand. Then make the easy test possible.

See you next time!