Improving our Singleton and using the idea elsewhere.

My motivation for doing the Singleton Pattern getInstance thing with Player was to remove objects’ dependencies on other objects, to avoid having to pass objects far down the code chain, and to improve reliability by ensuring that everyone who accesses a “distinguished object” accesses the correct one.

Having thought idly about it since yesterday noon, today I want to reflect on what we have, see how to improve it, and see where the idea might be applied again.

You can look up the Singleton pattern in the Gang of Four book, or just search for “singleton pattern”, and find useful info. The basic idea is that when a class needs to have a “distinguished” instance available over a wide range of objects, you can use this pattern to ensure that there is only one instance, and everyone gets that one.

You could just store the instance in a global. In fact that’s what I’ve been doing with Space Invaders. You could also keep the special instance in some private place and pass it around to everyone who needs it. I’ve done that, too, for example initializing one class by passing it the distinguished instance of another. An example is that Invaders know their Army:

function Army:marshalTroops()
    self.invaders = MarshallingCenter():marshalArmy(self)
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

Above, we see that the Army instance passes itself to the marshalArmy method, which then passes it to every new invader. Invaders can then reference the army when they need to:

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

Above, we see the killed invader reporting its score to army.

I’ve not been entirely consistent with this practice. For example, here’s Invader:update:

function Invader:update(army)
    self.pos = self.pos + army:invaderMotion() -- required for yPosition
    if not self.alive then return true end
    army:reportingForDuty()
    self.picture = (self.picture+1)%2
    Shield:checkForShieldDamage(self)
    if self:overEdge() then
        army:invaderOverEdge(self)
    end
    return false
end

Here, I’ve passed in the Army from the call to update. I’m not sure where that is, but I imagine it’s in Army. Ah, here it is:

function Army:updateOneLiveInvader()
    local continue = true
    while(continue) do
        continue = self:nextInvader():update(self)
    end
end

This is arguably better practice: provide the army only to those who need it. But it’s certainly inconsistent, since other invader methods are taking advantage of the pointer to army that they have already been given.

There may even be a hidden reason why it’s this way, and if there is, it would be bad. Perhaps we are passing in this variable because sometimes the army isn’t what we think it is. This could be needed by a test, which perhaps wants to field the calls back to army and check them. Or it could be something even more weird.

I’m inclined to find out. I’m going to change that method to use self.army everywhere and see if anything breaks.

function Invader:update()
    self.pos = self.pos + self.army:invaderMotion() -- required for yPosition
    if not self.alive then return true end
    self.army:reportingForDuty()
    self.picture = (self.picture+1)%2
    Shield:checkForShieldDamage(self)
    if self:overEdge() then
        self.army:invaderOverEdge(self)
    end
    return false
end

Everything works fine. I’ll remove the parameter from the call as well:

function Army:updateOneLiveInvader()
    local continue = true
    while(continue) do
        continue = self:nextInvader():update()
    end
end

Commit: invader update uses saved army variable.

It’s generally thought to be better to pass in a variable like army, that is called back with information like we do here, such as invaderOverEdge and reportingForDuty. The reason is that a cached value can be out of date.

As I write this I think “then why did you just make it worse”, and that’s a good question. I have made it more consistent, but let’s see who else says self.army and whether we could always pss it in:

Hm. Other than the ones I just changed, the only other use is this:

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

Who sends that message? Nice! They’re all right here in Army:

function Army:checkForKill(missile)
    for i, invader in ipairs(self.invaders) do
        if invader:killedBy(missile) then
            missile.v = 0
            return
        end
    end
    if self.rollingBomb:killedBy(missile) then
        missile.v = 0
        self.rollingBomb:explode(self)
    elseif self.plungerBomb:killedBy(missile) then
        missile.v = 0
        self.plungerBomb:explode(self)
    elseif self.squiggleBomb:killedBy(missile) then
        missile.v = 0
        self.squiggleBomb:explode(self)
    elseif self.saucer:killedBy(missile) then
        missile.v = 0
    end
end

If we pass self to all those, we can eliminate the caching of army in Invader and, very likely, in the bombs and saucer as well. Let’s dig further.

Bombs don’t know the army, and when they need to refer to it, it’s passed in. Ah, that’s not quite true. This is a bit nasty:

function Bomb:explode(army)
    self.pos = self.pos - vec2(2,3)
    self.army = army
    self.explodeCount = 15
end

When a bomb explodes, it caches the army variable. I wonder why. Ah:

function Bomb:draw()
    pushStyle()
    if self.explodeCount > 0 then
        sprite(self.explosion, self.pos.x, self.pos.y)
        self.explodeCount = self.explodeCount - 1
        if self.explodeCount == 0 then self.army:deleteBomb(self) end
    else
        sprite(self.shapes[self.shape + 1],self.pos.x,self.pos.y)
    end
    popStyle()
end

We call back to Army:deleteBomb when the bomb has finished its show. We could pass the army in to draw, which would be odd but eliminate the caching.

I say it would be odd. We could certainly go for a standard, which would be that when you call draw on an object you always pass in the object that’s doing the call, which will generally be that object’s owner or parent. Then it wouldn’t be so odd. We’d do the same on update, of course.

We begin to see that it gets tricky and somewhat entangled when you try to decide whether to give a distinguished object to another object on creation, or to pass it around only as needed. The former can lead to trouble if the object changes, the latter increases the complexity of many of your interfaces.

The Singleton Pattern has its own drawbacks, but it generally leaves the code less complex than when we pass in the object, and no more complex than if we cache it.

My inclination will be to push the Singleton idea throughout, wherever it reasonably applies, just to see what happens. The best way I know to learn when an idea is bad is to try it, at least when broken bones are not likely.

Before we push it further, I want to modify what we have now, so let’s do that.

Improving Our Singleton

Here’s our approach now:

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
    Player.instance = self
end

function Player:getInstance()
    return Player.instance
end

There are things not to like about this.

First in my mind is the name getInstance(). I don’t like names that start with “get”. I want the name of the method to be instance(). That means that the variable that stores the singleton can’t be named instance. We can call it anything else, like singleton or the_one.

Second, as written, if anyone creates a new player, it automatically becomes the official instance. I think that if we don’t do that, we’ll have tests failing, because tests that involve multiple objects need the inter-object connections.

I think a better way would be for calls to Player() to produce a free instance, and for calls to getInstance (or, later, instance) to lazy-init the singleton.

Let’s try that and see what breaks.

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
    --Player.instance = self
end

function Player:getInstance()
    if not Player.instance then
        Player.instance = Player()
    end
    return Player.instance
end

Now it happens that GameRunner creates a Player:

function GameRunner:init()
    self.lm = LifeManager(3, self.spawn, self.gameOver)
    Shield:createShields()
    Player()
    TheArmy = Army()
    self:resetTimeToUpdate()
    self.weaponsTime = 0
end

If the lazy init works, this shouldn’t be needed and in fact has no effect.

We do have a concern, though, which is that if the tests ever called Player:getInstance, they’d create the game version prematurely. That could be bad. For now, they don’t. So I’ll comment out the Player() call above and we’ll see what breaks. This is just an experiment to see what breaks.

Thirteen tests fail. Fascinating. Why? Here’s one:

19: rolling shot finds correct column  -- Actual: 3.0, Expected: 1

The rolling shot is the targeted one. The test has a few failures, and it looks like this:

        _:test("rolling shot finds correct column", function()
            local Gunner = Player()
            local army = Army(Gunner)
            local bomb = army.rollingBomb
            army:setPosition(vec2(10,180))
            Gunner.pos = vec2(10,32)
            local col = bomb:nextColumn(army)
            _:expect(col).is(1)
            Gunner.pos = vec2(26,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(2)
            Gunner.pos = vec2(30,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(2)
            Gunner.pos = vec2(91,32)
            col = bomb:nextColumn(army)
            _:expect(army:xPosition()).is(10)
            _:expect(col).is(6)
            Gunner.pos = vec2(0,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(1)
            Gunner.pos = vec2(208,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(11)
        end)

What will be wrong is that with the lazy init, we’ll get a new player, not the one we’re testing with, and the targeting code uses the new lazy-initialized one:

function Army:targetedColumn()
    local gunnerX = Player:getInstance().pos.x
    local armyX = self:xPosition()
    local relativeX = gunnerX - armyX
    local result = 1 + relativeX//16
    return math.max(math.min(result,11),1)
end

This is really intricate, isn’t it? Partly, that’s because we’re adapting a new structure, our Singleton, into an existing code base. Part is due to the tests, some of which integrate a few objects (including Player) and therefore need the system to be “hooked up”, including references to the globals and singletons.

What we’d like if for our tests to be completely decoupled from our “real” program We’d like them to create all new objects, exercise them as needed, and to leave the system in its pristine state right before setup, as if nothing had ever happened.

What we’d have to do with Player is typical of what we’ll need to do elsewhere, I expect.

When a test needs a Player, we see now that it needs the one that is saved in the Singleton, so that the code is integrated correctly. But if we do that, then we need the test to decouple everything, because it may have left Player in an odd state.

One way to do that would be with the CodeaUnit after capability, which runs after each test and allows you to unwind things. In particular, in the after processing, we could clear the singleton, allowing the real game, when it runs, to create the one it should use.

We don’t have a way to clear the instance.

This is still an experiment. Let’s do a clearInstance method, and change our tests to use Player:getInstance.

function Player:clearInstance()
    Player.instance = nil
end

        _:after(function()
            Player:clearInstance()
        end)

        _:test("rolling shot finds correct column", function()
            local Gunner = Player:getInstance()
            local army = Army(Gunner)
            local bomb = army.rollingBomb
            army:setPosition(vec2(10,180))
            ...

I don’t have a great deal of confidence in this but it’s just an experiment. However the rolling fails have gone away. This might be good. Next error:

32: Saucer Score depends on shots fired  -- Actual: 100, Expected: 50

        _:test("Saucer Score depends on shots fired", function()
            local army = Army()
            local Gunner = Player()
            local s = army.saucer
            _:expect(s:score()).is(100)
            Gunner:unconditionallyFireMissile(true)
            ...

Well, let’s do the same thing:

        _:test("Saucer Score depends on shots fired", function()
            local army = Army()
            local Gunner = Player:getInstance()
            local s = army.saucer
            _:expect(s:score()).is(100)

And all the tests pass. I think this is a good thing. Does the game still work? Yes it seems to work just fine. Ideally, of course, this all made sense, but there could be connections we weren’t aware of.

I’m going with this. One question is whether the GameRunner init should simply not mention Player, or whether it should have the honor of creating the instance. For a belt-suspenders kind of thing, we could even make it clear and then create. Or we could have yet another method newInstance that force-initializes.

These considerations are always there when we use distinguished objects. But for objects that maintain state, we quite often do need them. If there’s a “correct” answer, I don’t know it. In a given program, we pick a way and go with it. It’s probably best to pick one way, not the two or three we have right now.

What we have right now right now is a surprise: a lazy init of a distinguished object. I think that’s not good. Let’s set things up for our distinguished objects this way:

  • Access to the object is ClassName:instance().
  • A call to ClassName() returns a free instance.
  • A call to ClassName:newInstance() creates and saves the distinguished instance.
  • A call to ClassName:clearInstance() removes the distinguished instance and does not replace it.

That’s a bit intricate but it seems to cover the bases. Let’s do that for Player and then test and commit.

Player = class()    

-- class (singleton) methods

function Player:instance()
    return Player.singleton
end

function Player:clearInstance()
    Player.singleton = nil
end

function Player:newInstance()
    Player.singleton = Player()
end

Now I need to replace all the getInstance with instance. There were quite a few, perhaps as many as 20. I could, of course, have just added a deprecated getInstance, in a larger system, but even with Codea’s minimal editing it was easy in a program this small. Let’s test and see if we’re good.

Not quite. Four failures. Ah. Tests need to say newInstance now that we’ve removed the lazy init.

It would also help if newInstance actually worked:

function Player:newInstance()
    Player.singleton = Player()
    return Player.singleton
end

It wasn’t returning the value. This does make me wonder a bit about whether this is the ideal easy to use protocol.

Game doesn’t work because GameRunner was no longer creating the new instance. Again this makes me wonder.

Be that as it may, tests are green and the game works. Commit: new implementation of Player singleton.

It’s Saturday, and my dear wife is making breakfast. We’ll call this done and sum up.

Summing Up

Our game needs at least one distinguished object, the GameRunner, and could benefit from others, including the SoundPlayer, the Army, the Player, and perhaps others.

Some of those have been stored in globals, including Sound, Gunner, and TheArmy. No one even remembers TheArmy. We’d do well to avoid global names where we can, because they fill up our personal memory with information we rarely need.

Class names in Lua are always global. There is no good way around that. (I can think of a bad way.) If we use a singleton pattern, we can have only the class name and fetch our distinguished object using that.

I’m not certain that this will be a better approach, and probably at least sometimes we’ll wind up using another, perhaps passing in the necessary object. That would be good if we could avoid using any global definition at all, whether in a global variable or singleton. We might find a place where we prefer it anyway, but I have my doubts.

I’m not fully convinced about the protocol. The lazy init seems cleaner in some ways but it’s always rather mysterious. For now, we’ll give this a go and see whether it works well on the next object.

That will be for later.

Bonus Track

I’ve not filed this report yet, but have done a little study and it turns out that if you declare a variable local, but outside any other scope, it is visible in the file or chunk that it occurs in, but not elsewhere. That allows us to make the singleton instance private to the file, like this:

Player = class()    

-- class (singleton) methods

local singleton

function Player:instance()
    return singleton
end

function Player:clearInstance()
    singleton = nil
end

function Player:newInstance()
    singleton = Player()
    return singleton
end

That’s cleaner and works just fine.

I remain ambivalent about the lazy init versus explicit init. But why not both? If we allow the lazy init, then the simplest usage is just that everyone calls instance, no huhu. I like that. And it’s my program, so there. How about this:

Player = class()    

-- class (singleton) methods

local singleton

function Player:instance()
    if not singleton then singleton = Player() end
    return singleton
end

function Player:clearInstance()
    singleton = nil
end

function Player:newInstance()
    self:clearInstance()
    return self:instance()
end

See what I did there at the end? I removed duplicate lines doing singleton=Player(). Why? First, a good habit. Second, it seemed less error prone the second way.

Anyway, everything works fine and I think this is just a touch better.

See you next time!

Invaders.zip