Kotlin 279 - Cleaning

GitHub Decentralized Repo
GitHub Centralized Repo
GitHub Flat Repo

This code is hard to navigate. What can we do to make it easier? Nothing to see here except … Squirrel!!

Here are the line counts in the four files that make up this version of Asteroids:

 30 Component.kt
380 Game.kt
152 SpaceObject.kt
 52 TemplateProgram.kt

Almost everything I actually work on seems to be in Game.kt, and while the functions are organized somewhat by function, too many of them are more or less randomly positioned around the file.

SpaceObject.kt is a bit better, but it contains a wide range of things, including the points tables for the objects, the kill radius functions, the space object enum, the space object data class definition with its utility getters and setters, some collection operations to return missiles, active asteroids and the like, the deactivate function, the draw function, two move functions and a related limit function.

TemplateProgram.kt is fine, with just the main program, and Component.kt just contains the two components, Timer and SaucerTimer. It would be shorter but the components have their construction parameters on separate lines for readability.

Let’s do some cleanup. We’ll start with some very low-hanging fruit. The two move functions are these:

fun move(spaceObject: SpaceObject, width: Int, height: Int, deltaTime: Double) {
    move(spaceObject, width.toDouble(), height.toDouble(), deltaTime)
}

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

The first one just converts its parameters to Double for the second. It’s used just once, in Game.

And it exists solely because the main program defies the game size in Int, and the game itself wants Double. However, we have now gone to the point of having the game size defined in the universe object U:

object U {
	...
    const val ScreenHeight = 1024
    const val ScreenWidth = 1024
    ...
}

I thought this was going to be low-hanging fruit. Well, it still might be. Let’s change the U values to Double, then configure them back to Int for the setup:

    configure {
        title = "Asteroids"
        width = U.ScreenWidth.toInt()
        height = U.ScreenHeight.toInt()
    }

And let’s change startGame:

fun startGame(width: Int, height: Int) {
    saucerSpeed = U.SaucerSpeed
    Ship.active = true
    Ship.position = Vector2(width/2.0, height/2.0)
    Saucer.active = false
    activateAsteroids(4)
}

We’ll use the U values here, and change the signature to take no parameters.

fun startGame() {
    saucerSpeed = U.SaucerSpeed
    Ship.active = true
    Ship.position = Vector2(U.ScreenWidth/2.0, U.ScreenHeight/2.0)
    Saucer.active = false
    activateAsteroids(4)
}

The Change Signature refactoring fixes up the call to this, of course. But we also have the gameCycle function being called with integers. I have the usual trouble finding it, by the way, although IDEA has a very nice search that I can use.

I thought this was supposed to be low-hanging. Well, it is. We can certainly test and commit right now. Let’s start developing that habit.

Hahahaha, he laughed. A test fails, one that used to set a different width and height to test where the ship starts:

    @Test
    fun `start game makes ship active`() {
        createGame(6, 26)
        assertThat(Ship.active).isEqualTo(false)
        startGame()
        assertThat(Ship.active).isEqualTo(true)
        assertThat(Ship.x).isEqualTo(250.0)
        assertThat(Ship.y).isEqualTo(300.0)
    }

We’ll remove the last two assertions. This may mean that we aren’t sure that the ship goes where it should except that we just saw the code up above. Green. Commit: Change U.ScreenHeight and U.ScreenWidth to Double, begin process of eliminating integer height and width.

I clear a few warnings before pushing, IDEA reminded me to remove a few toDouble calls that are no longer needed. Now let’s get after those integers.

The main calls gameCycle with width and height and we don’t really need them, but we need to push downward:

fun gameCycle(
    spaceObjects: Array<SpaceObject>,
    width: Int,
    height: Int,
    drawer: Drawer,
    deltaTime: Double
) {
    updateEverything(spaceObjects, deltaTime, width, height)
    drawEverything(spaceObjects, drawer, deltaTime)
    checkCollisions()
    drawScore(drawer)
    checkIfAsteroidsNeeded(deltaTime)
}

private fun updateEverything(
    spaceObjects: Array<SpaceObject>,
    deltaTime: Double,
    width: Int,
    height: Int
) {
    updateTimers(deltaTime)
    for (spaceObject in spaceObjects) {
        for (component in spaceObject.components) update(component, deltaTime)
        if (spaceObject.type == SpaceObjectType.SHIP) applyControls(spaceObject, deltaTime)
        move(spaceObject, width, height, deltaTime)
    }
}

We have a version of move that accepts double. Therefore change the types here. I do think we want to keep these calls parameterized by screen parameters. We just want them to be double.

Why not use the globals down in move? I guess we could. Let’s try that, bottom up.

fun move(spaceObject: SpaceObject, width: Int, height: Int, deltaTime: Double) {
    move(spaceObject, width.toDouble(), height.toDouble(), deltaTime)
}

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
}

If we knew the screen is square … but no. Change the bottom move:

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

Change its signature:

fun move(spaceObject: SpaceObject, deltaTime: Double) {
    with (spaceObject) {
        x = limit(x+dx*deltaTime, U.ScreenWidth)
        y = limit(y+dy*deltaTime, U.ScreenHeight)
    }
}

Find callers of the upper one. I think there is just the one.

{
    updateTimers(deltaTime)
    for (spaceObject in spaceObjects) {
        for (component in spaceObject.components) update(component, deltaTime)
        if (spaceObject.type == SpaceObjectType.SHIP) applyControls(spaceObject, deltaTime)
        move(spaceObject, deltaTime)
    }
}

Remove the upper one as unused. Test. Two tests fail, probably similar to the other situation. Irritating. Here are the tests:

    @Test
    fun `asteroid wraps high`() {
        val spaceObject = SpaceObject(SpaceObjectType.ASTEROID,95.0, 98.0, 10.0, 10.0)
        move(spaceObject, 1.0)
        assertThat(spaceObject.x).isEqualTo(5.0, within(0.01))
        assertThat(spaceObject.y).isEqualTo(8.0, within(0.01))
    }

    @Test
    fun `asteroid wraps low`() {
        val spaceObject = SpaceObject(SpaceObjectType.ASTEROID, 5.0, 8.0, -10.0, -10.0,)
        move(spaceObject, 1.0)
        assertThat(spaceObject.x).isEqualTo(95.0, within(0.01))
        assertThat(spaceObject.y).isEqualTo(98.0, within(0.01))
    }

I think we’ve just learned that using the globals down that low is a poor idea. Revert.

Let’s go the whole way in the other direction: we’ll have the width and height continue to be passed in to gameCycle from main … and into startGame as well. Let’s undo the whole morning. I think IDEA can undo a commit.

Yes, we are clear back to U.ScreenWidth and Height as Int.

Low-hanging fruit my aging caboose!

All I really wanted to do was to get rid of that one move function that takes the Ints. Let’s go this way:

fun move(spaceObject: SpaceObject, width: Int, height: Int, deltaTime: Double) {
    move(spaceObject, width.toDouble(), height.toDouble(), deltaTime)
}

We’ll inline it!

private fun updateEverything(
    spaceObjects: Array<SpaceObject>,
    deltaTime: Double,
    width: Int,
    height: Int
) {
    updateTimers(deltaTime)
    for (spaceObject in spaceObjects) {
        for (component in spaceObject.components) update(component, deltaTime)
        if (spaceObject.type == SpaceObjectType.SHIP) applyControls(spaceObject, deltaTime)
        move(spaceObject, width.toDouble(), height.toDouble(), deltaTime)
    }
}

I knew it was low-hanging! I just got distracted by a chrome squirrel¹.

Green. Commit: inline move function to its one caller.

Reflection

What we see here are a couple of mistakes.

First, the globals. As it stands, the game is pretty well parameterized with regard to screen size, in that the key functions that apply limits accept the size as parameters, which means that we can test them with unusual values and see that they work. What’s the possible error? Suppose that we wrapped not at a width parameter, but instead in a fit of efficiency madness, wrapped at 1024. That’s a bug waiting to happen if we ever change the screen size. And since it is a constant in the U object, the implication is that we might.

So converting to use the constant down low in the code kept the code working, but it made tests fail. Making those tests work involved either removing potentially useful checks, which I did, or changing the values in the test to use the defaults, which would allow errors like setting the ship position to 512 instead of U.ScreenWidth/2.

Pushing globals down in the code saves parameters in calling sequences, but bears risks.

My second mistake was to let my attention move away from the very simple in-lining operation allowing removal of the redundant move that I was after. I got to thinking about why there were both Int and Double height and width and started to scurry around to catch that squirrel. Only when I realized how many tests I’d be trashing did I come to my senses and do the simple thing.

Now there still is a bit of confusion between the Int and Double, there’s no denying that. I think that we could improve things, perhaps by making the core values Double and down-casting them only where needed, probably in the main program.

But it’s not bothering anyone. I just got distracted.

OK, Where Were We?

Oh, right, wanting to clean things up. I’m 300 lines into this article and have carefully removed one one-line function. Productivity a bit low this morning. Let’s do some other things.

What if we were to move the points and other drawing code into a new file, Drawing.kt?

We have to declare the points public so that the enum can see them, but that’s fine, they won’t bother anyone over in the new Drawing file. I move the two draw functions over as well.

val asteroidPoints = listOf(...)
val missilePoints = listOf(...)
val saucerPoints = listOf(...)
val shipPoints = listOf(...)
val shipFlare = listOf(...)

fun draw(spaceObject: SpaceObject, drawer: Drawer, deltaTime: Double) {
    drawer.isolated {
        val scale = 4.0 *spaceObject.scale
        drawer.translate(spaceObject.x, spaceObject.y)
        drawer.scale(scale, scale)
        drawer.rotate(spaceObject.angle)
        drawer.stroke = ColorRGBa.WHITE
        drawer.strokeWeight = 1.0/scale
        shipSpecialHandling(spaceObject, drawer, deltaTime)
        drawer.lineStrip(spaceObject.type.points)
    }
}

private fun shipSpecialHandling(spaceObject: SpaceObject, drawer: Drawer, deltaTime: Double) {
    if (spaceObject.type == SpaceObjectType.SHIP) {
        dropScale = max(dropScale - U.ShipDropInScale*deltaTime, 1.0)
        drawer.scale(dropScale, dropScale)
        if (Controls.accelerate && Random.nextInt(1, 3) == 1) {
            drawer.lineStrip(shipFlare)
        }
    }
}

Commit: move points and drawing to Drawing.kt.

Now SpaceObject.kt contains much less. I think I’ll alphabetize its functions, and spare you the printout. Green. Commit: reorder functions, tidying.

We’re still left with Game.kt. Let’s see if we can detect any major subgroups. Certainly there are game creation, game cycling, maybe collision handling. What else?

Well, there is the Control object:

object Controls {
    var left: Boolean = false
    var right: Boolean = false
    var accelerate: Boolean = false
    var fire: Boolean = false
    var hyperspace: Boolean = false
}

There is the universal constant object U with its 22 lines of useful constants.

There are some key game-related variables:

var Score: Int = 0
lateinit var SpaceObjects: Array<SpaceObject>
lateinit var Ship: SpaceObject
lateinit var Saucer: SpaceObject
var TimerTable: List<Timer> = mutableListOf<Timer>()

And there are three other vars, currently stored near the functions that use them. I’ll move them up with the others. No, there were four. I alphabetize them on general principles:

var Score: Int = 0
var AsteroidsGoneFor = 0.0
private var currentWaveSize = 0
var dropScale = U.ShipDropInScale
lateinit var Saucer: SpaceObject
var saucerSpeed = U.SaucerSpeed
lateinit var Ship: SpaceObject
lateinit var SpaceObjects: Array<SpaceObject>
var TimerTable: List<Timer> = mutableListOf<Timer>()

Test. Green. Commit: move all variables in Game together.

I think I’ll do one ore thing and call it a morning. How about moving all the game creation stuff to its own file. Create a new file Creation.kt. First move createGame:

fun createGame(missileCount: Int, asteroidCount: Int) {
    Score = 0
    val objects = mutableListOf<SpaceObject>()
    for (i in 1..missileCount) objects.add(newMissile())
    Ship = newShip()
    objects.add(Ship)
    Saucer = newSaucer()
    objects.add(Saucer)
    for (i in 1..asteroidCount) objects.add(newAsteroid())
    SpaceObjects = objects.toTypedArray()
}

Green, might as well commit: New file creation includes createGame.

Now I think I’ll move over the various newThisAndThat functions. IDEA helps with its Move capability. Click the function name, F6, and specify the file. Slightly easier than cut paste, and more likely to be correct. Now Creation.kt looks like this:

fun createGame(missileCount: Int, asteroidCount: Int) {
    Score = 0
    val objects = mutableListOf<SpaceObject>()
    for (i in 1..missileCount) objects.add(newMissile())
    Ship = newShip()
    objects.add(Ship)
    Saucer = newSaucer()
    objects.add(Saucer)
    for (i in 1..asteroidCount) objects.add(newAsteroid())
    SpaceObjects = objects.toTypedArray()
}

fun newAsteroid(): SpaceObject = SpaceObject(SpaceObjectType.ASTEROID, 0.0, 0.0, 0.0, 0.0, 0.0, false)
    .also { it.scale = 4.0 }

fun newMissile(): SpaceObject {
    return SpaceObject(SpaceObjectType.MISSILE, 0.0, 0.0, 0.0, 0.0, 0.0, false)
        .also { spaceObject ->
            val missileTimer = Timer(spaceObject, U.MissileTime, true) { timer -> deactivate(timer.entity) }
            addComponent(spaceObject, missileTimer)
        }
}

fun newSaucer(): SpaceObject = SpaceObject(SpaceObjectType.SAUCER, 0.0, 0.0, 0.0, 0.0, 0.0, false).also { addComponent(it, SaucerTimer(it)) }

fun newShip(): SpaceObject = SpaceObject(SpaceObjectType.SHIP, 0.0, 0.0, 0.0, 0.0, 0.0, false)
    .also { spaceObject ->
        val shipTimer = Timer(
            spaceObject,
            U.ShipDelay,
            false,
            { timer: Timer -> safeToEmerge(timer) }
        ) { activateShip() }
        addComponent(spaceObject, shipTimer)
    }

Game is still 336 lines, 44 lines shorter, and SpaceObject is down to 84, formerly 152.

So we’ve improved our navigation issues a little bit and we’re green. Commit: move new object creation to Creation.kt.

Let’s sum up and get out of here.

Summary

All the actual work was quick, easy, and broke nothing. I did get seriously distracted in a vain attempt to sort out duplicated code at the same time as an Int vs Double issue that wasn’t really bothering anyone. It happens sometimes, that we think some simple thing needs improvement, and we set out to do it and it turns into a mess. The trick is to notice quickly and revert. I actually reverted a whole commit this time. Woot!

All we did today was clean up the work area. That’s going to help us go a bit faster and some days about all we’ve got going for us is a willingness to do some simple cleanup. If you’re like me, and tend to leave a bit of a mess in your excitement, those days are actually pretty valuable.

Watch out for squirrels, and I’ll see you next time!