Asteroids 52
Let’s clean up the Aimer and look around. I think we need options.
I’ve decided that the new Aimer is good enough, and that while we might refine it, we’ll stick with it. Until at some future time we don’t, but frankly I think it’s fine. The code around Aimer is a bit, shall we say, unfinished, and its home is presently in the test tab for aiming, which is odd.
An interesting notion comes to mind. To the extent that classes have tests, having the tests reside in the same Codea tab as the class might be a nice idea. It would cut the number of tabs, and you’d always know where to look for the tests for something. (And where to be sad if you didn’t find any.)
At least two things push back against that idea: First, as we evolve the design, it’s possible that we have tests for a capability long before we have classes for it. In fact, the general pattern is to test “results”, often without regard to the class or classes that produce that result. Often it takes a few classes to produce a capability or result.
Second, as in our tests here, we often start with an undifferentiated pile of tests and let it grow. It seems that it would be difficult, or at least busy-work, to reorganize them. There is a real sense in which we write them and forget them, trusting them to alert us when they need attention. Maybe it shouldn’t be that way, but it is.
Still, it’s an interesting notion, tests with the code they test.
And I would like to reduce the number of tabs I have. Codea only has one row of tabs, and they’re wide enough to display the names, so you quickly wind up scrolling them back and forth.
Most of these items are details, but the ability to move easily among our objects is part of being able to grow and evolve the program easily. We try to identify the “important” details, but often we’re just guessing. I suspect there is no “right way”, but there’s usually a “better way”.
But our first point of attention is Aimer, which seems to live in the wrong place and whose interface is … odd.
Aimer
Aimer presently lives in the TestAiming tab, the complete contents of which are this:
-- TestAiming
-- RJ 20200624
function testAiming()
--CodeaUnit.detailed = true
CodeaUnit.oldU = nil
local gun
local tgt
local aim
local ang
_:describe("Aiming Tests", function()
_:before(function()
CodeaUnit.oldU = U
U = Universe()
end)
_:after(function()
U = CodeaUnit.oldU
end)
_:test("Hookup", function()
_:expect( 2+1 ).is(3)
end)
_:test("Pure Aiming Angle 45", function()
gun = vec2(400,400)
tgt = vec2(600,600)
aim = Aimer(gun,tgt)
ang = math.deg(aim:pureAngle())
_:expect(ang).is(45)
end)
_:test("Pure Aiming Angle -45", function()
gun = vec2(400,400)
tgt = vec2(600,200)
aim = Aimer(gun,tgt)
ang = math.deg(aim:pureAngle())
_:expect(ang).is(-45)
end)
end)
end
Aimer = class()
function Aimer:init(gunPos,tgtPos)
self.gunPos = gunPos
self.tgtPos = tgtPos
end
function Aimer:pureAngle()
local toTarget = self.tgtPos - self.gunPos
return vec2(0,0):angleBetween(toTarget)
end
A couple of tests, some testing hints we could do without, and a tiny class. We use Aimer like this:
function SaucerMissile:fromSaucer(saucer)
if not Ship:instance() or math.random() > saucer:accuracyFraction() then
return SaucerMissile:randomFromSaucer(saucer)
end
local gunPos = saucer.pos
local ship = Ship:instance()
local tgtPos = ship.pos + ship.step*120
local aim = Aimer(gunPos,tgtPos)
local ang = aim:pureAngle()
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
The Aimer itself isn’t doing much at all, is it? Now I do have in mind that it could do more, such as adjust for the distance between saucer and ship and the like. But I’m also just about satisfied with it as it stands.
There are really only two lines of interest in Aimer:
local toTarget = self.tgtPos - self.gunPos
return vec2(0,0):angleBetween(toTarget)
Let’s inline that code into SaucerMissile:fromSaucer:
function SaucerMissile:fromSaucer(saucer)
if not Ship:instance() or math.random() > saucer:accuracyFraction() then
return SaucerMissile:randomFromSaucer(saucer)
end
local gunPos = saucer.pos
local ship = Ship:instance()
local tgtPos = ship.pos + ship.step*120
local toTarget = self.tgtPos - self.gunPos
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
We don’t have any useful tests for this, and we’re probably about to delete TestAiming entirely. I’m troubled by that. Let’s refactor a bit toward testability, and then repurpose the aimer tests.
function SaucerMissile:fromSaucer(saucer)
local ship = Ship:istance()
if not ship or math.random() > saucer:accuracyFraction() then
return SaucerMissile:randomFromSaucer(saucer)
else
return SaucerMissile:aimedFromSaucer(saucer,ship)
end
end
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = self.tgtPos - self.gunPos
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
That’s more symmetrical at the top, in fromSaucer, and avoids an extra call to Ship:instance(). Now I think we can repurpose these tests:
_:test("Pure Aiming Angle 45", function()
gun = vec2(400,400)
tgt = vec2(600,600)
aim = Aimer(gun,tgt)
ang = math.deg(aim:pureAngle())
_:expect(ang).is(45)
end)
_:test("Pure Aiming Angle -45", function()
gun = vec2(400,400)
tgt = vec2(600,200)
aim = Aimer(gun,tgt)
ang = math.deg(aim:pureAngle())
_:expect(ang).is(-45)
end)
We’ll call `SaucerMissile:aimedFromSaucer. We’ll need to enhance our inputs into little tables. I’ll remove Aimer, to make these two tests fail, then hook them up.
1: Pure Aiming Angle 45 -- TestAiming:18: attempt to call a nil value (global 'Aimer')
As expected.
I translate one test to this:
_:test("Pure Aiming Angle 45", function()
gun = {pos=vec2(400,400)}
tgt = {pos=vec2(600,600), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
ang = atan2(bullet.step.y, bullet.step.x)
_:expect(ang).is(45)
end)
With this result:
_:test("Pure Aiming Angle 45", function()
gun = {pos=vec2(400,400)}
tgt = {pos=vec2(600,600), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
ang = atan2(bullet.step.y, bullet.step.x)
_:expect(ang).is(45)
end)
Referring to line 101:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = self.tgtPos - self.gunPos -- <-- 101
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
Which is a bug in my refactoring, since those variables are local:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = tgtPos - gunPos
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
The test now fails:
1: Pure Aiming Angle 45 -- TestAiming:19: attempt to call a nil value (global 'atan2')
Because it’s math.atan2, and now the test fails saying angle is zero and should be 45, so at last something actually isn’t working. Adjust the test to be sure we understand:
_:test("Pure Aiming Angle 45", function()
gun = {pos=vec2(400,400)}
tgt = {pos=vec2(600,600), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
_:expect(bullet.step).isnt(vec2(0,0))
ang = math.atan2(bullet.step.y, bullet.step.x)
_:expect(ang).is(45)
end)
I’m pretty sure the bullet step is coming back zero, so I test it before looking for the problem. That fails, so let’s look at what’s up:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = tgtPos - gunPos
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
So I’m expecting toTarget to be (200,200) and wait what is that zero vector, does that even work? It was working in Aimer wasn’t it? Let’s first be sure toTarget is what we expect:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = tgtPos - gunPos
assert(toTarget == vec2(200,200), "toTarget")
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
The assert doesn’t fire, so that seems OK. And I wrote this test:
_:test("angle between", function()
_:expect(math.deg(vec2(0,0):angleBetween(vec2(200,200)))).is(45)
end)
That does pass, although I’m a bit confused over why. I enhance that test for my own education:
_:test("angle between", function()
_:expect(math.deg(vec2(0,0):angleBetween(vec2(200,200)))).is(45)
_:expect(math.deg(vec2(1,0):angleBetween(vec2(200,200)))).is(45)
_:expect(math.deg(vec2(200,0):angleBetween(vec2(200,200)))).is(45)
end)
Those are all correct. What am I not seeing in the function under test. Let’s look again:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = tgtPos - gunPos
assert(toTarget == vec2(200,200), "toTarget")
local ang = vec2(0,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
I’m going to assert ang non zero:
And I see the problem. My fake saucer doesn’t have a shot speed!
_:test("Pure Aiming Angle 45", function()
gun = {pos=vec2(400,400), shotSpeed=3}
tgt = {pos=vec2(600,600), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
_:expect(bullet.step).isnt(vec2(0,0))
ang = math.atan2(bullet.step.y, bullet.step.x)
_:expect(math.deg(ang)).is(45)
end)
What happened was that vec2(nil,0) apparently counts as a zero vector, which rotated is zero vector, which wasn’t what we wanted. I expect this to work now. And it does, oddly:
2: Pure Aiming Angle 45 -- Actual: 45.0, Expected: 45
This is of course a rounding error, thank you floating point, but we have enhanced CodeaUnit for just this situation, so the test becomes:
_:test("Pure Aiming Angle 45", function()
gun = {pos=vec2(400,400), shotSpeed=3}
tgt = {pos=vec2(600,600), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
_:expect(bullet.step).isnt(vec2(0,0))
ang = math.atan2(bullet.step.y, bullet.step.x)
_:expect(math.deg(ang)).is(45, 0.5)
end)
And that works. Now let’s fix the other test accordingly:
_:test("Pure Aiming Angle -45", function()
gun = {pos=vec2(400,400), shotSpeed=3}
tgt = {pos=vec2(600,200), step=vec2(0,0)}
bullet = SaucerMissile:aimedFromSaucer(gun,tgt)
ang = math.atan2(bullet.step.y, bullet.step.x)
_:expect(math.deg(ang)).is(-45, 0.5)
end)
That passes. Another test fails:
26: Small Saucer shoots at 4/20 accuracy -- Saucer:90: attempt to call a nil value (method 'istance')
That’ll be a typo. I suspect that was failing all along and scrolled out of the tiny window tests appear in:
function SaucerMissile:fromSaucer(saucer)
local ship = Ship:istance()
Gets the obvious fix. That passes but this is still failing:
~~~lua 26: Small Saucer shoots at 4/20 accuracy – Actual: 0, Expected: 200
That test has been flaky for ages:
~~~lua
_:test("Small Saucer shoots at 4/20 accuracy", function()
U = FakeUniverse()
local score = Score(3)
score:addScore(3000)
local ship = Ship(vec2(400,800))
local saucer = Saucer(vec2(500,800))
local count = 0
for i = 1,1000 do
local m = SaucerMissile:fromSaucer(saucer)
if m.step.y == 0 then
count = count + 1
end
end
saucer:dieQuietly()
_:expect(count).is(200,50)
end)
This is a probabilistic test which fails sporadically. It is now failing all the time because the new method isn’t ever returning an exactly level shot, I suspect. I’ll tweak the test to see. Changing the loop:
for i = 1,1000 do
local m = SaucerMissile:fromSaucer(saucer)
if m.step.y < 0.001 and m.step.y > -0.001 then
count = count + 1
end
end
Test runs fine now. Commit: “removed Aimer, tests now refer to SaucerMissile”.
Let’s do a quick …
Retrospective
What just happened? It probably seems chaotic to read, just one thing after another coming out of the test failing after our refactoring. But with the exception of the confusion over now having shotSpeed in my fake saucer, there was essentially no confusion at all. The test says “this broke”, we fix this, it says “that broke”, we fix that. Sometimes we have to think a bit to notice the “self.” or something, but generally the test leads us by the nose until the code works.
That is exactly what is supposed to happen with decent tests. Mostly they lead us straight to the problem. Sometimes they surprise and confuse us, but there’s always a problem or misunderstanding at the bottom of it all.
This is exactly why I love having good tests and exactly why I wonder why I don’t always have them. The only explanation I can come up with is that I’m a bad person, and that’s clearly false. So I don’t know.
Anyway, tests are great and this was an example of why.
What about that class Aimer though? We TDD’d it into existence and then a day later threw it away. What’s up with that?
Well, we had originally had more functionality in mind for it, so isolating it made double sense. It was easier to test, and it was a place for that functionality to reside. But when we tried it, it worked just fine in its baby state, so we stopped developing it. Since it only had two lines of actual function, it makes sense to move those two lines together with the rest of the code that sets up the saucer missile.
Those lines are still a bit odd, but they’re all in one place. So there’s no reason to feel badly for poor old Aimer, it did its job as intended and the code as wound up where it belongs.
I remain a bit confused about the zero vector going into vec2:angleBetween, because a zero vector is rather stubby and doesn’t point anywhere. It did what I expected thoughtlessly but now that I’ve thought about it, I wonder what’s up: I think it’s a special case in angleBetween. I’m not going to rely on that:
function SaucerMissile:aimedFromSaucer(saucer, ship)
local gunPos = saucer.pos
local tgtPos = ship.pos + ship.step*120
local toTarget = tgtPos - gunPos
local ang = vec2(1,0):angleBetween(toTarget)
local bulletStep = vec2(saucer.shotSpeed, 0):rotate(ang)
return SaucerMissile(gunPos, bulletStep)
end
Tests still run, of course. Commit: used nonzero vector in angleBetween.
So that’s better. I’ve been playing here for a couple of hours now, so let’s take a break.
Break’s Over
On reflection, mixing the code and tests probably isn’t a good idea for anyone who wanted just to play the game. With the tests in separate tabs, they could just delete those tabs. So I won’t do that.
There is a bit of other neatening and organization “behind the scenes” work to do. One thing that has not shown up here is that the SaucerMissile class is split between the Saucer tab and the SaucerMissile tab, just because I wrote the factory methods near Saucer` to make it easier. I’ll move those over where they belong. Nothing to see here.
I think I’ll move the TestAiming tab contents into TestAsteroids, leaving the tests as a separate “describe”, which is as it should be. I should likely do that more often than I do.
To do that, you have to fold the outer functions textXYZ into one, because CodeaUnit only detects one testXYZ per tab. That’s OK, it’s the describe that does the test partitioning.
I should mention that CodeaUnit does some very magical things to run the tests, not least that it finds the tests in the text in the tabs, copies the extent of the test into a string and executes the string. It’s quite weird, and one of those things you mostly leave alone.
What else?
Since SaucerMissile is a subclass of Missile, I think I’ll move those two together, to save a tab. I don’t think it’ll confuse me. And possibly there’s no real need for two classes to handle the somewhat odd collision logic. We’ll see. If it confuses me, I’ll mention it and we’ll do something about it.
Fragments are just parts of Explosions. I’ll move the Fragment class into the Explosion tab.
I could move the Rocks tab into the Asteroids tab, since Rocks is just the line definitions for the asteroids shapes. Saves a tab, but somehow doesn’t seem right.
I think there’s more cleanup that could be done with SaucerMissile, an abstraction waiting to be discovered, perhaps, but I think we’re done for today.
Summing Up
Final commit: “moved things around, removed some tabs”.
Some reduction of complexity, some cleanup, and some added certainty and value from the tests. What’s not to like?
I wish you could feel the difference between working through those little tests not working, and working without tests. The best way to understand is probably to try it yourself.
Next time, I think I’ll put some “dip switches” into this thing, for tuning and testing. See you then!