GitHub Decentralized Repo
GitHub Centralized Repo
GitHub Flat Repo

I just noticed an opportunity for improvement. Will I stop at just one?

I noticed this:

fun fireMissile() {
    Controls.fire = false
    withAvailableMissile { missile ->
        missile.position = Ship.position + Vector2(U.MissileOffset, 0.0).rotate(Ship.angle)
        setVelocityRelativeToShip(missile, Vector2(U.MissileSpeed, 0.0).rotate(Ship.angle))
        missile.active = true
    }
}

private fun setVelocityRelativeToShip(spaceObject: SpaceObject, velocity: Vector2) {
    spaceObject.velocity = velocity + Vector2(Ship.dx, Ship.dy)
}

Let’s adjust that in a few steps:

private fun setVelocityRelativeToShip(spaceObject: SpaceObject, velocity: Vector2) {
    spaceObject.velocity = velocity + Ship.velocity
}

And then …

private fun setVelocityRelativeToShip(spaceObject: SpaceObject, velocity: Vector2) {
    spaceObject.velocity = Ship.velocity + velocity
}

And then inline:

fun fireMissile() {
    Controls.fire = false
    withAvailableMissile { missile ->
        missile.position = Ship.position + Vector2(U.MissileOffset, 0.0).rotate(Ship.angle)
        missile.velocity = Ship.velocity + Vector2(U.MissileSpeed, 0.0).rotate(Ship.angle)
        missile.active = true
    }
}

I think that’s clear enough for most porpoises. I don’t feel that the Explaining Function Name really helped us much.

Test. Green. Commit: Tidying.

I think if I were to explain one of those, I should explain both. Possibly we’d do better with better constants, vector ones, for missile offset and speed. Those are the only two usages of those constants. Let’s do two new constants:

    const val MissileOffset = 50.0
          val MissileOffsetFromShip = Vector2(MissileOffset, 0.o)
    const val MissileSpeed = LightSpeed / 3.0
          val MissileVelocity = Vector2(MissileSpeed, 0.0)

Then use those:

fun fireMissile() {
    Controls.fire = false
    withAvailableMissile { missile ->
        missile.position = Ship.position + U.MissileOffsetFromShip.rotate(Ship.angle)
        missile.velocity = Ship.velocity + U.MissileVelocity.rotate(Ship.angle)
        missile.active = true
    }
}

Then rename the offset one in U … no, let’s inline them.

    val MissileOffsetFromShip = Vector2(50.0, 0.o)
    val MissileVelocity = Vector2(LightSpeed / 3.0, 0.0)

Test. Ha! I typed 0.o, which is cute but didn’t work. Fixed. Green. Commit: Improved constants relating to missile firing.

What other prospects do we see?

fun move(spaceObject: SpaceObject, width: Double, height: Double, deltaTime: Double) {
    with (spaceObject) {
        x = limit(x+dx*deltaTime, width)
        y = limit(y+dy*deltaTime, height)
    }
}

private fun limit(value: Double, max: Double): Double {
    var result = value
    while (result < 0) result += max
    while (result > max) result -= max
    return result
}

That function would be better named wrap than limit:

fun move(spaceObject: SpaceObject, width: Double, height: Double, deltaTime: Double) {
    with (spaceObject) {
        x = wrap(x+dx*deltaTime, width)
        y = wrap(y+dy*deltaTime, height)
    }
}

private fun wrap(value: Double, max: Double): Double {
    var result = value
    while (result < 0) result += max
    while (result > max) result -= max
    return result
}

Commit: rename limit to wrap.

We can convert a few things to expression body, a style that I like although some do not agree:

fun missiles(spaceObjects: Array<SpaceObject>): List<SpaceObject> =
    spaceObjects.filter {it.type == SpaceObjectType.MISSILE}

fun activeMissiles(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 
    missiles(spaceObjects).filter { it.active}

Commit: Tidying

I notice that there is activeAsteroids and no just plain asteroids but I think we do fetch them somewhere.

Yes, there’s this:

private fun deactivateAsteroids() {
    SpaceObjects.filter { it.type == SpaceObjectType.ASTEROID }.forEach { deactivate(it) }
}

Of course here, we could use activeAsteroids to advantage, so let’s do that.

private fun deactivateAsteroids() = 
	activeAsteroids(SpaceObjects).forEach { deactivate(it) }

Commit: Tidying.

I wonder if there are others fetching vanilla asteroids.

fun activateAsteroidAtEdge() {
    val asteroids = SpaceObjects.filter { it.type == SpaceObjectType.ASTEROID }
    val available = asteroids.firstOrNull { !it.active }
    if (available != null) {
        val edgePosition = Vector2(0.0, Random.nextDouble(U.ScreenHeight.toDouble()))
        activateAsteroid(available, 4.0, edgePosition, randomVelocity())
    }
}

This cries out to have those calls covered. (Some may disagree.)

fun activateAsteroidAtEdge() {
    val asteroids = asteroids()
    val available = asteroids.firstOrNull { !it.active }
    if (available != null) {
        val edgePosition = Vector2(0.0, Random.nextDouble(U.ScreenHeight.toDouble()))
        activateAsteroid(available, 4.0, edgePosition, randomVelocity())
    }
}

fun asteroids() = SpaceObjects.filter { it.type == SpaceObjectType.ASTEROID }

And then let’s use that:

fun activeAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> {
    return asteroids().filter {it.active}
}

And then let’s do the inactive ones too, while we’re at it:

fun activeAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 	
    asteroids().filter {it.active}

fun inactiveAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 
    asteroids().filter { ! it.active}

And then use that:

fun activateAsteroidAtEdge() {
    val available = inactiveAsteroids((SpaceObjects)).firstOrNull { !it.active }
    if (available != null) {
        val edgePosition = Vector2(0.0, Random.nextDouble(U.ScreenHeight.toDouble()))
        activateAsteroid(available, 4.0, edgePosition, randomVelocity())
    }
}

Test. Green. Commit: create and use utility functions for finding active and inactive asteroids.

Warning show me that I’ve not been consistent. My new asteroids function refers to SpaceObjects directly. I think that’s not OK.

fun asteroids(spaceObjects: Array<SpaceObject>) = 
	spaceObjects.filter { it.type == SpaceObjectType.ASTEROID }

fun activeAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 
	asteroids(spaceObjects).filter {it.active}

fun inactiveAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 
	asteroids(spaceObjects).filter { ! it.active}

That’s better. Test. Green. Commit: asteroids function takes required parameter.

I noticed that my last move was a bit sloppy. I must be tiring or distracted (Squirrel!). Time to stop.

Summary

Just some little bits of clean-up to make things a bit more tidy. Today is a day for cleaning up, I guess.

I think there are a few other places where the code checks for various types. I’ll add it to my list to be on the alert for those. I prefer to have such things all coming down to a few core functions. But some, again, may disagree.

The main gripe about that approach would be a concern for efficiency. For example, there are two filtering operations done here:

fun activeAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> = 
	asteroids(spaceObjects).filter {it.active}

fun asteroids(spaceObjects: Array<SpaceObject>) = 
	spaceObjects.filter { it.type == SpaceObjectType.ASTEROID }

Those certainly could be combined into one with an &&. In fact, we had it that way. I prefer this formulation but I would grant that unless the compiler is quite clever, it’s probably slower than the &&. As we go forward here, maybe we’ll change things around, but I like it when everything filters down to the same bottom level.

YMMV, and on your projects you should do as I do, that is, whatever you think is best!

See you next time!