GitHub Decentralized Repo
GitHub Centralized Repo

Today, probably, we’ll push on with making things more cognizant of type. Probably in Transaction. But let’s think first. (Added in Post: we don’t do what I thought.)

Looking Forward

I think that when we’re done with this, we’ll have removed most, ideally all, of the inheritance in the SpaceObject, and probably Collider, interfaces. We’ll have [almost?] every call in the system knowing the exact type of all its arguments. Then we’ll assess what we think of it. We’ll be assessing as we go, of course, but part of my intention here is to really bear down on the idea I got from Hill earlier this week.

Seen from today, I think the changes will involve things like this:

  • Make the Transaction sort adds, and possibly removes, into separate collections by class, using the class-specific calls in SpaceObjectCollection to build the class-specific collections there.

  • Ensure that each of Asteroid, Missile, Ship, and Saucer have specific methods for interacting with anything they interact with: interactWithMissile, and so on. The objects already have the code for doing that, but often it is just embedded in a lambda in subscriptions today, so we’ll need to extract methods.

  • Change the interaction code in Game class, from the current begin / interact / finish style, to iterate class-specific collections and call the proper class-specific methods.

  • Remove the need for callOther and subscriptions in the SpaceObject interface.

  • Iterate the class-specific collections to call class-specific update, eliminating the need for update in the SpaceObject interface. When both of these steps are done, the interface can be emptied.

  • Deal somehow with the Collider interface, which imposes the requirement to know position and kill radius on its implementors.

It is worth remembering that the individual class-specific collections are now fully operational inside SpaceObjectCollection. That means that we could defer updating Transaction to preserve class, because we sort it out inside SpaceObjectCollection, at the cost of a when:

    fun add(spaceObject: SpaceObject) {
        when (spaceObject) {
            is Missile -> add(spaceObject)
            is Asteroid -> add(spaceObject)
            is Ship -> add(spaceObject)
            is Saucer -> add(spaceObject)
            is Splat -> add(spaceObject)
        }
    }

    fun add(asteroid: Asteroid) { asteroids.add(asteroid) }
    fun add(missile: Missile) { missiles.add(missile) }
    fun add(saucer: Saucer) { saucers.add(saucer) }
    fun add(ship: Ship) { ships.add(ship) }
    fun add(splat: Splat) { splats.add(splat) }

When Transaction is improved, there will be no hand-coded dispatch on class, but the effect will be the same.

So we could work now on changing the interaction code. Let’s at least explore where we stand.

Current Interactions

Objects can subscribe to beforeInteraction, interactWithXXX, and afterInteraction, where XXX is some specific class such as interactWithMissile, and so on. They can also subscribe to draw and finalize. Let’s see what they actually do:

class Asteroid
    override val subscriptions = Subscriptions(
        interactWithMissile = { missile, trans -> dieIfColliding(missile, trans) },
        interactWithShip = { ship, trans -> if (Collision(ship).hit(this)) trans.remove(this) },
        interactWithSaucer = { saucer, trans -> if (Collision(saucer).hit(this)) trans.remove(this) },
        draw = this::draw,
        finalize = this::finalize
    )

class Missile
    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans -> ...
        interactWithSaucer = { saucer, trans -> ...
        interactWithShip = { ship, trans -> ...
        interactWithMissile = { missile, trans -> ...
        draw = this::draw,
    )

class Saucer
    override val subscriptions = Subscriptions(
        draw = this::draw,
        beforeInteractions = { sawShip = false; missileReady = true },
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithShip = { ship, trans -> ...
        interactWithMissile = { missile, trans -> ...
        finalize = this::finalize
    )

class Ship
    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithSaucer = { saucer, trans -> checkCollision(saucer, trans) },
        interactWithMissile = { missile, trans -> checkCollision(missile, trans) },
        draw = this::draw,
        finalize = this::finalizeObject
    )

class Splat
    override val subscriptions = Subscriptions(draw = this::draw)

I’ve left out the details. All we really care about is which capabilities they implement. The others are all defaulted.

What else is there? DeferredAction is the only other one. What does it subscribe to?

class DeferredAction
    override val subscriptions: Subscriptions = Subscriptions()

It subscribes to nothing at all. Sweet! This says that we can remove the use of beforeInteraction and afterInteraction right now.

It’s always time to lose a little weight here in the code, so let’s have a look at subscriptions and see if we can remove those two.

class Subscriptions(
    val beforeInteractions: () -> Unit = {},

    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 afterInteractions: (trans: Transaction) -> Unit = {_ -> },

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

I don’t see anything stopping us from removing these, and in fact I suspect we could remove others, breaking the system but not bothering Kotlin or IDEA. Let’s remove both those and see how our tests respond. Should be just fine.

Ah, not quite. Game refers to them. Oops.

class Game
    fun cycle(elapsedSeconds: Double, drawer: Drawer? = null) {
        val deltaTime = elapsedSeconds - lastTime
        lastTime = elapsedSeconds
        tick(deltaTime)
        beginInteractions()
        processInteractions()
        finishInteractions()
        U.AsteroidTally = knownObjects.asteroidCount()
        createNewWaveIfNeeded()
        createSaucerIfNeeded()
        createShipIfNeeded()
        drawer?.let { draw(drawer) }
    }

    private fun beginInteractions() = knownObjects.forEachInteracting { it.subscriptions.beforeInteractions() }

    private fun finishInteractions() {
        val buffer = Transaction()
        knownObjects.forEachInteracting { it.subscriptions.afterInteractions(buffer) }
        knownObjects.applyChanges(buffer)
    }

Fine, remove those calls and functions.

Oops again. I didn’t notice that Saucer uses the before for its ship flag. Right there in front of me. Let's change Saucer:

No, let’s roll back and go a bit more carefully.

Here’s what goes on in Saucer:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        beforeInteractions = { sawShip = false; missileReady = true },
        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
    )

Rather than just paste it for our pleasure, we’ll actually pay attention this time.

Saucer wants to know if the ship is present. It uses that information to decide whether to fire a missile. It also wants to know whether the missile it previously fired is still in flight: if it is, it will be seen in interactWithMissile.

Saucer doesn’t have access to Game or knownObjects, so it can’t readily check to see if the ship and missile are around, other than by observing that it sees them during interaction. It follows that Saucer probably really does need beginInteractions to initialize itself.

What about afterInteractions? Can we remove that with impunity? Let’s try. Tests are green, game works. Commit: afterInteractions removed from subscriptions and everywhere else.

So that seemed like an odd digression from our plan. Let’s talk about that.

Reflection

We started out doing some large-ish planning, looking at what will be at least a few days’ work, and what might require a longer period in a larger program. Then, in our studies, we noticed something that wasn’t very big and that could be an immediate improvement. Even better, the improvement was in a direction we want to go longer term, namely eliminating the subscriptions model.

One quick experiment showed that we couldn’t do both of the things we had noticed, but that we could do one of the two. So we did it, tested, and committed it.

Was this a proper use of our time? I would argue that if it had taken us days, and caused us to fail to ship a feature or let the side down in some other way, it would be bad. But small changes, made safely and quickly, to improve the code, are always worth doing. If we always take small opportunities, the code will improve and our pace of delivery will not slow down, for two reasons:

  • First, we always use a bit of time to make the code better, so our average speed remains the same;
  • Second, we’re making the code better, and with better code we go faster. Our average speed may actually improve.

You do you, but I think the wise developer and wise team are always alert for small improvements, and make them when they arise.

Back to Our Plan

I think we’re deciding whether to work on Transaction, or to move directly to moving things out of subscriptions and calling them directly. I’m leaning, and I’m sorry, but I’m the only one here to vote, I’m leaning toward going after the bigger game. We know that the dispatch works, and it’s clearly not slowing down the game. We’re not running a 6502 here, we have a computer that could probably run Asteroids for the entire state of Michigan.

So let’s do it.

Remove beforeInteractions

There’s only one object that wants to do beginInteractions. Let’s just do that. In Game, we have this:

    private fun beginInteractions() 
    	= knownObjects.forEachInteracting { 
    		it.subscriptions.beforeInteractions() }

No wonder I’m always confused over whether it’s begin or before! I’ve used both. Tiny fool!

Let’s rename the method. OK, now it’s beforeInteractions. We only want to send to the Saucer, so this should work just fine:

    private fun beforeInteractions() 
    	= knownObjects.saucers.forEach { 
    		it.subscriptions.beforeInteractions() }

This will do zero or one saucers. This is why it was a good idea to keep ship and saucer in zero- or one-element collections: it’s handy for things like this.

I expect green and a good game. I get it. Commit: Only saucer gets beforeInteractions

Now in Saucer, let’s give ourselves a method, extracting from this:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        beforeInteractions = { sawShip = false; missileReady = true },

To get this:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        beforeInteractions = { beforeInteractions() },

    fun beforeInteractions() {
        sawShip = false; missileReady = true
    }

Test. No, really. Green. Commit: break out beforeInteractions method.

Why? Because we can. We’ve made a change, the tests are green, Many More Much Smaller Steps. Thanks, Hill.

Now we can call that method directly from Game:

    private fun beforeInteractions() 
    	= knownObjects.saucers.forEach { 
    		it.beforeInteractions() }

Test. Green. Commit: Game calls beforeInteractions directly on Saucer.

Now we should be able to remove the thing from subscriptions … but not before fixing some tests.

I remove it from Subscriptions and the reference from Saucer. Now some tests complain, like this:

    @Test
    fun `saucer knows if ship present`() {
        val saucer = Saucer()
        saucer.subscriptions.beforeInteractions()
        assertThat(saucer.sawShip).isEqualTo(false)
        saucer.subscriptions.interactWithShip(Ship(U.CENTER_OF_UNIVERSE), Transaction())
        assertThat(saucer.sawShip).isEqualTo(true)
    }

They no longer need to defer through subscriptions. Two tests need that treatment. We are green. Commit: Remove beforeInteractions from Subscriptions. Adjust tests.

Let’s reflect.

Reflection

So. We have our first taste of what it takes to remove something from Subscriptions. We had to

  • Ensure that a method of the correct name was implemented on the classes using it in subscriptions;
  • Ensure that game called that method directly;
  • Adjust some tests;
  • Remove the name from Subscriptions and everyone’s override of subscriptions.

When the last one goes, we will of course be able to remove Subscriptions itself and the need to have it in the SpaceObject interface.

It’s a bit intricate. There are individual steps that can be committed, however, such as breaking out the methods.

Shall we look at another? We’re only 90 minutes in. Let’s see about finalize. It might be easy. And I think draw may turn out to be easy as well.

Finalize

Who does finalize?

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

class Asteroid
    private fun finalize(trans: Transaction) {
        if (splitCount >= 1) {
            trans.add(asSplit(this))
            trans.add(asSplit(this))
        }
    }

class Ship
    fun finalizeObject(trans: Transaction) {
        position = U.CENTER_OF_UNIVERSE
        velocity = Velocity.ZERO
        heading = 0.0
    }

Why did I name that finalizeObject? Rename to finalize, test, commit: rename finalizeObject to finalize.

About time. What about these finalize folx tho? Both Ship and Saucer are using it to get ready for the next time they emerge. I don’t think there’s a better time to do that.

Finalize is sent by Transaction, during applyChanges when something is being removed:

    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)}
    }

It turns out that DeferredAction doesn’t implement finalize. Some of the others do, but not all. This will be easier when Transaction has the classes broken out. Even then it’ll be a bit odd. Frankly I would like it better if all the flying space objects implemented finalize, even if they ignore it. That would require Splat and Missile to implement it.

An alternative would be to find places to do what is done in finalize elsewhere in the code.

Let’s explore that before we give up the quest for finalize.

Ships that are shot down call collision:

    fun collision(trans: Transaction) {
        trans.add(Splat(this))
        trans.remove(this)
    }

We already deal with hyperspace elsewhere, so we could init the location here just fine. We can try that in a minute if the others make sense. I think there will be tests that get unhappy, but we’ll see.

In Saucer:

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

Again, no reason we couldn’t do the wakeUp there.

Asteroid:

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

Sure, we could do the splitting there.

I say we try it, one at a time, just moving the code out of finalize and up to the place where the object removes itself.

Saucer is easiest.

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

Test, kind of expecting trouble, and we’ll also try the game. A couple of tests do fail. I’ll try the game: I think it should work. As I expected, the game works as advertised. Check the tests:

    @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)
    }

Right. These need to be recast somehow. Let’s do this. Extract another method in Saucer:

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

    fun dieDueToCollision(trans: Transaction) {
        trans.add(Splat(this))
        trans.remove(this)
        wakeUp()
    }

And call that in the tests.

    @Test
    fun `right left next time`() {
        val saucer = Saucer()
        val trans = Transaction()
        saucer.dieDueToCollision(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.dieDueToCollision(trans) // right to left
        saucer.zigZag()
        assertThat(saucer.velocity.x).isLessThan(0.0)
    }

Expect those to be green. There is one more, however.

    @Test
    fun `saucer asteroid collision`() {
        val saucer = Saucer() // kr = 10
        saucer.position = Point(73.0, 0.0)
        val asteroid = Asteroid(Point.ZERO) // kr = 64 tot = 74, test 73
        val trans = Transaction()
        saucer.subscriptions.interactWithAsteroid(asteroid, trans)
        asteroid.subscriptions.interactWithSaucer(saucer, trans)
        assertThat(trans.removes).contains(asteroid)
        assertThat(trans.removes).contains(saucer)
    }

That’s failing to find the asteroid in there. That’s odd. And suddenly, when I test the game, the Saucer flies once and seems never to come back. I do not understand. Roll back.

This was supposed to be easy. What happened? wakeUp just does this:

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

Certainly if elapsedTime kept getting zeroed, we’d never come back. Or if Game thought that the saucer wasn’t missing. How is that checked? I thought it was direct:

class SpaceObjectCollection
    fun saucerIsMissing(): Boolean {
        return saucers.isEmpty()
    }

I suppose saucers could fail to be empty but I don’t see how it could then not draw the saucer. Well …

    private fun draw(drawer: Drawer) {
        knownObjects.forEachInteracting { drawer.isolated { it.subscriptions.draw(drawer) } }
        knownObjects.scoreKeeper.draw(drawer)
    }

That’s a bit iffy now. What is that method?

    fun forEachInteracting(action: (SpaceObject)->Unit) =
        spaceObjects().forEach(action)
    fun spaceObjects():List<SpaceObject> = asteroids + missiles + saucers + ships + splats

I’m pretty sure we’re not failing to draw it. Ah! I see:

    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()
    }

When we time out, we just remove the saucer, relying on finalize to give it the wakeup call. So its elapsed time doesn’t get zeroed and as soon as the game starts it, it gets timed out again.

Let’s refactor a bit.

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

    private fun dieDueToCollision(trans: Transaction) {
        trans.add(Splat(this))
        removeSaucer(trans)
    }

    private fun removeSaucer(trans: Transaction) {
        trans.remove(this)
        wakeUp()
    }

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

    private fun finalize(trans: Transaction) {
    }

The tests will fail again but does the game work? Well, no, but in an odd way. the Saucer isn’t splitting asteroids. Oh, I see why. No, actually, I don’t. Roll back again, prepared to give up.

Ah. I do see why. The saucer has changed its location in wakeUp. If the asteroid is checked after the saucer decides to die, it no longer sees the collision and is preserved.

Nasty. This makes me think that we’re going to want to preserve the finalize notion, because the Ship is also resetting its position. Either that … or we have to do the wakeup call when we create the things. We could do that. Let’s consider it, but I think we’re done for the morning.

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

Maybe in the saucerOneShot?

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

Yes, we could certainly do it here. The only thing that would change is that the saucer direction would be toggled one extra time, making it come in from the right rather than the left. We could randomize that or init it the other way.

Would this same idea work for the Ship?

    private val shipOneShot = OneShot(U.SHIP_MAKER_DELAY, { canShipEmerge() }) {
       if ( scoreKeeper.takeShip() ) it.add(ship)
    }

I think we could. Ship.finalize just does this:

    fun finalize(trans: Transaction) {
        position = U.CENTER_OF_UNIVERSE
        velocity = Velocity.ZERO
        heading = 0.0
    }

That’s just fine, I think.

Let’s summarize our learning and get out of here.

Summary

Learning One

When you try something three times and it doesn’t work, you’re missing something, and even if you figure it out, it’s time for a break. I am quite sure I could make Saucer.finalize work right now, but I think I’m smart enough to wait until I’m fresh.

Learning Two

We can very likely remove finalize by moving initialization of Ship and Saucer to when they are created, and creating the new Asteroids at the same time as we remove the old one.

Learning Three

We can almost certainly tick through the objects one at a time to remove finalize, but we may have to change interaction to use more specific looping. I should explain that, because I haven’t mentioned it before.

Interactions now proceed by interacting all pairs of objects. We don’t really need that, because the emergent behavior of the specials has all been moved out, as have those objects. That simplifies things a lot. We can, instead, take advantage of knowing which of the flying objects interacts with which, and do the interactions more directly, something like this:

    for ( missile in knownObjects.missiles ) {
    	for (ship in knownObjects.ships ) {
    		missile.interactWithShip(ship)
    		ship.interactWithMissile(missile)
    	}
    	for (saucer in knownObjects.saucers ) {
    		missile.interactWithShip(saucer)
    		saucer.interactWithMissile(missile)
    	}
    	for (asteroid in knownObjects.asteroids ) {
    		missile.interactWithShip(asteroid)
    		asteroid.interactWithMissile(missile)
    	}
    }

There will be more: we have to interact ship and saucer with asteroids. All that will certainly create more code, probably with what looks like duplication, but in the end allow our objects and hierarchy to be substantially simpler. I believe it will be worth the change, but we’re doing this, in large part, to find out whether we like where it all leads.

Summary Findings

We need to remember that we actually accomplished something this morning. We did remove the beforeInteractions entirely from subscriptions, and in addition we made a couple of small name improvements.

More important, I think, is the learning summarized above. I think we’re much better positioned to start unwinding the subscriptions and interactions. I think we’ll start with finalize, expecting to get it right this time.

Small steps are great

This work is proceeding in very small steps, and we are keeping the program working properly on every commit. What this shows us is that — at least in this case — this very large refactoring can quite readily be done in a long series of very small, safe steps, each one leaving the program just a bit better designed, and still working.

Does this generalize?

Does this idea generalize? I think it does. I also think that it’s fair to say that it relies on there being decent tests, even if we have to create them. And it’s surely easier if the design we have has been kept alive by continual refactoring, so that while it may not be the design we want, it’s a design that deserves the name “design”. If the code is horrible … well, we know how to refactor from horrible to less horrible, so I think we can still do it. It might take a bit long.

In my view, it will take less time, and deliver more value than almost any “rewrite”. It’s often tempting to rewrite a program, but in my long experience, it doesn’t pay off as often as we think.

Should you be like me?

I’m not saying yes, I’m not saying no. Your mileage may vary, your decisions are yours. These are mine. You get to watch me make mistakes and fix them, and to think about what, if anything it means for you.

If you’re still here, I guess it’s at least amusing. See you next time!