Kotlin 127: Let's Refactor Something.

GitHub Repo

Looking at Flyer collisions, I wonder how it would look if it didn’t look like it does. P.S. Two more little refactorings. Free of charge!

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other) return emptyList()
        if ( this.ignoreCollisions && other.ignoreCollisions) return emptyList()
        val dist = position.distanceTo(other.position)
        val allowed = killRadius + other.killRadius
        return if (dist < allowed) listOf(this,other) else emptyList()
    }

There are a lot of guard clauses in here. Maybe the method would look better in some other fashion. (I’m also thinking about a collider object but I’m not sure just how that might work.)

Let’s first inline those two vals. It’ll be ugly but make the method’s shape a bit more obvious, perhaps.

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other) return emptyList()
        if ( this.ignoreCollisions && other.ignoreCollisions) return emptyList()
        return if (position.distanceTo(other.position) < killRadius + other.killRadius) listOf(this, other) else emptyList()
    }

I told you it’d be ugly. Reverse the sense of the if:

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other) return emptyList()
        if ( this.ignoreCollisions && other.ignoreCollisions) return emptyList()
        return if (position.distanceTo(other.position) >= killRadius + other.killRadius) emptyList() else listOf(this, other)
    }

Now all those empty list ones are all in a row. Can we get IDEA to combine them? IDEA isn’t helping. I’ll see what I can do by hand.

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other || this.ignoreCollisions && other.ignoreCollisions) return emptyList()
        return if (position.distanceTo(other.position) >= killRadius + other.killRadius) emptyList() else listOf(this, other)
    }

That’s still passing the tests … again …

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other || this.ignoreCollisions && other.ignoreCollisions || position.distanceTo(other.position) >= killRadius + other.killRadius) return emptyList()
        return listOf(this, other)
    }

Let me format that a bit more nicely.

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if (this === other 
            || this.ignoreCollisions && other.ignoreCollisions 
            || position.distanceTo(other.position) >= killRadius + other.killRadius) 
            return emptyList()
        else
            return listOf(this, other)
    }

Hm, not a lot better. Lift the return:

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        return if (this === other
            || this.ignoreCollisions && other.ignoreCollisions
            || position.distanceTo(other.position) >= killRadius + other.killRadius)
            emptyList()
        else
            listOf(this, other)
    }

What if we were to extract those various clauses as methods?

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        return if (itIsMe(other)
            || weIgnoreMutualCollisions(other)
            || tooFarAway(other)
        )
            emptyList()
        else
            listOf(this, other)
    }

    private fun tooFarAway(other: IFlyer) =
        position.distanceTo(other.position) >= killRadius + other.killRadius

    private fun weIgnoreMutualCollisions(other: IFlyer) = this.ignoreCollisions && other.ignoreCollisions

    private fun itIsMe(other: IFlyer) = this === other

It occurs to me that we never collide an object with itself. So the first bit isn’t even needed.

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        return if (weIgnoreMutualCollisions(other) || tooFarAway(other))
            emptyList()
        else
            listOf(this, other)
    }

    private fun weIgnoreMutualCollisions(other: IFlyer) = 
    	this.ignoreCollisions && other.ignoreCollisions

    private fun tooFarAway(other: IFlyer) =
        position.distanceTo(other.position) >= killRadius + other.killRadius

I rename once more:

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        return if (weIgnoreMutualCollisions(other) || outOfRange(other))
            emptyList()
        else
            listOf(this, other)
    }

    private fun outOfRange(other: IFlyer) =
        position.distanceTo(other.position) >= killRadius + other.killRadius

    private fun weIgnoreMutualCollisions(other: IFlyer) = this.ignoreCollisions && other.ignoreCollisions

Now that it’s down to that simple, I don’t mind it. I don’t feel a need to factor that apart any further. Here’s something that I know that Kotlin probably doesn’t: the other in there must be of type Flyer, not just IFlyer, because of the intricate way that the special objects capture all collisions.

Let’s rename these things to interactWith. And then I think we might do something else, but I’ll decide after I see how it looks.

In IFlyer:

    fun draw(drawer: Drawer) {}
    fun interactWith(other: IFlyer): List<IFlyer>
    fun interactWithOther(other: IFlyer): List<IFlyer>
    fun move(deltaTime: Double) {}
    fun finalize(): List<IFlyer> { return emptyList() }
    fun update(deltaTime: Double): List<IFlyer>

Back here in Flyer:

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

    override fun interactWithOther(other: IFlyer): List<IFlyer> {
        return if (weIgnoreMutualCollisions(other) || outOfRange(other))
            emptyList()
        else
            listOf(this, other)
    }

Now, fact is, since we know that we’re a flyer interacting with a flyer, the only thing we do is collide. Let’s reverse the if again:

    override fun interactWithOther(other: IFlyer): List<IFlyer> {
        return if (!weIgnoreMutualCollisions(other) && !outOfRange(other))
            listOf(this, other)
        else
            emptyList()
    }

That’s nasty but now the meaning of the if is collidingWith.

    override fun interactWithOther(other: IFlyer): List<IFlyer> {
        return if (weAreCollidingWith(other))
            listOf(this, other)
        else
            emptyList()
    }

    private fun weAreCollidingWith(other: IFlyer) = !weIgnoreMutualCollisions(other) && !outOfRange(other)

    private fun outOfRange(other: IFlyer) =
        position.distanceTo(other.position) >= killRadius + other.killRadius

    private fun weIgnoreMutualCollisions(other: IFlyer) = this.ignoreCollisions && other.ignoreCollisions

That’s nearly good. What about a when?

Oh yes!

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

Honestly, brothers, sisters, and siblings, I like this.

But wait, there’s more.

    private fun weAreCollidingWith(other: IFlyer) = !weIgnoreMutualCollisions(other) && !outOfRange(other)

    private fun outOfRange(other: IFlyer) =
        position.distanceTo(other.position) >= killRadius + other.killRadius

    private fun weIgnoreMutualCollisions(other: IFlyer) = this.ignoreCollisions && other.ignoreCollisions

Let’s reverse the sense of those methods and rename them. I don’t see a canned refactoring to do this but I’m not afraid.

    private fun weAreCollidingWith(other: IFlyer) = weCanCollideWith(other) && weAreInrange(other)

    private fun weCanCollideWith(other: IFlyer) = !(this.ignoreCollisions && other.ignoreCollisions)
    
    private fun weAreInrange(other: IFlyer) =
        position.distanceTo(other.position) < killRadius + other.killRadius

Now let’s undo the not and stuff up there:

    private fun weCanCollideWith(other: IFlyer) = !this.ignoreCollisions || !other.ignoreCollisions

The ignoreCollisions flag is misnamed:

    private fun weCanCollideWith(other: IFlyer) = !this.ignoreMutualCollisions || !other.ignoreMutualCollisions

And, frankly, it’s backward. canMutuallyCollide would be better, or … oh, how about this rename?

    private fun weCanCollideWith(other: IFlyer) = !this.mutuallyInvulnerable || !other.mutuallyInvulnerable

I think that’s better, but we are into diminishing returns or diminishing mental acuity. Let’s see where we stand and sum up.

Before

    override fun collisionDamageWithOther(other: IFlyer): List<IFlyer> {
        if ( this === other) return emptyList()
        if ( this.ignoreCollisions && other.ignoreCollisions) return emptyList()
        val dist = position.distanceTo(other.position)
        val allowed = killRadius + other.killRadius
        return if (dist < allowed) listOf(this,other) else emptyList()
    }

After

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

    private fun weAreCollidingWith(other: IFlyer) 
    	= weCanCollideWith(other) && weAreInrange(other)

    private fun weCanCollideWith(other: IFlyer) 
    	= !this.mutuallyInvulnerable || !other.mutuallyInvulnerable

    private fun weAreInrange(other: IFlyer) =
        position.distanceTo(other.position) < killRadius + other.killRadius

Assessment

This is my opinion, but my house my rules.

The top method, interactWithOther says exactly what we do. If colliding, return both, otherwise return empty.
If you wonder how we know we’re colliding, it’s that we can collide, and we are in range.
If you want to know if we can collide, the logic is a bit messy but if we’re not both mutually invulnerable, we can collide.
If you want to know if we’re in range, our distance is less than the sum of our kill distances.

We’ve gone from one hard-to-understand seven-liner to four methods whose ability to be understood is quite good in three of them and pretty good in the fourth. I do think there’s something to be done about the mutuallyInvulnerable notion but I’m not sure what it is. Yet.

The process was very incremental and most of the steps were automated refactorings. First I made it worse, then slowly better. It was fun and interesting. Maybe you’d enjoy trying something similar yourself sometime.

I am pleased. Comments via Twitter or email welcome. See you next time!

P.S.

What about this:

    override fun update(deltaTime: Double): List<Flyer> {
        tick(deltaTime)
        val result: MutableList<Flyer> = mutableListOf()
        val additions = controls.control(this, deltaTime)
        result.addAll(additions)
        move(deltaTime)
        return result
    }

First of all, a list is a list, so:

    override fun update(deltaTime: Double): List<IFlyer> {
        tick(deltaTime)
        val objectsToAdd = controls.control(this, deltaTime)
        move(deltaTime)
        return objectsToAdd
    }

Assessment 2

Just by inlining, we’ve gone from six lines down to four, and removed that very messy first line. Not bad. Used a more meaningful name than result for extra points.

Now, what about this method:

    fun control(obj:Flyer, deltaTime: Double): List<IFlyer> {
        turn(obj,deltaTime)
        accelerate(obj,deltaTime)
        val result =  fire(obj)
        return result
    }

That becomes:

    fun control(obj: Flyer, deltaTime: Double): List<IFlyer> {
        turn(obj, deltaTime)
        accelerate(obj, deltaTime)
        return fire(obj)
    }

Assessment 3

From four lines down to three. Simple inlining. Worth doing.

And what about fire?

    private fun fire(obj: Flyer): List<IFlyer> {
        // too tricky? deponent denieth accusation.
        val result: MutableList<Flyer> = mutableListOf()
        if (fire && !holdFire ) {
            val missile = createMissile(obj)
            result.add(missile)
        }
        holdFire = fire
        return result
    }

We can inline to get rid of the missile temp:

    private fun fire(obj: Flyer): List<IFlyer> {
        val result: MutableList<Flyer> = mutableListOf()
        if (fire && !holdFire ) {
            result.add(createMissile(obj))
        }
        holdFire = fire
        return result
    }

Now, we’d like to get rid of the odd skipping sequence: result, result.add, return result on every other line. It’s not as clear as it might be. What if we extracted a createMissileList …

    private fun fire(obj: Flyer): List<IFlyer> {
        var result: MutableList<Flyer> = mutableListOf()
        if (fire && !holdFire ) {
            result = createMissileList(obj))
        }
        holdFire = fire
        return result
    }
    
    private fun createMissileList(ship: Flyer): List<IFlyer> {
        return listOf(createMissile(ship))
    }

Aside: What is interesting is that most of these extracts are going to be inlined again, but we have to get the method shaped right to allow it. I’m just following my nose.

Add an explicit else. That will get the two list creations inside one block of code. That seems like a good thing:

    private fun fire(obj: Flyer): List<IFlyer> {
        var result: List<IFlyer> = mutableListOf()
        if (fire && !holdFire ) {
            result = createMissileList(obj))
        } else {
            result = emptyList()
        }
        holdFire = fire
        return result
    }

Lift the assignment outside the if:

    private fun fire(obj: Flyer): List<IFlyer> {
        var result: List<IFlyer> = mutableListOf()
        result = if (fire && !holdFire ) {
            createMissileList(obj)
        } else {
            emptyList()
        }
        holdFire = fire
        return result
    }

We could have inlined the var result, I notice in post. But we didn’t.

We extract the if into a method:

    private fun fire(obj: Flyer): List<IFlyer> {
        var result: List<IFlyer> = mutableListOf()
        result = getAnyMissiles(obj)
        holdFire = fire
        return result
    }

    private fun getAnyMissiles(obj: Flyer) = if (fire && !holdFire) {
        createMissileList(obj)
    } else {
        emptyList()
    }

Now, with a bit of trickery, we remove the temp variable result:

    private fun fire(obj: Flyer): List<IFlyer> {
        return getAnyMissiles(obj).also { holdFire = fire }
    }

    private fun getAnyMissiles(obj: Flyer): List<IFlyer> {
        return if (fire && !holdFire) {
            createMissileList(obj)
        } else {
            emptyList()
        }
    }

    private fun createMissileList(ship: Flyer): List<IFlyer> {
        return listOf(createMissile(ship))
    }

    private fun createMissile(ship: Flyer): Flyer {
        return Flyer.missile(ship)
    }

Note the also, necessary as things stand, because it has to be done after all the shoutin’ is over.

Let’s inline some of that stuff we extracted:

    private fun createMissileList(ship: Flyer): List<IFlyer> {
        return listOf(Flyer.missile(ship))
    }

And inline again:

    private fun fire(obj: Flyer): List<IFlyer> {
        return getAnyMissiles(obj).also { holdFire = fire }
    }

    private fun getAnyMissiles(obj: Flyer): List<IFlyer> {
        return if (fire && !holdFire) {
            listOf(Flyer.missile(obj))
        } else {
            emptyList()
        }
    }

Rename the get:

    private fun fire(obj: Flyer): List<IFlyer> {
        return missilesToFire(obj).also { holdFire = fire }
    }

    private fun missilesToFire(obj: Flyer): List<IFlyer> {
        return if (fire && !holdFire) {
            listOf(Flyer.missile(obj))
        } else {
            emptyList()
        }
    }

I’m going to leave it like that. A final Assessment:

Assessment 4

We went from here:

    private fun fire(obj: Flyer): List<IFlyer> {
        // too tricky? deponent denieth accusation.
        val result: MutableList<Flyer> = mutableListOf()
        if (fire && !holdFire ) {
            val missile = createMissile(obj)
            result.add(missile)
        }
        holdFire = fire
        return result
    }

To here:

    private fun fire(obj: Flyer): List<IFlyer> {
        return missilesToFire(obj).also { holdFire = fire }
    }

    private fun missilesToFire(obj: Flyer): List<IFlyer> {
        return if (fire && !holdFire) {
            listOf(Flyer.missile(obj))
        } else {
            emptyList()
        }
    }

There are two, or one-and-a-half things not to like about this. The holdFire logic is inherently weird: if the last time through fire was true, we leave holdFire true and the next time through, if fire’s still true (finger holding down the key), holdFire is true and we don’t fire. Sooner or later, you’re off the key and fire is false and so we set holdFire false … and the next time you hit the key, you can fire. So it’s tricky logic.

Also not to like, and not unrelated, is doing the holdFire = fire in an also, so ensure it happens after any missile are created and readied for return.

And, maybe this brings us up to two things not to like, the testing of the holdFire flag is in one method and the setting of it is in another.

There may be a formulation better than this one, but this one is pretty compact and that makes it easier to understand than it might be. I consider it an improvement. YMMV, feel free to write me with your views … and your better ideas.

Thank you, and good night!