GitHub Decentralized Repo
GitHub Centralized Repo

I’ll begin this new year as I ended the old one, programming and writing about it. Today, I think we’ll work on score.

Happy New Year! I hope that it is a much better year for all of us. We’ll have to work to make it so, each in our own way. I wish us all the best of fortune.

We’re working to move the Asteroids design away from the current decentralized version, where there are all kinds of objects in the mix, interacting in unique ways. We’re moving toward a more centralized design, with a game object that knows more about the objects and manages them more directly. We’re not hoping to make a ridiculous monolithic game, just one where the game object knows more about the game than our current one does.

In the decentralized version, the game object knows essentially nothing about what kind of game it even is. It knows to insert a Quarter object into its mix. The Quarter object knows to insert a few objects, a ScoreKeeper, WaveMaker, SaucerMaker and a ShipChecker that has a Ship. The Quarter object doesn’t know what those objects do. The actual game play emerges from how those objects interact. I could readily convert the game to SpaceWar, possibly even SpaceInvaders, just by changing the mix.

But that design is complex in a few ways that we might consider either too general for the situation in hand (Asteroids), or too hard to understand. Certainly a new team member, if just handed the code, might find it difficult to understand where the game comes from.

I don’t think it’s too complex: I think it’s rather nice. But I do want to compare it to a more obvious kind of design. Rather than start over, I’ve decided to refactor the existing game into a more conventional form, so I’ve forked my own project and am presently working in the one that’s becoming more centralized.

Quarter

Last year, yesterday, we moved the game setup into Game class, removing the Quarter object, instead doing the adding of objects in Game.

ScoreKeeping

Today I propose to start moving score-keeping into Game. I have some starting ideas. Let’s begin by thinking about how it works now.

There is a ScoreKeeper object in the mix. It’s created when the game starts and it stays in existence throughout. It maintains a single variable, the score of the game, totalScore. It has one interaction, with Score objects, where it simply accumulates their score into itself.

    interactWithScore = { score, _ -> totalScore += score.score },
Aside
We might have done that differently. We could have had the scorekeeper have no interactions at all, and had the Score object implement an interaction with ScoreKeeper, where it sends the score to the ScoreKeeper, probably via a call like scoreKeeper.addScore(this.score). As things stand, the Score just removes itself upon interacting:
class Score(val score: Int): ISpaceObject, InteractingSpaceObject {

    override val subscriptions = Subscriptions(
        interactWithScoreKeeper = {_, trans -> trans.remove(this) }
    )

    override fun callOther(other: InteractingSpaceObject, trans: Transaction) {
        other.subscriptions.interactWithScore(this, trans)
    }

    override fun update(deltaTime: Double, trans: Transaction) { }
}

Either way would have worked, although right now I almost wish I had done it the other way. As we work through this change, perhaps we’ll see why: I think we’ll add an addScore function now anyway. Shouldn’t make much difference, it’s just one function.

My Cunning Plan

Well, I don’t really have enough of a plan to call it cunning. I do have a notion, that we’ll do something like this:

  1. We’ll have the SpaceObjectCollection, knownObjects, implement a new function scoreKeeper, that returns the ScoreKeeper;
  2. Then, I’m thinking, when a Score instance is added via a transaction, we’ll have knownObjects detect it and apply the score directly to the ScoreKeeper, instead of adding it to the mix.

On the plus side of thinking about this, we already have knownObjects checking the types of its transactions, so as to parse out the attackers and targets. Therefore this is just a continuation of that design decision.

On the minus side, checking object types isn’t a good design move, in my view, but I see it as an interim step toward all the objects of interest being known in the game object.

I think we should drive this change with some tests. I think I’ll start in SpaceObjectCollectionTest class.

    @Test
    fun `can return the ScoreKeeper`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        val found = s.scoreKeeper()
        assertThat(found).isEqualTo(keeper)
    }

This won’t compile, for want of the scoreKeeper function. I’ll try this:

    fun scoreKeeper(): ScoreKeeper? {
        return spaceObjects.find {
            it is ScoreKeeper
        } as ScoreKeeper
    }

It would of course be possible to check for ScoreKeeper on adding and retain it but this seems better to me for some reason. I think we may have some issues with the nullability but IDEA isn’t complaining yet.

Test. Green. Let’s enhance the test just a bit.

    @Test
    fun `can return the ScoreKeeper`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        assertThat(s.scoreKeeper()).isEqualTo(null)
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        val found = s.scoreKeeper()
        assertThat(found).isEqualTo(keeper)
    }

Test. Fails, because the implementation can’t cast the null. This implementation runs green:

    fun scoreKeeper(): ISpaceObject? {
        return spaceObjects.find {
            it is ScoreKeeper
        }
    }

We’ll worry about casting in what follows. We are green. Commit: knownObjects can return score keeper.

Now let’s update the score keeper via the transaction, without adding the Score object to the mix:

    @Test
    fun `score accumulates without updating mix`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        val scoreTrans = Transaction()
        val score = Score(123)
        scoreTrans.add(score)
        s.applyChanges(scoreTrans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        assertThat(keeper.totalScore).isEqualTo(123)
    }

This will fail on the second check of size. It does. Implement. Here’s where all the nasty stuff is now:

    fun add(spaceObject: ISpaceObject) {
        spaceObjects.add(spaceObject)
        if (spaceObject is Missile) attackers.add(spaceObject)
        if (spaceObject is Ship) {
            attackers.add(spaceObject)
            targets.add(spaceObject)
        }
        if (spaceObject is Saucer)  {
            attackers.add(spaceObject)
            targets.add(spaceObject)
        }
        if (spaceObject is Asteroid) targets.add(spaceObject)
    }

We want to deal with Score before we do any adding:

    fun add(spaceObject: ISpaceObject) {
        if ( spaceObject is Score ) {
            (scoreKeeper() as ScoreKeeper).addScore(spaceObject.score)
            return
        }

This demands a new function on ScoreKeeper. We’ll implement it:

    fun addScore(score: Int) {
        totalScore += score
    }

And, even though I expect to remove this very soon, let’s be sure to use this function in our interaction:

        interactWithScore = { score, _ -> addScore(score.score) },

This was totally unnecessary, because the interaction can no longer occur, but it felt right to do it. I think our test should run. It does. I think the game should also run. And it does. Commit: knownObjects applies Score directly, does not add it to the mix.

Now we can remove much of the space code from Score, and some from ScoreKeeper as well.

class Score(val score: Int): ISpaceObject {
    override val subscriptions = Subscriptions()
    override fun callOther(other: InteractingSpaceObject, trans: Transaction) {}
    override fun update(deltaTime: Double, trans: Transaction) { }
}

A couple of tests fail:

    @Test
    fun `scorekeeper captures keeper vs other`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val p = Pair(keeper, score)
        val first = p.first
        val second = p.second
        val trans = Transaction()
        first.callOther(second, trans)
        second.callOther(first, trans)
        val discards = trans.removes.toList()
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

    @Test
    fun `scorekeeper captures other vs keeper`() {
        val score = Score(20)
        val keeper = ScoreKeeper()
        val p = Pair(score, keeper)
        val first = p.first
        val second = p.second
        val trans = Transaction()
        first.callOther(second, trans)
        second.callOther(first, trans)
        val discards = trans.removes.toList()
        assertThat(discards.size).isEqualTo(1)
        assertThat(discards).contains(score)
        assertThat(keeper.formatted()).isEqualTo("00020")
    }

These are redundant now. They’re checking the interaction between ScoreKeeper and Score, which no longer applies. Remove them. Commit: Remove most contents from Score object.

Let’s do the same with ScoreKeeper, which no longer needs to do much more than draw. Two functions go empty (but must still be overridden) and we only subscribe to draw:

    override val subscriptions = Subscriptions(
        draw = this::draw
    )

    override fun callOther(other: InteractingSpaceObject, trans: Transaction) {}

    override fun update(deltaTime: Double, trans: Transaction) {}

Test. Green. Commit: Remove unnecessary functions from ScoreKeeper.

We need to reflect.

Reflection

This is sort of done. We can’t really go much further as things stand but there are issues:

ScoreKeeper is in the mix
We have to add ScoreKeeper to the mix, so that it will draw. We could fix that by having the game draw it explicitly, perhaps pulling it out of knownObjects to do it.
Score and ScoreKeeper have to do the overrides
We are passing both around as ISpaceObject instances, which means that they must implement all those methods, none of which matter. I am hopeful that when we’ve completed more of this kind of work, we’ll be able to remove more.

Another possibility could be to implement another interface on top of ISpaceObject and InteractingSpaceObject, and to accept that interface in SpaceObjectCollection.

I noticed in writing this that InteractingSpaceObject is the top interface and ISpaceObject implements it. The names suggest the other order. That might want to be improved in the decentralized version. In this version, I think we’ll find an entirely different way to go, removing most of the hierarchy entirely.

interface InteractingSpaceObject {
    val subscriptions: Subscriptions
    fun callOther(other: InteractingSpaceObject, trans: Transaction)
}

interface ISpaceObject: InteractingSpaceObject {
    fun update(deltaTime: Double, trans: Transaction)
}

Action

Overall, the good news is that Score is not added to the mix. The less good news is that ScoreKeeper still is. Let’s change that. What if we continue to let knownObjects know the ScoreKeeper, but we create it directly? Then when we init the game, we’ll still create the ScoreKeeper, but we’ll pass it directly to knownObjects to be tucked away.

Let’s try that. I think we’ll change that test we wrote.

    @Test
    fun `can return the ScoreKeeper`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        assertThat(s.scoreKeeper()).isEqualTo(null)
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        val found = s.scoreKeeper()
        assertThat(found).isEqualTo(keeper)
    }

Let’s just set the keeper in there:

class SpaceObjectCollection {
    val spaceObjects = mutableListOf<ISpaceObject>()
    val attackers = mutableListOf<ISpaceObject>()
    val targets = mutableListOf<ISpaceObject>()
    var scoreKeeper = ScoreKeeper(0)

    fun add(spaceObject: ISpaceObject) {
        if ( spaceObject is ScoreKeeper) return
        if ( spaceObject is Score ) {
            scoreKeeper.addScore(spaceObject.score)
            return
        }

Remove the scoreKeeper function. Test, expecting that something has been forgotten. No, wait. The first test, shown above, is useless. The second one can handle our needs:

    @Test
    fun `score accumulates without updating mix`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        s.scoreKeeper = ScoreKeeper()
        assertThat(s.spaceObjects.size).isEqualTo(0)
        val scoreTrans = Transaction()
        val score = Score(123)
        scoreTrans.add(score)
        s.applyChanges(scoreTrans)
        assertThat(s.spaceObjects.size).isEqualTo(0)
        assertThat(keeper.totalScore).isEqualTo(123)
    }

This may run but things aren’t right yet. Test to see what happens. The score does not accumulate. And, when I run the game, I don’t get a ship, because it’s the ScoreKeeper that tells ShipChecker whether there is a ship to be made. This scheme cannot work. Roll it all back.

We’re green and the game works. Whew.

Here’s where the main rub was:

class ShipChecker
        afterInteractions = { trans ->
            if ( missingShip && (ship.inHyperspace || scoreKeeper.takeShip())) {
                trans.add(ShipMaker(ship, scoreKeeper))
                trans.remove(this)
            }
        }

Maybe this wasn’t as bad as I thought. I may have rolled back unnecessarily. But I’ve learned a bit and that will let me do this better. What we’re dealing with includes:

  1. knownObjects has the current ScoreKeeper, which retains the free ship count as well as totalScore;
  2. We want knownObjects not to put ScoreKeeper in the mix but to hold it separately;
  3. Therefore creating the game must provide a new ScoreKeeper directly to knownObjects, and also to the ShipChecker.
  4. There is some hanky-panky going on with the checker removing itself and the maker adding the checker back, but I think we can ignore that.

I think that in this light, both those new ScoreKeeper tests are invalid, and I’m afraid we need a new Game-creation test. We have no tests that really test game creation, but we do at least have a GameTest class. Let’s start there this time.

    @Test
    fun `game creation creates ScoreKeeper`() {
        val game = Game()
        val controls = Controls()
        game.insertQuarter(controls)
        val keeper: ScoreKeeper = game.knownObjects.scoreKeeper
        assertThat(keeper.shipCount).isEqualTo(U.SHIPS_PER_QUARTER)
    }

This won’t quite compile, because knownOtjects.scoreKeeper isn’t a ScoreKeeper … yet. So:

class SpaceObjectCollection {
    var scoreKeeper = ScoreKeeper()

That change demands this one:

    fun add(spaceObject: ISpaceObject) {
        if ( spaceObject is Score ) {
            scoreKeeper.addScore(spaceObject.score)
            return
        }

Now I’d really like not to allow insertion of a ScoreKeeper via transaction:

    fun add(spaceObject: ISpaceObject) {
        if ( spaceObject is ScoreKeeper) error("Do not add ScoreKeeper in transaction")
        if ( spaceObject is Score ) {
            scoreKeeper.addScore(spaceObject.score)
            return
        }

That should give me an error in the test. Ah, but to compile, I have to remove this useless test:

    @Test
    fun `can return the ScoreKeeper`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        assertThat(s.scoreKeeper()).isEqualTo(null)
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        val found = s.scoreKeeper()
        assertThat(found).isEqualTo(keeper)
    }

Fine. What else? My new test fails with the add error and so does the slightly less new test for scoring:

    @Test
    fun `score accumulates without updating mix`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        val scoreTrans = Transaction()
        val score = Score(123)
        scoreTrans.add(score)
        s.applyChanges(scoreTrans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        assertThat(keeper.totalScore).isEqualTo(123)
    }

I’ll deal with the new one and then the other. We need to change how adding a quarter works. That comes down to this:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        trans.add(scoreKeeper)
        trans.add(WaveMaker())
        trans.add(SaucerMaker())
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

And there, we need to put in the ScoreKeeper directly rather than via transaction. I think this does the trick:

    private fun createInitialObjects(
        trans: Transaction,
        shipCount: Int,
        controls: Controls
    ) {
        trans.clear()
        val scoreKeeper = ScoreKeeper(shipCount)
        knownObjects.scoreKeeper = scoreKeeper
        trans.add(WaveMaker())
        trans.add(SaucerMaker())
        val shipPosition = U.CENTER_OF_UNIVERSE
        val ship = Ship(shipPosition, controls)
        val shipChecker = ShipChecker(ship, scoreKeeper)
        trans.add(shipChecker)
    }

Test. The Game test runs. It just checks to be sure the right keeper is present:

    @Test
    fun `game creation creates ScoreKeeper`() {
        val game = Game()
        val controls = Controls()
        game.insertQuarter(controls)
        val keeper: ScoreKeeper = game.knownObjects.scoreKeeper
        assertThat(keeper.shipCount).isEqualTo(U.SHIPS_PER_QUARTER)
    }

The other test still fails and needs to be changed.

    @Test
    fun `score accumulates without updating mix`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        val trans = Transaction()
        trans.add(keeper)
        s.applyChanges(trans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        val scoreTrans = Transaction()
        val score = Score(123)
        scoreTrans.add(score)
        s.applyChanges(scoreTrans)
        assertThat(s.spaceObjects.size).isEqualTo(1)
        assertThat(keeper.totalScore).isEqualTo(123)
    }

If we add the keeper directly, that should be good to go.

    fun `score accumulates without updating mix`() {
        val s = SpaceObjectCollection()
        val keeper = ScoreKeeper()
        s.scoreKeeper = keeper
        assertThat(s.spaceObjects.size).isEqualTo(0)
        val scoreTrans = Transaction()
        val score = Score(123)
        scoreTrans.add(score)
        s.applyChanges(scoreTrans)
        assertThat(s.spaceObjects.size).isEqualTo(0)
        assertThat(keeper.totalScore).isEqualTo(123)
    }

Green. Does the game work? No … the score is not displayed. Have to do that explicitly when the game draws. We have to improve this:

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

This needs:

    private fun draw(drawer: Drawer) {
        knownObjects.forEach { drawer.isolated { it.subscriptions.draw(drawer) } }
        knownObjects.scoreKeeper.draw(drawer)
    }

Check the game again. We are good. I think we’ve made another step forward. Commit: ScoreKeeper is not in the mix, is drawn directly by Game. Score objects do not enter mix.

The length of that commit message tells us that the changes we just made were perhaps not as micro as they could have been, but I don’t see a smaller series of steps that could have done the job. And it really wasn’t a lot of changing. Let’s reflect again:

Reflection

We have removed both Score and ScoreKeeper from the mix. ScoreKeeper is added directly to the knownObjects, as an identified member rather than an anonymous collection member. Score, on the other hand, is still added via a transaction which is intercepted and the score extracted:

        if ( spaceObject is Score ) {
            scoreKeeper.addScore(spaceObject.score)
            return
        }

Therefore we can probably remove ScoreKeeper from the space object hierarchy, but we can’t do that with Score yet. Let’s try removing ScoreKeeper:

class ScoreKeeper(var shipCount: Int = 3): ISpaceObject {

Action

I’ll just remove the inheritance and see what overrides I can remove. These are right out:

    override val subscriptions = Subscriptions(
        draw = this::draw
    )

    override fun callOther(other: InteractingSpaceObject, trans: Transaction) {}

    override fun update(deltaTime: Double, trans: Transaction) {}

Will this compile and run? I think it should. Kotlin will tell me if I’m mistaken. It reminds me to remove this, which can no longer happen:

        if ( spaceObject is ScoreKeeper) error("Do not add ScoreKeeper in transaction")

Tests are green. Game runs. Commit: ScoreKeeper no longer inherits from ISpaceObject.

Sweet. We’ve reduced the size of ScoreKeeper, which is down now to adding up the score, counting the ships, and drawing itself, which is its whole useful capability, and we’ve been able to remove three override methods and removed it from the hierarchy entirely. It’s now just a small free-standing class.

Let’s sum up.

Summary

This is enough for New Year’s Day. In a few simple steps (with one misstep which probably could have been recovered without a rollback), we’ve removed the score-keeping notion from the mix. One of the objects involved is completely out from under the mix hierarchy, and the other still exists in the hierarchy but has empty implementations for all the overrides. That is, of course, a solid hint that it wants to be out of the hierarchy, but we can’t quite do that yet, because a Score is created by an Asteroid when it splits, and a saucer when it dies:

class Asteroid
        interactWithAsteroid = { asteroid, trans ->
            if (checkCollision(asteroid)) {
                if (missileIsFromShip) trans.add(asteroid.getScore())
                terminateMissile(trans)
            }
        },

    fun getScore(): Score {
        return Score(
            when (splitCount) {
                2 -> 20
                1 -> 50
                0 -> 100
                else -> 0
            }
        )
    }
class Saucer
    fun getScore() = Score(200)

class Missile
        interactWithSaucer = { saucer, trans ->
            if (checkCollision(saucer)) {
                if (missileIsFromShip) trans.add(saucer.getScore())
                terminateMissile(trans)
            }
        },

This is a bit odd, isn’t it? The Asteroid adds its own score, while the Saucer’s score gets added by the missile. You’d think we’d pick a way and use it. This is a kind of hidden complexity of the decentralized design: there is more than one perfectly good way of doing some things. Ideally, there’d be only one way. Less ideally, we’d always use the same way. Still less ideally, which seems to be what I’ve picked, we’d do the same thing in different ways.

In both styles, however, we adjust the score by adding the Score object to a transaction. The Score gets intercepted in applyChanges, and the score is added to the total. We’d like to do this more directly, but as things work now, the transaction is the only connection between the game and its knownObjects, and the objects themselves. That is, arguably, part of the elegance of the decentralized mode: the game and its objects are not cognizant of what kind of objects there are. In addition, it doesn’t know what kind of objects there are. It is ignorant of … well, you get the idea.

We’re probably going to want a more direct connection between the game and its objects … although, we can imagine a transaction as a sort of message pouch that is passed around and processed at suitable times during game execution. So we might evolve toward a situation where, instead of tossing objects into the mix, the transaction is decoded more and more, in this code:

    fun add(spaceObject: ISpaceObject) {
        if ( spaceObject is Score ) {
            scoreKeeper.addScore(spaceObject.score)
            return
        }
        spaceObjects.add(spaceObject)
        if (spaceObject is Missile) attackers.add(spaceObject)
        if (spaceObject is Ship) {
            attackers.add(spaceObject)
            targets.add(spaceObject)
        }
        if (spaceObject is Saucer)  {
            attackers.add(spaceObject)
            targets.add(spaceObject)
        }
        if (spaceObject is Asteroid) targets.add(spaceObject)
    }

As things evolve, more and more of the actions taken might be direct, like the first one. The main objects, missile, saucer, ship, and asteroid, should finally be the only objects in knownObjects, with everything else dealt with more directly, as we do now.

We’ll be left with the ignorance gap between game and knownObjects. Game knows a bit more about knownObjects now, having injected the ScoreKeeper. As time goes on, we’ll need to deal with making waves (fun!) and saucers and ships, all without the clever mix objects.

I don’t see just how we’ll do that. I am confident that we’ll find reasonable ways … and that those reasonable ways will suggest refactorings.

We’re moving along the path to centralization. It’s going pretty well.

Emphasis

Think about this! We are completely changing the architecture of this program, from decentralized cooperating objects to a more centralized style. And we are doing it in small steps, with the game working at each stage.

I wonder … in situations where I’ve called for a rewrite of a product that needed a better design or architecture … could we perhaps have refactored instead? I’ve done some long, grueling projects replacing an existing product, and they’ve always been hard and often have ultimately failed ever to catch up.

If I were condemned to work on a real project and folks were asking for a rewrite … I can assure you that I’d look very hard to find a way to move to a new design incrementally rather than via a rewrite.

Maybe there’s a little lesson here.

For now, things are going very nicely.

What will we do next? I don’t know. I’ll decide tomorrow, probably. Join me then?