Kotlin 198 - This way? Or that way?

GitHub Decentralized Repo
GitHub Centralized Repo

As we centralize more and more code, the Game class is getting messy. Should we start to improve it now, or let it get worse before it gets better? Some thoughts about thinking.

When I start waking up in the morning, I think about code. I do it to keep the morning demons away, but it’s also good for the code … usually. This morning I thought briefly about moving the SaucerMaker logic into the game next. That wasn’t very deep thinking, and I think it’ll go much like the WaveMaker.

So next, I thought about the Game class, which is not quite as cohesive as it might be. Its main responsibility, I’d say, is to run the game cycle. The whole game works by a single setup call from the main, and that setup is immediately followed by repeated calls to Game.cycle, 120 times a second or whatever the machine can manage. The Game class also includes some related responsibilities, including setting up the game objects for initial play or insertion of a quarter, and it will “soon” include the interaction logic, if, as I currently intend, I follow the style of the original game.

Caveat

The original game, written in assembler, was tight on memory and used no modern facilities like collections or objects. So it would find itself pointing to two little data structures and it would pull out bits of those structures and compare them, set them, whatever was needed. Although I’m sure we could find code out in the wild that worked similarly to that, with some “big” program fiddling the bits of some fairly dumb data objects, it’s really not what I’d call good code.

Accordingly, when we get there, I can imagine that we’ll defer questions to the individual objects that the original code would have done by interrogation. At this writing, I’m not sure how complex the interaction’s main loops will be, and that complexity will dictate whether it needs to be broken out or not.

Where was I? Oh, yes. There are so far just a few things that Game knows:

• knownObjects, the smart collection that contains all the objects from the mix, with fairly intelligent support for grouping. knownObjects also manages the DeferredAction objects that control timed events.
• lastTime, a variable used to produce deltaTime, which the objects use to know how far to move and such.
• numberOfAsteroidsToCreate, a variable that keeps track of how many asteroids to create in the next wave.
• oneShot, a OneShot object used to trigger wave creation. oneShot will need a new name, because we will have more than one OneShot instance as we add Saucer and Ship timing.

We could imagine separating out the creation of these objects, in a sort of factory style. We could imagine a little object for making waves … we could call it WaveMaker … like the class we removed just yesterday. Ha ha, but in fact it would probably be much simpler than yesterday’s and it would be nicely cohesive, and would improve the cohesion of Game as well.

Speculation

All this is speculation, of course, both the part of the thinking I did in the dark, and the part I’m doing now as I write this. But it’s not without value: I believe we benefit from thinking about our design. We shouldn’t spend days thinking and not coding, but a bit of speculation and planning helps us ready our minds for actually doing the work. It helps us see options. It gives us ideas, some of which will be useful.

How much? How long should we think before acting? I’ve found it productive to think all the time, and to let my thoughts interact with my code as much as possible. Or, as Kent Beck put it, to let my code participate in my design sessions. I’ve found that moving to code early works just fine, with the caveat that I’ve built up a good sense of when to be doing real production coding and when I’m trying an experiment.

How do you build up that sense? By making mistakes. If we wait too long, our over-thought idea will almost always be rejected by the code, which doesn’t want to do it quite that way. If we don’t wait long enough, or we try to production code when we should have experimented, we’ll make a mess and we need to have the courage to roll back and try again, more simply.

Will Rogers may have said: “Good judgment comes from experience, and a lot of that comes from bad judgment.” Whether he did or not, it’s true. My experience has been that I can move to code much sooner than I would have thought. And sometimes, knowing that, I still move too soon. Thing is, moving too soon provides earlier feedback than waiting, because waiting provides very little feedback. I’d rather move to code too soon than too late.

Therefore, let us code

After all this, I think it’s premature to try to improve this design. It’s not quite messy enough to need refactoring. It’s close, and if we wanted to polish it, there are things that we could do. But let’s instead make a bit more of a mess. We’ll be careful not to let things get too awful … and even if they do, we know we can improve anything.

So let’s do …

SaucerMaker

Here’s SaucerMaker in all its glory:

class SaucerMaker(saucer: Saucer = Saucer()): InteractingSpaceObject, ISpaceObject  {
    private var oneShot = OneShot(7.0) { it.add(saucer) }
    var saucerMissing: Boolean = true

    override fun update(deltaTime: Double, trans: Transaction) {}
    override fun callOther(other: InteractingSpaceObject, trans: Transaction) {}

    override val subscriptions: Subscriptions = Subscriptions(
        beforeInteractions = { saucerMissing = true },
        interactWithSaucer = { _, _ -> saucerMissing = false },
        afterInteractions = { if ( saucerMissing ) oneShot.execute(it) }
    )
}

This is very much like the WaveMaker. To put this into Game, we “just” need to move the OneShot, and trigger it when we detect that there is no saucer. The OneShot will go into its triggered state, and ignore our subsequent frantic attempts to add the Saucer, until finally the deferred object that OneShot creates times out, adds the Saucer, and resets the OneShot for next time.

We need a function on knownObjects that tells us whether there’s a Saucer. What we’ll be doing is going to be much like we did for the wave:

    private fun createNewWaveIfNeeded() {
        if ( knownObjects.asteroidCount() == 0 ) {
            val trans = Transaction()
            oneShot.execute(trans)
            knownObjects.applyChanges(trans)
        }
    }

This seems easy. I think I’ll try it all in one go. What do we have for tests? There is a SaucerMakerTest. It’s pretty solidly focused on the SaucerMaker object. I see one test there that I think we might be able to adapt to our use:

    @Test
    fun `game-centric saucer appears after seven seconds`() {
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val saucer = Saucer()
        val maker = SaucerMaker(saucer)
        mix.add(maker)
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        assertThat(mix.size).describedAs("mix size").isEqualTo(1)
        assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(2)
        val found = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
        assertThat(found).isNotNull
        assertThat(mix.contains(maker)).describedAs("maker sticks around").isEqualTo(true)
        game.cycle(7.2) //ELAPSED seconds
        assertThat(mix.contains(saucer)).describedAs("saucer missing").isEqualTo(true)
        assertThat(mix.contains(maker)).describedAs("maker missing").isEqualTo(true)
        assertThat(mix.size).isEqualTo(2)
    }

I’ll copy that test and modify it right now:

    @Test
    fun `game-centric NO MAKER saucer appears after seven seconds`() {
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        assertThat(mix.size).describedAs("mix size").isEqualTo(1)
        assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(2)
        val deferred = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
        assertThat(deferred).isNotNull
        game.cycle(7.2) //ELAPSED seconds
        val saucer = mix.targets.find { it is Saucer }
        assertThat(saucer).isNotNull
    }

This should fail. I’m not sure just how. I should be able to call it. I’ll trust the test.

[mix size] 
expected: 1
 but was: 0

Makes sense. There’s no SaucerMaker in there. There’s nothing. OK, fine. That’ll be useful. Let’s also drive out the saucer question from knownObjects (SpaceObjectCollection):

    @Test
    fun `can detect saucer`() {
        val s = SpaceObjectCollection()
        assertThat(s.saucerExists()).isEqualTo(false)
        s.add(Saucer())
        assertThat(s.saucerExists()).isEqualTo(true)
    }

This demands a small function in SpaceObjectCollection:

    fun saucerExists(): Boolean {
        return targets.filterIsInstance<Saucer>().isNotEmpty()
    }

I think that’ll do the job. Test. Yes, we’re good.

Now I plan a fairly big bite: I’m going to try to add the saucer check and creation all in one go.

    private fun createSaucerIfNeeded() {
        if ( ! knownObjects.saucerExists() ) {
            val trans = Transaction()
            saucerOneShot.execute(trans)
            knownObjects.applyChanges(trans)
        }
    }

I realized that I’d like the saucerExists function to be saucerMissing but I’ll deal with that in a moment. This code demands:

    private val saucerOneShot = OneShot( 7.0) {it.add(Saucer())}

And I need not to create the SaucerMaker:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        oneShot.cancel(Transaction())
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
//        trans.add(SaucerMaker())
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

I think this should make my new test run and make the game work. Let’s find out. No. The old test fails, no surprise there I guess, but my new one fails too:

[mix size] 
expected: 1
 but was: 0

That might be OK, let’s think about it now. My brain was busy wanting to implement before.

    @Test
    fun `game-centric NO MAKER saucer appears after seven seconds`() {
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        assertThat(mix.size).describedAs("mix size").isEqualTo(1)
        assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(2)
        val deferred = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
        assertThat(deferred).isNotNull
        game.cycle(7.2) //ELAPSED seconds
        val saucer = mix.targets.find { it is Saucer }
        assertThat(saucer).isNotNull
    }

I don’t think there should be anything in the mix, but there should be a deferred action. Yes. When I set that check to zero, the test runs. The Saucer got created as intended. I have to try this in the game. (And I think we need a cancel somewhere.) The game runs. Where did I put that cancel for the other OneShot? Ah, right here:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        oneShot.cancel(Transaction())
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
//        trans.add(SaucerMaker())
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

Let’s first rename to waveOneShot. I’d like to have a test for this but I’m not sure how to do it. Let’s do a collection thing like the collections in SpaceObjectCollection:

class Game(val knownObjects:SpaceObjectCollection = SpaceObjectCollection()) {
    private var lastTime = 0.0
    private var numberOfAsteroidsToCreate = 0
    
    private val waveOneShot = OneShot(4.0) { makeWave(it) }
    private val saucerOneShot = OneShot( 7.0) {it.add(Saucer())}
    // all OneShot instances go here:
    private val allOneShots = listOf(waveOneShot, saucerOneShot)

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        val ignored = Transaction()
        for (oneShot in allOneShots) { oneShot.cancel(ignored) }
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

Test, expecting all to be well except for unneeded SaucerMaker tests. I delete them and rename the class from SaucerMakerTest to:

class SaucerMakingTest {
    @Test
    fun `game-centric NO MAKER saucer appears after seven seconds`() {
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        assertThat(mix.size).describedAs("mix size").isEqualTo(0)
        assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(2)
        val deferred = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
        assertThat(deferred).isNotNull
        game.cycle(7.2) //ELAPSED seconds
        val saucer = mix.targets.find { it is Saucer }
        assertThat(saucer).isNotNull
    }
}

I’m green. I think SaucerMaker can be removed. Yes, IDEA agrees. Let’s commit: SaucerMaker removed, saucer management done in Game.

So that was nice. We added about 13 lines, removed about 15, not counting the tests. I would argue that the code is at least as clear as it was with the SaucerMaker flying about in the mix. Now we explicitly check for a Saucer and trigger the sauceOneShot if there is none.

I’m reminded that I’d like the check reversed. Let’s do that.

    fun saucerMissing(): Boolean {
        return targets.filterIsInstance<Saucer>().isEmpty()
    }

    private fun createSaucerIfNeeded() {
        if ( knownObjects.saucerMissing() ) {
            val trans = Transaction()
            saucerOneShot.execute(trans)
            knownObjects.applyChanges(trans)
        }
    }

    @Test
    fun `can detect saucer`() {
        val s = SpaceObjectCollection()
        assertThat(s.saucerMissing()).isEqualTo(true)
        s.add(Saucer())
        assertThat(s.saucerMissing()).isEqualTo(false)
    }

Should be green. It is. Commit: reverse sense of saucer check.

A Defect!

I’ve detected a defect during Game play. In the original SaucerMaker, we used the same Saucer over and over. That was because the Saucer remembers its direction and switches right to left, left to right. I didn’t cache a Saucer in the new implementation, I created a new one every time:

    private val saucerOneShot = OneShot( 7.0) {it.add(Saucer())}

I should have a test for switching direction in the game. But I need a hot fix:

class Game(val knownObjects:SpaceObjectCollection = SpaceObjectCollection()) {
    private var lastTime = 0.0
    private var numberOfAsteroidsToCreate = 0
    private val saucer = Saucer()

    private val waveOneShot = OneShot(4.0) { makeWave(it) }
    private val saucerOneShot = OneShot( 7.0) {it.add(saucer)}
    // all OneShot instances go here:
    private val allOneShots = listOf(waveOneShot, saucerOneShot)

That should do it. It does. I’ll make a note to do the test but I think I’m done for this morning.

Summary

This went delightfully … except for that saucer-switching defect. And I did have a chance to think about it, because I consciously changed the sauceOneShot to create a Saucer. I didn’t give it a second thought, really, just “sure, I’ll just create a new one”.

There are tests on Saucer to check that it switches direction … but it always starts out left to right, and since I was creating new ones, the test ran fine. I kind of need a game-level test.

Maybe I have the energy left to test it. I’ll try. Better now than never.

    @Test
    fun `saucer changes direction`() {
        // test ensures that we cache the saucer rather than create new ones
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        game.cycle(7.2) //ELAPSED seconds
        val saucer = mix.targets.first() as Saucer
        assertThat(saucer.velocity.x > 0.0)
        mix.performWithTransaction { it.remove(saucer) }
        game.cycle(7.3)
        game.cycle(14.4)
        val nextSaucer = mix.targets.find { it is Saucer}
        val actual = nextSaucer as Saucer
        assertThat(actual.velocity.x < 0.0)
    }

That’s a bit ragged but it does the job. Commit: Add game level test for saucer switching direction.

I’m glad I did that: I think I forgot to commit my hot fix.

OK, all’s right with the world. We’ve successfully removed the SaucerMaker object, replaced it with a single method in Game, plus a saucer-finding method in SpaceObjectCollection. Just as intended, works as intended. One mistake, which might not have been caught.

There’s a sort of a lesson here that I think some of my colleagues would not agree with: it really seems to me that I need a game-level test for direction-switching. The Saucer object does switch direction … but the game kept creating new ones. I’m sure all my pals would agree that if we think we need a test we should write it, but some of them (cough Hill cough) tend not to write scenario tests like that because they find that their microtests cover all the cases. I must be doing something differently, but I don’t quite see what it would have been.

Be that as it may, things are progressing just as intended, with no real difficulties arising yet.

I think our next venture will be the Ship-making logic and that’s more complicated. I suspect it would be unwise to just fling all that code into Game, but we’ll see when we get to it. Perhaps tomorrow.

See you next time!

P.S.

A final thought as I was reviewing. It might be useful to make a smart collection for the OneShot instances, with a cancel method, or at least to extract a method here:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        val ignored = Transaction()
        for (oneShot in allOneShots) { oneShot.cancel(ignored) }
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

Like this:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        cancelAllOneShots()
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

    private fun cancelAllOneShots() {
        val ignored = Transaction()
        for (oneShot in allOneShots) {
            oneShot.cancel(ignored)
        }
    }

Slightly better. Testing, I discover something disconcerting: This test fails intermittently:

    @Test
    fun `saucer changes direction`() {
        // test ensures that we cache the saucer rather than create new ones
        // cycle receives ELAPSED TIME!
        val mix = SpaceObjectCollection()
        val game = Game(mix) // makes game without the standard init
        game.cycle(0.1) // ELAPSED seconds
        game.cycle(7.2) //ELAPSED seconds
        val saucer = mix.targets.first() as Saucer
        assertThat(saucer.velocity.x > 0.0)
        mix.performWithTransaction { it.remove(saucer) }
        game.cycle(7.3)
        game.cycle(14.4)
        game.cycle(14.5)
        val nextSaucer = mix.targets.find { it is Saucer}
        val actual = nextSaucer as Saucer
        assertThat(actual.velocity.x < 0.0)
    }

Sometimes it doesn’t find a saucer there at the end. I’ll have to chase that tomorrow. I’m going to commit anyway, with that test sometimes failing. Commit: Extract cancalAllOneShots method.

I really don’t know what that problem is. I haven’t seen an intermittent test in ages. I’m kind of sorry I came back.

See you next time!!