We start asteroids on the edge, which gives us an edge on refactoring.

What shall we do today? Here are some items deserving attention:

  • Finite number of ships per game
  • Free ships at 10,000 point crossovers
  • Large saucer, random fire
  • Small saucer, accurate fire
  • Better sounds - still not done
  • Asteroids start at edge
  • Asteroid fragments tend to move faster
  • Ship needs non-zero cross-section
  • Hyperspace escape

In addition, there are plenty of opportunities to clean up the code. If I’m not mistaken, there are even some tabs that are half procedural / half object-oriented.

Of the functional items, starting Asteroids at the edge seems easy, especially since I already did it once, and the large saucer might be kind of fun to do.

Let’s do the edge thing to warm up and then see about the large saucer.

Ah. Reviewing Asteroids 23, I see that starting on the edges wasn’t all that easy. Issues included:

  • Split asteroids don’t start on the edges. I suppose that could be dealt with based on asteroid starting size?
  • It seemed that often you’d have two of the four asteroids coming in pretty close together. I can imagine that we could create them one per edge, around and around.

Let’s just go ahead and do it in the simplest way we can think of that might possibly work and see what we get.

Here’s asteroid creation now:

function Asteroid:init()
    self.pos = vec2(math.random(WIDTH), math.random(HEIGHT))
    self.shape = Rocks[math.random(1,4)]
    self.scale = 16
    local angle = math.random()*2*math.pi
    self.step = vec2(Vel,0):rotate(angle)
end

And here’s split:

function splitAsteroid(asteroid, asteroids)
    if asteroid.scale == 16 then
        U:playStereo(U.sounds.bangLarge, asteroid)
    elseif asteroid.scale == 8 then
        U:playStereo(U.sounds.bangMedium, asteroid, asteroid)
    elseif asteroid.scale == 4 then
        U:playStereo(U.sounds.bangSmall, asteroid)
        Splat(asteroid.pos)
        DeadAsteroids[asteroid] = asteroid
        return
    end
    asteroid.scale = asteroid.scale//2
    asteroid.angle = math.random()*2*math.pi
    local new = Asteroid()
    new.pos = asteroid.pos
    new.scale = asteroid.scale
    asteroids[new] = new
    Splat(asteroid.pos)
end

That’s weird for a number of reasons, not least that it’s a public function rather than a method on Asteroid. This is one of those half-done situations I mentioned. Neat, we can talk about how to handle that sort of thing.

Where does this get called from, just from curiosity?

function Universe:checkShipCollision(asteroid)
    if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
        scoreAsteroid(asteroid)
        splitAsteroid(asteroid, self.asteroids)
        self:killShip()
    end
end

function Universe:checkMissileCollisions(asteroid)
    for k,m in pairs(self.missiles) do
        if m.pos:dist(asteroid.pos) < asteroid:killDist() then
            scoreAsteroid(asteroid)
            splitAsteroid(asteroid, self.asteroids)
            m:die()
        end
    end
end

OK, we split asteroids when we shoot them and when they crash into us. Neither of those users cares much what happens, the just ask to have it done.

I note in passing that scoreAsteroid, like splitAsteroid is a public function in what’s trying to grow up to be a class. There’s a lesson to draw here.

Small Refactorings

It’s my view that there are no “large refactorings”, there are only lots of small ones. Back when we started making Asteroid into a class, we stopped right in the middle of the change … with everything working. We didn’t do the whole “big rafactoring”, we just did what we considered enough.

When we do that, we leave code like we see here, with some procedural functions and some object ones. I do not recommend just going in to clean those up, unless you have a lot of time on your hands. If you’re under pressure to release features, and have no feature-focused reason to touch that partially converted code, just leave it alone: it’s not bothering anyone.

But one day a feature will come along that brings us back to that code. Today is that day for Asteroid. Today is the day we refactor a bit. We do it because we now have evidence that this not so perfect code does get visited, and its imperfection does slow us down, because we have to look more carefully than usual to understand it. So now is the time to improve it.

First thing to improve is this:

function splitAsteroid(asteroid, asteroids)
    if asteroid.scale == 16 then
        U:playStereo(U.sounds.bangLarge, asteroid)
    elseif asteroid.scale == 8 then
        U:playStereo(U.sounds.bangMedium, asteroid, asteroid)
    elseif asteroid.scale == 4 then
        U:playStereo(U.sounds.bangSmall, asteroid)
        Splat(asteroid.pos)
        DeadAsteroids[asteroid] = asteroid
        return
    end
    asteroid.scale = asteroid.scale//2
    asteroid.angle = math.random()*2*math.pi
    local new = Asteroid()
    new.pos = asteroid.pos -- <-- SLAM
    new.scale = asteroid.scale -- <-- SLAM
    asteroids[new] = new
    Splat(asteroid.pos)
end

Remember that we’re here to start large asteroids on the edge. But when an asteroid is created, its position is random, its scale is set to 16, and then the code above slams a new position and a smaller size into it! That’s not really good design: all the key values of an asteroid should be provided to it when it is created. (We can default some of those parameters if we want to be clever, which is of course a slippery slope.)

Let’s proceed by first making splitAsteroid into a method, and changing the callers. First the method:

function Asteroid:split(asteroids)
    if self.scale == 16 then
        U:playStereo(U.sounds.bangLarge, self)
    elseif self.scale == 8 then
        U:playStereo(U.sounds.bangMedium, self)
    elseif self.scale == 4 then
        U:playStereo(U.sounds.bangSmall, self)
        Splat(self.pos)
        DeadAsteroids[self] = self
        return
    end
    self.scale = self.scale//2
    self.angle = math.random()*2*math.pi
    local new = Asteroid()
    new.pos = self.pos
    new.scale = self.scale
    asteroids[new] = new
    Splat(self.pos)
end

We rename it split since we know we’re an asteroid. I’ve changed the calls as well:

function Universe:checkShipCollision(asteroid)
    if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
        scoreAsteroid(asteroid)
        asteroid:split(self.asteroids)
        self:killShip()
    end
end

function Universe:checkMissileCollisions(asteroid)
    for k,m in pairs(self.missiles) do
        if m.pos:dist(asteroid.pos) < asteroid:killDist() then
            scoreAsteroid(asteroid)
            asteroid:split(self.asteroids)
            m:die()
        end
    end
end

Everything works as intended. Because I am now in the area, and need to avoid confusion next time, I’ll convert scoreAsteroid similarly. Arguably, this isn’t necessary, but it is right to hand, and will take but moments.

function Universe:checkShipCollision(asteroid)
    if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
        asteroid:score()
        asteroid:split(self.asteroids)
        self:killShip()
    end
end

function Universe:checkMissileCollisions(asteroid)
    for k,m in pairs(self.missiles) do
        if m.pos:dist(asteroid.pos) < asteroid:killDist() then
            asteroid:score()
            asteroid:split(self.asteroids)
            m:die()
        end
    end
end

And the receiver:

function Asteroid:score()
    local s = self.scale
    local inc = 0
    if s == 16 then inc = 20
    elseif s == 8 then inc = 50
    else inc = 100
    end
    U.score = U.score + inc
end

Scoring still works. Are we done yet? No. Look at this again:

function Asteroid:split(asteroids)
    if self.scale == 16 then
        U:playStereo(U.sounds.bangLarge, self)
    elseif self.scale == 8 then
        U:playStereo(U.sounds.bangMedium, self)
    elseif self.scale == 4 then
        U:playStereo(U.sounds.bangSmall, self)
        Splat(self.pos)
        DeadAsteroids[self] = self
        return
    end
    self.scale = self.scale//2
    self.angle = math.random()*2*math.pi
    local new = Asteroid()
    new.pos = self.pos
    new.scale = self.scale
    asteroids[new] = new
    Splat(self.pos)
end

This code is weird. The if nest at the top plays sounds and might kill the asteroid if it is small enough. (We add to DeadAsteroids to kill it because we’re concerned about timing issues. That came up in article 15, I believe.)

Then we rescale the current asteroid and create a new one, jamming values into our size and angle, and into the new guy.

This is weird. First let’s pull out the sound:

function Asteroid:bang()
    if self.scale == 16 then
        U:playStereo(U.sounds.bangLarge, self)
    elseif self.scale == 8 then
        U:playStereo(U.sounds.bangMedium, self)
    elseif self.scale == 4 then
        U:playStereo(U.sounds.bangSmall, self)
    end
end

function Asteroid:split(asteroids)
    self:bang()
    if self.scale == 4 then
        Splat(self.pos)
        DeadAsteroids[self] = self
        return
    end
    self.scale = self.scale//2
    self.angle = math.random()*2*math.pi
    local new = Asteroid()
    new.pos = self.pos
    new.scale = self.scale
    asteroids[new] = new
    Splat(self.pos)
end

Now I ask myself, I ask me, why are we converting our existing asteroid into a smaller one? Why not just create two new ones, letting the old one die and go to a better place?

So let’s add parameters, position and scale to the init.

function Asteroid:init(pos, size)
    self.pos = pos or vec2(math.random(WIDTH), math.random(HEIGHT))
    self.scale = size or 16
    self.shape = Rocks[math.random(1,4)]
    local angle = math.random()*2*math.pi
    self.step = vec2(Vel,0):rotate(angle)
end

This code ensures that if we call without the pos and size, we get the same random pos and size we always get. Now let’s create two new asteroids in split, and make the old one die. We’ll do this naively, even though we see some commonality. Small simple steps, that’s my motto.1

Now in my view we should be able to write this:

function Asteroid:split(asteroids)
    self:bang()
    if self.scale == 4 then
        Splat(self.pos)
        DeadAsteroids[self] = self
        return
    end
    Asteroid(self.pos, self.scale//2)
    Asteroid(self.pos, self.scale//2)
    DeadAsteroids[self] = self
    Splat(self.pos)
end

We can’t, because asteroids don’t put themselves into U.asteroids. That was done here, and is done in newWave:

function Universe:newWave()
    self.beatDelay = 1 -- second
    self.timeOfNextWave = 0
    for i = 1, self:newWaveSize() do
        local a = Asteroid()
        self.asteroids[a] = a
    end
end

Let’s just fix that right now. Asteroids register with the Universe:

function Asteroid:init(pos, size)
    self.pos = pos or vec2(math.random(WIDTH), math.random(HEIGHT))
    self.scale = size or 16
    self.shape = Rocks[math.random(1,4)]
    local angle = math.random()*2*math.pi
    self.step = vec2(Vel,0):rotate(angle)
    U.asteroids[self] = self
end

OW! Just discovered a tiny problem. When we create the universe, it immediately does a newWave. The calling statement looks like this:

U = Universe()

So U isn’t set until Universe returns itself, so our reference to U in Asteroids isn’t valid.

This is another of those issues you run into with singletons and god objects. For now, I’m going to change the universe not to call newWave and to call it from setup. But we do need to think about this sooner. I argue, though, that a created object needs to be able to rely on the Universe being present, because we store all kinds of universal constants and collections in there.

It’s still an issue, however.

OK, with that change, all our asteroids are working correctly. Back to the plan, improving this method:

function Asteroid:split(asteroids)
    self:bang()
    if self.scale == 4 then
        Splat(self.pos)
        DeadAsteroids[self] = self
        return
    end
    Asteroid(self.pos, self.scale//2)
    Asteroid(self.pos, self.scale//2)
    DeadAsteroids[self] = self
    Splat(self.pos)
end

Now we always do a Splat, we always report ourselves dead, and we only do the new asteroids if scale isn’t 4. Let’s say that:

function Asteroid:split()
    self:bang()
    DeadAsteroids[self] = self
    Splat(self.pos)
    if self.scale ~= 4 then
        Asteroid(self.pos, self.scale//2)
        Asteroid(self.pos, self.scale//2)
    end
end

OK, we have nicely prepared the soil for our edge asteroids, because we’ve allowed for provision of the starting position in Asteroid:init. Let’s see what newWave tells us. But let’s commit first: “prepare soil for edge asteroids”.

function Universe:newWave()
    self.beatDelay = 1 -- second
    self.timeOfNextWave = 0
    for i = 1, self:newWaveSize() do
        Asteroid()
    end
end

Now we’d like to provide an edge location for each asteroid in our loop. That means that, somehow, we need to set one of the coordinates to zero or WIDTH-1 or HEIGHT-1, and leave the other random. I think if we just forced to zero, we’d be OK, because half the time it should go the other direction and be right at the high end anyway. So let’s try this:


function Universe:newWave()
    self.beatDelay = 1 -- second
    self.timeOfNextWave = 0
    for i = 1, self:newWaveSize() do
        local randPos = vec2(math.random(WIDTH), math.random(HEIGHT))
        local zeros = vec2(math.random(0,1), math.random(0,1))
        local pos = vec2(randPos.x*zeros.x, randPos.y,zeros.y)
        Asteroid(pos)
    end
end

That will start some of them in the corner but that seems OK to me. I’ll run it and see what happens.

Blah, what a dummy. This can also roll a (1,1) which won’t start on an edge. See what happens when I try to be too clever?

function Universe:newWave()
    local pos
    self.beatDelay = 1 -- second
    self.timeOfNextWave = 0
    for i = 1, self:newWaveSize() do
        if math.random(0,1) then
            pos = vec2(0,math.random(HEIGHT))
        else
            pos = vec2(math.random(WIDTH), 0)
        end
        Asteroid(pos)
    end
end

This does the job. Starting asteroids all start on the edge, and that’s what we asked for. They do kind of clump once in a while, so we need to think about that, but I think that’s another feature. Commit: “wave asteroids start on edge”.

Let’s take a quick glance at Asteroid and see if it needs any quick improvement. We’ve already left it better than we found it but maybe there are a couple of scraps we can pick up.

The only real oddity that I see is this public function:

function killDeadAsteroids(asteroids)
    for k,a in pairs(DeadAsteroids) do
        asteroids[a] = nil
    end
    DeadAsteroids = {}
end

We use that only in Universe:

function Universe:drawAsteroids()
    pushStyle()
    stroke(255)
    fill(0,0,0, 0)
    strokeWidth(2)
    rectMode(CENTER)
    for i,asteroid in pairs(self.asteroids) do
        asteroid:draw()
        asteroid:move()
    end
    popStyle()
    killDeadAsteroids(self.asteroids)
end

We built this facility because if we do asteroid:move in the middle of this loop, we were afraid we might kill it during the move. But that’s simply not the case. Collisions are the only things that can kill the asteroid, so this can’t really happen (he said).

And we’ve allowed the asteroids to know the Universe, so we can remove that call and have the asteroids kill themselves as needed:

function Asteroid:split()
    self:bang()
    U.asteroids[self] = nil
    Splat(self.pos)
    if self.scale ~= 4 then
        Asteroid(self.pos, self.scale//2)
        Asteroid(self.pos, self.scale//2)
    end
end

See above where I said “he said”. I was mistaken. Find collisions can destroy an asteroid while looping over them and that breaks the looping logic in Codea. So we’ll just revert that right back and call it good.

Now I have an errand to run and the battery is getting low, so I’ll sum up upon my return.

Summing Up

So the very good thing about today’s little exercise is that we added our new feature, starting asteroids at the edge of the screen, with relative ease, and we cleaned up the asteroids class as part of that effort.

I believe this is exactly what we should generally do. If the code has gotten a bit messy, we clean it up when we next have an occasion to work with it. If we never need to work with it, no harm done. This is the point of Refactoring: Not on the backlog!.

We don’t need to ask permission to work here: we have a feature story that lead us here. And surely we don’t need to ask permission to do good work. We do need to have the skill to do small simple improvements, and to let them add up to overall better design.

Better design lets us move faster. When things are where we expect to find them, when the code is short and sweet, our changes go in more readily.

If you do want to ask permission, please do two things: first, write to me and tell me why. Ask why a few times, to provide all the things that lie beneath your desire to ask permission.

Second, here’s how to ask permission: “Boss, should I do this in a way that makes me go slower, or a way that makes me go faster?”

Because that’s what the question really is, isn’t it?

OK, what else. The DeadAsteroids thing is odd. It’s odd because we can’t fiddle with a collection while it’s being iterated. But it’s the only place we have to do anything like that. We don’t do it to missiles, and there doesn’t seem to be any trouble with them.

So something is funky here.

Another thing that is funky is that the Universe is at the center of all things, and we’ve seen a few situations where that gives us trouble. Objects want to register themselves with the Universe, and at least once, the Universe wasn’t there to let them register. The code involved was temporally connected: things had to be done in a certain order that wasn’t obvious.

Things of course do have to be done in a certain order all the time. But we prefer to minimize those connections. The dead asteroids thing is an example of what gets weird when we have to consider temporality on a grander scale than this line then that line.

As I write this, I wonder how missiles get recorded and killed. Let’s look:

function Missile:init(ship)
    function die()
        self:die()
    end
    self.pos = ship.pos
    self.step = U.missileVelocity:rotate(ship.radians) + ship.step
    U.missiles[self] = self
    tween(3, self, {}, tween.easing.linear, die)
end

function Missile:die()
    U.missiles[self] = nil
end

OK, they’re basically the same as asteroids now, they record themselves with the Universe and they remove themselves from it.

There is some duplication here, in that these two things that can be created and die manipulate two different collections in very similar ways. This suggests, very gently, that there may be some commonality to remove.

And it’s one thing to say that you should register with the universe, and another to have such an explicit way of doing it by grabbing a collection from the universe and putting yourself (or a nil) into it.

The asteroid code and the missile code share information with the universe … but they also share implementation details, the names and forms of the collections they’re stored in.

It might be better if they just said something like U:register(self). Unfortunately, were we to do that, I don’t know quite how Universe would know what to do, if it wants to handle missiles and asteroids differently. l wonder if we can access the class of an object somehow …

And yes, we can. If you have some instance of some class, you can find out whether it is an instance of Foo class:

local f = Foo()
local b = Bar()
print(f:is_a(Foo)) -- prints true
print(b:is_a(Bar)) -- prints false

So that’s somewhat interesting. We could, if we choose, decide things based on class. I’m not fond of that. We’d do better to send them messages and let them do things.

Hmm. What if objects like missiles and ships and asteroids kept track of their own collections? Would that improve things?

When the Universe wanted to iterate over asteroids, it could ask Asteroid for the collection:

local asteroids = Asteroid:collection()

That might be “better” but it’s not obvious to me that it’s not just “different”. We’d still have the concern of iterating over a collection that’s being edited (and, I suppose, accessing something after it is logically dead or changed). So I’m not convinced that’s better at all.

At the current size and complexity of our little Asteroids game, this isn’t much of a concern. Yes, DeadAsteroids is a bit of a hack, but it’s not used all around and it works fine. We might be able to build some really nifty collection for objects that has a built-in trash can like DeadAsteroids that it cleans up at suitable times (whatever that would mean). That seems way more clever than we can afford.

I have this small red LED blinking in my mind that says “global singleton bad”, but at this moment I don’t see what’s better. We could pass our universe around but that seems to me to add a parameter to just about every calling sequence.

Meh. I don’t have a better idea right now. We’ll see. The code will tell us when it wants to be different.

For now, we have a new feature to try, and cleaner code to live with. Life is good.

See you next time!

Zipped Code

  1. A motto for every occasion, that’s my motto. ↩