GitHub Repo

Hill has been learning the code with an eye to helping me understand why he is right and I am wrong. I fully support this and have some comments.

Hill feels strongly that inheritance of implementation is almost always inferior to delegation. He’s not the only wise person to think this. Search those key words and you’ll find plenty to read. And I’m not saying he’s wrong, so much as I’m saying that in this case it isn’t bothering me and I don’t see another appealing way.

So, in a fit of something, he cloned my code and has been refactoring on a branch, which he has pushed back to the site after I made him a collaborator. I’ve learned a bit from the experience, including:

Why He Did It
One of the best ways we know to learn code is to change it. So Hill properly needs to modify the code to get up to speed on it. He can be much more helpful with a more broad understanding of the code.
Why He Did It in a Branch
Hill and I agree that branches are bad: we always work on HEAD. But here, his refactorings, even the best ones, are there in aid of his learning, and much of what he does shouldn’t be pushed to the front of the queue. But they do need recording, or at least we feel they do, so the best bet is a branch, which we can later peruse, look at, inspect, and examine.
Why He Did It Alone
Hill and I also agree that working together is better than working separately. He has already learned things that I need to know: I’ve discovered a few by examining his code. Other changes triggered WTF exceptions in my brain, and probably there are others that I’ve not noticed entirely. Working together, we’d have kept in mental sync and that’s much better than my having to swallow a horse-sized bitter bitter pill of learning from his work.

But we haven’t set up a working together schedule, and if we had, his inexperience in the code base would have made it hard to contribute as an equal member of the pair of us and might have made it harder for him to try some things because I could argue that they wouldn’t work because of some reason that only I know even if I’m dead wrong. So working alone for a while lets him build up understanding, and makes him a stronger contributor when we do work together.

Will we work together on this? I hope so, but we’ve made no commitment. Our commitment is to try to come to a better mutual understanding of the inheritance/delegation alternatives.

So that’s all good and you can browse his branch and compare it to the main if you wish. It’s up on the repo. Now let’s see what I’m going to do this morning.

An Opportunity

Yesterday, I converted update to return a Transaction, speculatively, because it make more sense to me that every function that returns things to be added or removed should be able to do both, so that converting all of them to Transaction will be a good thing even though there is no pressing need. Normally I’d recommend waiting for need and then making the change, but when you see a better architecture, working toward it can be a good thing as well.

But the good news is that I think I’ve found a place where this speculative change can already pay off. Even though my main interest here is in discovering things about the design and improving it, it’s good when the product code benefits. Here’s the opportunity, in WaveMaker, the object that creates the next wave of asteroids:

class WaveMaker(val numberToCreate: Int = 8): SpaceObject() {
    var done = false

    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        return if ( done ) listOf(this)
        else emptyList()
    }

    override fun interactWithOther(other: SpaceObject): List<SpaceObject> {
        return this.interactWith(other)
    }

    override fun update(deltaTime: Double): Transaction {
        if (elapsedTime < 3.0) return Transaction()

        val toAdd = mutableListOf<SpaceObject>()
        for (i in 1..numberToCreate) {
            val a = SolidObject.asteroid((U.randomEdgePoint()))
            toAdd.add(a)
        }
        done = true
        return Transaction().also { it.addAll(toAdd) }
    }
}

When the wave has been made, the work of the WaveMaker is done, and it wants to be deleted. Previously, the update function could only add things, so we set a done flag and remove this up in interactWith. We no longer need to do that, so we can remove the flag and the code in interactWith and do this:

    override fun update(deltaTime: Double): Transaction {
        if (elapsedTime < 3.0) return Transaction()

        val toAdd = mutableListOf<SpaceObject>()
        for (i in 1..numberToCreate) {
            val a = SolidObject.asteroid((U.randomEdgePoint()))
            toAdd.add(a)
        }
        val trans = Transaction()
        trans.addAll(toAdd)
        trans.remove(this)
        return trans
    }

Test. Red, on this test, unsurprisingly:

class WaveMakerTest {
    @Test
    fun `creates wave on update, removes self on interaction`() {
        val wm = WaveMaker(7)
        val toCreate = wm.tick(3.01)
        assertThat(toCreate.adds.size).isEqualTo(7)
        var toDestroy = wm.interactWithOther(wm)
        assertThat(toDestroy[0]).isEqualTo(wm)
        toDestroy = wm.interactWith(wm)
        assertThat(toDestroy[0]).isEqualTo(wm)
    }
}

Now we expect that we’ll have all our results after tick, so …

    @Test
    fun `creates wave on update, removes self immediately`() {
        val wm = WaveMaker(7)
        val trans = wm.tick(3.01)
        assertThat(trans.adds.size).isEqualTo(7)
        assertThat(trans.removes.size).isEqualTo(1)
        assertThat(trans.firstRemove()).isEqualTo(wm)
    }

Green. But wait, don’t answer yet. Now there is no need at all for the interactions in this object. Now that whole class reduces to this:

class WaveMaker(val numberToCreate: Int = 8): SpaceObject() {

    override fun update(deltaTime: Double): Transaction {
        if (elapsedTime < 3.0) return Transaction()

        val toAdd = mutableListOf<SpaceObject>()
        for (i in 1..numberToCreate) {
            val a = SolidObject.asteroid((U.randomEdgePoint()))
            toAdd.add(a)
        }
        val trans = Transaction()
        trans.addAll(toAdd)
        trans.remove(this)
        return trans
    }
}

This is the kind of improvement that I was hoping for. Commit: WaveMaker vastly simplified by using Transaction returned from update.

I wonder whether there are other such opportunities. Perhaps there are, but in the spirit of not always writing 1000-line articles, let’s push this one now.

Summary

We welcome Hill to the repo, and look forward to some mutual learning and increased common understanding.

We find that returning a Transaction from WaveMaker allows us to remove two of its three methods. (And we think this is evidence that there is value in the inheritance of implementation that we have, while remaining open to there being a better way that we just don’t know.)

And we observe a few common patterns with Transaction. This kind:

        val trans = Transaction()
        trans.addAll(toAdd)
        trans.remove(this)
        return trans

Here we create a Transaction, put some things into it, then return it. Clearly that package of code is an “idea” but it’s not as clear as it might be. The other form is this:

    private fun replaceTheShip(): Transaction {
        return Transaction().also {
            it.add(ship)
            it.add(ShipChecker(ship))
            it.remove(this)
            it.accumulate(Transaction.hyperspaceEmergence(ship,asteroidTally))
        }
    }

I prefer this second construction and consider it idiomatic Kotlin and thus worth using and getting used to. But the issue does make me wonder whether there should be other better ways of creating these moderately complicated transactions. A fluent interface is one possibility, where all the add and remove and such functions return the Transaction rather than nothing or a boolean. We might explore that later.

Right now, however, I’m going to change the WaveMaker to use the other form:

    override fun update(deltaTime: Double): Transaction {
        if (elapsedTime < 3.0) return Transaction()

        val toAdd = mutableListOf<SpaceObject>()
        for (i in 1..numberToCreate) {
            val a = SolidObject.asteroid((U.randomEdgePoint()))
            toAdd.add(a)
        }
        return Transaction().also {
            it.addAll(toAdd)
            it.remove(this)
        }
    }

Green. Commit: Use also format in returning Transaction.

Now let’s push this article and see what we do next. Join me then!