Kotlin 283 - Some Duplication

GitHub Decentralized Repo
GitHub Centralized Repo
GitHub Flat Repo

A bit of duplication starts me off this morning. A nice result, I think. YMMV.

Yesterday, or was it the day before, we made the ship vulnerable to the saucer’s missiles. In doing so, we created a bit of duplication. I noticed it at the time but decided to leave it and return later.

“Leave it and do it later”, of course, is all too often another way of saying “Leave it that way forever”. That’s a bit less true for me, since I feel no schedule pressure, and find it almost meditative to browse the code and clean it up. Even so, there’s no guarantee that I’ll remember.

Why do we care? I can think of at least two reasons. A lesser reason is that duplicated code takes up unnecessary space. A larger one is that when we need to change something, there’s a real risk that we will need to change the duplicated code as well, and we may not find it.

Over the years, I’ve come to keep removal of duplicate code very near the top of my priority list. It seems almost always to make things better when we remove it, and when there’s a good reason not to remove it … well, honestly, I can’t think of a good reason not to remove it. Maybe you have an example. If so, tweet or toot me up, or mention it on Agile Mentoring, and we’ll talk about it. I might learn something: it could happen.

Here’s the duplication I have in mind for this morning’s beginning:

private fun checkShipVsMissile(ship: SpaceObject, missile: SpaceObject){
    if ( colliding(ship,missile)) {
        deactivate(ship)
        deactivate(missile)
    }
}

fun checkSaucerVsMissile(saucer: SpaceObject, missile: SpaceObject) {
    if (colliding(saucer, missile)) {
        Score += U.SaucerScore
        deactivate(saucer)
        deactivate(missile)
    }
}

fun checkOneAsteroid(asteroid: SpaceObject, collider: SpaceObject) {
    if (colliding(asteroid, collider)) {
        Score += getScore(asteroid,collider)
        splitOrKillAsteroid(asteroid)
        deactivate(collider)
    }
}

Now when I came in here, I was thinking of only the first two of those. I had forgotten that asteroid checking was so similar, but there it was in front of me. In a way, it makes our situation even more juicy, since the three methods overlap but none of them quite duplicates any other one. Two of them deactivate both participants. Another two of them apply a score. But that one splits a participant rather than just deactivating it.

If we were programming with distinct objects with methods, we could just implement the deactivate on Asteroid to split or deactivate and then all three would be deactivating. Without methods, that’s a bit more tricky.

Let’s proceed this way.

Provide a new method that receives two space objects and a score value, decides whether they are colliding, and if they are, applies the score and deactivates them. That should allow us to combine the first two.
If it seems feasible, modify deactivate to handle asteroid splitting. That should allow us to roll in the third case.

Here goes:

fun checkSaucerVsMissile(saucer: SpaceObject, missile: SpaceObject) {
    checkCollisionWithScore(saucer, missile, U.SaucerScore)
}

fun checkCollisionWithScore(first: SpaceObject, second: SpaceObject, score: Int) {
    if (colliding(first, second)) {
        Score += score
        deactivate(first)
        deactivate(second)
    }
}

And with that in place we can do the ship:

private fun checkShipVsMissile(ship: SpaceObject, missile: SpaceObject){
    checkCollisionWithScore(ship, missile, 0)
}

Test. Green. But I noticed that the ship has 6 missiles. Is that intended? Well, I did change the U.MissileCount, so I must have decided to do that¹. We’ll let that ride.

Commit: refactor to combine collision checking for ship and saucer.

Now let’s have a look at the split code, and deactivate.

fun deactivate(entity: SpaceObject) {
    entity.active = false
    for (component in entity.components) {
        when (component) {
            is SaucerTimer -> component.time = U.SaucerDelay
        }
    }
    for (timer in TimerTable) {
        if (timer.entity == entity ) timer.time = timer.delayTime
    }
}

private fun splitOrKillAsteroid(asteroid: SpaceObject) {
    if (asteroid.scale > 1) {
        activateAsteroid(asteroid, asteroid.scale / 2, asteroid.position, randomVelocity())
        spawnNewAsteroid(asteroid)
    } else deactivate(asteroid)
}

Well, deactivate is already pretty complicated, so I don’t feel badly about making it more so, though I’ll surely try to make it more clear than it is. In fact, let’s do that first:

fun deactivate(entity: SpaceObject) {
    entity.active = false
    resetComponents(entity)
}

private fun resetComponents(entity: SpaceObject) {
    for (component in entity.components) {
        when (component) {
            is SaucerTimer -> component.time = U.SaucerDelay
        }
    }
    for (timer in TimerTable) {
        if (timer.entity == entity) timer.time = timer.delayTime
    }
}

Now, and I confess this was just programming. I didn’t see any clever refactoring tool way to do this:

fun deactivate(entity: SpaceObject) {
    if (splittable(entity)) {
        activateAsteroid(entity, entity.scale / 2, entity.position, randomVelocity())
        spawnNewAsteroid(entity)
    } else {
        entity.active = false
        resetComponents(entity)
    }
}

private fun splittable(entity: SpaceObject): Boolean {
    return entity.type == SpaceObjectType.ASTEROID && entity.scale > 1
}

Now I think we can change this:

fun checkOneAsteroid(asteroid: SpaceObject, collider: SpaceObject) {
    if (colliding(asteroid, collider)) {
        Score += getScore(asteroid,collider)
        splitOrKillAsteroid(asteroid)
        deactivate(collider)
    }
}

To this:

fun checkOneAsteroid(asteroid: SpaceObject, collider: SpaceObject) {
    if (colliding(asteroid, collider)) {
        Score += getScore(asteroid,collider)
        deactivate(asteroid)
        deactivate(collider)
    }
}

I decide to test that in game. Game works and tests run. One more step:

fun checkOneAsteroid(asteroid: SpaceObject, collider: SpaceObject) {
    checkCollisionWithScore(asteroid, collider, getScore(asteroid, collider))
}

Test. We’re good. Commit: refactor to combine ship, saucer, asteroid collision code.

Let’s review what we’ve got now:

fun checkSaucerVsMissile(saucer: SpaceObject, missile: SpaceObject) {
    checkCollisionWithScore(saucer, missile, U.SaucerScore)
}

private fun checkShipVsMissile(ship: SpaceObject, missile: SpaceObject){
    checkCollisionWithScore(ship, missile, 0)
}

fun checkOneAsteroid(asteroid: SpaceObject, collider: SpaceObject) {
    checkCollisionWithScore(asteroid, collider, getScore(asteroid, collider))
}

fun checkCollisionWithScore(first: SpaceObject, second: SpaceObject, score: Int) {
    if (colliding(first, second)) {
        Score += score
        deactivate(first)
        deactivate(second)
    }
}

It seems likely to me that we could push those calls up one level with inline. Here’s an example:

private fun checkAllMissilesVsSaucer(saucer: SpaceObject) {
    for (missile: SpaceObject in activeMissiles(SpaceObjects)) {
        checkSaucerVsMissile(saucer, missile)
    }
}

fun checkSaucerVsMissile(saucer: SpaceObject, missile: SpaceObject) {
    checkCollisionWithScore(saucer, missile, U.SaucerScore)
}

Inline to get this:

private fun checkAllMissilesVsSaucer(saucer: SpaceObject) {
    for (missile: SpaceObject in activeMissiles(SpaceObjects)) {
        checkCollisionWithScore(saucer, missile, U.SaucerScore)
    }
}

The inlining found a test and changed it as well. Nicely done, IDEA.

Let’s do the others.

private fun checkSaucerMissilesVsShip() {
    for (missile in activeSaucerMissiles(SpaceObjects)) {
        checkCollisionWithScore(Ship, missile, 0)
    }
}

private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkCollisionWithScore(asteroid, ship, getScore(asteroid, ship))
    }
}

That last one refactored nine tests as well as the main call. I’ll allow it, but let’s reflect:

Reflection

Think about what we had vs what we have now:

private fun checkAllMissilesVsSaucer(saucer: SpaceObject) {
    for (missile: SpaceObject in activeMissiles(SpaceObjects)) {
        checkSaucerVsMissile(saucer, missile)
    }
}

fun checkSaucerVsMissile(saucer: SpaceObject, missile: SpaceObject) {
    checkCollisionWithScore(saucer, missile, U.SaucerScore)
}

versus this:

private fun checkAllMissilesVsSaucer(saucer: SpaceObject) {
    for (missile: SpaceObject in activeMissiles(SpaceObjects)) {
        checkCollisionWithScore(saucer, missile, U.SaucerScore)
    }
}

The first one is a bit more explicit, saying OK we have a missile, check the saucer against it. The new formulation is just a tiny bit less communicative. And I have to admit I don’t love the phrase “check collision with score”. It sounds like we’re checking to see if we are colliding with a Score.

I don’t have a better idea right now, so I’ll let that stand. My overall assessment is that the net code simplification is an improvement. For me, removing duplication has again resulted in code that I prefer. YMMV², of course.

Let’s take a quick look around to see what might need a bit of cleansing. I know that doing the split refactoring required a couple of methods to be made public. I think the issue is that some things are in game that could be in space object.

Oh, right, it was activateAsteroid and spawnNewAsteroid. Moving activateAsteroid over doesn’t change the need to be public, it’s used in both files. We’ll leave those as they were.

Eeek, I forgot to commit that last refactoring: Commit: inline checkCollisionWithScore into callers.

Let’s sum up: it’s Saturday. Can’t work too hard on the weekend.

Summary

We observed a bit of duplication and in exploring it, found a bit more of the same. By passing a parameter, we made two of the three alike. By moving some functionality from one place to another, we made all three alike. By that time, all three functions were just one call, so we inlined those and eliminated all three of the original functions.

The code is more compact and I’d say the reduced code volume makes up for any loss from having a special name for three different calls to the same function. Again, you might not agree, and your coding standard belongs to you.

Me, I like what happens when I remove duplication. It seems always to improve things.

There is an exception to “improves”, and it’s this function, which is arguably a bit more complicated than it was:

fun deactivate(entity: SpaceObject) {
    if (splittable(entity)) {
        activateAsteroid(entity, entity.scale / 2, entity.position, randomVelocity())
        spawnNewAsteroid(entity)
    } else {
        entity.active = false
        resetComponents(entity)
    }
}

Now the original version of this was this:

fun deactivate(entity: SpaceObject) {
    entity.active = false
    for (component in entity.components) {
        when (component) {
            is SaucerTimer -> component.time = U.SaucerDelay
        }
    }
    for (timer in TimerTable) {
        if (timer.entity == entity ) timer.time = timer.delayTime
    }
}

So the new version looks nicer … and that’s a win. But it is calling a few more things, which hides the complexity … and that is rather the point with functions, isn’t it?

So I am satisfied with that as well, and again, you might not agree. That’s OK, we all get to do things our way. I’m just here to show you what I do and what happens when I do it.

See you next time!

In fact, this is a mistake. Jeffries didn’t notice, but well, Jeffries did notice later. Too late to make the press deadline, will report later. ↩
Your Mileage May Vary, a phrase from the distant past and probably local to the United States. Just in case you wondered. ↩