Kotlin 152: Frustration

GitHub Repo

In which our intrepid author expresses frustration and then gets down to making the program better.

GeePaw is doing some fascinating refactoring out on his branch of this program. You can find his work in the repo, on the branch aptly named “hill”. Out attempts to discuss his changes have been often productive, sometimes frustrating, apparently for both of us, certainly for me. We are long time good friends, so I am pretty confident that this won’t end our friendship, but at least one of us has been having some feelings around this. I’ll speak of mine, because they are central to how I work.

My business over the past couple of decades, to the extent that I had one, was to coach teams and teach them ideas and practices that , I believe, they’d find useful to know and to try, fitting them into their work as they themselves found suitable. In early days or years, I wanted to “change” those people, to get them to do the things that I thought were important. This was almost always frustrating, because people don’t want to be changed. And rightly so: I don’t want to be changed either.

I finally saw and set myself a new goal, which was not to cause other people to change, but to give them what I had, so that they understood it and could choose it if they wished. I wanted to be understood, not obeyed.

Insight?

As I write this, I reflect that GeePaw does not believe in information transfer the way that many of us do. I cannot explain this view: I frankly do not understand it, but what I think I’ve got right is that he fundamentally denies the notion that one person can have an idea and through any amount of “communication”, get that idea into another person’s head. As I maunder on, perhaps you’ll see how this might play in.

Where was I? Oh, right, understanding. GeePaw has said things that lead me to believe that (a) he finds the Asteroids program hard to understand, and (b) that my use of implementation inheritance is a big part of that. I’m certain of (a) and have the strong impression of (b).

So I want to be understood. Therefore, having no other resource that I can think of, I explain the program. He still doesn’t get it. I explain it again. I draw pictures of how it works. I make short cartoon films of the thing in operation. Well, apparently I am trying to teach a pig to sing here, because he still seems not to understand, and he’s getting annoyed. So, frustrating, for both of us, I think, for me for sure, and meanwhile …

Meanwhile, he is doing what he does with code that he doesn’t understand, he is spotting things that could be better by his lights and makes them better. Because he moves only in very small steps (about 1/3 the size of my small steps at a guess) and because he moves only from green to green, some of his changes seem clearly worse to me … because they are a bit of scaffolding that merely serves to support the transition from one point to a better point, and then they’ll (probably) be removed.

But unlike me, GeePaw doesn’t write down his every thought, so I do not know why he has done a particular thing, nor where he is trying to go. I only see the fewmets lying about. Sometimes he gets somewhere that’s clearly better. Sometimes he doesn’t: presumably whatever idea he had didn’t work out. That is natural. We think “the code might be better that way” and we push it over that way for a while, and then maybe it’s not all that much better. Maybe it’s so not better that we back our changes out. Maybe it’s better but the future isn’t promising and we proceed from wherever we are, to work on something else.

Even more frustrating for me is that while I think he’s on a track to show me a better way than my implementation inheritance, which might not even be true, he’s in there for his own purposes and I don’t know them at all perfectly. But if he is after the inheritance, why are his changes all over in places where the inheritance isn’t much in play.

Clear the Field

Well, I probably know the answer to that: when we want to make changes to the code, especially if it is tangled up by our lights, we may sometimes address the tangle itself, but often we instead clear away the brush around the tangle, so that we can see it better, having isolated the tangle away from other only somewhat related other issues. We clean up a lot of small things, because when the kitchen is more clean and well-organized we have enough space and visibility to do the work on the main issue.

So everything is proceeding much as one might expect, and yet I am frustrated and am probably providing a form of frustration or irritation to the pig GeePaw. I shall try to do less of that.

But dammit I want to be understood, and therefore I want this program to be understood, and if it is hard to understand, I want to understand why. And that’s not going to be aided if a few hours or days from now GeePaw says “Here, look, this is a program that I can understand”, because that program is going to be 50 or 100 commits different from the program I’m looking at, and it will be different enough that to me it looks a lot like a whole different way that one could have created an Asteroids program, where what I need, or at least want, is to know a way to make this program better that is emphatically not a hundred commits away from where I am now.

I believe that some aspects of this program are hard to understand, and I also cannot accept that what makes it hard to understand could be the 25 inherited lines in SpaceObject that “simply” provide defaults for the half-dozen methods whose cycling drives the system.

So. GeePaw and I continue to discuss this or that change. I try never to ask “why”, much less to express any particular view about a change. I might ask what it’s intended to do, or ask whether it covers this situation. And of course if I like it, I say so, and sometimes I borrow that idea and implement it, or one much like it, here.

But I wish so much that I could understand why he finds the program difficult to understand, and why he (seems to think) that the central problem is in the implementation inheritance. I may never know that. By the time he’s done doing what he is doing, GeePaw will understand the program, both the entirely different one he will have and, by analogy, the one that I have. But will he be able to transfer that understanding to me? According to his philosophical lights, it’s probably impossible, and even by my lights, it seems unlikely that standing where he then stands he’ll even know why the program seemed hard to grok. He might know and he might try to explain and I will try to understand. But we’ll be talking about two different things, program A and program B, and program B is already so unlike program A there won’t be one coherent answer.

And maybe there isn’t one. And maybe there is one and maybe it’s even implementation inheritance. Will program B diffed against program A look like an answer? Or hundreds? My bet’s on hundreds.

Your Point, Ron?

I just wanted to get that out of my system. A wiser man would save this away and never tell anyone he had written it. But my mission is to program and write about it and these thoughts are large in my mind and therefore large in impact on the program I’m working on.

Speaking of working on, let’s get working. Thanks for listening.

Pairs

One thing that is definitely confusing in my version is the interaction between pairs of objects. Each unique pair of objects gets to interact once. And from the viewpoint of the game, it gets a unique pair of space objects o1 and o2 and simply does o1.interactWith(o2). All the object implement interactWith and interactWithOther, and somehow that sorts out to the right objects getting a look at what they need to see.

Now interactsWithOther isn’t a great name. The interaction logic is a strange form of “double dispatch”. In a conventional double dispatch situation, if you were an Asteroid and you got the message interactWith(something), you might say something.interactWithAsteroid(this). Since something surely knows what type it is, it knows the types of both sides of the interaction, and can proceed to make a good decision about what to do. In the current scheme, the notion is not carrying type information so much as it is indicating whether the receiver of interactWith wants to handle the interaction or not.

Let’s start with a simple case, the Score object. The Score is just an ephemeral object carrying an increment of score, your winnings for having crushed some asteroid. It doesn’t really want to interact with anything, other than possibly the ScoreKeeper object, whose job is to sweep space looking for Score objects and to consume them. So when Score gets the message interactWith(something), it never wants to do anything. It always wants the other object to decide. So therefore, if we look in Score, we expect to see that its interactWith will defer to the other:

class Score(override val score: Int): SpaceObject() {
    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        return other.interactWithOther(this)
    }
}

Why doesn’t it also implement interactWithOther? What should it do if someone else who doesn’t want to interact comes in through that back door? Well, it shouldn’t do anything. Since it doesn’t implement interactsWithOther, we look in the superclass and sure enough:

    open fun interactWithOther(other: SpaceObject): List<SpaceObject>{ return emptyList() }

Having done that, I now know everything I need to know about how Score interacts: It gives the other guy a chance and if he’s the other guy, he does nothing. After a few glances upward, I might even learn that the default for all the interaction functions is to do nothing.

Now let’s think about ScoreKeeper. Sooner or later, the game will come up with the pair (score, scorekeeper) and send interactWith to one of them. From the above we know that if it sends interactWith to the score, the score will do scorekeeper.interactWithOther(score), and of course if the game does it the other way, we’ll see scorekeeper.interactWith(score), so in ScoreKeeper we expect to see handling of both options, the same way:

    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        if (other.score > 0) {
            totalScore += other.score
            return listOf(other)
        }
        return emptyList()
    }

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

The interactWith checks to see if we have a score (a dirty trick but one that has to be done) and accumulates the score if so. The interactWithOther does the same. However it is entered, the ScoreKeeper detects any Scores going by and adds them up.

Would this refactoring be more clear? Let’s extract a method, the entire contents of interactWith, and use it twice:

    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        return getScore(other)
    }

    override fun interactWithOther(other: SpaceObject): List<SpaceObject> {
        return getScore(other)
    }

    private fun getScore(other: SpaceObject): List<SpaceObject> {
        if (other.score > 0) {
            totalScore += other.score
            return listOf(other)
        }
        return emptyList()
    }

Now one thing is far from clear here, and I can see this now more clearly than I have before. What is that list that we’re returning? Well, with a key stroke we can find the sender:

Drilling Down

I should have committed here, but did not. No harm done, but it’s a bad habit. We move on now to clean up some code that our nose has led us to.

    fun colliders() = knownObjects.collectFromPairs { f1, f2 -> f1.interactWith(f2) }

And we can chase that just a bit …

class SpaceObjectCollection
    fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val pairs = mutableSetOf<SpaceObject>()
        pairsToCheck().forEach { p -> pairs.addAll(pairCondition(p.first, p.second))
        }
        return pairs
    }

What does this have to do with SpaceObjectCollection? Nothing whatsoever. Now that’s not making things more clear, is it? This method could be in game just as well as it is here. But anyway what do we do with the collection of SpaceObject that this thing generates?

    fun processInteractions() {
        val toBeRemoved = colliders()
        if ( toBeRemoved.size > 0 ) {
            knownObjects.removeAndFinalizeAll(toBeRemoved)
        }
    }

Ah. We remove them. So as written, the interact methods return a collection of things to be removed, and ScoreKeeper in particular, returns the Score object if it finds one.

And at last we see that ScoreKeeper accumulates the score from any score-bearing objects that pass its way, and it removes them.

In Hill’s defense, I can see how it would be difficult to track through that, and I probably picked the simplest case to explore. A new visitor to our program isn’t likely to pick that one and is likely, therefore, to find it even harder to sort out. Let’s see if we can make this more clear while we’re here. First, let’s commit that refactoring in ScoreKeeper: Extract scoring method for increased clarity.

Can we make game a bit more clear? Can we move that method out of SpaceObjectCollection?

SpaceObjectCollection does need to provide us the pairs to check, but it need not and probably should not be accumulating the results. I think this is just too fancy.

GeePaw would perhaps know a better way to do this. I myself do not, so starting here:

    fun colliders() = knownObjects.collectFromPairs { f1, f2 -> f1.interactWith(f2) }

We want to do the accumulation here rather than in SpaceObjectCollection. Copy the method over:

    fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val pairs = mutableSetOf<SpaceObject>()
        pairsToCheck().forEach { p -> pairs.addAll(pairCondition(p.first, p.second))
        }
        return pairs
    }

The pairsToCheck is red because we don’t know it. SpaceObjectCollection (knownObjects) does:

    fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val pairs = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> pairs.addAll(pairCondition(p.first, p.second))
        }
        return pairs
    }

The word pairs is deeply wrong and confusing. Let’s rename it here. Since it’s local to this method it’s a harmless change prior to testing.

    private fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val objectsToRemove = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> objectsToRemove.addAll(pairCondition(p.first, p.second))
        }
        return objectsToRemove
    }

Let’s make sure this works. Test. Green. Remove unused method from SpaceObjectCollection. Test. Commit? Not yet. What is this method? collectFromPairs isn’t a good name. And there more than that not to like, I think. We’ll talk about that.

No, wait, we have to talk about it now. This method just calls an unknown function on each pair of space objects and collects the returns. So it doesn’t know it’s doing “objects to remove”. The set certainly isn’t named pairs but what is it? And do we really want to pass in that function? It’s the only case we have. Why make it so indirect?

Rename to result just to let my brain be unbiased. We have these two methods:

    private fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> result.addAll(pairCondition(p.first, p.second))
        }
        return result
    }

    fun colliders() = collectFromPairs { f1, f2 -> f1.interactWith(f2) }

The name colliders isn’t a great name for what we have. This method is, oh, removalsDueToInteraction?

Try it on for size.

    fun removalsDueToInteraction() = collectFromPairs { f1, f2 -> f1.interactWith(f2) }

Because I can predict the future, or believe I can, I predict that this name will soon be mistaken, because what we’ll be getting back is a change due to interaction. We’ll let this ride for a moment.

Should we fold in the interaction function that we use there?

Let’s see if we can inline it. IDEA declines to help me. Let’s test and commit to get a save point.

Now, as much as I like passing lambdas around, this code seems unnecessarily tricky, given that we only use it once, right here. Plugging the interactWith call directly in, I get this:

    private fun collectFromPairs(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> result.addAll(p.first.interactWith(p.second)) }
        return result
    }

    fun removalsDueToInteraction() = collectFromPairs()

Some redundancy there. Let’s rename the first to the second and remove the second.

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> result.addAll(p.first.interactWith(p.second)) }
        return result
    }

Who calls that? A few tests, all nicely renamed, and in Game:

    fun processInteractions() {
        val toBeRemoved = removalsDueToInteraction()
        if ( toBeRemoved.size > 0 ) {
            knownObjects.removeAndFinalizeAll(toBeRemoved)
        }
    }

Test. Green. Commit.

Now a target of opportunity … that removeAndFinalizeAll. Have we not rigged SpaceObjectCollection to auto-finalize everything it removes?

Yes, we have, and it is that method, the only one that does any removing:

    fun removeAndFinalizeAll(moribund: Set<SpaceObject>): Boolean{
        moribund.forEach { spaceObjects += it.finalize() }
        return spaceObjects.removeAll(moribund)
    }

Good enough, we’ll let that be.

Reflecting

I’m not sure but I may have just wandered off into a low-value refactoring when I could have done more good. I mean, every improvement is an improvement and these changes probably count as improvements, but if it’s true that interaction is confusing, this last set of changes was a digression relative to improving that larger situation.

Well, not wasted, because it focused our attention on this:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> result.addAll(p.first.interactWith(p.second)) }
        return result
    }

The whole point of the interactWith / interactWithOther is to decide, for each possible pairing, which one of the pair should deal with the interaction. We could make a table to see how it sorts out. I think GeePaw actually did that, which I guess is actually an important point. If you have to browse a bunch of classes and make a table of what they do … what they do is not clear.

I’m not offended by this observation: I’ve been saying here again and again that the interactWith stuff is complicated. I just haven’t had a better idea.

Shall we make the table ourselves? Sure, let’s do.

In the above table, I’ve recorded what the various objects who care about interactions do. There is at least one that does not care, WaveMaker, which doesn’t implement either interaction method, thus defaulting to ignore all interactions.

It seems to me that if we knew, given a pair, which of the two should be sent interactWith, we would never send interactWithOther and could remove the method.

Let’s make a couple more observations. First, almost all of the special (non-Solid) objects are what I’ve called greedy, that is, they want to be the receiver of the interaction, so that they can interrogate the other objects for whatever property they are concerned about. Exceptions are WaveMaker, which doesn’t want to do anything with interactions, ever, and Score, which wants to behave like a Solid object, that is, it’s happy to be second in any interaction.

I’m toying with the idea of making the score object into a Solid. If we create it outside normal space it wouldn’t collide with anyone, and it would defer its interactions to the other object, including ScoreKeeper. No, that way lies madness. When we start moving things around in the hierarchy to get the behavior we want, we’re definitely in the dirty space of implementation inheritance, even if in some cases we find it just fine.

GeePaw created a concept and some code to address a similar idea, which he called shyInteractor and eagerInteractor, if I recall correctly.

What is at issue here is that, right now, I’m really making the decision on who gets the interactWith message based on the class (or some subordinate notion that’s “type” or “greediness” or something), and sorting it out via two methods that bounce back and forth.

Let’s try something. We’ll write some nasty type-dependent code in the pair interaction code, to try to select the right one to talk with, for each possible pair. I don’t think we’ll have to do all umpty pairs.

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach { p -> result.addAll(p.first.interactWith(p.second)) }
        return result
    }

We’re in that {} bit with a pair(first,second) and right now we always send to the first. Let’s do this:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p -> 
            val first = p.first
            val second = p.second
            result.addAll(first.interactWith(second)) 
        }
        return result
    }

That’s the same, no change. Test. Green. Now let’s sort first and second so that first is always right:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val newP = prioritize(p)
            val first = newP.first
            val second = newP.second
            result.addAll(first.interactWith(second))
        }
        return result
    }

We need prioritize …

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        return p
    }

No change yet. Still green. Now let’s do some sorting.

If the first is Solid, we want to send to the second, no matter what it is.

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first is SolidObject) return Pair(second,first))
        return p
    }

This is what we want, but it isn’t right, yet. Yes, we’ll send to Solid.interactWith, but it will defer again. We want to change SolidObject:

    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        return when {
            weAreCollidingWith(other) -> listOf(this, other)
            else -> emptyList()
        }
    }

    override fun interactWithOther(other: SpaceObject): List<SpaceObject> {
        TODO("impossible")
    }

I expect green. I don’t get it, because LifetimeClock test fails. Why? We can’t treat the Solid as our first check, I betcha. I’d like to remove LifetimeClock altogether but we’re on a different path just now.

Rollback the Solid change, leave the other. Add:

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first !is SolidObject && second !is Score) return Pair(second,first)
        if (first is SolidObject) return Pair(second,first)
        return p
    }

Still green. Now can we change Solid?

Well yes, but we don’t seem to have solved the lifetime clock problem.

That if is wrong. Let’s create a scoring problem first.

After too long a wait, I look at the test instead of assuming that I know why it failed.

        val clock = LifetimeClock()
        var discards = missile.interactWith(clock)
        assertThat(discards).isEmpty()
        discards = clock.interactWith(missile)
        assertThat(discards).isEmpty()
        missile.tick(4.0, Transaction())
        assertThat(missile.elapsedTime).isEqualTo(4.0)
        assertThat(missile.elapsedTime).isGreaterThan(missile.lifetime)
        discards = missile.interactWith(clock)
        assertThat(discards).contains(missile)
        discards = clock.interactWith(missile)
        assertThat(discards).contains(missile)

This test is testing things that aren’t supposed to happen any more. Clock has the right to go first.

Fix the test, get back to work. I have this:

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first !is SolidObject) return Pair(first,second)
        if (first is SolidObject) return Pair(second,first)
        return p
    }

Tests are green. It can’t be this simple. What about Score? Oh, well, it still has its deferral going:

class Score(override val score: Int): SpaceObject() {
    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        TODO("don't come here")
    }
}

THis should break things. It breaks just one test, this one:

    @Test
    fun `scorekeeper captures other vs keeper`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val discards = score.interactWith(keeper)
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

We no longer permit this. Remove the test, but I feel sure that we have a problem. We have. In the game if we shoot an asteroid, the inverted call happens and we hit the TODO. We need a test for this.

This code needs to support our testing, and I’m not even done with it, quite:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val newP = prioritize(p)
            val first = newP.first
            val second = newP.second
            result.addAll(first.interactWith(second))
        }
        return result
    }

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first !is SolidObject) return Pair(first,second)
        if (first is SolidObject) return Pair(second,first)
        return p
    }

Let’s extract that prioritize chunk and call it interactProperly. First, extract variable:

fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val newP = prioritize(p)
            val first = newP.first
            val second = newP.second
            val removes = first.interactWith(second)
            result.addAll(removes)
        }
        return result
    }

Then extract method. IDEA doesn’t give me a chance to name this:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val removes = spaceObjects(p)
            result.addAll(removes)
        }
        return result
    }

    private fun spaceObjects(p: Pair<SpaceObject, SpaceObject>): List<SpaceObject> {
        val newP = prioritize(p)
        val first = newP.first
        val second = newP.second
        val removes = first.interactWith(second)
        return removes
    }

I’m not at all sure why it didn’t let me choose the name, because spaceObjects isn’t what I’ve have chosen. Rename:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val removes = findRemovals(p)
            result.addAll(removes)
        }
        return result
    }

    private fun findRemovals(p: Pair<SpaceObject, SpaceObject>): List<SpaceObject> {
        val newP = prioritize(p)
        val first = newP.first
        val second = newP.second
        val removes = first.interactWith(second)
        return removes
    }

IDEA wants to inline that final return. I’ll allow it:

    private fun findRemovals(p: Pair<SpaceObject, SpaceObject>): List<SpaceObject> {
        val newP = prioritize(p)
        val first = newP.first
        val second = newP.second
        return first.interactWith(second)
    }

Now I can write my test, I think. No, I can’t, not unless I want to create a Game instance. This code is in the wrong place. I can make it work but I can’t test it. Let’s make it work.

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first is Score) return Pair(second,first)
        if (second is Score) return Pair(first, second)
        if (first is SolidObject) return Pair(second,first)
        return p
    }

This works, but I am really ticked off about not being able to test it. Let’s improve this, such as it is. It appears that we can change the order of these, but that’s not really the case. For example, we can’t move the third one ahead of the second. How can we make this more clear?

Aside

What we really need to do is to produce this result without resorting to type checking. We should at least be able to remove all the interactWithOther after we do this. Let’s use this order, which I believe to be correct:

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (second is Score) return Pair(first, second)
        if (first is Score || first is SolidObject) return Pair(second,first)
        return p
    }

We are green but we are missing a test for Score. The tests for Score are ill-conceived, because they are essentially testing the inheritance of interactWith and interactWithOther, which we are on the way to remove. I’m going to leave that one failing until I can figure out a better way. I think we’re going to find more of these both-ways tests.

I think those tests tell us that the interaction code is weird. They do not convince me that my defaults are bad.

Can we remove all the interactsWithOther now? Let’s remove it from the superclass and see.

I need this as a save point. Sorry, broken test, gotta commit.

I’ve removed everything but the TODO in Score and after substantial game play I hit that. Someone has sent interactWith to Score. There’s only one way that could happen:

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (second is Score) return Pair(first, second)
        if (first is Score || first is SolidObject) return Pair(second,first)
        return p
    }

If the second was Score … and the first was ALSO Score we really wouldn’t like to send the message to either one. But we have to send it to someone. Therefore Score.interactWith must default, not error. Default it is:

class Score(override val score: Int): SpaceObject() {
}

Can we simplify that prioritize a bit more? I don’t think so, we know from the table that Solids and Scores are special and so we need to do them both. The logic is convoluted, though, and we really really need a way to test it. But … we have removed interactWithOther from the system entirely.

But we sorely need better tests for Score and ScoreKeeper than these two. The second one actually fails.

    @Test
    fun `scorekeeper captures keeper vs other`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val discards = keeper.interactWith(score)
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

    @Test
    fun `scorekeeper captures other vs keeper`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val discards = score.interactWith(keeper)
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

The case of score.interactWith(keeper) can’t occur — if the logic in prioritize is correct — but we have no test for it.

We need an object to test. Testing in the game is too much hassle. Is there an object wishing to be born here?

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val removes = findRemovals(p)
            result.addAll(removes)
        }
        return result
    }

    private fun findRemovals(p: Pair<SpaceObject, SpaceObject>): List<SpaceObject> {
        val newP = prioritize(p)
        val first = newP.first
        val second = newP.second
        return first.interactWith(second)
    }

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (second is Score) return Pair(first, second) // could be ScoreKeeper
        if (first is SolidObject) return Pair(second,first) // others want a chance
        return p
    }

We have plenty of feature envy going on, which is often a sign we need a new object, or that we’re using the wrong one. Here we see that findRemovals is operating on a pair of SpaceObjects and returning a list of them. It’s not using any variable or methods in Game other than prioritize, which also has nothing to do with Game. Let’s extract those off to a new object, an Interactor.

Let’s capture our running code first. Commit: interactWithOther gone. using prioritize in Game.

If IDEA knows how to help me on this next step, I don’t see how to invoke it. We’ll do it this way, positing a class and using it:

    fun removalsDueToInteraction(): MutableSet<SpaceObject> {
        val result = mutableSetOf<SpaceObject>()
        knownObjects.pairsToCheck().forEach {p ->
            val removes = Interactor(p).findRemovals()
            result.addAll(removes)
        }
        return result
    }

This might better be done with Kotlin’s object. We’ll go this way for now, we know this trick and not the other.

class Interactor(val p: Pair<SpaceObject, SpaceObject>) {
    fun findRemovals(): List<SpaceObject> {
        val newP = prioritize(p)
        val first = newP.first
        val second = newP.second
        return first.interactWith(second)
    }

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (second is Score) return Pair(first, second) // could be ScoreKeeper
        if (first is SolidObject) return Pair(second,first) // others want a chance
        return p
    }
}

Let’s see how this runs. Tests all run except our one that we’re here to work on. Should be able to recast this now:

    @Test
    fun `scorekeeper captures other vs keeper`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val p = Pair(score, keeper)
        val interactor = Interactor(p)
        val prior = interactor.prioritize(p)
        assertThat(prior.first).isEqualTo(keeper)
        assertThat(prior.second).isEqualTo(score)
        val discards = interactor.findRemovals()
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

The prioritize is not returning keeper,score as it should. I’m glad we had this little chat. Fix, again:

    fun prioritize(p: Pair<SpaceObject, SpaceObject>): Pair<SpaceObject, SpaceObject> {
        val first = p.first
        val second = p.second
        if (first is Score) return Pair(second,first) // could be ScoreKeeper
        if (second is Score) return Pair(first, second) // could be ScoreKeeper
        if (first is SolidObject) return Pair(second,first) // others want a chance
        return p
    }

We are green, running on prioritize. Game even plays. Commit: removal of interactWithOther complete.

This has been longer and more stressful than I thought. Let’s sum up, thinking about what happened.

Summary

The good news is that we have completely removed one method, interactWithOther, from the system. This is good because it makes the system simpler, because it removes one defaulted method from the hierarchy, and because it was the source of much confusion even for me. So, woot!

It’s replaced by a notion that prioritizes a pair of objects to decide which one should get the interactWith message. The rules are a bit odd, but they are isolated into a class of their own and into about three lines of code. We might be able to make it more clear but cut me a break here, it used to be spread over nine or ten objects and now it’s all in one spot.

Digression

As I review what’s above to write the summary, I notice this:

    override fun interactWith(other: SpaceObject): List<SpaceObject> {
        return getScore(other)
    }

    private fun getScore(other: SpaceObject): List<SpaceObject> {
        if (other.score > 0) {
            totalScore += other.score
            return listOf(other)
        }
        return emptyList()
    }

I like that we call the getScore method even though we could embed it. It makes it a bit more clear why we’re doing what we’re doing. But I think we can do something a bit better that may pay off soon.

    private fun getScore(other: SpaceObject): List<SpaceObject> {
        if (other is Score) {
            totalScore += other.score
            return listOf(other)
        }
        return emptyList()
    }

This should mean that we can remove score from all the other objects …

abstract class SpaceObject {
    var elapsedTime = 0.0
    open val lifetime
        get() = Double.MAX_VALUE
    open val score: Int
        get() = 0

We should be able to remove that last couple of lines, and do this:

class Score(public val score: Int): SpaceObject() {
}

Green. Commit: remove score from SpaceObject class interface.

End digression.

Explicit type checking

Now let me say right here that I don’t like checking the types of things. But the score == 0 thing was a hack, and for now, we don’t have a better way to do things.

What one could imagine would be that we could specify for each object the ones it is interested in. In the case of ScoreKeeper, it would only be interested in Score objects. We might even avoid sending it any other messages somehow. That would be nice, if it was clean enough in expression.

We’ll save that idea for a fresher mental moment. Where were we?

We have the prioritizing going on, filtering interactions enough so that no one misses an interaction that they need to see, and no one gets an interaction that they can’t handle.

We cleaned up the collision logic quite a bit along the way, giving things better names, moving things away from places they didn’t belong, and generally simplifying matters. Good steps, Ron.

Naming issues

I think that I had some issues coming up with good names, which is an argument that the code isn’t clear. Guilty as charged, some of my code isn’t as clear as it could be. I improve it when I notice, and don’t obsess over it when I don’t notice.

Tests mostly solid

We didn’t have many tests fail us today and the main one that did showed up a serious difficulty in testing interactions, namely that some key code was embedded deep in Game. Extracting that new class, Interactor, helped with that.

Not perfect yet

I’m not sure that we’ve taken advantage of all the information in the table above, but since interactWithObject is completely gone, we have probably got most of the juice out of it. I’d like something less procedural than what we have now. We’ll see if we can thin of something.

Overall a pretty good morning. Is everything perfect? Not possible, but it’s better, by my lights at least. And removing two things from the inherited superclass … I think even GeePaw would give me a grudging nod over that.

Let’s ship it. See you next time! And … if you find something in the program hard to understand, and can express your concern … I’d love to hear from you. I accept that it’s not intuitively obvious. I just don’t know what’s opaque. So hit me up.