Space Invaders 52

I have an idea. Let’s find out if it’s a good idea.

We looked at this code yesterday as we worked on making the Player’s status cause the Army to stop firing for a bit.

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        sprite(asset.play, self.pos.x, self.pos.y)
    else
        if self.count > 210 then
            local c = self.count//8
            if c%2 == 0 then
                sprite(self.ex1, self.pos.x, self.pos.y)
            else
                sprite(self.ex2, self.pos.x, self.pos.y)
            end
        end
        self.count = self.count - 1
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
    popStyle()
    popMatrix()
end

I think I even commented what a mess it was. I have an idea for something to do about that. In the spirit of calling my shot, where I think we’ll be going is to reduce the drawing of the player down to a single method call, without all the if statements. I’m not sure we’ll get there. The idea might not work, or along the way we might decide we’ve gone far enough. But the idea is this:

Reduce the Player drawing to a single call to a “strategy”, which will be a stored function that does one part of the drawing represented in our current if-else mess. Do that in phases, first extracting smaller methods, then later, if it makes sense, plugging the appropriate one into the “strategy”.

Here goes. I’ll extract the big one first:

selection

We extract the above selection to get this code:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        sprite(asset.play, self.pos.x, self.pos.y)
    else
        self:drawExplosion()
    end
    popStyle()
    popMatrix()
end

function Player:drawExplosion()
    if self.count > 210 then
        local c = self.count//8
        if c%2 == 0 then
            sprite(self.ex1, self.pos.x, self.pos.y)
        else
            sprite(self.ex2, self.pos.x, self.pos.y)
        end
    end
    self.count = self.count - 1
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

This is about as pure an Extract Method pattern as we could want, so I expect that the program still works. You are perhaps surprised to know that it still does. (We do have that ignored method about hacking Lives to 4 so that we are allowed three, because of the change in sequencing of respawn. We’ll deal with that in due time.)

Now for something odd. I’m going to extract that single sprite call when we’re alive:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    else
        self:drawExplosion()
    end
    popStyle()
    popMatrix()
end

I did that because I want all my detailed draw methods to have the same trivial calling sequence.

Now, as I think of what to do next, I’m worried about drawExplosion:

function Player:drawExplosion()
    if self.count > 210 then
        local c = self.count//8
        if c%2 == 0 then
            sprite(self.ex1, self.pos.x, self.pos.y)
        else
            sprite(self.ex2, self.pos.x, self.pos.y)
        end
    end
    self.count = self.count - 1
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

Why, once that counts down the explosion, doesn’t that repeatedly call requestSpawn on every cycle?

The answer must be in requestSpawn:

function GameRunner:requestSpawn()
    if Lives > 0 then
        Lives = Lives - 1
        self.player:spawn()
        self:setWeaponsDelay()
    end
end

OK, so the player is spawned immediately. But that’s not what I see. I see the explosion happen, then a delay, then the player spawns, then firing resumes.

Oh my that’s too cute:

function Player:explode()
    if Cheat then return end
    Runner:playerExploding()
    self.alive = false
    self.count = 240
    SoundPlayer:play("explosion")
end

function Player:drawExplosion()
    if self.count > 210 then
        local c = self.count//8
        if c%2 == 0 then
            sprite(self.ex1, self.pos.x, self.pos.y)
        else
            sprite(self.ex2, self.pos.x, self.pos.y)
        end
    end
    self.count = self.count - 1
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

When we explode we set count to 240 (sixtieths of a second). When count is greater than 210 we show the explosion. When it’s not, we don’t show anything but we continue to count down another half second, then spawn. In a language where I could define constants without burning globals, I’d give those values names.

This is interfering with my neat idea even sooner than I expected. I had in mind three drawing strategies, drawNothing, drawPlayer, drawExplosion. And I had in mind that we could get there smoothly. Now I think we can get there, but I’m not sure how smoothly. Rather than push in a direction that may not be natural, let’s just improve targets of opportunity for a bit and see what happens. First, extract the toggling explosion drawing:

function Player:drawExplosion()
    if self.count > 210 then
        self:toggleExplosion(count)
    end
    self.count = self.count - 1
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

function Player:toggleExplosion(count)
    local c = self.count//8
    if c%2 == 0 then
        sprite(self.ex1, self.pos.x, self.pos.y)
    else
        sprite(self.ex2, self.pos.x, self.pos.y)
    end
end

We can possibly improve that last function a bit by folding the c value back in:

function Player:toggleExplosion(count)
    if (self.count//8)%2 == 0 then
        sprite(self.ex1, self.pos.x, self.pos.y)
    else
        sprite(self.ex2, self.pos.x, self.pos.y)
    end
end

Maybe that’s better. I’m not entirely convinced. We’d really like some convenient thing that worked directly from time rather than these counts, but I’m not sure what that might be, nor whether it would actually be better.

Because of the time, I’m calling a chai break right here in the middle. We’ll see, when I return, whether that was a good idea. I think we run OK and can commit: refactoring player draw.

Back, with chai

OK, where were we?

Let’s look at the whole drawing picture so far:

Now folks will tell you that if statements and similar conditionals are “bad”, and certainly when the nests get deep and intertwingled, they can get weird. I see one odd little opportunity here for an anti-if fanatic:

function Player:toggleExplosion(count)
    local select = (self.count//8)%2 + 1
    sprite(({self.ex1, self.ex2})[select], self.pos.x, self.pos.y)
end

Here we just play whichever explosion we select from the little table. This is nice enough that I could like it.

Let’s do this:

function Player:init(pos)
    self:setPos(pos)
    self.count = 0
    self.missile = Missile()
    self.gunMove = vec2(0,0)
    self.ex1 = readImage(asset.playx1)
    self.ex2 = readImage(asset.playx2)
    self.missileCount = 0
    self.alive = false
end

Let’s make the explosion table here:

function Player:init(pos)
    self:setPos(pos)
    self.count = 0
    self.missile = Missile()
    self.gunMove = vec2(0,0)
    self.explosions = { readImage(asset.playx1), readImage(asset.playx2) }
    self.missileCount = 0
    self.alive = false
end

Then use it here:

function Player:toggleExplosion(count)
    local select = (self.count//8)%2 + 1
    sprite(self.explosions[select], self.pos.x, self.pos.y)
end

This works as before. That count expression is irritating, though, isn’t it? What does it mean?

Well, count is 60ths of a second, and count//6 would be tenths of a second, so count//8 is a bit more (0.133 seconds in fact). So we’re toggling the explosion every 0.133 seconds. I just tried it at 6 instead of 8 and it still looks fine. I think I probably selected 8 because of the old program.

Either way, what have we got going on here? We’re trying to switch images “every so often”, where that means “about every tenth of a second”.

Well, with enough thought for now, I don’t see an easy way to improve that name or abstract out a better notion, at least not right now. So we’ll leave it and move on.

We can commit: refactored player explosion toggling. I left the divide at 6 for no good reason. Not quite a refactoring but close enough.

Now we have this weird thing going on. When the player is alive we draw it. When it isn’t, we “draw explosion”, but the truth is that we only draw the explosion when count > 210. When it isn’t, we draw nothing.

Then we futz around with counting on down to zero.

Let’s pull the futzing out and put it after the drawing of explosion:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    else
        self:drawExplosion()
    end
    self.count = self.count - 1
    if self.count <= 0 then
        Runner:requestSpawn()
    end
    popStyle()
    popMatrix()
end

No, wait, that’s not a refactoring: it has to be conditioned on not being alive. Undo that.

Here’s what I was imagining I could do:

Bring that count handling out
Now the two drawing methods are just drawing
Add in another drawing method that draws nothing
Switch between the three by storing the right one to use in a member variable.

That’s perhaps not clear, in part because it’s just a vision in my head of a promised land, seen vaguely through the fog.

Let’s do part of it now. I want to replace the if-else there with execution of a single member variable containing a function. Let’s break out a separate if and see if that helps:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    elseif self.count > 210 then
        self:toggleExplosion(count)
    else
        -- drawNothing
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
    popStyle()
    popMatrix()
end

I think this should work.

Apparently I’m wrong. No player ever appears. Why? Because now count is never checked because it inits to zero. We used to check it unconditionally if we weren’t alive. Now we don’t. I see two possible “fixes”. One is to check the alive flag ahead of the self.count > 0. The other, a bit more strange, would be to initialize count to 210 instead of 0 in init. That will draw nothing, count down 210 60ths, then spawn. Let’s try that.

function Player:init(pos)
    self:setPos(pos)
    self.count = 210 -- count down to first player
    self.missile = Missile()
    self.gunMove = vec2(0,0)
    self.explosions = { readImage(asset.playx1), readImage(asset.playx2) }
    self.missileCount = 0
    self.alive = false
end

That works, and rather neatly: there’s a nice pause before the first player shows up.

You may be thinking that right now this code is no better than it was when we started, and it may be worse. You’re not entirely wrong, although we do have some improvements in place. We have the count handling isolated at a higher level than deep inside some drawing function. (It should probably really be handled in update, shouldn’t it?) And we have three separate drawing functions broken out, if you count a comment as a function.

Let’s do move the count handling to update. It should work and it’s more like where it belongs:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
end

That leaves draw looking nearly good:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    elseif self.count > 210 then
        self:toggleExplosion(count)
    else
        -- drawNothing
    end
    popStyle()
    popMatrix()
end

Now I can begin to do what I set out to do. Let me re-express where I want to go.

I want to have a new member variable in Player, called drawingStrategy. I want it to contain a function, either drawPlayer or drawExplosion or drawNothing. And in the draw, I want to just call that function unconditionally.

Right now we’re calling toggleExplosion but I can move that logic into drawExplosion instead:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    elseif self.count > 210 then
        self:drawExplosion(self.count)
    else
        self:drawNothing()
    end
    popStyle()
    popMatrix()
end

function Player:drawNothing()
end

function Player:drawPlayer()
    sprite(asset.play, self.pos.x, self.pos.y)
end

function Player:drawExplosion()
    local select = (self.count//6)%2 + 1
    sprite(self.explosions[select], self.pos.x, self.pos.y)
end

I also implemented drawNothing. Now these three functions don’t have the same calling sequence, since drawExplosion wants to think about count. We can provide count to all three, or we can fetch it internally in the explosion drawing thing. I think we’ll pass it in.

No, there’s no reason to pass it. It’s an unadorned member variable, we can access it. And in fact, we were already accessing it inside, despite an attempt to pass it in. So now we’re here:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        self:drawPlayer()
    elseif self.count > 210 then
        self:drawExplosion()
    else
        self:drawNothing()
    end
    popStyle()
    popMatrix()
end

Let’s arrange for our new member variable drawingStrategy to always have the right contents:

function Player:init(pos)
    self:setPos(pos)
    self.count = 210 -- count down to first player
    self.missile = Missile()
    self.gunMove = vec2(0,0)
    self.explosions = { readImage(asset.playx1), readImage(asset.playx2) }
    self.missileCount = 0
    self.alive = false
    self.drawingStrategy = self.drawNothing
end

function Player:spawn(pos)
    self.alive = true
    self.drawingStrategy = self.drawPlayer
    self:setPos(pos)
end

function Player:explode()
    if Cheat then return end
    Runner:playerExploding()
    self.alive = false
    self.count = 240
    self.drawingStrategy = self.drawExplosion
    SoundPlayer:play("explosion")
end

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count <= 0 then
            self.drawingStrategy = self.drawNothing
            Runner:requestSpawn()
        end
    end
end

I’ll run this expecting no trouble, because the strategy isn’t in use. So far so good. Now we should be able to do this:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    self.drawingStrategy(self)
    popStyle()
    popMatrix()
end

This works with one exception: the explosion graphic continues until the new player is called for. That’ll be a problem here:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count <= 0 then
            self.drawingStrategy = self.drawNothing
            Runner:requestSpawn()
        end
    end
end

We need to set drawNothing, not when count goes to zero, but when count goes below 210:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count == 210 then
            self.drawingStrategy = self.drawNothing
        end
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
end

And that works charmingly.

Rather amazingly, though a series of small steps all of which worked (modulo some typos), we’ve moved from a very complex drawing function to a very simple one:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    self.drawingStrategy(self)
    popStyle()
    popMatrix()
end

Now I thought we couldn’t say this:

    self:drawingStrategy()

I thought we had to say:

    self.drawingStrategy(self)

I “learned” that when trying something fancy a month or more back. I was mistaken. This works fine and looks more rational:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    self:drawingStrategy()
    popStyle()
    popMatrix()
end

This all works, Commit: refactor Player to drawStrategy method.

We might well ask the question “is this really better”, so let’s talk about that as our summing up.

Is this really better?

Well, we went from this:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    if self.alive then
        sprite(asset.play, self.pos.x, self.pos.y)
    else
        if self.count > 210 then
            local c = self.count//8
            if c%2 == 0 then
                sprite(self.ex1, self.pos.x, self.pos.y)
            else
                sprite(self.ex2, self.pos.x, self.pos.y)
            end
        end
        self.count = self.count - 1
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
    popStyle()
    popMatrix()
end

To this:

function Player:draw()
    pushMatrix()
    pushStyle()
    self.missile:draw()
    tint(0,255,0)
    self:drawingStrategy()
    popStyle()
    popMatrix()
end

I’d certainly say that’s better. Our drawing strategies are all nearly trivial:

function Player:drawNothing()
end

function Player:drawPlayer()
    sprite(asset.play, self.pos.x, self.pos.y)
end

function Player:drawExplosion()
    local select = (self.count//6)%2 + 1
    sprite(self.explosions[select], self.pos.x, self.pos.y)
end

We reduced the visible complexity of the count handling, and moved it all inside one method:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    if self.count > 0 then
        self.count = self.count - 1
        if self.count == 210 then
            self.drawingStrategy = self.drawNothing
        end
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
end

We see that that method is a bit of a mess, but it’s isolated. Still, it could “use a little improvement”. Let’s do some:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    self:manageCount()
end

function Player:manageCount()
    if self.count > 0 then
        self.count = self.count - 1
        if self.count == 210 then
            self.drawingStrategy = self.drawNothing
        end
        if self.count <= 0 then
            Runner:requestSpawn()
        end
    end
end

We can improve manageCount, at least by my standards, by using Guard Clause:

function Player:manageCount()
    if self.count <= 0 then return end
    self.count = self.count - 1
    if self.count == 210 then
        self.drawingStrategy = self.drawNothing
    end
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

We could do this with Explaining Method Name

function Player:manageCount()
    if self.count <= 0 then return end
    self.count = self.count - 1
    if self:explosionOver() then self.drawingStrategy = self.drawNothing end
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

We could isolate that whole if:

function Player:manageCount()
    if self.count <= 0 then return end
    self.count = self.count - 1
    self:manageExplosion()
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

function Player:manageExplosion()
    if self:explosionOver() then self.drawingStrategy = self.drawNothing end
end

We could do similarly with spawning:

function Player:manageCount()
    if self.count <= 0 then return end
    self.count = self.count - 1
    self:manageExplosion()
    self:manageSpawning()
end

function Player:manageSpawning()
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

Here’s the whole update picture if you want it:

function Player:update()
    self.missile:update()
    if self.alive then
        self.pos = self.pos + self.gunMove + vec2(self:effectOfGravity(),0)
        self.pos.x = math.max(math.min(self.pos.x,208),0)
    end
    self:manageCount()
end

function Player:manageCount()
    if self.count <= 0 then return end
    self.count = self.count - 1
    self:manageExplosion()
    self:manageSpawning()
end

function Player:manageSpawning()
    if self.count <= 0 then
        Runner:requestSpawn()
    end
end

function Player:manageExplosion()
    if self:explosionOver() then self.drawingStrategy = self.drawNothing end
end

function Player:explosionOver()
    return self.count == 210
end

I think that’s nice except for the bit about self.alive. First commit: refactor Player:update.

We can’t quite remove the self.alive. If we did, it would move the explosion along as the player moved. That might look odd.

I’d like to fix that but we’ll leave it for now. Back to summing up.

Now our draw is entirely free of conditions, our update is nearly free of conditions, and what conditions we do have are nicely hidden inside tiny methods. We represent what to draw with a single member variable drawStrategy, that draws whatever’s currently appropriate. Basically drawStrategy is a function pointer except that it’s safe.

I like this result: it is consistent with my long-acquired value system for object-oriented programming. I expect that some of my readers will agree, and some may not. I’d enjoy hearing either view, via twitter or ronjeffries at acm dot org. You could even sign up on the Codea forum and comment on my invaders thread there. You could even get an iPad and Codea and play along.

Anyway this whole morning went swimmingly. We didn’t break any tests, but on the other hand, we didn’t write any new ones. At this moment I’m not sure how we might have better test-driven this. Arguably we did manual test-driven, but I don’t even want to give that concept the time of day.

We programmed in the old style, except in very small safe steps. Each change gave us something. Most of them gave us cleaner code by the local standard. At least one of them intentionally made things worse, so that things were broken out. Once they were broken out, it was easy to isolate them and plug in our strategy object.

Now I wonder if there could be an updateStrategy too.

Hmm. That’s for another day, if ever.

See you next time!

Invaders.zip