Kotlin 265 - Lovely Refactoring

GitHub Decentralized Repo
GitHub Centralized Repo
GitHub Flat Repo

There’s duplication in the collision logic, and I manage to eliminate most of it with some very simple steps, almost all automated. Nice!

It’s perhaps a bit premature to go after duplication in the collision logic, but it’s never too soon to review the code and see what it needs. And there is certainly code that looks similar in there.

private fun checkCollisions() {
    checkAllMissilesVsAsteroids()
    if ( Ship.active ) checkShipVsAsteroids(Ship)
}

private fun checkAllMissilesVsAsteroids() {
    for (missile in activeMissiles(SpaceObjects)) {
        checkAllAsteroids(missile)
    }
}

private fun checkAllAsteroids(missile: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, missile)
    }
}

fun checkOneAsteroid(
    asteroid: SpaceObject,
    missile: SpaceObject
) {
    if (colliding(asteroid, missile)) {
        Score += getScore(asteroid)
        if (asteroid.scale > 1) {
            asteroid.scale /= 2
            asteroid.velocity = randomVelocity()
            spawnNewAsteroid(asteroid)
        } else deactivate(asteroid)
        deactivate(missile)
    }
}

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (collidingShip(asteroid, ship)) {
            if (asteroid.scale > 1) {
                asteroid.scale /= 2
                asteroid.velocity = randomVelocity()
                spawnNewAsteroid(asteroid)
            } else deactivate(asteroid)
            deactivate(ship)
        }
    }
}

fun colliding(asteroid: SpaceObject, missile: SpaceObject) =
    missile.position.distanceTo(asteroid.position) <= U.AsteroidKillRadius * asteroid.scale + U.MissileKillRadius

private fun collidingShip(asteroid: SpaceObject, ship: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius*asteroid.scale
    val shipSize = U.ShipKillRadius
    return asteroid.position.distanceTo(ship.position) < asteroidSize+shipSize
}

There are at least two pretty clear duplications. The asteroid splitting logic is in there twice, and the colliding and collidingShip functions come down to pretty similar. I note the < vs <= by the way. Those should probably be the same.

The difference between those last two is that they use a different pair of numbers to check distance, both using U.AsteroidKillRadius, but one using U.ShipKillRadius and the other U.MissileKillRadius. We could combine those readily if our SpaceObjects had real types, but here it’s not so easy.

Let’s see if we can make these two methods more alike, the better to see how to combine them. First, of course, I’ll change the comparator. Let’s use less than or equal in both.

Actually the first first thing I do is put the two methods adjacent to each other in the file, so that I can easily compare them. Then I change the less than.

fun colliding(asteroid: SpaceObject, missile: SpaceObject) =
    missile.position.distanceTo(asteroid.position) <= U.AsteroidKillRadius * asteroid.scale + U.MissileKillRadius

private fun collidingShip(asteroid: SpaceObject, ship: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius*asteroid.scale
    val shipSize = U.ShipKillRadius
    return asteroid.position.distanceTo(ship.position) <= asteroidSize+shipSize
}

Let’s inline the asteroidSize in the second one:

fun colliding(asteroid: SpaceObject, missile: SpaceObject) =
    missile.position.distanceTo(asteroid.position) <= U.AsteroidKillRadius * asteroid.scale + U.MissileKillRadius

private fun collidingShip(asteroid: SpaceObject, ship: SpaceObject): Boolean {
    val shipSize = U.ShipKillRadius
    return asteroid.position.distanceTo(ship.position) <= U.AsteroidKillRadius * asteroid.scale + shipSize
}

No, belay that. Let’s instead pull out both components in the first one.

fun colliding(asteroid: SpaceObject, missile: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    val missileSize = U.MissileKillRadius
    return missile.position.distanceTo(asteroid.position) <= asteroidSize + missileSize
}

private fun collidingShip(asteroid: SpaceObject, ship: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius*asteroid.scale
    val shipSize = U.ShipKillRadius
    return asteroid.position.distanceTo(ship.position) <= asteroidSize+shipSize
}

Just to make things starkly clear, rename the second val in both cases:

fun colliding(asteroid: SpaceObject, missile: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    val colliderSize = U.MissileKillRadius
    return missile.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

private fun collidingShip(asteroid: SpaceObject, ship: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius*asteroid.scale
    val colliderSize = U.ShipKillRadius
    return asteroid.position.distanceTo(ship.position) <= asteroidSize+colliderSize
}

Rename the second parameter to collider:

fun colliding(asteroid: SpaceObject, collider: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    val colliderSize = U.MissileKillRadius
    return collider.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

private fun collidingShip(asteroid: SpaceObject, collider: SpaceObject): Boolean {
    val asteroidSize = U.AsteroidKillRadius*asteroid.scale
    val colliderSize = U.ShipKillRadius
    return asteroid.position.distanceTo(collider.position) <= asteroidSize+colliderSize
}

Now change the signature of colliding to require colliderSize:

fun colliding(
	asteroid: SpaceObject, 
	collider: SpaceObject, 
	colliderSize: Double): Boolean 
	{
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    val colliderSize = U.MissileKillRadius
    return collider.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

Remove the second line:

fun colliding(
	asteroid: SpaceObject, 
	collider: SpaceObject, 
	colliderSize: Double): Boolean 
	{
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    return collider.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

Find the senders and give them the desired size, U.MissileKillRadius:

fun checkOneAsteroid(
    asteroid: SpaceObject,
    missile: SpaceObject
) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid)
        if (asteroid.scale > 1) {
            asteroid.scale /= 2
            asteroid.velocity = randomVelocity()
            spawnNewAsteroid(asteroid)
        } else deactivate(asteroid)
        deactivate(missile)
    }
}

    @Test
    fun `asteroid and missile close enough do collide`() {
        val asteroid = newAsteroid()
        asteroid.position = Vector2(100.0, 100.0)
        val missile :SpaceObject = newMissile()
        missile.position = Vector2(110.0,100.0)
        assertThat(colliding(asteroid, missile,U.MissileKillRadius)).isEqualTo(true)
    }

    @Test
    fun `asteroid and missile far apart do not collide`() {
        val asteroid = newAsteroid()
        asteroid.position = Vector2(100.0, 100.0)
        val missile :SpaceObject = newMissile()
        missile.position = Vector2(200.0,200.0)
        assertThat(colliding(asteroid, missile,U.MissileKillRadius)).isEqualTo(false)
    }

Test. Tests green, game works. Let’s commit, though we’re not quite done. Commit: refactor colliding to accept radius of non-asteroid parameter.

Now senders of collidingShip can use colliding instead:

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (colliding(asteroid, ship, U.ShipKillRadius)) {
            if (asteroid.scale > 1) {
                asteroid.scale /= 2
                asteroid.velocity = randomVelocity()
                spawnNewAsteroid(asteroid)
            } else deactivate(asteroid)
            deactivate(ship)
        }
    }
}

The collidingShip function is no longer used. Delete it. Green, game works. Commit: remove duplication in collidingShip, use colliding.

Let’s review that process because it was, I think, more interesting than it may have seemed.

Reflection

Wow, that was nice!

A little thinking and study would of course have told us that if we passed in the size values, we could combine colliding and collidingShip. And we could have added the parameter, referred to it, etc etc, a small matter of manual refactoring. When done, we’d have gone through and changed the handful of references to the two functions, just as we did above.

But instead of using my brain¹ to figure it out, I just made the two methods more and more alike, using automated refactorings like Inline, Extract Variable, and so on. After a few automated moves, the two methods were the same except for the value assigned to the colliderSize, which made it abundantly clear that we needed that to be a parameter. The automated refactoring Change Signature changed the calling sequence, and even added a comma to each caller, so that all the lines that needed changing were flagged. Tick through and change them.

In this case, the change was easy enough that even I could surely have done it in a couple of tries, but the approach, making the code more and more alike until it basically demands to be combined, can be applied to more complicated situations. Furthermore it was a heck of a lot of fun this way and the other way would have involved more thinking, possibly more mistakes, and I wouldn’t have had that little jolt of “wow, that was nice”.

Something to think about.

Is there more?

Here’s the situation now:

private fun checkCollisions() {
    checkAllMissilesVsAsteroids()
    if ( Ship.active ) checkShipVsAsteroids(Ship)
}

fun checkOneAsteroid(
    asteroid: SpaceObject,
    missile: SpaceObject
) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid)
        if (asteroid.scale > 1) {
            asteroid.scale /= 2
            asteroid.velocity = randomVelocity()
            spawnNewAsteroid(asteroid)
        } else deactivate(asteroid)
        deactivate(missile)
    }
}

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (colliding(asteroid, ship, U.ShipKillRadius)) {
            if (asteroid.scale > 1) {
                asteroid.scale /= 2
                asteroid.velocity = randomVelocity()
                spawnNewAsteroid(asteroid)
            } else deactivate(asteroid)
            deactivate(ship)
        }
    }
}

private fun checkAllMissilesVsAsteroids() {
    for (missile in activeMissiles(SpaceObjects)) {
        checkAllAsteroids(missile)
    }
}

private fun checkAllAsteroids(missile: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, missile)
    }
}

fun colliding(asteroid: SpaceObject, collider: SpaceObject, colliderSize: Double): Boolean {
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    return collider.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

We can certainly pull out that duplicated asteroid splitting code:

fun checkOneAsteroid(
    asteroid: SpaceObject,
    missile: SpaceObject
) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid)
        splitOrKillAsteroid(asteroid)
        deactivate(missile)
    }
}

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (colliding(asteroid, ship, U.ShipKillRadius)) {
            splitOrKillAsteroid(asteroid)
            deactivate(ship)
        }
    }
}

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

So that’s nice. Let’s test and commit that. Commit: refactor common code to split or kill asteroid.

But wait, there’s more!

Let’s zoom out for a larger view, but not too far:

private fun checkCollisions() {
    checkAllMissilesVsAsteroids()
    if ( Ship.active ) checkShipVsAsteroids(Ship)
}

private fun checkAllMissilesVsAsteroids() {
    for (missile in activeMissiles(SpaceObjects)) {
        checkAllAsteroids(missile)
    }
}

private fun checkAllAsteroids(missile: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, missile)
    }
}

fun checkOneAsteroid(
    asteroid: SpaceObject,
    missile: SpaceObject
) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid)
        splitOrKillAsteroid(asteroid)
        deactivate(missile)
    }
}

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (colliding(asteroid, ship, U.ShipKillRadius)) {
            splitOrKillAsteroid(asteroid)
            deactivate(ship)
        }
    }
}

Let’s make checkAllAsteroids and checkShipVsAsteroids look more alike.

Rename checkAllAsteroids:

private fun checkMissileVsAsteroids(missile: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, missile)
    }
}

Compare with:

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        if (colliding(asteroid, ship, U.ShipKillRadius)) {
            splitOrKillAsteroid(asteroid)
            deactivate(ship)
        }
    }
}

Extract method from the latter:

// possibility for refactoring by making this more like the other?
private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroidVsShip(asteroid, ship)
    }
}

private fun checkOneAsteroidVsShip(asteroid: SpaceObject, ship: SpaceObject) {
    if (colliding(asteroid, ship, U.ShipKillRadius)) {
        splitOrKillAsteroid(asteroid)
        deactivate(ship)
    }
}

Compare that second function with:

fun checkOneAsteroid(asteroid: SpaceObject, missile: SpaceObject) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid)
        splitOrKillAsteroid(asteroid)
        deactivate(missile)
    }
}

We have two issues, one easy, one less so. Easy: pass in the kill radius and we can combine the two, except less easy: one of them increases Score and one does not.

Let’s modify the signature of getScore, and change how it works just a bit:

private fun getScore(asteroid: SpaceObject, collider: SpaceObject): Int {
    if (collider.type != SpaceObjectType.MISSILE) return 0
    return when (asteroid.scale) {
        4.0 -> 20
        2.0 -> 50
        1.0 -> 100
        else -> 0
    }
}

Senders need fixing. There’s just the one:

fun checkOneAsteroid(asteroid: SpaceObject, missile: SpaceObject) {
    if (colliding(asteroid, missile,U.MissileKillRadius)) {
        Score += getScore(asteroid,missile)
        splitOrKillAsteroid(asteroid)
        deactivate(missile)
    }
}

But now we can add another one:

private fun checkOneAsteroidVsShip(asteroid: SpaceObject, ship: SpaceObject) {
    if (colliding(asteroid, ship, U.ShipKillRadius)) {
        Score += getScore(asteroid,ship)
        splitOrKillAsteroid(asteroid)
        deactivate(ship)
    }
}

Test. Green. Commit: refactor to make CheckOneAsteroid and CheckOneAsteroidVsShip similar.

Now let’s change those to accept the radius of the collider and then just use one of them.

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

And now I can call that here:

private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, ship, U.ShipKillRadius)
    }
}

Fix up a bunch of tests to add U.MissileKillRadius to their calls, nd remove the VsShip function.

Test, Green Commit: refactoring common code out of collision logic.

Let’s review the code and sum up.

Code Review

private fun checkCollisions() {
    checkAllMissilesVsAsteroids()
    if ( Ship.active ) checkShipVsAsteroids(Ship)
}

private fun checkAllMissilesVsAsteroids() {
    for (missile in activeMissiles(SpaceObjects)) {
        checkMissileVsAsteroids(missile)
    }
}

private fun checkMissileVsAsteroids(missile: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, missile, U.MissileKillRadius)
    }
}

private fun checkShipVsAsteroids(ship: SpaceObject) {
    for (asteroid in activeAsteroids(SpaceObjects)) {
        checkOneAsteroid(asteroid, ship, U.ShipKillRadius)
    }
}

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

fun colliding(asteroid: SpaceObject, collider: SpaceObject, colliderSize: Double): Boolean {
    val asteroidSize = U.AsteroidKillRadius * asteroid.scale
    return collider.position.distanceTo(asteroid.position) <= asteroidSize + colliderSize
}

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

private fun getScore(asteroid: SpaceObject, collider: SpaceObject): Int {
    if (collider.type != SpaceObjectType.MISSILE) return 0
    return when (asteroid.scale) {
        4.0 -> 20
        2.0 -> 50
        1.0 -> 100
        else -> 0
    }
}

We’ve removed most of the duplication here, and almost everything we did was either an automated refactoring, or required by the compiler because we had added a parameter to a function. There was almost no brain power wasted, and most amazingly, every test worked and the game worked perfectly all the time. I made no mistakes as far as the tests and my game playing can tell. You never know for sure, but let’s say I detected no mistakes.

Could we do better? Yes at least ideally. It would be nice not to have to pass in the kill radius along with the colliding object. We might be able to look those up. We could create a little table indexed by SpaceObject.type, or perhaps add a killRadius field to the enum, or something. Essentially emulate a method on a real type.

And there might be other things. We’ll review the code again later, of course, and see what we see then. One such occasion will be when we add the saucer.

Summary

Today I approached refactoring using a few key ideas:

When two things are similar, make them more similar, providing more duplication that can be combined;
Use automated refactorings, like Inline, Extract Variable, Extract Method, and Change Signature, reducing opportunity for error;
Proceed in tiny steps, each one leaving the program still working as before.

I did five commits and probably could have done 15 or more, one after each individual automated refactoring. I was committing, not on every change, as GeePaw Hill probably would have done, but essentially every time I completed a phrase, an operation that in my head seemed to need punctuation, but that perhaps used more than one operation.

I think I’d argue that that was not ideal, that I’d do better to build the habit of committing on every single change that leaves the code running.

I had fun today, and recommend that you try something similar yourself. Good results combined with fun makes for a very pleasant session.

See you next time!

“A mind is a terrible thing to waste, so use yours sparingly.” – Chet Hendrickson ↩