Kotlin 208 - Schooled

GitHub Decentralized Repo
GitHub Centralized Repo

Last night, in a stroke of good fortune, I was schooled by GeePaw Hill. I may have learned an idea, and I may be able to learn to apply it. Also, ducks, and an oblique reference to a koala bear. Added in Post: Nice result so far.

In this and the next few articles, I’ll try to explain what I was told last night, and I’ll try to explain it repeatedly, hoping to understand and explain it better each time. But first I want to reflect on why my approach to this program has an important, fundamental, flaw.

I’ve programmed almost entirely, for the past quarter century, in languages that support “duck typing”¹, the notion that if an object can respond to the message you’re about to send it, it is legitimate to send the message, and the object will sort it out. So in a duck-typing language, like Smalltalk, Ruby, Lua, Python and others, I could have missiles, asteroids, ships, saucers, all with the same protocol but different implementations. I could put all of those in a single collection, say “knownObjects”, and when I iterate over that collection, I don’t need to know or ask what class an object is, I just send it the message and it does its thing.

In a duck-typing language, if you put an object of a specific class into a general collection, when it comes back out, it’s still of that same class and will act accordingly.

In a language like Kotlin, with strict static typing, this is not the case. In the Asteroids program, the knownObjects collection accepts objects that implement the interface SpaceObject, and all of asteroid, missile, ship, and saucer do implement that interface. But when I iterate that collection or otherwise use something out of it, the specifics are lost. An Asteroid instance is no longer known to be an Asteroid: it’s just a SpaceObject. I can only call SpaceObject functions. I can’t ask it any asteroid-specific questions.

If you’re at all expert with static typing languages, you know that. And, in fact, I know it. But I don’t feel it in my bones, because I’ve been working for ages almost entirely in languages with dynamic, duck, typing.

So, during last night’s Zoom, GeePaw Hill showed us some of his code, in comparison to mine, and the lights went on in my head, at least dimly. His way was better, because it is more suited to a language like Kotlin. I expect we’ll see that, over the next few days.

Why are you rabbiting on about this?

Well, because doing what I did, putting all the nicely separately typed objects under one interface, the way I did, and putting them all into a single collection, was a fundamental design error that was almost certain to cause me trouble. I threw away information, the specific type of the object in hand, and thus, if I ever needed that information again, I was forced into asking, with is Ship, or casting, with as Ship, when I needed to get specific.

Now, curiously enough, I’ve been moving away from that mistake in recent iterations. The knownObjects collection now has a number of sub-collections, which are not always type-specific, but are moving in that direction:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()
    val spaceObjects = mutableListOf<SpaceObject>()
    val attackers = mutableListOf<SpaceObject>()
    val targets = mutableListOf<SpaceObject>()
    val deferredActions = mutableListOf<DeferredAction>()

The scoreKeeper member is a size 1 collection of ScoreKeeper instances and deferredActions is a collection of DeferredAction instances. The attackers and targets collections aren’t quite type-specific, but they are moving in that direction. All the Missile instances are in attackers, and all the Asteroid instances are in targets. Oddly, both those collections also include the Ship and the Saucer. But my point remains: in my iterative fashion, I’ve been moving in the direction of SpaceObjectCollection, knownObjects, maintaining type information for me.

It’s not that there was no way to maintain type information that I needed: it is that I mistakenly did something that would work very well with duck typing, but terribly with static compile-time typing.

Therefore, what?

Well, three things, one for sure and two possible.

For sure, I’m going to move the centralized design in a direction that will let me preserve the type information that I need, in a style that is more appropriate to a language like Kotlin. As I am not really expert at that, I’ll be following one of the ideas that GeePaw Hill showed us last night, and I will almost certainly get it somewhat wrong. That’s fine, I’m here to learn.

Possibly, along the way, we’ll try another idea that Hill offered last night, that being the Kotlin “by” keyword that supports delegation. I’m not sure we’ll want that, but I think we may. We’ll see on that one.

And once we’re done here in the centralized version, we will consider whether the decentralized version would be better with similar changes to these, and if so, whether we want to refactor it to a better design, still decentralized. It’s too soon for me to say whether I’ll choose to do that or not. My guess is that at least part of what we do here will be worth doing in the other version, offering a nice dose of learning. How far we’ll go, over there, I can’t yet predict.

Do you have a plan?

Well, sort of. Last night, Hill showed a style for the Transaction and SpaceObjectCollection that I want to emulate. So we’ll start there. The rough description is that we’ll retain all the type information we can, using method overloads to accomplish it.

What I do not quite see is how to do this in very tiny steps, and I don’t know what I should do about testing and in particular TDD.

And, for now, I’m just going to stop what I was doing with the Interaction object and its tests. I think we’ll wind up tossing that out.

Let’s get started.

DeferredAction

Let’s look at how we handle the DeferredAction instances in Transaction and SpaceObjectColllection. I think they are close to what we really want.

In Transaction, right now, everything just goes into basic collections of things to add and things to remove:

class Transaction {
    val adds = mutableSetOf<SpaceObject>()
    val removes = mutableSetOf<SpaceObject>()
    var shouldClear = false
    var score = 0

In SpaceObjectCollection, we break things out a bit:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()
    val spaceObjects = mutableListOf<SpaceObject>()
    val attackers = mutableListOf<SpaceObject>()
    val targets = mutableListOf<SpaceObject>()
    val deferredActions = mutableListOf<DeferredAction>()

    fun add(spaceObject: SpaceObject) {
        when (spaceObject) {
            is DeferredAction -> deferredActions.add(spaceObject)
            else -> addActualSpaceObjects(spaceObject)
        }
    }

We can do better. Our mission is to avoid that type check. First, I need at least one test, so let’s see if I can find one that will do. Otherwise, I’ll write it. A quick look makes me think that the DeferredActionTest class will do for our purpose. First, I’ll modify Transaction.

I add two new collections to Transaction:

class Transaction {
    val adds = mutableSetOf<SpaceObject>()
    val removes = mutableSetOf<SpaceObject>()
    val deferredActionAdds = mutableSetOf<DeferredAction>()
    val deferredActionRemoves = mutableSetOf<DeferredAction>()

I add a new add method and remove method:

    fun add(deferredAction: DeferredAction) {
        deferredActionAdds.add(deferredAction)
    }
    
    fun remove(deferredAction: DeferredAction) {
        deferredActionRemoves.add(deferredAction)
    }

This should put all the deferred adds and removes into those new collections. That should mean that none of them come through the basic add in SpaceObjectCollection, because we’re not applying those new collections yet. So my tests should fail.

And 12 out of 95 tests fail. Super. Now I’ll push the deferred adds and removes through the apply. It starts like this:

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        spaceObjectCollection.removeAndFinalizeAll(removes)
        spaceObjectCollection.addAll(adds)
    }

We’ll need to do this:

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        spaceObjectCollection.removeAndFinalizeAll(removes)
        spaceObjectCollection.removeAndFinalizeAll(deferredActionRemoves)
        spaceObjectCollection.addAll(adds)
        spaceObjectCollection.addAll(deferredActionAdds)
    }

I think this makes all the tests run again. Irritatingly, it does not. I’ll inspect a few of them.

    @Test
    fun `triggers after n seconds`() {
        val trans = Transaction()
        DeferredAction(2.0, trans) { _ -> done = true}
        val tmw = trans.firstAdd() as DeferredAction
        val newTrans = Transaction()
        tmw.update(1.1, newTrans)
        assertThat(done).isEqualTo(false)
        assertThat(newTrans.adds).isEmpty()
        assertThat(newTrans.removes).isEmpty()
        tmw.update(1.1, newTrans)
        assertThat(done).isEqualTo(true)
        assertThat(newTrans.adds).isEmpty()
        assertThat(newTrans.removes).contains(tmw)
    }

Super. This test is assuming that you can look into adds to find the deferred action. That’s no longer true. Let’s recode that …

        val tmw = trans.deferredActionAdds.first()

I expect this test to run. It doesn’t, quite, because its last check needs to be changed as well.

    @Test
    fun `triggers after n seconds`() {
        val trans = Transaction()
        DeferredAction(2.0, trans) { _ -> done = true}
        val tmw = trans.deferredActionAdds.first()
        val newTrans = Transaction()
        tmw.update(1.1, newTrans)
        assertThat(done).isEqualTo(false)
        assertThat(newTrans.adds).isEmpty()
        assertThat(newTrans.removes).isEmpty()
        tmw.update(1.1, newTrans)
        assertThat(done).isEqualTo(true)
        assertThat(newTrans.adds).isEmpty()
        assertThat(newTrans.deferredActionRemoves).contains(tmw)
    }

It’s green. I’ll tick through the others and tell you if any are interesting. I expect they’ll all be like this. These tests are too conscious of the internals of Transaction. Remind me to think about that and try to learn something.

All the changes were just like those two. We are green. Commit: Transaction now has special add and remove for DeferredAction, keeps them in separate collections.

Now let’s observe what happens when we apply the changes. Transaction does this:

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        spaceObjectCollection.removeAndFinalizeAll(removes)
        spaceObjectCollection.removeAndFinalizeAll(deferredActionRemoves)
        spaceObjectCollection.addAll(adds)
        spaceObjectCollection.addAll(deferredActionAdds)
    }

I really would like to apply all those transactions separately. Why? Because I plan to implement the add and remove to preserve type information. The adds are easy:

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

I wonder if anyone is calling addAll now. Just removeAndFinalizeAll. We’ll deal with that in a moment. I think at this point our tests should still all run. They do. Shall we commit? Sure. Commit: Transaction no longer uses SpaceObjectCollection.addAll.

Why commit? Because smaller changes are better and because I want to get better at making smaller changes.

Now let’s break the tests again. We’ll remove the type check in SpaceObjectCollection.add. This:

    fun add(spaceObject: SpaceObject) {
        when (spaceObject) {
            is DeferredAction -> deferredActions.add(spaceObject)
            else -> addActualSpaceObjects(spaceObject)
        }
    }

That becomes:

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

This should break a bunch of deferred tests and perhaps others. Only three. I don’t care what they are, I’ll make them work this way:

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

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

This should make the tests run again. They do. Woot! Commit: SpaceObjectCollection uses overloaded add for DeferredActions.

We’re done with the adds for DeferredAction. Now let’s untangle the remove and do the same thing with it

I think I’d like to just have removeAndFinalize method and perhaps a remove, broken out by type as we’ve done with add. Let’s review the “all” method we have now:

    fun removeAndFinalizeAll(moribund: Set<SpaceObject>) {
        moribund.forEach {
            addAll(it.subscriptions.finalize())
        }
        removeAll(moribund)
    }

    fun removeAll(moribund: Set<SpaceObject>) {
        spaceObjects.removeAll(moribund)
        attackers.removeAll(moribund)
        targets.removeAll(moribund)
        deferredActions.removeAll(moribund)
    }

I sort of wish I hadn’t taken the shortcut of using removeAll, but at the time it seemed to make sense.

Ah. We have a long-standing oddity in our design that is a bit in our way here: finalize does not use a Transaction, it simply returns a list:

    val finalize: () -> List<SpaceObject> = { emptyList() }

So if, just if², finalize ever returns a DeferredAction, it won’t be baked into a Transaction and we won’t be able to sort it back out. That doesn’t happen at this time.

I think that to make this work cleanly, we need to change finalize to require and use a Transaction. We’re green, so let’s try changing the signature and see what breaks. I’ll start here:

    fun removeAndFinalizeAll(moribund: Set<SpaceObject>) {
        val trans = Transaction()
        moribund.forEach {
            it.subscriptions.finalize(trans)
        }
        trans.applyChanges(this)
        removeAll(moribund)
    }

This won’t compile yet, because finalize doesn’t expect a transaction and returns a list. We’ll change that in the interface:

    val finalize: (trans: Transaction) -> Unit = { }

Now all our implementors will be unhappy. I’ll let the compiler find them for me. The changes should be straightforward, I think.

In asteroid, for example:

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

Saucer is easy:

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

Who else? Ship. Also easy:

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

I rather expect green here. Well, except for a thousand tests. Darn.

I add the transaction to all the calls to finalize in the tests. However, there is another more important issue that arises.: I’m getting a stack overflow. I’m calling removeAndFinalizeAll recursively:

class SpaceObjectCollection
    fun removeAndFinalizeAll(moribund: Set<SpaceObject>) {
        val trans = Transaction()
        moribund.forEach {
            it.subscriptions.finalize(trans)
        }
        trans.applyChanges(this)
        removeAll(moribund)
    }

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

I think we need to condition the calls to removeAndFinalize on there being anything to remove. Let’s try that at least.

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

If this fixes it, as I expect it will, we can continue. We’ll be sorting out this remove stuff further and I think we can probably remove this check later. Does it work?

Yes, it does. I am a bit relieved, even though the issue seemed pretty obvious. I can’t resist trying the game, though I’ve been relying on my tests so far and I am quite confident that we’re OK. The game is fine.

Commit: finalize now uses a Transaction.

I’m two hours in. We still haven’t really unwound the remove, which is still checking types in SpaceObjectCollection:

What we really need to do is to have Transaction take responsibility here, so that the SpaceObjectCollection only has elementary add and remove, allowing us to break them out by type. That will shift the finalizing responsibility over to Transaction. I think we can do this:

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

I’m finalizing in Transaction, and accumulating the results into the current one. Then I remove any removes and do the adds. I expect this to work. It does. I should be able to remove the removeAndFinalizeAll method from SpaceObjectCollection. Yes, it is unused. Remove. We are green. Commit: finalize is called from Transaction upon application, not from SpaceObjectCollection.

Now let’s look at removeAll:

class SpaceObjectCollection
    fun removeAll(moribund: Set<SpaceObject>) {
        spaceObjects.removeAll(moribund)
        attackers.removeAll(moribund)
        targets.removeAll(moribund)
        deferredActions.removeAll(moribund)
    }

Here again, we need to do this from the outside. We don’t even have a real remove method that removes one object:

    fun remove(spaceObject: SpaceObject) {
        removeAll(setOf(spaceObject))
    }

We’ll change those two methods:

    fun removeAll(moribund: Set<SpaceObject>) {
        moribund.forEach { remove(it) }
    }

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

I expect this to work. It does. Commit: SpaceObjectCollection now implements remove of a single object.

Now we’ll change Transaction to call the individual remove:

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

I expect this to work. It does. Remove removeAll from SpaceObjectCollection. Also remove addAll, which has been unused for a while now. Test. Green. Commit: Remove removeAll from SpaceObjectCollection. Also remove addAll, which has been unused for a while now.

Now then. We can finally do our typed remove:

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

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

Test. Green. Commit: Transaction and SpaceObjectCollection preserve type of DeferredAction.

QED, as we used to say in the Math Department.

Summary

One particularly good thing is that this morning’s work was mostly done in very small steps, with minimal changes to the production code between commits. There were, however, a number of test changes, all trivial but pervasive. Nonetheless, we managed very small steps and that’s a good thing.

When we came in this morning, SpaceObjectCollection had a collection for DeferredAction instances that were known to be of that type. However it was managed by checking for the type on the way in:

    fun add(spaceObject: SpaceObject) {
        when (spaceObject) {
            is DeferredAction -> deferredActions.add(spaceObject)
            else -> addActualSpaceObjects(spaceObject)
        }
    }

Our big accomplishment for the morning was to get rid of that type check is DeferredAction. In a sense, that’s all that was done, replacing those few lines with a few other lines. We still distinguish the DeferredAction instances, but we do it without an explicit type check.

We did that, of course, by having two add methods in SpaceObjectCollection. If you’re adept with languages like Kotlin, you might have done that right off the bat. I am only beginning to get adept, and I did not. But after this morning’s work we have this:

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

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

And we have a similar pair in Transaction:

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

    fun add(spaceObject: SpaceObject) {
        adds.add(spaceObject)
    }

These duplicate add functions have exactly the same effect as the old way, but they do it the way it “should” be done, by overloading methods with differently-typed parameters.

That part was easy.

We probably could have left the removal logic alone. It just blindly removed any removed object from all the collections, but that just seemed wrong to me, so we modified the removal logic to work similarly to the adding. That change was rather more complicated: We had to change finalize to expect a Transaction, so that when we get to preserving types, they are preserved there as well. We had to change Transaction to do removes one at a time, and to do finalization in Transaction rather than in SpaceObjectCollection.

Along the way I caused a stack overflow, due to recursively calling the removeAndFinalizeAll method. That was readily fixed with a check to see if there was really anything to be removed. Interesting defect. An old nemesis that I haven’t seen for a long time.

It was all pretty straightforward, and there was a nice use of the tests. I’d break them intentionally, and then make them work again. Somewhat unfortunate was that the change to finalize meant that a lot of tests needed repair but the repairs were all similar and simple. I’ve promised to think about what that tells me about the tests, and I will do so, but my first take is that when we change the parameters to a commonly used and heavily tested function, we have to expect to change the tests. I don’t see how to have avoided that, other than always providing a Transaction to finalize and that really didn’t look easy back when it was done.

I have come to suspect that perhaps finalize shouldn’t do any adding or removing at all, and that anything that needs to be done should be done sooner. Two of our three remaining implementations don’t add or remove anything anyway.

So, possibly, we’d have done better to remove the one case that changes things, and proceed from there. Easy to say in hindsight, hard to see ahead of time. I’m content with what we did, at least for now.

Overall, we have made a small but important step toward having all the type information preserved inside SpaceObjectCollection. I think that will lead to some very interesting changes to our class hierarchy, all of which will make it simpler.

This is an interesting new path. I’m grateful to Hill for his clear examples last night. And to the other Zoomies for letting us look at my code and his code.

Next time, more breaking out of types, I think. But we’ll see what we see. Join me then, if you please.