Asteroids 35: Further refactoring
I’ve determined that it’s time to collapse some ‘technical debt’, in small, incremental steps.
Technical Debt
When the great Ward Cunningham originally described “technical debt”, he referred to the common situation that the programmers’ understanding of the problem and solution grow, and they begin to see “what they should have done”.
Caveat: I’m doing my best to explain Ward’s idea here, in the context of our program. Any errors or fuzziness are mine.
Of course “should have done” is hindsight. Since we didn’t have that understanding of the problem and solution at an earlier time, the design we have is the best we could have had at that time. Be that as it may, we now see a better way.
Ward described the program growing and becoming more complex by moving his hands as if holding a growing ball of jello. At some point, he said, the team decides to put their new knowledge into the software, and the system contracts back down to a less complex state, generally smaller than it was before.
In Ward’s original concept, technical debt was not the result of intentionally deferring good design for some bad – or even good – reason. It was the inevitable result of creating software and learning about what we’re creating.
Asteroid Debt
That’s what has happened here. It made perfect sense to check asteroids against missiles. It made no discernible sense to check asteroids against other asteroids, since they don’t interact. And since missiles in Asteroids don’t kill each other, it made no sense to check missiles against missiles.
That was a good idea, and you’ve watched me try hard to keep the code clean. You’ve also watched me make mistakes, some of which are only visible in retrospect, some of which were the inevitable result of my being human.
Following that original notion, the collision logic of the system has become more and more complex, embedding the various special cases. We’ve had to do odd things to keep track of dying objects, and we’ve seen bizarre things happen when adding new objects. And there are things behind the scenes that I wonder about.
For example, if a missile and asteroid are found to be colliding, the asteroid splits and the missile dies. What if there are two asteroids at that location? Will the missile split the second one? I think it won’t, but I wouldn’t be the BMW on it.
So we’ve just begun (at 0300 this morning) to consolidate our understanding of the program and solution into the code, letting the code better reflect what we now understand. And, I would say that better understanding is this:
It will be better to consider all pairs of objects, and if they are close enough to collide, let the objects sort out among themselves what is happening.
This could, of course be a misunderstanding. I don’t think it is but we’ll find out. We’ll try to work so as to find out as soon as possible.
So that’s what we’re about. Let’s get back to doing.
Doing
Last night / this morning, I started the process of making a collection of all the objects in the game. I completed putting the asteroids, missiles, and saucer into the master collection. It remains to do the Explosions and the Ship. Let’s do that and then assess.
The ship, on creation, does not add itself to the universe, as do the other objects we’ve dealt with so far. Aside from the tests (more about those later), all Ship creations are in Universe:
in startGame
: self.ship = Ship()
.
And in killShip
:
function Universe:killShip()
local f = function()
self.ship = Ship()
end
Explosion(U.ship)
U.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
We have here a delayed ship creation after killing the existing ship. There is also a lot of code that is conditioned by whether or not the ship exists, such as this, in draw
:
if self.ship then self.ship:draw() end
if self.ship then self.ship:move() end
We should expect to have to be careful with those, though most of them, such as those in draw, should just go away. We’ll draw everything that can be drawn. We’ll move everything that can be moved. We won’t go around asking questions about other people’s concerns.
I’ll mention here a future concern, mostly in the hope that I’ll remember it when the time comes. When an object is found to be destroyed in the middle of our loop, I think we should tell it that it is dead, somehow, and check that status as part of colliding objects. The reason is that we might be holding onto that object in the outer loop, and might therefore check it against many more objects in the inner loop. Once it is dead, those interactions should have no effect.
Maybe there’s a better way to handle it, but right now I don’t see it.
Moving right along … we’re following the rule that objects register with the Universe, and that they do so by calling, e.g. U:addAsteroid(self)
. We may still regret this, especially during the tests, but it’s the best idea the cat and I have. So we’ll go ahead and make Ship and Explosion do that.
function Universe:startGame(currentTime)
self.currentTime = currentTime
self.saucerTime = currentTime
self.attractMode = false
createButtons()
Ship() -- <--- was self.ship = Ship())
self.asteroids = {}
self.waveSize = nil
self.lastBeatTime = self.currentTime
self:newWave()
end
And similarly:
function Universe:killShip()
local f = function()
Ship()
end
Explosion(U.ship)
U.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
Here it gets tricky. The creation is easy:
function Ship:init()
self.pos = vec2(WIDTH, HEIGHT)/2
self.radians = 0
self.step = vec2(0,0)
U:addShip(self)
end
We give that the obvious implementation:
function Universe:addShip(ship)
self.objects[ship] = ship
self.ship = ship
end
What about our delete function? At this point, the universe is deciding that the ship should be deleted, and it calls killShip
to do it. I think we can, and should, rename that to deleteShip
and put it with the rest:
function Universe:deleteShip(ship)
local f = function()
Ship()
end
Explosion(ship)
self.objects[ship] = nil
self.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
function Universe:checkMissileCollisions(asteroid)
for k,m in pairs(self.missiles) do
if self.ship and m.pos:dist(self.ship.pos) < self.ship:killDist() then
self:deleteShip(self.ship)
m:die()
end
if m.pos:dist(asteroid.pos) < asteroid:killDist() then
asteroid:score()
asteroid:split()
m:die()
end
end
end
function Universe:checkShipCollision(asteroid)
if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
asteroid:score()
asteroid:split()
self:deleteShip(self.ship)
end
end
This should complete adding and removing the ship from the master collection. I’m going to test and then commit this. Game runs, tests run. Commit: “ship added to objects collection”.
Speaking of tests. We could, and likely should test to be sure these things to into the objects collection. Later on, as we begin to do the object interactions, we really ought to test to be sure they work. The double dispatch is the right idea, I believe, but it is tricky in a different way from our current more random comparing, adding, and deleting.
And here’s where one of our fundamental design issues arises.
All the objects that are created or destroyed now register with the global Universe U
. And our tests run with the game running at the same time. (We can be pretty sure that our tests run in a single draw cycle, because Codea won’t interrupt us while we’re thinking.) So the state of the U
variable is contaminated by the running game, when we run the tests.
We should at least save and restore the Universe. CodeaUnit does have a before
and after
facility that will let us do that.
As a warmup, let’s test our objects collection building to make sure we’re getting all the right things into the collection. This should be “fun”.
I think I’ll add new fields to CodeaUnit, to use in saving and restoring the Universe:
function testAsteroids()
CodeaUnit.detailed = true
CodeaUnit.oldU = nil
_:describe("Asteroids First Tests", function()
_:before(function()
CodeaUnit.oldU = U
U = Universe()
end)
_:after(function()
U = CodeaUnit.oldU
end)
With this in place, you can start a game, press the test button, run the tests, and the game just continues. Just as intended.
In the light of this saving and restoring of the universe in the tests, I thought a bit about whether we should get rid of the prominent global U
. I think we should and can, but I’ve decided to wait until the current improvements are further along, probably until they’re done.
I feel sure that testing the object interactions will be useful. The tests will specify the behavior we need, solidifying our design thinking. And they’ll give us confidence that we’ve done it.
To warm up, let’s test that everything gets properly added to the universe. We’ll do the tests one at a time until we’re tired and/or happy.
_:test("Asteroids added to objects", function()
U:newWave()
_:expect(countObjects()).is(4)
end)
I’ve not written countObjects
yet but the test seems right to me. I’ll go ahead:
function countObjects()
local c = 0
for k,v in pairs(U.objects) do
c = c + 1
end
return c
end
I even remembered to call pairs
. I expect this to work. Let’s find out why it doesn’t. It does work. Sweet.
Missiles will be trickier. Here’s their init:
function Missile:init(pos, step)
function die()
self:die()
end
self.pos = pos
self.step = step
U:addMissile(self)
tween(3, self, {}, tween.easing.linear, die)
end
We can clearly create a few and count them, but when the tween times out all hell my break loose. This is a example of a place where passing the Universe in would have been better. Let’s just try the test and see what happens.
_:test("Missiles added to objects", function()
_:expect(countObjects()).is(0)
Missile(vec2(100,100), vec2(0,0))
_:expect(countObjects()).is(1)
Missile(vec2(100,100), vec2(0,0))
_:expect(countObjects()).is(2)
end)
Nothing bad happens after the tests, so while I’m still nervous about the global, I’m not terribly worried.
Now the saucer:
_:test("Saucer added to objects", function()
_:expect(countObjects()).is(0)
Saucer()
_:expect(countObjects()).is(1)
end)
This one fails, oddly:
14: Saucer added to objects -- Saucer:10: attempt to perform arithmetic on a nil value (field 'currentTime')
It appears that our fake Universe needs to know the time, so we’ll give it one in the test:
_:test("Saucer added to objects", function()
_:expect(countObjects()).is(0)
U.currentTime = ElapsedTime
Saucer()
_:expect(countObjects()).is(1)
end)
That runs correctly. Now Ship:
_:test("Ship added to objects", function()
_:expect(countObjects()).is(0)
Ship()
_:expect(countObjects()).is(1)
end)
That passes. I’m inclined to commit these new tests: “test objects added to objects collection”.
Now to test the Explosion:
_:test("Explosion added to objects", function()
_:expect(countObjects()).is(0)
Ship()
_:expect(countObjects()).is(1)
Explosion(U.ship)
_:expect(countObjects()).is(2)
end)
The explosion is based on a ship, to get its location. (That may not be ideal but it remains true.) So we create a ship, count it, create an explosion and count again. This test will fail expected 2 but got 1, because Explosions aren’t done in the new fashion yet:
16: Explosion added to objects -- Actual: 1, Expected: 2
Sweet, actual test-driven. Let’s make it work. This:
function Explosion:init(ship)
local f = function()
U.explosions[self] = nil
end
self.pos = ship.pos
self.step = vec2(0,0)
U.explosions[self] = self
tween(4, self, {}, tween.easing.linear, f)
end
Becomes this:
function Explosion:init(ship)
local f = function()
U:deleteExplosion(self)
end
self.pos = ship.pos
self.step = vec2(0,0)
U:addExplosion(self)
tween(4, self, {}, tween.easing.linear, f)
end
If we run the test now, we get this:
16: Explosion added to objects -- Explosion:9: attempt to call a nil value (method 'addExplosion')
This reminds us to implement addExplosion
:
function Universe:addExplosion(explosion)
self.objects[explosion] = explosion
self.explosions[explosion] = explosion
end
I know I need the delete but I want to see what happens if I don’t build it. Running the test again … Sweet!
Explosion:5: attempt to call a nil value (method 'deleteExplosion')
stack traceback:
Explosion:5: in field 'callback'
...in pairs(tweens) do
c = c + 1
end
return c
end
This came up well after the tests completed, when the tween timed out and tried to delete the explosion. We’ll add that feature:
function Universe:deleteExplosion(explosion)
self.objects[explosion] = nil
self.explosions[explosion] = nil
end
Testing … tests all run and the game runs as well. We’ve just test-driven the change to Explosion and tested the others. What about Splat? They manage themselves right now, and I decided an article or two ago to leave them alone. I think I’ll stick with that decision for now. So we can commit: “explosions added to objects”.
Looking Around
It’s time to lift our head up and look around. We’ve been heads-down in this for a bit and need to break and think about direction a bit.
I like where we are. We’ve begun the process of putting all the objects into a single collection, so that we can collide each with each and leave the decisions more up to the individuals. I still think that’ll turn out to be a good way to go.
We presently have ten (ten!) add and delete methods in Universe, one of each for asteroid, explosion, missile, saucer, and ship. That’s a lot. But they will all collapse down to one add and one delete, as soon as we start using the master objects
collection instead of the specialized ones. I’d like to start reaping that benefit (harvesting the change, as GeePaw Hill puts it) as soon as possible.
So I’m thinking that our next step will be to change all the loops over other collections into loops over objects
right away. We can do that pretty simply, either by creating the other lists on the fly, or by selecting inside the loops.
In Codea, given an object, you can say
if object:is_a(Missile) then ...
That will do what it says, returning true if object
is an instance of Missile
. That will be enough to allow us to manage the transition as we wish, with the concern that using is_a
is not a good idea over the longer term. And we might even get a better idea before going ahead.
For this morning, since it just turned noon, we’re done. I’m happy with where we’re going, and have a renewed interest and confidence in the tests.
See you next time!