GitHub Decentralized Repo
GitHub Centralized Repo

I think we just have one more finalize to remove. After that, well, I hope not the Deluge. Also: Do you have a lessón for your minkey?

General Remarks

Let me make a few general remarks about how I try to use IDEA for best results.

I’ve stopped using the displayed hierarchy almost entirely. Instead, I use the search that comes up when i rapidly type Shift twice. Then I type the name of the class/file I want to look at. Even just a few characters tends to get it close to the top of the list, but I type fairly rapidly and it’s generally right at the top when I stop typing. Enter, and there it is.

I try to use the key strokes for the refactoring steps that I take most often. Shift-F6 for rename, Command-B to find definitions or accesses, depending what you’re looking at. Option-Command-M to extract a method or function. Command-Option-N to inline something.

Those I know by heart. When I try some other more powerful thing, I may bring up the menu, but I try to remember to close the menu and do the keystroke, to try to burn it into memory.

I generally do use the mouse to find a method, if command-B isn’t available. I’m sure there are more keystrokes that would speed me up, but it’s not so much a matter of going fast. I don’t care to “go fast”. As the sages tell us, “Typing is not the bottleneck”. But I do like to move efficiently and with a modicum of what feels like grace.

Today

If I’m not mistaken, the only remaining implementor of finalize is Saucer. Let’s find out.

Shift-Shift type “subsc” until Subscriptions.kt is at the top. Enter. Happens that the cursor is on finalize. Command B, and we get this:

all the references to finalize

Super. What does Saucer do in finalize? Whatever it is, we want to do it sooner.

    private fun finalize(trans: Transaction) {
        wakeUp()
    }

    private fun wakeUp() {
        direction = -direction
        position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
        velocity = Velocity(direction, 0.0) * speed
        elapsedTime = 0.0
    }

Right, I remember that. One reason why we have finalize is that things can be removed in more than one way. Saucer is no exception:

    override fun update(deltaTime: Double, trans: Transaction) {
        elapsedTime += deltaTime
        if (elapsedTime > U.SAUCER_LIFETIME) trans.remove(this)
        timeSinceSaucerSeen += deltaTime
        if (timeSinceSaucerSeen > 1.5) zigZag()
        timeSinceLastMissileFired += deltaTime
        if (timeSinceLastMissileFired > 0.5) fire(trans)
        position = (position + velocity * deltaTime).cap()
    }

    private fun checkCollision(collider: Collider, trans: Transaction) {
        if (Collision(collider).hit(this)) {
            trans.add(Splat(this))
            trans.remove(this)
        }
    }

Can we do wakeUp on removal? No. I made that mistake already this week. If we were to do that, then Saucer would change its location in the middle of interactions, and if whatever it was colliding with had not been processed, that object would no longer be colliding with the saucer. The effect of that defect was that the saucer would destroy itself against an asteroid, but the asteroid would not break. We want the asteroid to break. So we can’t do wakeUp when we remove the saucer.

Game puts it back it uses this OneShot:

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

I reckon we can put the wake up call there. We should extract a little method, perhaps.

    private val saucerOneShot = OneShot( 7.0) { startSaucer(it) }

    private fun startSaucer(it: Transaction) {
        saucer.wakeUp()
        it.add(saucer)
    }

I like that. IDEA politely makes wakeUp public for me. Remove it from finalize.

I believe some tests will now fail, if memory serves. Yes, two:

    @Test
    fun `right left next time`() {
        val saucer = Saucer()
        val trans = Transaction()
        saucer.subscriptions.finalize(trans)
        assertThat(saucer.velocity.x).isLessThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `direction changes maintain left-right`() {
        val saucer = Saucer() // left to right
        val trans = Transaction()
        saucer.subscriptions.finalize(trans) // right to left
        saucer.zigZag()
        assertThat(saucer.velocity.x).isLessThan(0.0)
    }

I think we’ll just call wakeUp on these. Well, we could call our new method on Game, startSaucer. That’s better. But to do that, we’d have to set up a game with all that jazz. I know. Let’s make Game forward to Saucer to do the starting.

class Game
    private fun startSaucer(trans: Transaction) {
        saucer.start(trans)
    }

Now IDEA wants to make a method for me.

class Saucer
    fun start(trans: Transaction) {
        wakeUp()
        trans.add(this)
    }

Right. Now we can change the tests to call start, but they will still fail, I think, moving in the wrong direction.

    @Test
    fun `right left next time`() {
        val saucer = Saucer()
        val trans = Transaction()
        saucer.start(trans)
        assertThat(saucer.velocity.x).isLessThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `direction changes maintain left-right`() {
        val saucer = Saucer() // left to right
        val trans = Transaction()
        saucer.start(trans) // right to left
        saucer.zigZag()
        assertThat(saucer.velocity.x).isLessThan(0.0)
    }

Test, expecting both to fail their “isLessThan”. I could be wrong. I am wrong. They pass. Why? I don’t think we care. However, when I try the game, the saucer starts right to left. I really want it to start left to right. I’m not sure why, and I actually think we should randomize it, but it’s a change. We don’t want changes. Change these tests to isGreaterThan, let them fail.

Fix the initialization in Saucer:

    fun initialize() {
        direction = -1.0
        wakeUp()
    }

Change to:

    fun initialize() {
        direction = 1.0
        wakeUp()
    }

Test. Those tests pass and another fails. Weird, what is it?

    @Test
    fun `starts left-right`() {
        val saucer = Saucer()
        assertThat(saucer.velocity.x).isGreaterThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

Well, huh. You know, we could do this another way. Let’s invert direction after using it.

No, bad idea. That messes up zigzag. I’ll revert my tests and see what we have. No, I’m going to back out a few other changes in Saucer. IDEA and Git will help me with that.

I leave the start method and the empty finalize. That gets me back to where I started fiddling. This time less fiddling. Those same tests fail, but let’s consider their sibling as well. Together they tell the story:

    @Test
    fun `starts left-right`() {
        val saucer = Saucer()
        assertThat(saucer.velocity.x).isGreaterThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `right left next time`() {
        val saucer = Saucer()
        val trans = Transaction()
        saucer.subscriptions.finalize(trans)
        assertThat(saucer.velocity.x).isLessThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `direction changes maintain left-right`() {
        val saucer = Saucer() // left to right
        val trans = Transaction()
        saucer.subscriptions.finalize(trans) // right to left
        saucer.zigZag()
        assertThat(saucer.velocity.x).isLessThan(0.0)
    }

Along the way here, I got another concern, which is that in Game, we do this:

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

class Saucer
    fun initialize() {
        direction = -1.0
        wakeUp()
    }

That sets direction left to right, by setting it right to left and letting wakeUp reverse it. If our tests are going to give us consistent results, we should write them to be consistent with each other and with the way the game uses the functions. I think all three of these ought to fail now.

    @Test
    fun `starts left-right on first start`() {
        val saucer = Saucer()
        saucer.start(Transaction())
        assertThat(saucer.velocity.x).isGreaterThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `right left second start`() {
        val saucer = Saucer()
        saucer.start(Transaction())
        saucer.start(Transaction())
        assertThat(saucer.velocity.x).isLessThan(0.0)
        assertThat(saucer.velocity.y).isEqualTo(0.0)
    }

    @Test
    fun `direction changes maintain left-right`() {
        val saucer = Saucer()
        saucer.start(Transaction()) 
        saucer.start(Transaction())
        saucer.zigZag()
        assertThat(saucer.velocity.x).isLessThan(0.0)
    }

They do. Now we change the direction set in initialize and we should be good.

    fun initialize() {
        direction = 1.0
        wakeUp()
    }

Green. I’ll draw out a lesson for myself in a moment. Short form: creation starting init.

First we have stuff to complete. Commit: finalize is empty, wakeUp now done in start.

Now a break. There’s something in my eye.

OK. Next step will be to remove finalize entirely, but first, what was that confusion with the tests telling us. I think there are two things:

Tests were inconsistent
The tests were not consistent with respect to our new changes. It only took one finalize to reverse a saucer, but it takes two starts. So changing the latter two to have two starts sorted things out. So we needed to make them consistent. They were OK in the past. I just didn’t change them correctly when things changed.

And that makes me think there’s another issue.

Muzzy ideas?
I think there’s something muzzy in the area of initialize, start, and wakeUp on Saucer. The notion of reversing direction is in wakeUp. Both initialize and start call wakeUp. Let’s think about this a bit, just to see if we can learn something.
class Saucer
    fun initialize() {
        direction = 1.0
        wakeUp()
    }
    
    fun start(trans: Transaction) {
        wakeUp()
        trans.add(this)
    }

    private fun wakeUp() {
        direction = -direction
        position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
        velocity = Velocity(direction, 0.0) * speed
        elapsedTime = 0.0
    }

What is odd about this is that we initialize direction and then immediately reverse it. It is also odd that when we start the saucer, we reverse it again. Let’s see if we can make more sense of this. Extract another method:

    fun initialize() {
        direction = 1.0
        wakeUp()
    }

    fun start(trans: Transaction) {
        wakeUp()
        trans.add(this)
    }

    private fun wakeUp() {
        direction = -direction
        setStartupValues()
    }

    private fun setStartupValues() {
        position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
        velocity = Velocity(direction, 0.0) * speed
        elapsedTime = 0.0
    }

Now let’s ask ourselves, should initialize really be calling wakeUp, or should it call setStartupValues? I think it’s the latter. And I think it’ll break all three of those tests. And it does. Now we have this:

    fun initialize() {
        direction = 1.0
        setStartupValues()
    }

    fun start(trans: Transaction) {
        wakeUp()
        trans.add(this)
    }

    private fun wakeUp() {
        direction = -direction
        setStartupValues()
    }

    private fun setStartupValues() {
        position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
        velocity = Velocity(direction, 0.0) * speed
        elapsedTime = 0.0
    }

There’s another issue, the class’s init:

    init {
        initialize()
    }

I broke that out so that someone could call it. Why? To avoid a lateinit on the Saucer instance in Game. I don’t love that but I like it better than creating a new saucer when I create a new game. Maybe I shouldn’t like that better. Let’s do it:

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

Now we can move initialize back inside init:

    init {
        direction = 1.0
        setStartupValues()
    }

    fun start(trans: Transaction) {
        wakeUp()
        trans.add(this)
    }

    private fun wakeUp() {
        direction = -direction
        setStartupValues()
    }

    private fun setStartupValues() {
        position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
        velocity = Velocity(direction, 0.0) * speed
        elapsedTime = 0.0
    }

How do the tests feel? Probably all three broken still. Right. But I think that setting direction opposite should fix them all.

    init {
        direction = -1.0
        setStartupValues()
    }

Yes. Green. Commit: refactor to remove initialize and better organize starting.

OK. The oddness there was trying to tell me something. I think things are a bit better, though there is something inherently weird about reversing direction before you ever start running.

Anyway we are here to remove our finalize from Saucer:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithShip = { ship, trans ->
            sawShip = true
            shipFuturePosition = ship.position + ship.velocity * 1.5
            checkCollision(ship, trans)
        },
        interactWithMissile = { missile, trans ->
            if (missile == currentMissile) missileReady = false
            checkCollision(missile, trans)
        },
        finalize = this::finalize
    )

    private fun finalize(trans: Transaction) {
    }

Remove the function and the final line of subscriptions. Test. Green. Commit: remove finalize reference. Last in the system?

Check Subscriptions for references. There are three, two in Transaction and one test:

    @Test
    fun `ships do not split on finalize`() {
        val ship = Ship(
            position = Vector2(100.0, 100.0)
        )
        val trans = Transaction()
        ship.subscriptions.finalize(trans)
        val didShipSplit = trans.adds
        assertThat(didShipSplit).isEmpty()
    }

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        removes.forEach { it.subscriptions.finalize(this)}
        deferredActionRemoves.forEach { it.subscriptions.finalize(this)}
        removes.forEach { spaceObjectCollection.remove(it)}
        deferredActionRemoves.forEach { spaceObjectCollection.remove(it)}
        adds.forEach { spaceObjectCollection.add(it)}
        deferredActionAdds.forEach { spaceObjectCollection.add(it)}
    }

The test seems ludicrous. Ships don’t split. I’ll remove that entirely. The two calls to finalize are also right out.

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        removes.forEach { spaceObjectCollection.remove(it)}
        deferredActionRemoves.forEach { spaceObjectCollection.remove(it)}
        adds.forEach { spaceObjectCollection.add(it)}
        deferredActionAdds.forEach { spaceObjectCollection.add(it)}
    }

We should be green. Indeed. Commit: remove all calls to finalize.

Now we can remove finalize from Subscriptions:

class Subscriptions(
    val interactWithAsteroid: (asteroid: Asteroid, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithMissile: (missile: Missile, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithSaucer: (saucer: Saucer, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithShip: (ship: Ship, trans: Transaction) -> Unit = { _, _, -> },

    val draw: (drawer: Drawer) -> Unit = {_ -> },
    val finalize: (trans: Transaction) -> Unit = { }
)

That last line goes. Test. Green. Commit: finalize removed.

I’ll celebrate with a short game. Short because I’m still not very good at this.

Summary

So that was interesting. It wasn’t difficult at all, except that the combination of moving reversal to the beginning of the saucer’s run instead of the end caused a few tests to break, and that was exacerbated by the use of initialize to avoid creating a new saucer. Creating a new saucer is inexpensive, happens only when we create a game, and is a more reasonable thing to do.

Do you have a lessón for your minkey?

There was a lesson for me, and I’ll underline it here: When I get confused about something, as I did with the saucer direction tests, odds are that the code is confusing me, and that means odds are that the code could be better. Often, once I’m unconfused, or muddle my way through, I back away slowly. It’s better to improve the code, even at the cost of a few more minutes of work.

How about you? Same? Maybe. Maybe not.

All this does leave me with one concern, which is that the Game object has almost all of its member variables as var. It would be better if it were immutable, because things always go better when objects are immutable.

class Game(val knownObjects:SpaceObjectCollection = SpaceObjectCollection()) {
    private var lastTime = 0.0
    private var numberOfAsteroidsToCreate = 0
    private var saucer = Saucer()
    private lateinit var ship: Ship
    private var scoreKeeper: ScoreKeeper = ScoreKeeper(-1)

    private val waveOneShot = OneShot(4.0) { makeWave(it) }
    private val saucerOneShot = OneShot( 7.0) { startSaucer(it) }

I think we could do better than that. I’ll add a note to Jira, which, you’ll recall, is the name of the keyboard tray of my desk, where I stick small yellow notes of things to do.

We are now down to just this in Subscriptions:

class Subscriptions(
    val interactWithAsteroid: (asteroid: Asteroid, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithMissile: (missile: Missile, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithSaucer: (saucer: Saucer, trans: Transaction) -> Unit = { _, _, -> },
    val interactWithShip: (ship: Ship, trans: Transaction) -> Unit = { _, _, -> },

    val draw: (drawer: Drawer) -> Unit = {_ -> },
)

If, just if, we were to change the interaction processing in Game to process the separate collections in knownObjects separately, it could call those functions directly, with the correct types. And the functions could be pulled out of the blocks they are now in.

Let me show you what I mean. Asteroid looks like this today:

    override val subscriptions = Subscriptions(
        interactWithMissile = { missile, trans -> dieIfColliding(missile, trans) },
        interactWithShip = { ship, trans -> dieIfColliding(ship, trans) },
        interactWithSaucer = { saucer, trans -> dieIfColliding(saucer, trans) },
        draw = this::draw,
    )

    private fun dieIfColliding(missile: Missile, trans: Transaction) {
        if (Collision(missile).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

    private fun dieIfColliding(ship: Ship, trans: Transaction) {
        if (Collision(ship).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

    private fun dieIfColliding(saucer: Saucer, trans: Transaction) {
        if (Collision(saucer).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

I think we can rename those die methods, like this:

    override val subscriptions = Subscriptions(
        interactWithMissile = { missile, trans -> interactWithMissile(missile, trans) },
        interactWithShip = { ship, trans -> interactWithMissile(ship, trans) },
        interactWithSaucer = { saucer, trans -> interactWithMissile(saucer, trans) },
        draw = this::draw,
    )

    private fun interactWithMissile(missile: Missile, trans: Transaction) {
        if (Collision(missile).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

    private fun interactWithMissile(ship: Ship, trans: Transaction) {
        if (Collision(ship).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

    private fun interactWithMissile(saucer: Saucer, trans: Transaction) {
        if (Collision(saucer).hit(this)) {
            dieDuetoCollision(trans)
        }
    }

Now if I’d make those public, Game could call them directly, and Asteroid would no longer need anything except draw in subscriptions. And if Game would call that directly, we wouldn’t need subscriptions at all.

So that’s where we’re going. I am so tempted to do it in one big sweep, but I think we’ll do better with smaller steps, and I need the practice anyway: I so often do more than I need to to get to a commit point. I truly believe, based on my own experience, that the fewer steps I take between commits, the faster I get things done. But my habits are old and ingrained. So I need to practice pushing to smaller steps.

Test this rename, though it has to work. LLewellyn Falco and Arlo Belshee, and probably James Shore, would all tell me that I can commit a machine refactoring with impunity. My gut doesn’t believe them, and the tests take less time to run than typing this paragraph did.

Test. Green. Commit: rename methods to interactWith....

We’ll wrap this up and ship the article. Next time: interaction. Probably a spike, because we’ll need to undo that full pairs thing. Or … maybe we can make the changes in there? We’ll see.

I hope you’ll stop by next time!