Today, I’m going to create a new class to deal with Monsters. And I’ll tell you why it’s OK to do that. And it’ll be in tiny increments!

If I were to offer advice, and I confess that I used to do that, even when no one was asking for it, one piece that I’d offer would be never to improve existing code unless and until we’re working on it. There’s some decent reasoning behind that idea.

Generally, no matter how horrible the code is, if we’re not working on it, it’s not bothering anyone. Therefore, working on it just because it’s there is wasting time that could better be spent on something we are working on, usually some capability that our product owner or customer actually wants.

See Refactoring - Not on the backlog! for more on this notion.

That advice, if I were to offer it, and I’m not going to, would make us wonder why, today of all days, I’m going to do some refactoring in GameRunner, specifically working on the Monsters. And here’s why: we’re working on, or with, GameRunner all the time.

Almost everything that goes into the system touches GameRunner. New loots or monsters or spikes in the floor have to be plugged in, and generally have to be drawn or initialized or otherwise processed by GameRunner. Even when we don’t actually change GameRunner, we find ourselves trekking through it to find out, oh right now I remember, how something happens elsewhere in the system.

And GameRunner has at least three different responsibilities and needs improvement. For these reasons, it does make sense to improve it as I plan to do today.

Also, I want to, and you are not the boss of me.

Monsters

My long-term goals for GameRunner include removing all the dungeon creation off to one or more other classes. I’m not going to start there, however, because to do so would rather clearly require me to move a lot of methods. There is an incremental way of doing that, that I’m not going to use today.

What one does when building a new class to take on the work of an existing one, is to build a small shell class and just let it call back to the old class. Over time, we move methods into the new one, so we don’t have to call back any more. We can usually do this one or two methods at a time,which means that even though the whole refactoring is large, it is done by lots of tiny refactorings. This is the way.

But it’s not today’s way. Today, I’m going to clean up one tiny mess, in part to try out an idea, and we’ll see how it goes.

In last Friday’s Zoom Ensemble, we got to talking about collection classes, and I remarked that one has to be careful, and that I had more than once foundered on the shoals of trying to make some collection-like class of my own include all the methods of a real collection or table. Chet mentioned that a practice he likes is to create a specialized class that just handles the methods used for some specific collection. And that’s what we’re going to do today.

We’re going to create a class called Monsters, that has exactly one instance, that goes just about right here:

function GameRunner:setupMonsters(n)
    self.monsters = Monster:getRandomMonsters(self, 6, self.dungeonLevel)
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
end

This is where, in createLevel, we create the monsters. We store them in a simple table in GameRunner, and we use that table frequently:

function GameRunner:drawMonsters()
    for i,m in ipairs(self.monsters) do
        m:draw()
    end
end

function GameRunner:hasMonsterNearPlayer(range)
    for k,m in pairs(self.monsters or {}) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters) do
        if m:chooseMove() then inRange = true end
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

My plan is to move all that code off to the new class Monsters. Furthermore, just because it’s possible, I’m going to do it one method at a time, committing in between each step, so that we can see how even a larger example could be done bit by bit as time and need permit.

Now, I freely grant that doing things in tiny increments is sometimes tricky. If we don’t pick up the tangled ball of thread at the right spot, sometimes we can’t untangle it. So there will probably be mistakes along here, and if there aren’t, you can chalk some amount of it up to luck. Luck, as so often is the case, is at least partly making the right choices intuitively. So maybe it’s skill. Maybe it’s Maybelline.

I’m not sure how much of this thing needs TDD, but I guess I’d better start out that way at least. I’ll make a new tab, which makes me sad: I already have a lot of tabs in this program and they are hard to manage. Codea wasn’t made for the kind of modularity I like to use. A new version is coming, but it’s not here yet.

New tab, I guess, called Monsters. I’m going to put both the tests and the class in it.

-- Monsters
-- RJ 20210228

function testMonsters()
    CodeaUnit.detailed = true
    
    _:describe("Monsters class", function()
        
        local runner
        local room
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("Create the monsters", function()
        end)
        
    end)
end

So far so good. I plan to use the new Monsters class to implement this:

function GameRunner:setupMonsters(n)
    self.monsters = Monster:getRandomMonsters(self, 6, self.dungeonLevel)
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
end

That means that, for now, I’ll need to return a raw collection as part of that call. But I am allowed to change the method above, of course.

I think it should be changed to something roughly like this:

local monsters = Monsters()
monsters:getRandomMonsters(self,6, self.dungeonLevel)
monsters:startAllTimers()
return monsters:rawTable()

So our new class needs to be able to create itself, and to have those three methods.

Let’s see what we can test.

        _:test("Create the monsters", function()
            local monsters = Monsters()
            local raw = monsters:rawTable()
            _:expect(#raw).is(0)
        end)

The table coming back is going to be indexed, not hashed, so we can check its size. It should be empty at this point.

1: Create the monsters -- Monsters:19: attempt to call a nil value (global 'Monsters')

We code.

Monsters = class()

function Monsters:init()
    self.table = {}
end

function Monsters:rawTable()
    return self.table
end

Test runs. We could commit, but of course nothing is even plugged in yet. I guess we’ll get the monsters now:

        _:test("Create the monsters", function()
            local monsters = Monsters()
            local raw = monsters:rawTable()
            _:expect(#raw).is(0)
            local runner = nil
            monsters:getRandomMonsters(runner,6,1)
            raw = monsters:rawTable()
            _:expect(#raw).is(6)
        end)

Right. So we code:

function Monsters:getRandomMonsters(runner, count, level)
    self.table = Monster:getRandomMonsters(runner, count, level)
end

I was hoping this would work with a nil runner, but that was not to be:

1: Create the monsters -- Monster:24: attempt to index a nil value (local 'runner')

Monster does this:

function Monster:getRandomMonsters(runner, number, level)
    local t = self:getMonstersAtLevel(level)
    local result = {}
    for i = 1,number do
        local mtEntry = t[math.random(#t)]
        local tile = runner:randomRoomTile(1)
        local monster = Monster(tile, runner, mtEntry)
        table.insert(result, monster)
    end
    return result
end

Let’s make a fake runner:

FakeRunner = class()

function FakeRunner:randomRoomTile()
    return nil
end

        _:test("Create the monsters", function()
            local monsters = Monsters()
            local raw = monsters:rawTable()
            _:expect(#raw).is(0)
            local runner = FakeRunner()
            monsters:getRandomMonsters(runner,6,1)
            raw = monsters:rawTable()
            _:expect(#raw).is(6)
        end)

That might suffice, or we might have to beef it up. Run the tests to find out.

1: Create the monsters -- Monster:35: attempt to index a nil value (field 'tile')

That’s the call to addDeferredContents, which adds the monster to the tile in question. We can do this, I think:

FakeRunner = class()

function FakeRunner:addDeferredContents()
end

function FakeRunner:randomRoomTile()
    return self
end

And our test runs! Because we know that Monster:getRandomMonsters works, there’s no need to drill down further.

We can now use this capability in GameRunner. Let’s do it:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:getRandomMonsters(self, 6, self.dungeonLevel)
    self.monsters = monsters:rawTable()
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
end

This works, and we can commit: GameRunner uses Monsters class to create monsters.

The first part of our creation and insertion of the new Monsters class is done, committed, in the system, and in use. That’s how incrementally we want to do things.

Pretend it’s three days later instead of three seconds.

I realize that the name getRandomMonsters would be better if it were create. So be it:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel)
    self.monsters = monsters:rawTable()
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
end

function Monsters:createRandomMonsters(runner, count, level)
    self.table = Monster:getRandomMonsters(runner, count, level)
end

Now I want to do the rest of the GameRunner method:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel)
    self.monsters = monsters:rawTable()
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
end

I want to find up with this:

function GameRunner:setupMonsters(n)
    local monsters = Monsters()
    monsters:createRandomMonsters(self, 6, self.dungeonLevel)
    monsters:startAllTimers()
    self.monsters = monsters:rawTable()
end

Because I’m confident, and because I have revert if I need it, I leave that code in and do this:

function Monsters:startAllTimers()
    for i,m in ipairs(self.table) do
        m:startAllTimers()
    end
end

I expect this to work. And it does. I didn’t test that with a TDD test, because it seemed sure to work, would fail immediately if it failed in production, and because I wasn’t sure how to test it anyway.

We can commit: Monster timers started via Monsters class. Again, a tiny change toward our goal, and we can commit the whole system, working a bit better.

I don’t like fetching the raw table from Monsters, but the other methods of GameRunner use it. Let’s go after those:

There’s this:

function GameRunner:drawMonsters()
    for i,m in ipairs(self.monsters) do
        m:draw()
    end
end

That’s called … nowhere. Fascinating. Remove it. Tests run, games run, monsters move. Monsters are drawn as part of tiles, it would appear. Works for me.

Commit: remove unused GameRunner:drawMonsters.

That was easier than I had thought. What’s next?

function GameRunner:hasMonsterNearPlayer(range)
    for k,m in pairs(self.monsters or {}) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

This will be semi-interesting. It’s used only here:

function MonsterPlayer:checkForDanger()
    local close = self.runner:hasMonsterNearPlayer(5)
    if close then self.sp:play("battle") else self.sp:play("dungeon") end
end

This is the code that plays the dramatic battle music when you’re near a monster.

This code is known to work, though I am certain that it does not have a test. It’s not going to be easy to test, either, but I’m comfortable moving it. However, to move it, I need to have the real Monsters saved in the GameRunner, not the raw table, so that I can use it here.

I see two possible approaches. One is a new member variable, the other is to slam Monsters into GameRunner.monsters and make it work. There are only a few uses, so I decide to make it work.

function GameRunner:setupMonsters(n)
    self.monsters = Monsters()
    self.monsters:createRandomMonsters(self, 6, self.dungeonLevel)
    self.monsters:startAllTimers()
end

Now I have this:

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters) do
        if m:chooseMove() then inRange = true end
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

Which goes to this:

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters:rawTable()) do
        if m:chooseMove() then inRange = true end
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

And this:

function GameRunner:hasMonsterNearPlayer(range)
    for k,m in pairs(self.monsters or {}) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

Which goes similarly to this:

function GameRunner:hasMonsterNearPlayer(range)
    for k,m in pairs(self.monsters:rawTable() or {}) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

And these, similarly:

        _:test("monsters correctly initialized", function()
            local gm = GameRunner()
            Runner = gm
            gm:createLevel(12)
            local mt = gm.monsters:rawTable()
            _:expect(#mt).is(6)
            for i,m in ipairs(mt) do
                _:expect(m.level).is(1)
            end
        end)
        
        _:test("monsters correctly initialized level 2", function()
            local gm = GameRunner()
            Runner=gm
            gm:createLevel(12)
            gm:createLevel(12)
            local mt = gm.monsters:rawTable()
            _:expect(#mt).is(6)
            for i,m in ipairs(mt) do
                _:expect(m.level).is(2)
            end
        end)

I expect this has converted us to saving monsters as Monsters. Tests green, game runs. Commit: GameRunner uses Monsters instead of raw table.

Note that here again, we could leave the system in this state as long as needed, until another opportunity arises to complete this move of capability over to Monsters class.

What’s left? I decided to look at the other option for next step, this one:

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters:rawTable()) do
        if m:chooseMove() then inRange = true end
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

inRange is a global! and it isn’t used anywhere else. Therefore this code can be:

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters:rawTable()) do
        m:chooseMove()
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

That must be left over from when I was working on music. Should have cleaned that up. Tiny fool. In testing this, I found this interesting crossing in the map:

interesting

That’s due to two right-angle paths just touching at the corners. Rather nice. I love my dungeon carving code.

But I digress, let’s choose moves:

function GameRunner:moveMonsters()
    for k,m in pairs(self.monsters:rawTable()) do
        m:chooseMove()
        --if math.random() > 0.67 then m:chooseMove() end
        --if math.random() > 0.67 then m:chooseMove() end
    end
end

That’s only called from here:

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

So that becomes:

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

And:

function Monsters:move()
    for i,m in ipairs(self.table) do
        m:chooseMove()
    end
end

Game works just fine. Commit: monsters moved via Monsters class.

All that’s left using the raw table are tests, and this:

function GameRunner:hasMonsterNearPlayer(range)
    for k,m in pairs(self.monsters:rawTable() or {}) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

We do this:

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

function Monsters:hasMonsterNearPlayer(range)
    for i,m in ipairs(self.table) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

Tests run, game runs. Commit: monster action all handled by Monsters class.

Let’s sum up.

Summary

We’ve done six independent commits between 0900 and 1000. Each one left the game in running condition, and moved closer to moving GameRunner’s manipulation of the monsters table over to the Monsters class. The final commit has completed that move. GameRunner creates an instance of Monsters, and defers all action to it.

Now GameRunner has no responsibility for maintaining or processing the monsters, it just sends messages to the Monsters class, causing it to do the work. And the Monsters class? Quite simple:

Monsters = class()

function Monsters:init()
    self.table = {}
end

function Monsters:createRandomMonsters(runner, count, level)
    self.table = Monster:getRandomMonsters(runner, count, level)
end

function Monsters:hasMonsterNearPlayer(range)
    for i,m in ipairs(self.table) do
        if m:isAlive() and m:distanceFromPlayer() <= range then return true end
    end
    return false
end

function Monsters:move()
    for i,m in ipairs(self.table) do
        m:chooseMove()
    end
end

function Monsters:rawTable()
    return self.table
end

function Monsters:startAllTimers()
    for i,m in ipairs(self.table) do
        m:startAllTimers()
    end
end

That’s really straightforward. We do notice some duplication in there, namely

    for i,m in ipairs(self.table) do

We could probably improve that with something semi-fancy, but we’ll leave that for another day.

Today we moved some responsibility out of the over-burdened GameRunner, and we did it in tiny increments.

This is the way.

D2.zip