Another day of improv. Let’s see what looks worth doing. (Wow, a factory object!)

I’m going to start slowly today, see if I can warm up to something beyond trivial. I’m feeling particularly unintelligent so far this morning, which is probably good if I remain intelligent enough to be cautious. The basic plan is first to look for some simple opportunities to clean things up, then think of a feature that is within my limited abilities.

The Main tab now has only a few functions in it:

  • setup - which belongs here
  • touched - which belongs here
  • draw - which belongs here
  • runTests - which I want here
  • showTests - which I want here
  • drawSpeedIndicator
  • rectanglesIntersectAt
  • rectanglesIntersect
  • addToScore

The addToScore looks particularly out of place. We do init Score in Main:setup, but addtoScore is called from Invader and Saucer, when those objects are hit.

The Score is displayed in GameRunner. The Army knows the score values for the invaders and tells each invader her score when she is created. When the score changes, the Army changes the timing between bombs according to a table that it knows. The addScore function calls the function that adjusts that timing.

The Saucer is supposed to have a dynamically-calculated score but as yet we have not built that. The score depends on the number of shots that the Player has fired, based on a table lookup.

The Invaders know the Army so that they can talk to it. The Saucer does not, but probably should.

It seems to me, looking at this, that addScore should be a member function on Army, and that we should give the Army instance to the Saucer, and deal with the shot count connection when we improve the Saucer’s scoring.

I’ll begin by giving the army instance to Saucer, since it has an iffy initialization trick right now:

function Saucer:init(test)
    self.exploding = 0
    if test == nil then
        self.alive = false
    else
        self:go(test)
    end
end

The reason for this is that the tests want to test the saucer’s direction, which is based on the number of shots fired, so they pass in a value which runs the saucer’s “go” code right away. Normally the Army tells it to go when it wants it to go. So we’ll make that value second and give it a better name:

function Saucer:init(army, optionalShotsFired)
    self.exploding = 0
    if optionalShotsFired == nil then
        self.alive = false
    else
        self:go(optionalShotsFired)
    end
end

Then the saucer creators need to adjust their calling sequences:

function Army:init(player)
    self.player = player
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
        {vader31,vader32},
        {vader21,vader22}, {vader21,vader22},
        {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], army))
        end
    end
    self.weaponsAreFree = false
    self.armySize = 55
    self.invaderCount = 0
    self.invaderNumber = 1
    self.overTheEdge = false
    self.motion = vec2(2,0)
    self:defineBombs()
    self.bombDropCycleLimit = 0x30
    self.bombCycle = 0
    self.saucer = Saucer(self) -- <---
end

Wow, it’s past time to clean that up, isn’t it?

And in tests:

        _:test("Saucer starts left, goes right, stops right if shots even", function()
            local s = Saucer(nil, 2) -- <---
            _:expect(s:startPosition()).is(vec2(8,185))
            _:expect(s:stepSize()).is(vec2(2,0))
            _:expect(s:stopPosition()).is(vec2(208,185))
        end)

And a couple of others.

I expect this all to work. Let’s see why I’m wrong.

Strangely enough, everything works. Commit: saucer knows army.

It’s about time for my chai run but let’s move the addToScore method over to Army and use it there.

function Army:addToScore(added)
    Score = Score + added
    TheArmy:adjustTiming(Score)
end


function Invader:killedBy(missile)
    if not self.alive then return false end
    if self:isHit(missile) then
        self.alive = false
        self.army:addToScore(self.score) -- <---
        self.exploding =15
        SoundPlayer:play("killed")
        return true
    else
        return false
    end
end

function Saucer:killedBy(missile)
    if not self.alive then return false end
    if self:isHit(missile) then
        self.alive = false
        self.army:addToScore(self:score())
        self.exploding =15
        SoundPlayer:play("killed")
        return true
    else
        return false
    end
end

To my surprise that doesn’t work because it turns out that invaders don’t know the army after all. The code that creates them looks like this:

    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], 
            scores[row], army))
        end
    end

army in this context is undefined, therefore nil. Brilliant. Should say:

    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], self))
        end
    end

The invaders now score, but the saucer doesn’t go any more. This will take judgment, brains, and maturity to solve. And chai. Back in a bit …

And … back. Generally by now I’d have an idea for what happened. But in fact I’ve got nothin. Let’s see what the code says.

function Army:runSaucer()
    if self.saucer:isRunning() then return false end
    if self:yPosition() > 89 then return false end
    if math.random(1,20) < 10 then return false end
    self.saucer:go(self.player:shotsFired())
    return true
end

When is the saucer running?

function Saucer:isRunning()
    return self.alive or self.exploding > 0
end

So if the saucer showed alive we wouldn’t run it. Did our change make us think it’s alive?

function Saucer:init(army, optionalShotsFired)
    self.exploding = 0
    if optionalShotsFired == nil then
        self.alive = false
    else
        self:go(optionalShotsFired)
    end
end

I sure don’t think so. Better recheck the creation:

    self.saucer = Saucer(self)

That seems OK too.

When I ran it again, the saucer flew. So maybe it was just that I didn’t beat the odds? I’m not sure. I do know that when I managed to shoot the saucer, I got this:

Saucer:61: attempt to index a nil value (field 'army')
stack traceback:
	Saucer:61: in method 'killedBy'
	Army:248: in method 'checkForKill'
	Army:228: in method 'processMissile'
	Missile:26: in method 'draw'
	Player:61: in method 'draw'
	GameRunner:24: in method 'draw'
	Main:27: in function 'draw'

function Saucer:killedBy(missile)
    if not self.alive then return false end
    if self:isHit(missile) then
        self.alive = false
        self.army:addToScore(self:score()) -- <--- 61
        self.exploding =15
        SoundPlayer:play("killed")
        return true
    else
        return false
    end
end

I am confused. How can that be nil? Oh. I guess I needed that chai more than I realized:

function Saucer:init(army, optionalShotsFired)
    self.exploding = 0
    if optionalShotsFired == nil then
        self.alive = false
    else
        self:go(optionalShotsFired)
    end
end

We didn’t save it. Brilliant again:

function Saucer:init(army, optionalShotsFired)
    self.army = army
    self.exploding = 0
    if optionalShotsFired == nil then
        self.alive = false
    else
        self:go(optionalShotsFired)
    end
end

That should improve things, but I wonder whether there should be a test on Saucer that’s sufficiently robust to have caught that.

There will be in the future, when we implement the scoring, and therefore (probably) exercise the call to army. Right now our score is a literal and the only tests address hits and direction of motion. Now that it’s fixed, I don’t see a test that makes sense to write. Well, that’s “lack thinking”. Let’s do write one.

While writing the test, I notice this code, because I want my test to kill the Saucer:

function Saucer:killedBy(missile)
    if not self.alive then return false end
    if self:isHit(missile) then
        self.alive = false
        self.army:addToScore(self:score())
        self.exploding =15
        SoundPlayer:play("killed")
        return true
    else
        return false
    end
end

The code for saucer death is there in the middle of that method. Let’s pull it out:

function Saucer:killedBy(missile)
    if not self.alive then return false end
    if self:isHit(missile) then
        self:die()
        return true
    else
        return false
    end
end

function Saucer:die()
    self.alive = false
    self.army:addToScore(self:score())
    self.exploding =15
    SoundPlayer:play("killed")
end

Now I can test:

        _:test("Saucers score", function()
            local army = Army()
            local s = army.saucer
            local oldScore = Score
            s:die()
            _:expect(Score).isnt(oldScore)
        end)

Pretty weak test, but we don’t know what the score should be yet. I expect this to run. It doesn’t, quite, because Score isn’t initialized. This test will have to change when we get a little further down the Score road, but for now:

        _:test("Saucers score", function()
            local army = Army()
            local s = army.saucer
            Score = 0
            s:die()
            _:expect(Score).isnt(0)
        end)

That runs. If I remove the saving of army, which is why we’re writing this, I get:

32: Saucers score -- Saucer:70: attempt to index a nil value (field 'army')

So we have herded the cow back in and closed the barn door behind it. Now we have score updated by army. I so commit: score updated by Army

We could ship this if we needed to. Everything works. Unless I’ve missed something (again) but I am confident.

Let’s pull the score away from a global and put it into Army. I expect this will require a number of changes.

function setup()
    runTests()
    parameter.boolean("Cheat", false)
    Runner = GameRunner()
    SoundPlayer = Sound()
    invaderNumber = 1
    Lives = 4
    Score = 0 -- <--- this is right out
    Line = image(208,1)
    for x = 1,208 do
        Line:set(x,1,255, 255, 255)
    end
end

Now GameRunner actually displays the score, and I think it should. However, I want to argue that it makes sense for Army to keep the score, since the invaders and saucers all belong to it, and it would rightly be keeping track of casualties. So we’ll have GameRunner ask Army:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(Line,8,16)
    textMode(CORNER)
    fontSize(10)
    text("SCORE " .. tostring(TheArmy:score()), 144, 4) -- <---
    tint(0,255,0)
    local lives = self.lm:livesRemaining()
    text(tostring(lives), 24, 4)
    local addr = 40
    for i = 1,lives-1 do
        sprite(asset.play,40+16*i,4)
    end
    if lives == 0 then
        textMode(CENTER)
        text("GAME OVER", 112, 32)
    end
    drawSpeedIndicator()
    popStyle()
end

I’m using a method, not just accessing a variable because our growing coding standard is that we don’t mess with other people’s insides. So in Army:

function Army:score()
    return self.score
end

Some inits and other references:

function Army:init(player)
    self.player = player
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
        {vader31,vader32},
        {vader21,vader22}, {vader21,vader22},
        {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], self))
        end
    end
    self.weaponsAreFree = false
    self.armySize = 55
    self.invaderCount = 0
    self.invaderNumber = 1
    self.overTheEdge = false
    self.motion = vec2(2,0)
    self:defineBombs()
    self.bombDropCycleLimit = 0x30
    self.bombCycle = 0
    self.saucer = Saucer(self)
    self.score = 0
end

function Army:addToScore(added)
    self.score = self.score + added
    TheArmy:adjustTiming(self.score)
end

There’s that messy init again. We’ll do something with that soon, I promise myself.

There’s just one reference in tests, fixed as follows:

        _:test("Saucers score", function()
            local army = Army()
            local s = army.saucer
            local oldScore = army:score()
            s:die()
            _:expect(army:score()).isnt(oldScore)
        end)

That’s how I wanted to do it anyway. I think we should be good to go. Let’s try. A big mistake:

GameRunner:37: attempt to call a number value (method 'score')
stack traceback:
	GameRunner:37: in method 'drawStatus'
	GameRunner:26: in method 'draw'
	Main:26: in function 'draw'

We can’t call the method score and the variable score. Let’s change the method to getScore(). I’ll spare you the listings.

We are now all green except for that ignored test. What was that, anyway, it was supposed to remind me of something:

        _:ignore("Lives hack initialized to 4", function()
            
        end)

Yes, we init Lives to 4 and then immediately decrement it when we spawn the first player. That’s odd. We’ll leave it as is.

We can commit: Score is now internal to Army.

Moving Right Along

Let’s do at least some cleaning up in Army:init:

function Army:init(player)
    self.player = player
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
        {vader31,vader32},
        {vader21,vader22}, {vader21,vader22},
        {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], self))
        end
    end
    self.weaponsAreFree = false
    self.armySize = 55
    self.invaderCount = 0
    self.invaderNumber = 1
    self.overTheEdge = false
    self.motion = vec2(2,0)
    self:defineBombs()
    self.bombDropCycleLimit = 0x30
    self.bombCycle = 0
    self.saucer = Saucer(self)
    self.score = 0
end

Let’s just extract all the invader creation stuff at the beginning:

function Army:init(player)
    self.player = player
    self:marshalTroops()
    self.weaponsAreFree = false
    self.armySize = 55
    self.invaderCount = 0
    self.invaderNumber = 1
    self.overTheEdge = false
    self.motion = vec2(2,0)
    self:defineBombs()
    self.bombDropCycleLimit = 0x30
    self.bombCycle = 0
    self.saucer = Saucer(self)
    self.score = 0
end

function Army:marshalTroops()
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
    {vader31,vader32},
    {vader21,vader22}, {vader21,vader22},
    {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], self))
        end
    end
end

That’s better already. Now I’d like to extract two methods from marshalTroops, one to set up the tables and one to use them to create the invaders. But the local variables kind of thread down into the vaders table, which is custom-made to fit the fact that there is one rank of type 3 invaders, and two ranks of types 2 and 1. And each invader gets two images.

I think we need a new object to help us with this. It’s an army factory or something similar. It’s a MarshallingCenter:

function Army:marshalTroops()
    self.invaders = MarshallingCenter():marshalArmy()
end

When we need an army, we just phone up the marshalling center and have them marshal up an army. Should be easy enough:

MarshallingCenter = class()

function MarshallingCenter:init()
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
    {vader31,vader32},
    {vader21,vader22}, {vader21,vader22},
    {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], self))
        end
    end
end

function MarshallingCenter:marshalArmy()
    return self.invaders
end

This was just a paste into the new class. I propose to refactor it a bit but I rather think this is supposed to work. Well, nearly:

Invader:45: attempt to call a nil value (method 'addToScore')
stack traceback:
	Invader:45: in method 'killedBy'
	Army:224: in method 'checkForKill'
	Army:218: in method 'processMissile'
	Missile:26: in method 'draw'
	Player:61: in method 'draw'
	GameRunner:24: in method 'draw'
	Main:26: in function 'draw'

We need to pass in the army to init the invaders. Curiously, that will put the code back the way it was earlier today:

function MarshallingCenter:init(army)
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    local vaders = {
    {vader31,vader32},
    {vader21,vader22}, {vader21,vader22},
    {vader11,vader12}, {vader11,vader12}
    }
    local scores = {30,20,20,10,10}
    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], army))
        end
    end
end

function Army:marshalTroops()
    self.invaders = MarshallingCenter(army):marshalArmy()
end

Curiously, I get the same error:

Invader:45: attempt to index a nil value (field 'army')
stack traceback:
	Invader:45: in method 'killedBy'
	Army:224: in method 'checkForKill'
	Army:218: in method 'processMissile'
	Missile:26: in method 'draw'
	Player:61: in method 'draw'
	GameRunner:24: in method 'draw'
	Main:26: in function 'draw'

Ah. Silly me:

function Army:marshalTroops()
    self.invaders = MarshallingCenter(self):marshalArmy()
end

I think this is going to have to change, but it really ought to work now. And it does. But I want to do the marshalling in the marshal function and just the initialization reads in the init.

And (darn it) I really think the army should be passed in to the marshal call, not the init.

function MarshallingCenter:init()
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    self.vaders = {
    {vader31,vader32},
    {vader21,vader22}, {vader21,vader22},
    {vader11,vader12}, {vader11,vader12}
    }
    self.scores = {30,20,20,10,10}
end

function MarshallingCenter:marshalArmy(army)
    local invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(invaders, 1, Invader(p,self.vaders[row], self.scores[row], army))
        end
    end
    return invaders
end

And I have to reverse out my previous change to this:

function Army:marshalTroops()
    self.invaders = MarshallingCenter():marshalArmy(self)
end

This all works. Commit: Army created in MarshallingCenter.

Let’s sum up, mostly focused on Army and the MarshallingCenter, which now look like this:

function Army:init(player)
    self.player = player
    self:marshalTroops()
    self.weaponsAreFree = false
    self.armySize = 55
    self.invaderCount = 0
    self.invaderNumber = 1
    self.overTheEdge = false
    self.motion = vec2(2,0)
    self:defineBombs()
    self.bombDropCycleLimit = 0x30
    self.bombCycle = 0
    self.saucer = Saucer(self)
    self.score = 0
end

function Army:marshalTroops()
    self.invaders = MarshallingCenter():marshalArmy(self)
end

MarshallingCenter = class()

function MarshallingCenter:init()
    local vader11 = readImage(asset.inv11)
    local vader12 = readImage(asset.inv12)
    local vader21 = readImage(asset.inv21)
    local vader22 = readImage(asset.inv22)
    local vader31 = readImage(asset.inv31)
    local vader32 = readImage(asset.inv32)
    self.vaders = {
    {vader31,vader32},
    {vader21,vader22}, {vader21,vader22},
    {vader11,vader12}, {vader11,vader12}
    }
    self.scores = {30,20,20,10,10}
end

function MarshallingCenter:marshalArmy(army)
    local invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(invaders, 1, Invader(p,self.vaders[row], self.scores[row], army))
        end
    end
    return invaders
end

There’s still more to do to make Army’s init really good looking but it’s much improved. We’ve added a sort of Invader factory object, which I decided to call a Marshalling Center, which is a place where things are arranged for shipping out. Maybe it should be called InvaderFactory but for now I like the name. Invaders are people too.

But why did I break the function out between init and marshalArmy? I did that because what’s in init is the setup for creating an army, building the data structures we need, and marshalArmy does the work of creating a new army. We could now use a single marshalling center to create many armies. True, we don’t need that now (nor probably ever) but the distinction between setting up the factory and manufacturing something makes sense to me.

No Idea

I had no idea that we’d build this object. Certainly I wasn’t planning on it for today, and I’m not sure if I’ve even ever thought in passing that a thing like this was needed. I’m sure I’ve noticed how complicated Army:init was getting, but if I thought of an invader factory object before, I don’t recall it.

What made it come to mind was that there is an obvious break in the original blob of code, between getting ready to create the invaders array, and then creating it. But all the information to create it was in local variables, which meant that to break the method up into the two stages of “prepare” and “do” was difficult. We could have created a “do” method that took a whole raft of parameters, but that was so messy that it didn’t even enter my mind to do that.

When a chunk of code is mostly concerned with its own internals, as this one was, with just the one outside reference to army, there’s usually an opportunity for a small class that handles just the items in that chunk. And in our case, since it was a creator of a structure, making it into a factory kind of object made sense. Arguably, because it is a factory, we should call it that. I’m going to let the domain name prevail over the implementation name, at least for now.

So my thoughts went quickly to extracting the class, as soon as I had that code isolated into the marshalTroops method. It was clear that that method had almost nothing to do with the Army.

If that notion doesn’t just spring into a developer’s mind, as it did into mine, what rule of thumb might they follow? Perhaps something like:

If a method, function, or patch of code pays little attention to its current home, and lots of attention elsewhere, it is a candidate for moving. If it mostly refers to things of its own creation, it may be calling out for a new class.

This is a type of “feature envy”, of course, where code refers to information in some other class, but in this case, it refers to code that isn’t in any class … yet.

Anyway, that went well, I think, and we have improved the code substantially this morning. We have no new features, but we have a version that is improved and worth building further upon. We’ll ship it, and ship this article, content with the morning’s work.

Invaders.zip