GitHub Decentralized Repo
GitHub Centralized Repo

We’re engaged in a somewhat extended diversion in our centralization effort, aimed at making better use of Kotlin’s strict typing. Let me explain.

“No, there is too much. Let me sum up.”

The overall flow of this small effort inside the larger decentralizing effort is to use overloaded add methods to maintain the different classes of SpaceObject in separate collections in Transaction and SpaceObjectCollection. Then, with that advantage, we should be able to remove the subscription model entirely, and we will probably be able to remove all common interfaces from the system,, allowing Asteroid, Missile, Saucer, and Ship to have similar interfaces without sticking them under a common superclass.

We’ll assess later whether we might actually want the interface, merely as a way to document the common aspects of these objects.

Let’s get to it

I guess I’ll just repeat what we did yesterday for each of the other classes that make up SpaceObject, creating unique collections in Transaction and SpaceObjectCollection, with an eye to eliminating this branching code:

    private fun addActualSpaceObjects(spaceObject: SpaceObject) {
        spaceObjects.add(spaceObject)
        when (spaceObject) {
            is Missile -> attackers.add(spaceObject)
            is Asteroid -> targets.add(spaceObject)
            is Ship -> {
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
            is Saucer -> {
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
        }
    }

For now, we’ll maintain the larger collections, spaceObjects, attackers, and targets, because we use them, and our changes can be smaller if we eliminate their use incrementally over time.

Aside
In a much larger system, this kind of refactoring could take place over weeks, even months, as we slowly bring the system into alignment with a better design vision. Here, we can do it all in days, but it’s the same process: We build up some auxiliary structures, then incrementally move over to using them, and then, one day, the old stuff is no longer needed.

What we might do, I haven’t decided yet, could be to make the larger collections virtual, so that attackers = missiles + ship + saucer, and so on. One way or another, we want to move incrementally toward only having the individual collections.

Added in Post
Below, it turns out that we do make spaceObjects virtual. It goes well.

It seems to me that we have a dispatch in SpaceObjectCollection, as shown above, and that therefore we can probably put in the type-specific versions of add right now. We’ll have to do something about removing things as well. Let’s try this idea: if it doesn’t work, our existing tests should tell us.

I do this:

    fun add(missile: Missile) {
        missiles.add(missile)
        attackers.add(missile)
    }

    private fun addActualSpaceObjects(spaceObject: SpaceObject) {
        spaceObjects.add(spaceObject)
        when (spaceObject) {
            is Missile -> add(spaceObject)
            ...

Sadly, IDEA can’t do that as an extract: It’s not quite smart enough to know that it can change the parameter type. No problem, it’s easily done by hand. Naturally, I’ve also added this:

    val missiles = mutableListOf<Missile>()

We can test this. Hm, a test fails:

expected: true
 but was: false

Not as informative as one might like. Here’s the test:

    @Test
    fun `missile and splat death`() {
        val mix = SpaceObjectCollection()
        val ship = Ship(
            position = U.randomPoint()
        )
        val missile = Missile(ship)
        mix.add(missile)
        val game = Game(mix)
        assertThat(mix.contains(missile)).isEqualTo(true)
        game.cycle(0.0)
        assertThat(mix.deferredActions.any { it is DeferredAction }).describedAs("deferred action should be present").isEqualTo(true)
        game.cycle(U.MISSILE_LIFETIME + 0.1)
        assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
        assertThat(mix.any { it is Splat }).describedAs("splat should be present").isEqualTo(true)
        game.cycle(U.MISSILE_LIFETIME + 0.2) // needs a tick to init
        game.cycle(U.MISSILE_LIFETIME + 2.3) // Splat lifetime is 2.0
        assertThat(mix.any { it is Splat }).describedAs("splat should be gone").isEqualTo(false)
    }

Ah, that’s interesting! The direct add was fired, because of our new method, and our new method does not add to spaceObject … it’s done at the top of our dispatching method:

    private fun addActualSpaceObjects(spaceObject: SpaceObject) {
        spaceObjects.add(spaceObject)
        when (spaceObject) {
            is Missile -> add(spaceObject)
            is Asteroid -> targets.add(spaceObject)
            is Ship -> {
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
            is Saucer -> {
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
        }
    }

We’ll make a point of adding to spaceObjects in all the new adds. I’ll ensure that by moving the call down into the branches:

    private fun addActualSpaceObjects(spaceObject: SpaceObject) {
        when (spaceObject) {
            is Missile -> add(spaceObject)
            is Asteroid -> {
                spaceObjects.add(spaceObject)
                targets.add(spaceObject)
            }
            is Ship -> {
                spaceObjects.add(spaceObject)
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
            is Saucer -> {
                spaceObjects.add(spaceObject)
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
        }
    }

We should be green now: I’ve added the spaceObjects insertion to my add:

    fun add(missile: Missile) {
        spaceObjects.add(missile)
        missiles.add(missile)
        attackers.add(missile)
    }

Oh my! I forgot that Splat is a space object. We have to cater to it:

    is Splat -> {
        spaceObjects.add(spaceObject)
    }

Now we’re green. Commit: missiles now added to missiles collection via typed add function.

I think we’d best deal with removal now.

    fun remove(spaceObject: SpaceObject) {
        spaceObjects.remove(spaceObject)
        attackers.remove(spaceObject)
        targets.remove(spaceObject)
    }

We have a collection that is intended for this purpose:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()
    val spaceObjects = mutableListOf<SpaceObject>()
    val attackers = mutableListOf<SpaceObject>()
    val targets = mutableListOf<SpaceObject>()
    val deferredActions = mutableListOf<DeferredAction>()
    val missiles = mutableListOf<Missile>()
    // update function below if you add to these
    fun allCollections(): List<MutableList<out SpaceObject>> {
        return listOf (spaceObjects, attackers, targets, deferredActions, missiles)
    }

We use that in clear, but not in remove. We’ll change that:

    fun remove(spaceObject: SpaceObject) {
        for ( coll in allCollections()) {
            coll.remove(spaceObject)
        }
    }

Let’s enhance that test that broke to check the removal.

    @Test
    fun `missile and splat death`() {
        val mix = SpaceObjectCollection()
        val ship = Ship(
            position = U.randomPoint()
        )
        val missile = Missile(ship)
        mix.add(missile)
        val game = Game(mix)
        assertThat(mix.contains(missile)).isEqualTo(true)
        assertThat(mix.missiles).contains(missile) // ADDED
        game.cycle(0.0)
        assertThat(mix.deferredActions.any { it is DeferredAction }).describedAs("deferred action should be present").isEqualTo(true)
        game.cycle(U.MISSILE_LIFETIME + 0.1)
        assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
        assertThat(mix.missiles).doesNotContain(missile) // ADDED
        assertThat(mix.any { it is Splat }).describedAs("splat should be present").isEqualTo(true)
        game.cycle(U.MISSILE_LIFETIME + 0.2) // needs a tick to init
        game.cycle(U.MISSILE_LIFETIME + 2.3) // Splat lifetime is 2.0
        assertThat(mix.any { it is Splat }).describedAs("splat should be gone").isEqualTo(false)
    }

Messy test but it tests what we care about.

Now that remove is fixed, I can do the other collections and adds. Let’s continue to do them one at a time with commits between. I decide to sort the lines in SpaceObjectCollection’s declarations. It’ll help me see what I’m doing:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()
    
    val asteroids = mutableListOf<Asteroid>()
    val attackers = mutableListOf<SpaceObject>()
    val deferredActions = mutableListOf<DeferredAction>()
    val missiles = mutableListOf<Missile>()
    val spaceObjects = mutableListOf<SpaceObject>()
    val targets = mutableListOf<SpaceObject>()
    // update function below if you add to these
    fun allCollections(): List<MutableList<out SpaceObject>> {
        return listOf (asteroids, attackers, deferredActions, missiles, spaceObjects, targets)
    }

With the asteroids collection in place, I extract:

    private fun add(asteroid: Asteroid) {
        spaceObjects.add(asteroid)
        asteroids.add(asteroid)
        targets.add(asteroid)
    }

I realize that I don’t have a check for adding asteroids to the asteroids collection. In principle, I should. In practice, I almost forgot to do it. Let’s add some testing.

For this one, I can use an existing test:

    @Test
    fun `can count asteroids`() {
        val s = SpaceObjectCollection()
        s.add(Asteroid(Point.ZERO))
        s.add(Ship(U.CENTER_OF_UNIVERSE))
        s.add(Asteroid(Point.ZERO))
        s.add(Asteroid(Point.ZERO))
        assertThat(s.size).isEqualTo(4)
        assertThat(s.asteroidCount()).isEqualTo(3)
    }

    fun asteroidCount(): Int = asteroids.size

If this runs green, this change is righteous, but I still want more direct tests. We are green. Commit: Asteroids have own collection and add method.

Let’s do a direct test, though:

    @Test
    fun `asteroid collection works`() {
        val s = SpaceObjectCollection()
        val a = Asteroid(U.CENTER_OF_UNIVERSE)
        s.add(a)
        assertThat(s.asteroids.size).isEqualTo(1)
        s.remove(a)
        assertThat(s.asteroids.size).isEqualTo(0)
    }

Kind of a dumb test but we want to be sure. We need one for missiles as well:

    @Test
    fun `missile collection works`() {
        val s = SpaceObjectCollection()
        val m = Missile(U.CENTER_OF_UNIVERSE)
        s.add(m)
        assertThat(s.missiles.size).isEqualTo(1)
        s.remove(m)
        assertThat(s.missiles.size).isEqualTo(0)
    }

Should be green. Yes. Commit: added tests for missile and asteroids collection.

Let’s do Splats, because we have a decision to make about ship and saucer and I don’t want to think about it yet.

    @Test
    fun `splats collection works`() {
        val s = SpaceObjectCollection()
        val splat = Splat(U.CENTER_OF_UNIVERSE)
        s.add(splat)
        assertThat(s.splats.size).isEqualTo(1)
        s.remove(splat)
        assertThat(s.splats.size).isEqualTo(0)
    }

This won’t even compile, but we can now quickly follow our pattern, after which the test will compile but it won’t run. Size will be zero expecting one:

expected: 1
 but was: 0

I love it when a plan comes together. Implement:

    fun add(splat: Splat) {
        spaceObjects.add(splat)
        splats.add(splat)
    }

And call it from the dispatch:

    is Splat -> {
        add(spaceObject)
    }

Should be green. Green we are. Commit: Splats have their own collection and add method.

We’re down to Ship and Saucer:

    private fun addActualSpaceObjects(spaceObject: SpaceObject) {
        when (spaceObject) {
            is Missile -> add(spaceObject)
            is Asteroid -> {
                add(spaceObject)
            }
            is Ship -> {
                spaceObjects.add(spaceObject)
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
            is Saucer -> {
                spaceObjects.add(spaceObject)
                attackers.add(spaceObject)
                targets.add(spaceObject)
            }
            is Splat -> {
                add(spaceObject)
            }
        }
    }

The decision to be made is whether to keep these two each in a collection, e.g. ships, that will only ever contain one ship, or to keep them in a member variable, e.g. ship. The member variable makes more sense, since there is never more than one of ship or saucer, but removal will be easier if we use the collection. We haven’t fully unwound remove yet, and we don’t need to. We’ll use collections for now.

So Ship. Let’s do a test:

    @Test
    fun `ship collection works`() {
        val s = SpaceObjectCollection()
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        s.add(ship)
        assertThat(s.ships.size).isEqualTo(1)
        s.remove(ship)
        assertThat(s.ships.size).isEqualTo(0)
    }

I do the standard thing, including:

    fun add(ship: Ship) {
        spaceObjects.add(ship)
        attackers.add(ship)
        targets.add(ship)
        ships.add(ship)
    }

Should be green. Yes. Commit: Ship has its own collection and add method.

This one makes me nervous for some reason, so I take a break and then try the game. It works fine.

We’re left with Saucer. Make the standard test:

    @Test
    fun `saucer collection works`() {
        val s = SpaceObjectCollection()
        val saucer = Saucer()
        s.add(saucer)
        assertThat(s.saucers.size).isEqualTo(1)
        s.remove(saucer)
        assertThat(s.saucers.size).isEqualTo(0)
    }

Add the collection to see the test fail. It does. Complete the move:

    fun add(saucer: Saucer) {
        spaceObjects.add(saucer)
        attackers.add(saucer)
        targets.add(saucer)
        saucers.add(saucer)
    }

Should be green. Yes. Commit: Saucer has its own collection and add method.

We have completed this phase. SpaceObjectCollection still accepts a generic add, but implements it only with class-specific adds. Here’s the whole add layout for your perusal:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()

    val asteroids = mutableListOf<Asteroid>()
    val attackers = mutableListOf<SpaceObject>()
    val deferredActions = mutableListOf<DeferredAction>()
    val missiles = mutableListOf<Missile>()
    val spaceObjects = mutableListOf<SpaceObject>()
    val saucers = mutableListOf<Saucer>()
    val ships = mutableListOf<Ship>()
    val splats = mutableListOf<Splat>()
    val targets = mutableListOf<SpaceObject>()
    // update function below if you add to these
    fun allCollections(): List<MutableList<out SpaceObject>> {
        return listOf (asteroids, attackers, deferredActions, missiles, spaceObjects, saucers, ships, splats, targets)
    }

    fun add(deferredAction: DeferredAction) {
        deferredActions.add(deferredAction)
    }

    fun add(spaceObject: SpaceObject) {
        addActualSpaceObjects(spaceObject)
    }

    private fun add(asteroid: Asteroid) {
        spaceObjects.add(asteroid)
        asteroids.add(asteroid)
        targets.add(asteroid)
    }

    fun add(missile: Missile) {
        spaceObjects.add(missile)
        attackers.add(missile)
        missiles.add(missile)
    }

    fun add(saucer: Saucer) {
        spaceObjects.add(saucer)
        attackers.add(saucer)
        targets.add(saucer)
        saucers.add(saucer)
    }

    fun add(ship: Ship) {
        spaceObjects.add(ship)
        attackers.add(ship)
        targets.add(ship)
        ships.add(ship)
    }

    fun add(splat: Splat) {
        spaceObjects.add(splat)
        splats.add(splat)
    }

    private fun addActualSpaceObjects(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 addScore(score: Int) {
        scoreKeeper.addScore(score)
    }

Now the next phase, really, is to make equivalent changes to Transaction, so that all the adds we do are class-specific, which will let us remove addActualSpaceObject. But first, I’m wondering, now that we have everything broken out into the class-specific collections, could we make the other collections virtual? If we do, we’ll have everything running on the class-specific collections, and the more vague operations will still really be using those. We’ll avoid duplicating our adds, although of course creating the virtual collections will be more costly.

If we were to do that, we should only do the single collection spaceObjects … the attackers and targets, I think, will go away on this new path.

I believe that Kotlin understands the + operator on lists. Could it be this simple?

    val spaceObjects:List<SpaceObject> = asteroids + missiles + saucers + ships + splats + deferredActions

No, that won’t work. It has to be this:

    fun spaceObjects():List<SpaceObject> = asteroids + missiles + saucers + ships + splats + deferredActions

That will require a lot of parentheses to be added. We’ll gut through it to see if this works. We were at a green bar, so we can revert if we have to.

With all the references changed to have spaceObjects(), and after removing deferredActions from the virtual collection, because they are now isolated, we are green. I have to try the game, this feels like a big change even though it was pretty trivial.

Game works and we are green. Commit: spaceObjects() is now a virtual collection based on the individual class-based collections.

I think this is a delicious place to stop. I’ve been here nearly three hours and have a lovely apple Danish waiting for me. Let’s do one more thing, though. Let’s remove the InteractionTests and Interaction class. I’m sure we’re not going to go that way.

Still green. Commit: Remove Interaction and its Tests. We’ll go another way.

Let’s sum up:

Summary

Inside SpaceObjectCollection, we now have each class of SpaceObject broken out into its own sub-collection, asteroids, deferredActions, missiles, and so on. We have recreated the big collection, spaceObjects as a function. It’s still in use in the tests and game, but we are on a path to change that.

We don’t really need attackers and targets, but they are covered by some tests, and I don’t want to remove those this morning. I want to get out of here. I’ve made a yellow sticky “Jira” note to remove them.

The changes were all trivial and we added quite a few little tests to check our work. We did eight commits, in about an hour and 40 minutes or less. An average of one every 12 or 13 minutes. Not bad at all, given that we’re writing an article concurrently. I should get a chess clock.

Our next move will be to expand Transaction to break out the classes, and that should let us remove some explicit class checks, though removing the Interaction sketch already removed a raft of them. Then, if my prediction is accurate, we’ll recast the interactions to be more type-specific, and soon, or soon-ish, we’ll be able to remove subscriptions, and finally, all the interfaces and inheritance in the small SpaceObject hierarchy.

In the end, things will be simpler. Arguably, they’re simpler now, but I wouldn’t want to debate that yet. Let’s say we’re on the road to simpler.

Duplication

We are creating what looks like duplication, with all those separate add methods. But they are all unique and will soon come down to one line each. And they’re not all that alike, really:

    private fun add(asteroid: Asteroid) {
        targets.add(asteroid)
        asteroids.add(asteroid)
    }

    fun add(missile: Missile) {
        attackers.add(missile)
        missiles.add(missile)
    }

Those will come down to this:

    private fun add(asteroid: Asteroid) {
        asteroids.add(asteroid)
    }

    fun add(missile: Missile) {
        missiles.add(missile)
    }

And that’s not all that much duplication, though they do kind of rhyme. I think that’s the price one pays for all that cool type-checking.

It occurs to me that I can make attackers and target virtual, but why bother: they’ll be deleted this afternoon or tomorrow.

For now, we’ve preserved class information inside SpaceObjectCollection, at a very low cost. Soon, we’ll make use of that preservation and, if my guess is accurate, the world will be a better place.

I still think it’s probable that these changes would improve the decentralized version as well. We’ll see about that in due time.

Big picture

We are making very extensive changes to the inside of SpaceObjectCollection, but we are well-supported by our tests and we’ve added new ones as well. We’ve kept everything working even as we make our changes. We could be inserting new features or making other desired changes, with this refactoring going on behind the curtains.

I think this is almost always possible, and I believe that when we are tempted to call for a rewrite of a system, or a long delay for a “big refactoring”, there is almost always a step-by-step incremental way to get what we need. When such a path exists, it may not be quite as much fun as starting over, but starting over is really only fun until they put the pressure on us to ship. Refactoring is much safer, in a business sense, than rewriting.

How often is “almost always”? Well, if the base system is clean enough, it’s almost certain that we can refactor to get what we need. I think it’s the way to bet, and the way to succeed.

Next time

Next time we’ll get rid of those two extra collections, and work on type preservation in Transaction. Should be easy, and fun.

Please join me then!