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:

        _: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!

Asteroids.zip