Kotlin 236 - Tiny Changes

GitHub Decentralized Repo
GitHub Centralized Repo

It is not given to us to be perfect, nor to do perfect work. But this program, surely it’s close? Let’s talk about the value of tiny improvements.

It is my habit, while resisting getting up too early, to think about what I might do with the program I’m currently writing about, these days, of course, Asteroids. I think about things that aren’t quite right, and about what I might do to make them better. Sometimes my plans work out, sometimes my vision of the program and the change is inadequate, and something else happens. It’s always interesting enough to fill my morning.

This morning, there wasn’t really anything coming to mind. I would up in a sort of semi-dream state, thinking about refactoring the cat’s code. This may have something to do with the fact that she wanted to be fed and was standing on me with clear intent to wake me up. Cats, of course, are perfect and quite aware of that fact:

¹

We do still have one feature to implement, the small saucer, so we could always do that if all else fails. And I did a couple of small refactorings yesterday that weren’t worth an article. For example, the kill radius for every object is a constant (well, except for asteroids) so I moved the killRadius out of the object constructors and made it a property in the instance instead. The only mildly interesting case is Asteroid:

class Asteroid(
    override var position: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    val splitCount: Int = 2
) : SpaceObject, Collidable {
    override val killRadius: Double =
        when (splitCount) {
            2 -> U.ASTEROID_KILL_RADIUS
            1 -> U.ASTEROID_KILL_RADIUS/2
            else -> U.ASTEROID_KILL_RADIUS/4
        }

I’m really down to the dregs when that’s the best I can find … or am I?

Stayin’ Alive

It seems that as a program gets more mature, it becomes stiff, less flexible, more resistance to change. What causes this, you may be wondering. In my case, it’s because of an accumulation of small bits of work that fall just slightly short of being as good as they need to be. Now, mind you, I think that the first work I do is always a bit short of as good as it needs to be.

Our work doesn’t need to be perfect. Certainly mine never is. I’m thinking here about writing, as in articles, stories, or books.

My articles are almost always little more than first draft. I do quickly scan them for spelling, typos, grammos, infelicities, and opportunities for footnotes, but the essential point of my articles is that they are a reflection of my real process, which is error prone. I’ve never seen a book or story author recommend shipping their first draft. They always polish, refine, get their work reviewed, stick it in a drawer and read it much later with fresh eyes, and so on. They never get it to perfect, but finally they abandon it, stick it in the mail … and then, with luck, are offered a chance to revise it again, for publication.

I am fortunate to have stumbled into a way of working where publishing early drafts is an advantage, because my main point is that we need to refactor, and that we all need it, and so my articles have to show me doing less than stellar work. My critics might argue that I am particularly good at doing less than stellar work and to them I say REDACTED².

But my point, and I do have one, is that for a program to stay flexible, it needs more than first or second draft code, it needs constant revision to maintain its ability to adjust to the changes we need to make. We don’t have to look around in the crevices for things to improve, but when we’re working in some area, we’ll do well to improve things in that area.

Possibly it’s a good idea, when we look in the dishwasher for a particular item, to put at least some of the other things away. Possibly it’s a good idea, when we dig something out of the pile of papers in our tray, to separate out some additional items and file them where they belong. Sure, if we were perfect, we’d file everything in the right place the first time we touch it. Some of us are not up to that, and toss things into the “later” pile, and I am one of them.

And possibly it’s a good idea to build up a skill of noticing something that could be just a bit better, and making it just a bit better, because we happen to be working in that area right now.

So, possibly, it’s OK if I just wander around in my code, finding little things to improve and showing how I can improve them with tiny changes, because that will remind me, and maybe some readers, that keeping the code alive can be done with very small quick changes.

Or, possibly, it’s OK because I enjoy doing it. I suspect no one reads these articles anyway. If you do, let me know. I’ll count the messages.

Looking …

I was about to say that Asteroid, which happens to be on my screen since I wrote about the score, looks pretty good. Then I noticed this:

    fun splitIfPossible(trans: Transaction) {
        if (splitCount >= 1) {
            trans.add(asSplit(this))
            trans.add(asSplit(this))
        }
    }

    private fun asSplit(asteroid: Asteroid): Asteroid =
        Asteroid(
            position = asteroid.position,
            splitCount = asteroid.splitCount - 1
        )

This is odd, isn’t it? It’s this way because I just moved it back to Asteroid from AsteroidCollisionStrategy yesterday and I just made it work, I didn’t make it right. This is more nearly right:

    fun splitIfPossible(trans: Transaction) {
        if (splitCount >= 1) {
            trans.add(this.asSplit())
            trans.add(this.asSplit())
        }
    }

    private fun asSplit(): Asteroid =
        Asteroid(
            position = position,
            splitCount = splitCount - 1
        )

I’ve configured IDEA to run my tests before a commit, if I haven’t just run them. This is surely right so I’ll commit: refactor asSplit. Tests run, commit runs. Life is good.

Why was that worth doing? Because today, I happen to remember how it got that way, not for a good reason, but because a quick move of some code left it that way. A month from now, or tomorrow with a different programmer, that code might make someone scratch their head, become slightly confused, and possibly lose track of something important. Now it looks more sensible.

While we’re thinking about this code, let’s think a bit more. Does this look like it might be a mistake?

    fun splitIfPossible(trans: Transaction) {
        if (splitCount >= 1) {
            trans.add(this.asSplit())
            trans.add(this.asSplit())
        }
    }

Two absolutely identical lines? Maybe the programmer accidentally duplicated a line? Would changing the name of this method, or the asSplit method, make it more clear? It’s tempting. We could rename this to splitIntoTwoSmallerAsteroidsIfPossible. We could rename asSplit to makeSmallerAsteroid. Might be silly. What about splitInTwoIfPossible?

We’ll leave it … but I do think a better name would be a worthy notion, and IDEA would quickly change anyone who uses the old name. The idea just doesn’t quite rise to the level of “yeah, we should do it”. I could be wrong.

Looking …

Looking in alphabetical order, I am reminded that yesterday I changed the interact functions in the Strategy objects to be like this, from AsteroidCollisionStrategy:

    override fun interact(asteroid: Asteroid, trans: Transaction) =
        Unit
    override fun interact(missile: Missile, trans: Transaction) =
        checkCollision(missile, trans)
    override fun interact(saucer: Saucer, trans: Transaction) =
        checkCollision(saucer, trans)
    override fun interact(ship: Ship, trans: Transaction) =
        checkCollision(ship, trans)

Note the Unit in the first one. I used to say { }, but the Kotlin way is to say Unit. And I wanted to use the expression format here, because I want those overloaded functions to appear to me first as a block, since one rarely needs to look at them individually. The expression format saves me a nearly blank line.

Tiny changes, just slightly better by my lights.

Looking …

Let’s look at Game. It has become quite small:

class Game() {
    private var lastTime = 0.0
    private var cycler: GameCycler = GameCycler(SpaceObjectCollection(), 0, Controls())

    fun currentMix(): SpaceObjectCollection = cycler.currentMix()

    fun createInitialContents(controls: Controls) {
        initializeGame(controls, -1)
    }

    fun insertQuarter(controls: Controls) {
        initializeGame(controls, U.SHIPS_PER_QUARTER)
    }

    private fun initializeGame(controls: Controls, shipCount: Int) {
        val knownObjects = SpaceObjectCollection()
        knownObjects.performWithTransaction { trans ->
            createInitialObjects(trans,shipCount, controls, knownObjects)
        }
    }

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls,
        knownObjects: SpaceObjectCollection
    ) {
        knownObjects.scoreKeeper = ScoreKeeper(shipCount)
        cycler = GameCycler(knownObjects, U.ASTEROID_STARTING_COUNT, controls)
        trans.clear()
    }

    fun cycle(elapsedSeconds: Double, drawer: Drawer? = null) {
        val deltaTime = elapsedSeconds - lastTime
        lastTime = elapsedSeconds
        cycler.cycle(deltaTime, drawer)
    }
}

Perfect? Well, what about that lastTime thing? Game is just doing GameCycler a favor, by passing it deltatTime, which GameCycler must desire. Let’s look:

class GameCycler(
    private val knownObjects: SpaceObjectCollection,
    initialNumberOfAsteroidsToCreate: Int = -1,
    private val controls: Controls = Controls(),
) {

    fun cycle(deltaTime: Double, drawer: Drawer?= null) {
        tick(deltaTime)
        beforeInteractions()
        processInteractions()
        U.AsteroidTally = knownObjects.asteroidCount()
        createNewWaveIfNeeded()
        createSaucerIfNeeded()
        createShipIfNeeded()
        drawer?.let { draw(drawer) }
    }

We could move the calculation of deltaTime over to GameCycler. That would further simplify Game, which would then have only one variable, cycler.

Recall that one of our clues about lack of cohesion is the existence of members that change at different times. In Game, lastTime changes about 120 times a second, while cycler changes only when you insert a quarter.

Of course, moving it down to GameCycler gives it a thing that changes every 120th of a second. So it really doesn’t want to do that accounting either. What about our main program? We could do it there:

"main"...
        extend {
            drawer.fontMap = font
            game.cycle(seconds, drawer)
        }

But don’t we have tests that call game.cycle? Wouldn’t they need to be changed? Let’s find out. There are nine of them.

They’d be easy to fix. Is it worth doing? I vote no, but it’s a near thing. Why not? Because this is quite clear:

    fun cycle(elapsedSeconds: Double, drawer: Drawer? = null) {
        val deltaTime = elapsedSeconds - lastTime
        lastTime = elapsedSeconds
        cycler.cycle(deltaTime, drawer)
    }

It won’t be more clear anywhere else that I can think of, so we’ll leave it. I did reorder the functions a bit, so I’ll commit that: reorder functions slightly. Tests green, of course.

Looking …

Missile seems to me to be a bit messy, especially at the beginning:

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, Collidable {
    override val collisionStrategy: Collider
        get() = MissileCollisionStrategy(this)
    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
    override val killRadius: Double = U.MISSILE_KILL_RADIUS
    val velocity: Velocity
    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
    }

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

    override fun draw(drawer: Drawer) {
        drawer.translate(position)
        drawer.stroke = color
        drawer.fill = color
        drawer.circle(Point.ZERO, killRadius * 2.0)
    }

    override fun interactWith(other: Collidable, trans: Transaction) {
        other.collisionStrategy.interact(this, trans)
    }

    override fun toString(): String = "Missile $position ($killRadius)"

    fun score(trans: Transaction, score: Int) {
        if (missileIsFromShip) trans.addScore(score)
    }

    fun prepareToDie(trans: Transaction) {
        timeOut.cancel(trans)
    }
}

Eww, partly that’s because I put the collision strategy at the very top. That’s not good. And IDEA tells me I can make a member³ private. These changes leave me here:

class Missile(
    shooterPosition: Point,
    shooterHeading: Double = 0.0,
    shooterKillRadius: Double = U.SHIP_KILL_RADIUS,
    shooterVelocity: Velocity = Velocity.ZERO,
    val color: ColorRGBa = ColorRGBa.WHITE,
    private val missileIsFromShip: Boolean = false
): SpaceObject, Collidable {
    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
    override val killRadius: Double = U.MISSILE_KILL_RADIUS
    override val collisionStrategy: Collider
        get() = MissileCollisionStrategy(this)

    val velocity: Velocity
    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
    }
    ...

That’s marginally better. Let’s reorder things a bit more:

class Missile(
    shooterPosition: Point,
    shooterHeading: Double = 0.0,
    shooterKillRadius: Double = U.SHIP_KILL_RADIUS,
    shooterVelocity: Velocity = Velocity.ZERO,
    val color: ColorRGBa = ColorRGBa.WHITE,
    private val missileIsFromShip: Boolean = false
): SpaceObject, Collidable {
    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 lateinit var position: Point
    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
    }
    
    override val killRadius: Double = U.MISSILE_KILL_RADIUS
    override val collisionStrategy: Collider
        get() = MissileCollisionStrategy(this)

    private val timeOut = OneShot(U.MISSILE_LIFETIME) {
        it.remove(this)
        it.add(Splat(this))
    }
    ...

Possibly better. Commit: tidying.

Moohahaha, the auto-testing finds that I need to mention killRadius before using it:

    override lateinit var position: Point
    override val killRadius: Double = U.MISSILE_KILL_RADIUS
    val velocity: Velocity

Again. We’re good. I think I’d like Missile to be nicer, but it’s a bit intricate because missiles have to start far enough away from their source to avoid killing it, and their velocity is the sum of their own velocity and that of the firing entity. We’ll settle for this, until we get another idea.

A quick scan of the rest leaves me thinking that SpaceObjectCollection and Transaction might need a bit of improvement, but otherwise I found nothing other than some members that could be private and some blank lines that needed to be added or removed. Nothing worth showing you … but worth doing while passing through, because we always want to “leave the campground cleaner than we found it”.

Summary

Tiny improvements, each one taking just moments. Because my tests are fast, I can make a small change and commit it, improving the code wherever I see the opportunity, at very low cost. On a day when maybe I’m not up to a big change, or when I just have a short time before whatever’s next, small changes can make the job easier.

Worth thinking about, and in my opinion, worth doing. A good habit to cultivate.

See you next time!