GitHub Repo

Too much duplication in colliding. Let’s sort it a bit.

I think what we’ll do is have everyone who can collide via position and killRadius implement an interface requiring those values. Then we’ll make an object to help solve the problem. Then we’ll see what else to do … or what would have been better.

interface Collider {
    val position: Point
    val killRadius: Double
}

We’ll make two classes implement this at first, Asteroid and Saucer, and get it working there. Let’s do some tests just for the fun of it.

    @Test
    fun `saucer asteroid collision`() {
        val saucer = Saucer()
        saucer.position = Point(249.0, 0.0)
        val asteroid = Asteroid(Point.ZERO)
        val trans = Transaction()
        saucer.subscriptions.interactWithAsteroid(asteroid, trans)
        asteroid.subscriptions.interactWithSaucer(saucer, trans)
        assertThat(trans.removes).contains(asteroid)
        assertThat(trans.removes).contains(saucer)
    }

Test runs. Good news. Now we’ll inherit Collider from each.

class Asteroid(
    override var position: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    override val killRadius: Double = 500.0,
    private val splitCount: Int = 2
) : ISpaceObject, InteractingSpaceObject, Collider {

class Saucer(): ISpaceObject, InteractingSpaceObject, Collider {
    override var position = Point(0.0, Random.nextDouble(U.UNIVERSE_SIZE))
    override val killRadius = 100.0
    var direction = -1.0 // right to left, will invert on `wakeUp`
    var velocity = Velocity.ZERO
    ...

No harm done so far. Now let’s imagine an object that could help us and call it. In Saucer:

    override
    override val subscriptions = Subscriptions(
        draw = this::draw,
        interactWithAsteroid = { asteroid, trans ->
            if (Collision(asteroid).hit(this) ) {
                trans.remove(this)
            }
        },

This requires a simple class:

class Collision(private val collider: Collider) {
    fun hit(other: Collider): Boolean {
        return collider.position.distanceTo(other.position) < collider.killRadius + other.killRadius
    }
}

I expect green. I get it. In Saucer, I can use this same expression for my other collisions:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        interactWithAsteroid = { asteroid, trans ->
            if (Collision(asteroid).hit(this) ) {
                trans.remove(this)
            }
        },
        interactWithShip = { ship, trans ->
            if ( Collision(ship).hit(this) ) {
                trans.remove(this)
            }
        },
        interactWithMissile = { missile, trans ->
            if ( Collision(missile).hit(this) ) {
                trans.remove(this)
            }
        },
    )

To make that work, I had to inherit Collider and make the members override in Missile and Ship.

Now, unused in Saucer, we have these methods:


    private fun weAreCollidingWith(asteroid: Asteroid): Boolean {
        return position.distanceTo(asteroid.position) < killRadius + asteroid.killRadius
    }

    private fun weAreCollidingWith(missile: Missile): Boolean {
        return position.distanceTo(missile.position) < killRadius + missile.killRadius
    }

    private fun weAreCollidingWith(ship: Ship): Boolean {
        return position.distanceTo(ship.position) < killRadius + ship.killRadius
    }

Remove. One more thing, we have duplication in those methods. Extract:

    override val subscriptions = Subscriptions(
        draw = this::draw,
        interactWithAsteroid = { asteroid, trans ->
            checkCollision(asteroid, trans)
        },
        interactWithShip = { ship, trans ->
            checkCollision(ship, trans)
        },
        interactWithMissile = { missile, trans ->
            checkCollision(missile, trans)
        },
    )

    private fun checkCollision(asteroid: Collider, trans: Transaction) {
        if (Collision(asteroid).hit(this)) trans.remove(this)
    }

Tests should run fine. Yes. We need more though, let’s add a few before we commit:

    @Test
    fun `missile asteroid collision`() {
        val asteroid = Asteroid(Point.ZERO)
        asteroid.position = Point(249.0, 0.0)
        val missile = Missile(Point.ZERO)
        missile.position = asteroid.position
        val trans = Transaction()
        missile.subscriptions.interactWithAsteroid(asteroid, trans)
        assertThat(trans.removes).contains(missile)
        asteroid.subscriptions.interactWithMissile(missile, trans)
        assertThat(trans.removes).contains(asteroid)
    }

This is close to what I want to write. Missile really wants a ship to build from. Here, I just want to give it a point. Let’s learn how to do a second constructor.

It turns out that I had to move things around a bit to get it right:

class Missile(
    private val shipPosition: Point,
    private val shipHeading: Double = 0.0,
    private val shipKillRadius: Double = 100.0,
    private val shipVelocity: Velocity = Velocity.ZERO
): ISpaceObject, InteractingSpaceObject, Collider {
    constructor(ship: Ship): this(ship.position, ship.heading, ship.killRadius, ship.velocity){}

    override var position: Point = Point.ZERO
    var velocity: Velocity = Velocity.ZERO
    override val killRadius: Double = 10.0
    private var elapsedTime: Double = 0.0
    private val lifetime: Double = 3.0

There may have been a more clever way to do that, but this one works. My (limited) understanding suggests that the primary constructor has to be the most broad one. Anyway this works and my test runs.

But I want to improve Missile and Asteroid’s collision stuff before I do another test.

class Missile
    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans ->
            if ( Collision(asteroid).hit(this) ) { trans.remove(this) }
        },
        interactWithSaucer = { saucer, trans ->
            if (Collision(saucer).hit(this)) { trans.remove(this) }
        },
        draw = this::draw
    )

Let’s again extract to checkCollision, for a bit of simplification:

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans ->
            if (checkCollision(asteroid)) { trans.remove(this) }
        },
        interactWithSaucer = { saucer, trans ->
            if (checkCollision(saucer)) { trans.remove(this) }
        },
        draw = this::draw
    )

    private fun checkCollision(other: Collider) = Collision(other).hit(this)

Should be green. Commit: asteroid missile saucer collisions improved to use Collider/Collision.

Should probably have committed earlier.

I wonder if we should have tests for all possible combinations of collisions. Seems like a lot of tests that don’t add much value … until we break something or forget it.

For now, I’m going to do the improvement in the other colliding classes. But we do need at least the asteroid v asteroid test, because that should NOT collide. So:

    @Test
    fun `asteroid asteroid collision`() {
        val asteroid = Asteroid(Point.ZERO)
        asteroid.position = Point(249.0, 0.0)
        val asteroid2 = Asteroid(Point.ZERO)
        asteroid2.position = asteroid.position
        val trans = Transaction()
        asteroid2.subscriptions.interactWithAsteroid(asteroid, trans)
        asteroid.subscriptions.interactWithAsteroid(asteroid2, trans)
        assertThat(trans.removes).isEmpty()
    }

That should run green now … and it does. Now improve Asteroid:

It looks like this:


    override val subscriptions = Subscriptions(
        interactWithMissile = { missile, trans ->
            if (weAreCollidingWith(missile)) {
                trans.remove(this)
            }
        },
        interactWithShip = { ship, trans ->
            if (weAreCollidingWith(ship)) {
                trans.remove(this)
            }
        },
        interactWithSaucer = { saucer, trans ->
            if (weAreCollidingWith(saucer)) {
                trans.remove(this)
            }
        },
        draw = this::draw
    )

We can change the weAreCollidingWith function, renaming to checkCollision along the way:

    override val subscriptions = Subscriptions(
        interactWithMissile = { missile, trans ->
            if (checkCollision(missile)) trans.remove(this)
        },
        interactWithShip = { ship, trans ->
            if (checkCollision(ship)) trans.remove(this)
        },
        interactWithSaucer = { saucer, trans ->
            if (checkCollision(saucer)) trans.remove(this)
        },
        draw = this::draw
    )

    private fun checkCollision(other: Collider): Boolean {
        return Collision(other).hit(this)
    }

Test. Green. Commit: use Collision in Asteroid collision code. Previous commit message was premature.

This has been going swimmingly. What’s left? Ship is still doing it longhand.

If I just change this function to use Collider (and the name other):

    private fun weAreCollidingWith(other: Collider): Boolean {
        return Collision(other).hit(this)
    }

I’ll be done. I’ve done this the long way around in a couple of the other cases. And I think I like weAreCollidingWith better than checkCollision. We’ll address that in a moment but first let’s test.

We’re green but are our tests robust enough to even exercise this case? We’ll add one:

    @Test
    fun `ship asteroid collision`() {
        val asteroid = Asteroid(Point.ZERO)
        val ship = Ship(Point.ZERO)
        ship.position = asteroid.position
        val trans = Transaction()
        ship.subscriptions.interactWithAsteroid(asteroid, trans)
        assertThat(trans.removes).contains(ship)
        asteroid.subscriptions.interactWithShip(ship, trans)
        assertThat(trans.removes).contains(asteroid)
    }

Green. Commit: Ship uses new Collider/Collision.

I think I would like the names to be different, but we’ll sit with these for a while. Let’s sum up.

Summary

Adding an interface calling for two member variables gave us a layer of objects that can possibly collide. A bit of refactoring reduced the number of redundant methods substantially. We do seem to need at least to duplicated checkCollision function, but perhaps my betters will tell me how to avoid that. As it stands, it’s better than it was.

See you next time!