GitHub Decentralized Repo
GitHub Centralized Repo

Let’s explore some possibilities for improvement. Maybe we can even reduce some duplication. That would be nice. Added in Post: Almost a complete waste of time! Dammit! My hopes are dashed!

Immutable Objects

It is commonly held that immutable objects are more desirable than mutable ones. I confess freely that this is mostly received wisdom for me: my own experience has not given me an opinion either way. It does seem clear that the code would be simpler with immutable objects, and that at least some errors would be less likely. Kotlin caters to this notion with its distinction between val and var, as of course do many languages.

Let’s examine some of our objects to see what we’re doing with var and whether it would be better were it otherwise. I happen to have the Asteroid class open in IDEA, so we’ll start there:

class Asteroid(
    override var position: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    override val killRadius: Double = U.ASTEROID_KILL_RADIUS,
    private val splitCount: Int = 2
) : SpaceObject, Collider {
    private val view = AsteroidView()
    var heading: Double = Random.nextDouble(360.0)

    override fun update(deltaTime: Double, trans: Transaction) {
        position = (position + velocity * deltaTime).cap()
    }

I am curious about heading. Why does it need to be var?

There is a reference in the ScaleDisplay that I wrote to examine the sizes of things. Let’s remove that. Test. Green. Commit: change var heading to val.

Clearly we do have to modify position. We’ll come back to that. Now that we’re looking at the flying objects, let’s look at the others.

class Missile(
    shooterPosition: Point,
    shooterHeading: Double = 0.0,
    shooterKillRadius: Double = U.SHIP_KILL_RADIUS,
    shooterVelocity: Velocity = Velocity.ZERO,
    val color: ColorRGBa = ColorRGBa.WHITE,
    val missileIsFromShip: Boolean = false
): SpaceObject, Collider {
    constructor(ship: Ship): this(ship.position, ship.heading, ship.killRadius, ship.velocity, ColorRGBa.WHITE, true)
    constructor(saucer: Saucer): this(saucer.position, Random.nextDouble(360.0), saucer.killRadius, saucer.velocity, ColorRGBa.GREEN)

    override var position: Point = Point.ZERO
    var velocity: Velocity = Velocity.ZERO
    override val killRadius: Double = U.MISSILE_KILL_RADIUS
    private val timeOut = OneShot(U.MISSILE_LIFETIME) {
        it.remove(this)
        it.add(Splat(this))
    }

    init {
        val missileOwnVelocity = Velocity(U.MISSILE_SPEED, 0.0).rotate(shooterHeading)
        val standardOffset = Point(2 * (shooterKillRadius + killRadius), 0.0)
        val rotatedOffset = standardOffset.rotate(shooterHeading)
        position = shooterPosition + rotatedOffset
        velocity = shooterVelocity + missileOwnVelocity
    }

Here again is position, but also velocity. We really only set that once. Could we make that lateinit? It turns out we can just declare it val if we don’t set it in the declaration:

    val velocity: Velocity

    init {
        val missileOwnVelocity = Velocity(U.MISSILE_SPEED, 0.0).rotate(shooterHeading)
        val standardOffset = Point(2 * (shooterKillRadius + killRadius), 0.0)
        val rotatedOffset = standardOffset.rotate(shooterHeading)
        position = shooterPosition + rotatedOffset
        velocity = shooterVelocity + missileOwnVelocity
    }

I guess Kotlin is smart enough to recognize that the init is the only setter. Looks like something I should study a bit.

Let’s do Saucer. Wow:

class Saucer : SpaceObject, Collider {
    override lateinit var position: Point
    override val killRadius = U.SAUCER_KILL_RADIUS

    private var direction: Double = 1.0
    lateinit var velocity: Velocity
    private val speed = U.SAUCER_SPEED
    private var elapsedTime = 0.0
    private var timeSinceSaucerSeen = 0.0
    private var timeSinceLastMissileFired = 0.0
    var sawShip = false
    var shipFuturePosition = Point.ZERO
    private var missileReady = true
    private var currentMissile: Missile? = null

Saucer is highly mutable. Let’s see whether we can at least make velocity a val. Oh, no, we can’t … it changes too. OK, we’ll skip Saucer for now and move on to Ship.

class Ship(
    override var position: Point,
    val controls: Controls = Controls(),
    override val killRadius: Double = U.SHIP_KILL_RADIUS
) : SpaceObject, Collider {
    var velocity:  Velocity = Velocity.ZERO
    var heading: Double = 0.0
    private var dropScale = U.DROP_SCALE
    var accelerating: Boolean = false
    var displayAcceleration: Int = 0

Here again there’s a lot of var going on. It seems possible that we could perhaps move displayAcceleration and dropScale off to a ShipView, but in fact Ship doesn’t use a ShipView: it draws itself. We’ll set Ship aside for now as well. What about Splat?

class Splat(
    var position: Point,
    var scale: Double = 1.0,
    var color: ColorRGBa = ColorRGBa.WHITE,
    val velocity: Velocity = Velocity.ZERO
) : SpaceObject {
    constructor(ship: Ship) : this(ship.position, 2.0, ColorRGBa.WHITE, ship.velocity*0.5)
    constructor(missile: Missile) : this(missile.position, 0.5, missile.color, missile.velocity*0.5)
    constructor(saucer: Saucer) : this(saucer.position, 2.0, ColorRGBa.GREEN, saucer.velocity*0.5)
    constructor(asteroid: Asteroid) : this(asteroid.position, 2.0, ColorRGBa.WHITE, asteroid.velocity*0.5)

    var elapsedTime = 0.0
    private val lifetime = U.SPLAT_LIFETIME
    private var view = SplatView(lifetime)

I should think that the construction variables could all be val except for position. We might find some test creating a Splat without parameters, and if we do, we’ll fix it. Yes, we can make most everything val:

class Splat(
    var position: Point,
    val scale: Double = 1.0,
    val color: ColorRGBa = ColorRGBa.WHITE,
    val velocity: Velocity = Velocity.ZERO
) : SpaceObject {
    constructor(ship: Ship) : this(ship.position, 2.0, ColorRGBa.WHITE, ship.velocity*0.5)
    constructor(missile: Missile) : this(missile.position, 0.5, missile.color, missile.velocity*0.5)
    constructor(saucer: Saucer) : this(saucer.position, 2.0, ColorRGBa.GREEN, saucer.velocity*0.5)
    constructor(asteroid: Asteroid) : this(asteroid.position, 2.0, ColorRGBa.WHITE, asteroid.velocity*0.5)

    var elapsedTime = 0.0
    private val lifetime = U.SPLAT_LIFETIME
    private val view = SplatView(lifetime)

Test. Green. Commit: change vars to vals.

Position

Dammit! My hopes are dashed!

I’ll spare you a hundred lines of trying to work out a way to turn the position Point into an object that could be val in the flying objects, and var inside itself. Far too many changes. Not worth doing.

This, plus some other matters, has taken the wind out of my sails.

Maybe It’s OK

Maybe these objects having var members is OK. They are, in a very real sense, “active” objects. They do things and they remember enough state so that they can have complex behavior. The Saucer moves and turns and fires a missile when it can, sometimes aiming it, sometimes randomly. The Ship can go to hyperspace, which may or may not be fatal, and it can accelerate and turn and all that.

These objects are “stateful” and I think it’s in their nature. That’s not to say that they’re perfect, by any means: I’m sure they could be improved.

As for position, and moving, there is a bit of common behavior that you’d think could be partitioned off into a little object that manages that issue. But there are tests all over, with constructors all over, all setting position to a desired point. So if it’s to be done, I’ve not quite found the way … yet.

The Interfaces

I wonder whether we could benefit from eliminating the interfaces. That would free the objects to evolve separately, which might make improvements easier. Let’s look at them:

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

interface SpaceObject {
    fun draw(drawer: Drawer)
    fun update(deltaTime: Double, trans: Transaction)
}

All of Asteroid, DeferredAction, Missile, Saucer, Ship, and Splat implement both of these interfaces. Are there cases where we actually need the interface? I think that we have a virtual collection in SpaceObjectCollection, spaceObjects() that we use in some places.

    fun spaceObjects():List<SpaceObject> = asteroids + missiles + saucers + ships + splats

    fun forEachInteracting(action: (SpaceObject)->Unit) =
        spaceObjects().forEach(action)

Who is using that? Some tests, and in Game:

class Game
    private fun draw(drawer: Drawer) {
        knownObjects.forEachInteracting { drawer.isolated { it.draw(drawer) } }
        knownObjects.scoreKeeper.draw(drawer)
    }

    fun tick(deltaTime: Double) {
        val trans = Transaction()
        knownObjects.deferredActions.forEach { it.update(deltaTime, trans)}
        knownObjects.forEachInteracting { it.update(deltaTime, trans) }
        knownObjects.applyChanges(trans)
    }

We could expand both of those to iterate over each individual collection and then the specific class would be known and we could refer to update or draw without the interface. That said, there is another advantage to an interface, which is that it expresses the requirement that the objects implement those methods, arguably a good way of documenting and enforcing a rule.

Collider is implemented by Asteroid, Missile, Saucer, and Ship. Neither DeferredAction nor Splat can collide with anything. That interface allows all those objects to use this class:

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

    fun executeOnHit(other: Collider, action: () -> Unit) {
        if (hit(other)) action()
    }
}

I see that we’re not ever calling hit from outside: we only use the executeOnHit. Make the other method private. Test. Green. Commit: make hit private.

Dammit! My hopes are dashed!

I think that interface bears its relatively light weight. I’ll allow it. I really was hoping to get rid of it.

To get rid of update and draw, and the SpaceObject interface, we’d have to expand those two loops forEachInteracting.

Let’s do the expansion just to see how it feels:

    fun tick(deltaTime: Double) {
        val trans = Transaction()
        knownObjects.deferredActions.forEach { it.update(deltaTime, trans)}
        knownObjects.asteroids.forEach { it.update(deltaTime, trans)}
        knownObjects.missiles.forEach { it.update(deltaTime, trans)}
        knownObjects.saucers.forEach { it.update(deltaTime, trans)}
        knownObjects.ships.forEach { it.update(deltaTime, trans)}
        knownObjects.applyChanges(trans)
    }

And:

    private fun draw(drawer: Drawer) {
        knownObjects.asteroids.forEach { drawer.isolated { it.draw(drawer) } }
        knownObjects.missiles.forEach { drawer.isolated { it.draw(drawer) } }
        knownObjects.saucers.forEach { drawer.isolated { it.draw(drawer) } }
        knownObjects.ships.forEach { drawer.isolated { it.draw(drawer) } }
        knownObjects.scoreKeeper.draw(drawer)
    }

Certainly a lot of duplication there but the bottom-level calls are all type-specific. I think I can remove the SpaceObject interface now. Let’s try.

Dammit! My hopes are dashed!

No, we really can’t. There are a number of methods in SpaceObjectCollection that are used in testing. More important is the fact that we have not broken out remove in Transaction and SpaceObjectCollection:

class Transaction
    fun remove(spaceObject: SpaceObject) {
        removes.add(spaceObject)
    }

    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        if (shouldClear ) spaceObjectCollection.clear()
        spaceObjectCollection.addScore(score)
        removes.forEach { spaceObjectCollection.remove(it)}
        deferredActionRemoves.forEach { spaceObjectCollection.remove(it)}
        asteroids.forEach { spaceObjectCollection.add(it)}
        missiles.forEach { spaceObjectCollection.add(it)}
        saucers.forEach { spaceObjectCollection.add(it)}
        ships.forEach { spaceObjectCollection.add(it)}
        splats.forEach { spaceObjectCollection.add(it)}
        deferredActionAdds.forEach { spaceObjectCollection.add(it)}
    }

class SpaceObjectCollection
    fun remove(spaceObject: SpaceObject) {
        for ( coll in allCollections()) {
            coll.remove(spaceObject)
        }
    }

Dammit! My hopes are dashed!

We could, of course, sort down the removes into separate collections etc etc. But all that to remove an interface that isn’t really bothering anyone? No.

Again, no real progress. Let’s roll back those breakouts.

What a Morning!

So far, almost nothing I’ve looked at has been fruitful. It’s almost as if this program is good enough. I give up, let’s sum up.

Summary

On the bright side, we changed a few vars to vals, which is nice, and we made one method private, again nice but hardly impressive.

Dammit! My hopes are dashed!

We explored two ideas that seemed potentially valuable. One was a complete boondoggle, an attempt to make the position of a SpaceObject become a val to which operations could be deferred. This might have made one object immutable, maybe two if we were lucky. But the change, at least the ways I could think of to do it, had huge ramifications in the code and tests, and did not seem worth trekking through.

I suppose it’s good to know that doing that is probably not a good idea, or that it requires a better idea than I have had so far, but it’s not the sort of thing one leaps about cheering.

Dammit! My hopes are dashed!

The other exploration was to examine whether we could eliminate the SpaceObject and Collider interfaces, which has been a sort of goal of this whole centralization effort. The answer appears to be that we can’t do either one readily. To remove Collider, we’d have to add about 8 methods to Collision, and to remove SpaceObject we’d have to add a similar number to Transaction and SpaceObjectCollection. Since both interfaces are quite small and working fine, again it’s not worth doing.

A Light Dawns?
I do get a sort of idea. Suppose that instead of a bunch of dumb collections, Transaction contained a SpaceObjectCollection, and that it added specific types to that collection. If it did that, the SpaceObjectCollection would sort them out.

Suppose further that we put type-specific removes on SpaceObjectCollection, and in Transaction. Then Transaction would have another SpaceObjectCollection for removes, and it would add to that collection (as it does now for removes).

Then, finally, we’d change the apply changes to do type-specific adds and removes from the Transaction’s collections to the real knownObjects.

Interesting but not quite tantalizing.

Another Idea
What if we got rid of SpaceObjectCollection entirely, and maintained separate collections of asteroids, missiles, and so on, right in Game? Would that be better? No. It would complicate game, where we have a whole lot of complication now pushed down into SpaceObjectCollection. Pulling that up into Game would be a step backward.
Swinging Back …
I kind of like the notion of using SpaceObjectCollection to help Transaction out. I don’t think I’m up for that this morning, I feel that the computer has already beaten me around the head and shoulders and kicked me lower down, so I’ll break. But maybe, just maybe, there’s an improvement waiting to be found.

A deeper question is whether we should be doing this at all. If this were a real product, I think I’d say probably not. A little thinking and experimenting, that’s OK, but unless I saw a real improvement pretty clearly, I don’t think I’d spend much more effort on this exploration. The centralized game is pretty decent, and unless we could point to some real simplification, we’d be better off shipping it, and enhancing the actual game rather than refining the code.

For learning purposes, however, I’m willing to spend a lot of time, if I can learn something. Today, I mostly learned that I didn’t have a good idea. But tomorrow, or the next day, if I can actually have a good idea and learn from it, I’ll make that investment.

And I did learn that I need to learn a bit more about when I can declare things val in Kotlin. It seems that it is OK with a defaulted variable that is initialized just once being declared val. I’ll try to read up on that.

Tomorrow’s another day. (And I have an appointment, so maybe Saturday is the next “another day”. Be that as it may, another day will come soon, and I hope you’ll join me then.)