Space Invaders 79

Today, some code review. No big plans. We’ll see what we see. I expect to mention Beck’s Four Rules.

In fact, let’s get those rules right out here.

Kent Beck’s Four Rules of Simple Design

Runs all the tests;
Contains no duplication;
Expresses all the programmers’ ideas;
Minimizes the number of programming entities.

These rules are in priority order. There is dispute and discussion about the order of rules 2 and 3, and in fact Beck himself has expressed the rules in both orders. I prefer this order, because it makes me think whenever I encounter duplication.

If you’ve read my many articles that include programming, you’ll have seen me discovering and removing duplication many times. Duplication in code reflects the fact that there is an idea–whatever the code does–that is not explicit We fix the problem by pulling the duplication into a function, a method, or even a class, and giving it a name, which can then be used by all those who formerly duplicated the code.

You can see why the duplication/idea expression are so closely related: we fix the one by better applying the other.

You may be wondering why I mention these rules just now. We’re going to be looking at the code today, browsing around, and looking for things that could use a little improvement. We’ll be driven primarily by these four rules. You may be surprised how much our code can be improved by just these four simple ideas.

Let’s get started.

Looking Around

The Main tab isn’t perfect, but I’m interested in bigger fish today. The next tab is GameRunner. Should I be including the whole tab contents for you to review? I think I should. You can skip over it, browse it, or compile it. It’s up to you.

-- GameRunner
-- RJ 20200831
-- now creates shields

GameRunner = class()

function GameRunner:init(numberOfLives, playerName)
    self.lm = LifeManager(numberOfLives or 3, self.spawn, self.gameOver)
    self.playerName = playerName or "void"
    Shield:createShields()
    self.shields = Shield.shields
    Gunner:newInstance()
    self.army = Army()
    self.sounder = Sound()
    self.line = image(208,1)
    for x = 1,208 do
        self.line:set(x,1,255, 255, 255)
    end
    self:resetTimeToUpdate()
    self.weaponsTime = 0
    self.runningZombie = true
end

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    stroke(255)
    fill(255)
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    textMode(CORNER)
    fontSize(10)
    self:drawScores()
    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

function GameRunner:drawScores()
    if self.runningZombie then return end
    Player1:drawScore()
    if Player1 ~= Player2 then
        Player2:drawScore()
    end
end

function GameRunner:drawScore()
    local x = 8
    if self.playerName == "2" then
        x = 224-60
    end
    local score = "0000" .. tostring(self.army:getScore())
    score = score:sub(-4,-1)
    local output = "SCORE <"..self.playerName..">\n    " .. score
    sx,sy = textSize(output)
    print(sx)
    text(output, x, 252-sy)
end

function GameRunner:scaleAndTranslate()
    local sc,tr = self:scaleAndTranslationValues(WIDTH, HEIGHT)
    scale(sc)
    translate(tr,0)
end

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 224
    local gameScaleY = 256
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(W/gameScaleX, H/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

function GameRunner:spawningZombie()
    self.runningZombie = true
    self.playerName = "Z"
    self.army:zeroScore()
    self:soundPlayer():setVolume(0.05)
end

function GameRunner:spawningNormal()
    self.runningZombie = false
    self:soundPlayer():setVolume(1.0)
end

function GameRunner:update()
    self:updateAlways()
    self:update60ths()
end

function GameRunner:updateAlways()
    self.sounder:update()
end

function GameRunner:update60ths()
    self.time120 = self.time120 + (DeltaTime < 0.016 and 1 or 2)
    if self.time120 < 2 then return end
    self.time120 = 0
    Gunner:instance():update()
    self.army:update()
    if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

function GameRunner:resetTimeToUpdate()
    self.time120 = 0
end

function GameRunner:gunnerDead()
    nextPlayersTurn()
end

function GameRunner:requestSpawn()
    Shield.shields = self.shields
    self.lm:next()(self)
end

function GameRunner:gameOver()
    self:spawn(true)
end

function GameRunner:spawn(isZombie)
    Gunner:instance():spawn(isZombie)
    self:requestWeaponsHold()
end

function GameRunner:playerExploding()
    self.army:weaponsHold()
end

function GameRunner:setWeaponsDelay(seconds)
    self.weaponsTime = ElapsedTime + (seconds or 2)
end

function GameRunner:livesRemaining()
    return self.lm:livesRemaining()
end

function GameRunner:bottomLine()
    return self.line
end
    
function GameRunner:soundPlayer()
    return self.sounder
end

function GameRunner:requestWeaponsHold(seconds)
    self.army:weaponsHold()
    self:setWeaponsDelay(seconds or 2)
end

function GameRunner:forceGameOver()
    self.lm:forceGameOver()
    Gunner:instance():explode()
    self.army:forceMarshalling()
end

I see a lot of tiny methods here, which I like. I think I haven’t displayed a full tab for a while, so you may notice one particularly irritating thing: the methods do not appear to be in any discernible order. Historically, I’ve kept them alphabetical, with exceptions for things that really seem to belong together. And, as I mentioned in an earlier article, I sometimes group things in preparation for pasting them into the article. Again, these will be related methods

However. Finding a given method in a tab that’s not arranged alphabetically is um fraught. And unlike more powerful IDEs, Codea doesn’t even provide an outline tab on the left with things in order.

So as the code gets more mature, I want more and more for it to deliver methods in alphabetical order. I’m almost tempted to see if I can write some code to do that but I’m not that desperate yet. If I do alphabetize, I won’t put you through it, and I apologize for … no, this is warts and all. The code you see is the code I see. If this gives you a greater incentive to organize your code better, I’ve done my job.

Anyway what do we see in here? This is odd:

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 224
    local gameScaleY = 256
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(W/gameScaleX, H/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

This function returns system scale and X translation values. They are used to scale the screen and position the zero location on the X axis. (That’s what makes the game appear centered on a portrait screen.)

So what are rectMode and spriteMode doing in there? They don’t affect the calculation. If they should be anywhere–and they should–it should be somewhere else. And here’s where:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    stroke(255)
    fill(255)
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

I’ll move them above the call to scaleAndTranslate. And commit: move draw modes in GameRunner.

Should I commit such tiny changes? I think yes. I wish I’d learn to do that every time I do anything and reach a point of “that’s done”. It would ratchet the system’s quality and capability forward in tiny steps, ensuring that when I inevitably do need to revert, the reversion is tiny.

As long as we’re here, let’s look at the whole drawing menagerie:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    stroke(255)
    fill(255)
    rectMode(CORNER)
    spriteMode(CORNER)
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    textMode(CORNER)
    fontSize(10)
    self:drawScores()
    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

function GameRunner:drawScores()
    if self.runningZombie then return end
    Player1:drawScore()
    if Player1 ~= Player2 then
        Player2:drawScore()
    end
end

function GameRunner:drawScore()
    local x = 8
    if self.playerName == "2" then
        x = 224-60
    end
    local score = "0000" .. tostring(self.army:getScore())
    score = score:sub(-4,-1)
    local output = "SCORE <"..self.playerName..">\n    " .. score
    sx,sy = textSize(output)
    print(sx)
    text(output, x, 252-sy)
end

The draw function isn’t too awful. But there is that setting up stuff at the beginning. To enable a method to communicate at its best, it helps to have it either only do things, or only call things, not both. Here, we have some doing and then a bunch of calling.

We could do this:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    self:setUpDrawing()
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:setUpDrawing()
    noSmooth()
    stroke(255)
    fill(255)
    rectMode(CORNER)
    spriteMode(CORNER)
end

Now it’s pretty obvious that scale and translate belongs inside setup, isn’t it? But if we put it there, we’ll again have code that does, and code that calls. I think leaving it out communicates a bit better, but it’s a judgment call. And of course we could create another deeper function to be called out of setUpDrawing, but no, I’m not going to do that.

Commit: refactor GameRunner:draw.

Yes, we know we’re going to do more, and we could wait on these commits. But why? The commit is easy and small. What if the code were to commit every time the tests ran. Why would one object to that? Hint: only because one didn’t trust one’s tests. What should one do about that? Do you need a hint?

Moving along, we observe drawStatus:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    textMode(CORNER)
    fontSize(10)
    self:drawScores()
    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

Well, that’s messy, isn’t it?

I notice those two calls to tint. You may be wondering whether those need comments or something to say what color they represent, but here’s why they don’t. This is what I see in Codea:

tint code

As you can see, the color shows up in the IDE. Still, one of those is redundant and should be removed. But there’s more going on. Let’s pull out the lives stuff.

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    textMode(CORNER)
    fontSize(10)
    self:drawScores()
    self:drawLifeInformation()
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawLifeInformation()
    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
end

This works and makes the text and font stuff look really out of place. We’ll move that in:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    self:drawScores()
    self:drawLifeInformation()
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawLifeInformation()
    textMode(CORNER)
    fontSize(10)
    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
end

We could do more extraction on this. Let me push it all the way and then discuss.

No. There’s no good way to extract that doesn’t duplicate the reference to lives. Wait, maybe there is.

First this:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawLifeInformation(lives)
    textMode(CORNER)
    fontSize(10)
    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
end

If the problem is that a number of chunks want to know how many lives are available, we can provide it as a parameter. Now we can freely extract:

function GameRunner:drawLifeInformation(lives)
    textMode(CORNER)
    fontSize(10)
    self:displayLives(lives)
    self:displayIfGameOver(lives)
end

function GameRunner:displayIfGameOver(lives)
    if lives == 0 then
        textMode(CENTER)
        text("GAME OVER", 112, 32)
    end
end

function GameRunner:displayLives(lives)
    text(tostring(lives), 24, 4)
    local addr = 40
    for i = 1,lives-1 do
        sprite(asset.play,40+16*i,4)
    end
end

Now that the code is broken out, I note that addr in displayLives. I might not have noticed it before. Clearly that was intended to be used as the starting offset for sprites. Let’s rename and use:

function GameRunner:displayLives(lives)
    text(tostring(lives), 24, 4)
    local firstSpriteX = 40
    for i = 1,lives-1 do
        sprite(asset.play,firstSpriteX+16*i,4)
    end
end

Better It’s really zeroth sprite x but I think that would be confusing. There may be a better name but that will do for now.

Let’s look at this whole thing and discuss, right after I commit: refactor GameRunner:draw further.

Our New GameRunner Draw

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    self:setUpDrawing()
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:setUpDrawing()
    noSmooth()
    stroke(255)
    fill(255)
    rectMode(CORNER)
    spriteMode(CORNER)
end

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawLifeInformation(lives)
    textMode(CORNER)
    fontSize(10)
    self:displayLives(lives)
    self:displayIfGameOver(lives)
end

function GameRunner:displayIfGameOver(lives)
    if lives == 0 then
        textMode(CENTER)
        text("GAME OVER", 112, 32)
    end
end

function GameRunner:displayLives(lives)
    text(tostring(lives), 24, 4)
    local firstSpriteX = 40
    for i = 1,lives-1 do
        sprite(asset.play,firstSpriteX+16*i,4)
    end
end

There’s more in there, but this is the part we’ve refactored.

The big question is “is this better?” And the answer, as always, is “it depends”.

There are at least two cost factors to consider. First, this code is unquestionably slower than it was before? Why? Because it includes about five method calls that were not there before. How much slower? You know, I don’t know that. Let’s see if we can find out.

-- TimeMethods

function setup()
    count = 10000000
    local t
    local c = Counter()
    t = os.clock()
    c:countEmpty()
    local short = os.clock()-t
    print("short ", short)
    t = os.clock()
    c:countFull()
    local long = os.clock()-t
    print("\nlong ", long)
    local diff = long - short
    local once = diff/count
    print("Diff ", diff, "single ", once)
end

Counter = class()

function Counter:countEmpty()
    for i = 1,count do
    end
end

function Counter:countFull()
    for i = 1,count do
        self:doThing()
    end
end

function Counter:doThing()
end

This code runs an empty loop ten million times and then does ten million method calls. The results are:

short 	0.070112999999992
long 	0.51338099999998
Diff 	0.44326799999999	
single 	4.4326799999999e-08

So ten million calls cost us 0.44 seconds, or, if I’ve done this right, less than 50 nanoseconds per method call. So our five calls every 60th (or 120th) of a second cost us about 250 microseconds. I think the cost argument doesn’t hold water.

The other argument is one of clarity. We’ve spoken of this before, and if you want to call it a matter of taste, I’m OK with that. I am used to tiny method and find them useful, and, as always, I’ve again found that they cost essentially nothing in almost every case. For clarity, I compare this:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

To this, as it was before:

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    textMode(CORNER)
    fontSize(10)
    self:drawScores()
    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

Reading the latter one, I have to read and think at the same time, which causes my gum to get stiff and lose flavor. I think something like:

OK save style, then a green sprite the line OK then some text and the scores why are those mode size things outside then green again, why, then get the lives and display the number hm down low then addr what is that loop over lives showing sprite asset.play that’s the gun thing ok then if lives what was that oh yeah if lives is zero display game over, draw the speed indicator and pop out.

And that’s if I can figure it out. And I never notice that addr never got used.

The version, here it is again,

function GameRunner:drawStatus()
    pushStyle()
    tint(0,255,0)
    sprite(self.line,8,16)
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

We read …. OK a green line, then the scores, then the life into, then the speed indicator.

If we wonder what the life info is, we look at it:

function GameRunner:drawLifeInformation(lives)
    textMode(CORNER)
    fontSize(10)
    self:displayLives(lives)
    self:displayIfGameOver(lives)
end

And so on.

I like that better, because usually I can either stop where I am, or drill down, and when I drill down, I generally wind up right where I want to be, with only one thing going on, something like displayIfGameOver.

It works for me. And also, we could do this:

function GameRunner:drawStatus()
    pushStyle()
    self:drawGreenLineAtBottom()
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawGreenLineAtBottom()
    tint(0,255,0)
    sprite(self.line,8,16)
end

I like that even better. YMMV, and that’s fine. I do recommend trying tiny methods for a while, until you’re used to them, and then decide which way you prefer. But if you don’t want to do that, it’s OK by me.

Commit: tiny refactoring in GameRunner drawing.

I’m asking myself now, self:doYouWantToDoAnyMore(), and the answer is, well, let’s at least look around a bit more in here.

Update 60ths

This is interesting:

function GameRunner:update60ths()
    self.time120 = self.time120 + (DeltaTime < 0.016 and 1 or 2)
    if self.time120 < 2 then return end
    self.time120 = 0
    Gunner:instance():update()
    self.army:update()
    if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

The purpose of this code is to update no faster than once per 60th of a second. This is because some iPads (and probably phones now) run their Codea draw cycle at 1/120th of a second, and others run at 1/60. (It can scale down lower but I’ve been assuming that never happens with our simple game.) So I want to run this update code every other 1/120th, i.e. every 60th, if we happen to come in 120 times a second.

The question is, how do we know? Codea doesn’t tell us. It does provide DeltaTime, which is the time since the last time through. If it’s running at 1/120, then DeltaTime will be about 0.008333, and if it’s running at 1/60, it’ll be about 0.01666.

I emphasize “about”, because this is floating point, this is Codea running at its best guess at 1/60th or 1/120th. So I decided, arbitrarily, that if DeltaTime is less than 0.016, we’re probably running at 1/120, otherwise 1/60.

The code above checks that and only runs the update every other time if we think we’re running at 1/120. Does it say that to you? It doesn’t say that to me.

Expresses all the programmers’ ideas.

Remember that? Let’s try to improve this code’s expression of what’s going on.

The method name itself helps, it says update60ths which is nearly good. Could be better. We’ll see. For now, let’s stick with the inside. What about these statements:

    self.time120 = self.time120 + (DeltaTime < 0.016 and 1 or 2)
    if self.time120 < 2 then return end
    self.time120 = 0

Well self.time120 is counting how may 120ths have gone by since it was set to zero. The first line there adds one or two to it depending on DeltaTime. Is that bit clear? Maybe not. How about this?

function GameRunner:update60ths()
    self.time120 = self.time120 + self:howMany120thsSinceLastUpdate()
    if self.time120 < 2 then return end
    self.time120 = 0
    Gunner:instance():update()
    self.army:update()
    if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

function GameRunner:howMany120thsSinceLastUpdate()
    return DeltaTime < 0.016 and 1 or 2
end

function GameRunner:resetTimeToUpdate()
    self.time120 = 0
end

I included that resetTimeToUpdate because I just noticed it. Someone must call that. Yes, it’s just called from init. But if we have a setter for our member variable, we should use it everywhere we set the variable, and we should probably have a getter as well, because symmetry helps us understand. I probably want to do something about that but first I see a bigger fish.

The primary function here is this:

function GameRunner:update60ths()
    self.time120 = self.time120 + self:howMany120thsSinceLastUpdate()
    if self.time120 < 2 then return end
    self.time120 = 0
    Gunner:instance():update()
    self.army:update()
    if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

It’s kind of doing three things, checking if its time to update, managing the update counter thing, and then doing the update. Let’s have it do one thing: update if necessary.

We could do that by extracting the updating or by extracting the checking and managing. I choose door number two.

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self.army:update()
        if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
            self.army:weaponsFree()
            self.weaponsTime = 0
        end
    end
end

function GameRunner:a60thHasElapsed()
    self.time120 = self.time120 + self:howMany120thsSinceLastUpdate()
    if self.time120 < 2 then
        return false
    else
        self.time120 = 0
        return true
    end
end

Now the upper function is more expressive of what it does, and the lower one is nearly good. I think the if should be reversed for improved clarity:

function GameRunner:a60thHasElapsed()
    self.time120 = self.time120 + self:howMany120thsSinceLastUpdate()
    if self.time120 >= 2 then
        self.time120 = 0
        return true
    else
        return false
    end
end

What about those self.time120 though? Should I at least use the setter to reset it? Probably, but I’m not sure. I think I’ll leave it for now.

The a60thHasElapsed function is still weird, but it is now isolated and given a reasonable name. At this moment I don’t see how to make it less weird.

Another thing that I see and don’t exactly like is this:

        if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then

This code is checking to see if it is time to release the weapons. We have a short delay after spawning a gunner before the army starts bombing. The code is written this way because weaponsTime isn’t guaranteed to hold a useful value. If it’s zero that basically means no hold is in effect.

This is surely overkill but let’s see if we can improve the clarity here.

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self.army:update()
        if self:timeToReleaseWeapons() then
            self.army:weaponsFree()
            self.weaponsTime = 0
        end
    end
end

function GameRunner:timeToReleaseWeapons()
    return self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime
end

So now the upper code says what it means. And the overkill:

function GameRunner:timeToReleaseWeapons()
    return self:weaponsHoldInEffect() and ElapsedTime >= self.weaponsTime
end

function GameRunner: weaponsHoldInEffect()
    return self.weaponsTime > 0
end

Now I’m wondering whether someone else is looking at weaponsTime directly who should call our hold in effect method. No one is. Good. So now we have this:

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self.army:update()
        if self:timeToReleaseWeapons() then
            self.army:weaponsFree()
            self.weaponsTime = 0
        end
    end
end

function GameRunner:timeToReleaseWeapons()
    return self:weaponsHoldInEffect() and ElapsedTime >= self.weaponsTime
end

function GameRunner: weaponsHoldInEffect()
    return self.weaponsTime > 0
end

And one more …

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self.army:update()
        self:manageWeaponsRelease()
    end
end

function GameRunner:manageWeaponsRelease()
    if self:timeToReleaseWeapons() then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

Arguably, we should release the weapons before we update the army. That gives them an additional 60th of a second to kill us, but it seems more logical to manage weapons before updating. I think I’ll reverse those.

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self:manageWeaponsRelease()
        self.army:update()
    end
end

OK, one more commit: refactor GameRunner:update60ths.

I’ll include the final file at the end, along with the link to the zip file for those who use it. Let’s first sum up.

Summing Up

We’ve refactored GameRunner in the direction I like, lots of very small methods. One result of that is that it now contains lots of very small methods, and it’s hard to find the one you’re looking for if you go in at random. The thing you’re looking for if you’re not going in at random is near where you are presently looking: things called tend to be closely below the caller.

This object now has 33 methods, if I’ve counted correctly, and that’s a lot. Now this is a big deal object, since it runs the whole game, but I do wonder whether this is telling us that its functionality needs to be spread over a couple more objects. Maybe we’ll look at that at some point.

What we’ve seen here today is a push to remove duplication, and mostly to express all our ideas, pushed pretty close to the reasonable limit, if not past it. I prefer to push past the limit on expressing ideas, since falling short leads to confusion. Here, our confusion should be limited to two kids, finding the method named X, which even Codea can do quickly, and understanding the details of a very small method, which is easy in all but a few cases.

So I am not displeased with what has happened here today, and I would not reverse it. What about you? Tweet me up if you want to discuss it.

Otherwise, see you next time! Thanks for tuning in!

Final Code for the Day

-- GameRunner
-- RJ 20200831
-- now creates shields

GameRunner = class()

function GameRunner:init(numberOfLives, playerName)
    self.lm = LifeManager(numberOfLives or 3, self.spawn, self.gameOver)
    self.playerName = playerName or "void"
    Shield:createShields()
    self.shields = Shield.shields
    Gunner:newInstance()
    self.army = Army()
    self.sounder = Sound()
    self.line = image(208,1)
    for x = 1,208 do
        self.line:set(x,1,255, 255, 255)
    end
    self:resetTimeToUpdate()
    self.weaponsTime = 0
    self.runningZombie = true
end

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    self:setUpDrawing()
    self:scaleAndTranslate()
    self.army:draw()
    Gunner:instance():draw()
    Shield:drawShields()
    self:drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:setUpDrawing()
    noSmooth()
    stroke(255)
    fill(255)
    rectMode(CORNER)
    spriteMode(CORNER)
end

function GameRunner:drawStatus()
    pushStyle()
    self:drawGreenLineAtBottom()
    self:drawScores()
    self:drawLifeInformation(self.lm:livesRemaining())
    drawSpeedIndicator()
    popStyle()
end

function GameRunner:drawGreenLineAtBottom()
    tint(0,255,0)
    sprite(self.line,8,16)
end

function GameRunner:drawLifeInformation(lives)
    textMode(CORNER)
    fontSize(10)
    self:displayLives(lives)
    self:displayIfGameOver(lives)
end

function GameRunner:displayIfGameOver(lives)
    if lives == 0 then
        textMode(CENTER)
        text("GAME OVER", 112, 32)
    end
end

function GameRunner:displayLives(lives)
    text(tostring(lives), 24, 4)
    local firstSpriteX = 40
    for i = 1,lives-1 do
        sprite(asset.play,firstSpriteX+16*i,4)
    end
end

function GameRunner:drawScores()
    if self.runningZombie then return end
    Player1:drawScore()
    if Player1 ~= Player2 then
        Player2:drawScore()
    end
end

function GameRunner:drawScore()
    local x = 8
    if self.playerName == "2" then
        x = 224-60
    end
    local score = "0000" .. tostring(self.army:getScore())
    score = score:sub(-4,-1)
    local output = "SCORE <"..self.playerName..">\n    " .. score
    sx,sy = textSize(output)
    print(sx)
    text(output, x, 252-sy)
end

function GameRunner:scaleAndTranslate()
    local sc,tr = self:scaleAndTranslationValues(WIDTH, HEIGHT)
    scale(sc)
    translate(tr,0)
end

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 224
    local gameScaleY = 256
    local sc = math.min(W/gameScaleX, H/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

function GameRunner:spawningZombie()
    self.runningZombie = true
    self.playerName = "Z"
    self.army:zeroScore()
    self:soundPlayer():setVolume(0.05)
end

function GameRunner:spawningNormal()
    self.runningZombie = false
    self:soundPlayer():setVolume(1.0)
end

function GameRunner:update()
    self:updateAlways()
    self:update60ths()
end

function GameRunner:updateAlways()
    self.sounder:update()
end

function GameRunner:update60ths()
    if self:a60thHasElapsed() then
        Gunner:instance():update()
        self:manageWeaponsRelease()
        self.army:update()
    end
end

function GameRunner:manageWeaponsRelease()
    if self:timeToReleaseWeapons() then
        self.army:weaponsFree()
        self.weaponsTime = 0
    end
end

function GameRunner:timeToReleaseWeapons()
    return self:weaponsHoldInEffect() and ElapsedTime >= self.weaponsTime
end

function GameRunner: weaponsHoldInEffect()
    return self.weaponsTime > 0
end

function GameRunner:a60thHasElapsed()
    self.time120 = self.time120 + self:howMany120thsSinceLastUpdate()
    if self.time120 >= 2 then
        self.time120 = 0
        return true
    else
        return false
    end
end

function GameRunner:howMany120thsSinceLastUpdate()
    return DeltaTime < 0.016 and 1 or 2
end

function GameRunner:resetTimeToUpdate()
    self.time120 = 0
end

function GameRunner:gunnerDead()
    nextPlayersTurn()
end

function GameRunner:requestSpawn()
    Shield.shields = self.shields
    self.lm:next()(self)
end

function GameRunner:gameOver()
    self:spawn(true)
end

function GameRunner:spawn(isZombie)
    Gunner:instance():spawn(isZombie)
    self:requestWeaponsHold()
end

function GameRunner:playerExploding()
    self.army:weaponsHold()
end

function GameRunner:setWeaponsDelay(seconds)
    self.weaponsTime = ElapsedTime + (seconds or 2)
end

function GameRunner:livesRemaining()
    return self.lm:livesRemaining()
end

function GameRunner:bottomLine()
    return self.line
end
    
function GameRunner:soundPlayer()
    return self.sounder
end

function GameRunner:requestWeaponsHold(seconds)
    self.army:weaponsHold()
    self:setWeaponsDelay(seconds or 2)
end

function GameRunner:forceGameOver()
    self.lm:forceGameOver()
    Gunner:instance():explode()
    self.army:forceMarshalling()
end

Invaders.zip