Kotlin 206 - Opportunities
GitHub Decentralized Repo
GitHub Centralized Repo
This morning, we Big Plan a bit, then do small changes. We’re refactoring toward a new design: we’re not doing a big rewrite. It’s going quite well.
Plan
The overall plan here was to move the design of Asteroids away from the very decentralized design, where everything that happens is based on the interaction of individual objects with very limited responsibility. Instead, the plan is to move most of what happens toward the “center” of the game, that is the Game object and a few larger-scale collaborators. This latter design is more like the design of the original program, which had no real object-orientation at all, and, I think, it’s more like the naive design one might use for. a small program like asteroids.
I don’t mean “naive” in a pejorative sense. I think there is a natural stage of programming that most of us go through, where we tend to build large main programs, large functions, and, when we do build objects, large objects with many related, and sometimes unrelated, responsibilities. I know that I went through that stage and even today I sometimes let things get too large before I break them apart. I suppose I’d call all the ideas that I passed through in the past “naive” and all the ones that I’m aspiring to, perhaps “sophisticated”. And the ones I use now? Maybe “standard”.
I jest, but there is, I think, a kind of growth path that we follow, with things that we found useful in the past becoming supplanted with different things.
Terms and trajectories aside, the centralized approach is, I think, more common because it’s more obvious, and because it’s more clear that it will work. Certainly when I started with the decentralized implementation, I thought that it might work, but I was aware that it might not. I wasn’t afraid, because what I write about is what happens, and something was sure to happen. Either way, I get articles out of what happens.
How’s it going?
Thanks, I went off track there. It’s going well. We’ve removed a number of objects entirely, including WaveMaker, SaucerMaker, ShipChecker, ShipMaker. Their responsibilities have all been moved into a few additional functions in Game. As it stands, here’s how we initialize the game:
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls
) {
cancelAllOneShots()
trans.clear()
saucer.initialize()
scoreKeeper = ScoreKeeper(shipCount)
knownObjects.scoreKeeper = scoreKeeper
val shipPosition = U.CENTER_OF_UNIVERSE
ship = Ship(shipPosition, controls)
}
In the decentralized version we start the game by adding a Quarter object to the knownObjects, and when that object runs, it does this:
// DECENTRALIZED VERSION
class Quarter
override fun update(deltaTime: Double, trans: Transaction) {
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)
}
Notice that it doesn’t add any asteroids or ship. It adds four objects, none of which is actually involved in flying around playing the game. Then, as time unfolds, the WaveMaker makes asteroids, the ShipChecker notices that the ship is missing and makes a ShipMaker, the ShipMaker makes a ship, the SaucerMaker makes a saucer … and we have a game.
Of the objects installed by the old version’s initialization, the only one we have retained at all is the ScoreKeeper, and it’s no longer actually in the mix. It’s held onto by knownObjects, and it is drawn separately when the game draws things, not as part of the mix as in the decentralized version.
So it’s going well: we’ve removed all the objects that were in the mix but not on screen, and we’ve even separated out the score-keeping from the actual asteroids, missiles, ship, and saucer that make up the game.
Better yet, we’ve done it day by day, with the game continually working just fine. A green check mark in the win column for incremental design change.
Is there bad news?
I wouldn’t say bad, but there are things of concern. Game presently has at least these responsibilities:
- Initialize the game;
- Keep track of time;
- Run the game cycle;
- Manage interactions between the flying objects;
- Keep track of how many asteroids there are;
- Create waves of asteroids as needed;
- Create the saucer as needed;
- Create the ship as needed.
- Determine when it is safe for the ship to emerge
- Draw everything that needs drawing.
The top level of the game cycle is this:
fun cycle(elapsedSeconds: Double, drawer: Drawer? = null) {
val deltaTime = elapsedSeconds - lastTime
lastTime = elapsedSeconds
tick(deltaTime)
beginInteractions()
processInteractions()
finishInteractions()
U.AsteroidTally = knownObjects.asteroidCount()
createNewWaveIfNeeded()
createSaucerIfNeeded()
createShipIfNeeded()
drawer?.let { draw(drawer) }
}
Game class is about 180 lines. Most of the objects that it’s concerned about are kept in knownObjects, an instance of SpaceObjectCollection, which is itself around 100 lines1. Game is the class that is most concerning, because it has so many responsibilities glommed into it. SpaceObjectCollection is less of a concern, but could probably use some improvement.
Are there other concerns?
We have a few stickies in Jira2 that are worth mentioning:
- Need new test for asteroid
howManyfunction; - Test Scorekeeper
takeShip? - Ship
lateinitin Game - Shouldn’t still have Score??
- Test hyperspace return distance;
- Rename ShipCheckerAndMakerTest
None of these are terribly important but they are all worth doing at some point, presumably the sooner the better, as they are piling up.
What else comes to mind?
The current hierarchy is better than what we had but not perfect. It is this:
interface SpaceObject {
}
interface InteractingSpaceObject: SpaceObject {
val subscriptions: Subscriptions
fun callOther(other: InteractingSpaceObject, trans: Transaction)
fun update(deltaTime: Double, trans: Transaction)
}
All of Asteroid, Missile, Ship, and Saucer are happy to implement all three of those requirements. However, DeferredAction really only implements update, and Score has all three of them empty. Score, one would think, could implement SpaceObject rather than InteractingSpaceObject.
Let’s try that right now. That works. Nice. Commit: Score implements SpaceObject, not InteractingSpaceObject. Has no behavior at all.
class Score(val score: Int): SpaceObject {
}
I guess it would be possible to jigger the hierarchy to allow DeferredAction to implement only update but that would require a whole new layer in the hierarchy, which isn’t worth it at this point. Maybe later, but I think that the interaction code will move into Game, simplifying the hierarchy that way.
Let’s look a bit more at Score. It scarcely partakes of being a SpaceObject at all.
We create Score objects when the player scores points, and put them in the Transaction. For example:
interactWithAsteroid = { asteroid, trans ->
if (checkCollision(asteroid)) {
if (missileIsFromShip) trans.add(asteroid.getScore())
terminateMissile(trans)
}
As soon as the Score instance appears during application of the Transaction in knownObjects, it is handled specially:
fun add(spaceObject: SpaceObject) {
when (spaceObject) {
is Score -> scoreKeeper.addScore(spaceObject.score)
is DeferredAction -> deferredActions.add(spaceObject)
else -> addActualSpaceObjects(spaceObject as InteractingSpaceObject)
}
}
If we were to make the Transaction just a little bit smarter, so that it has a score member, containing an Int score value, we could simply add that value to the scorekeeper right at the beginning of applying the transaction. I think that would amount to a net simplification. Let’s do it.
We can do this incrementally. First I’ll add the score member to Transaction.
Hold on there, podner. Let’s take a deeper look at Transaction first.
class Transaction {
val adds = mutableSetOf<SpaceObject>()
val removes = mutableSetOf<SpaceObject>()
var shouldClear = false
fun accumulate(t: Transaction) {
t.adds.forEach {add(it)}
t.removes.forEach {remove(it)}
}
We have the ability to accumulate transactions into one, and we use it. No, wait, we only have a test for it. Remove the test and the method. Green. Commit: remove unused accumulate function.
That makes me a bit more happy but look at how we apply transactions:
class SpaceObjectCollection
fun applyChanges(transaction: Transaction) = transaction.applyChanges(this)
class Transaction
fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
if (shouldClear ) spaceObjectCollection.clear()
spaceObjectCollection.removeAndFinalizeAll(removes)
spaceObjectCollection.addAll(adds)
}
As things stand, rather than rip the guts out of Transaction, it sends fairly sensible messages to SpaceObjectCollection. That’s OK. We’ll do the score thing, in stages. Let’s do a test for this.
@Test
fun `accumulates score via member`() {
val coll = SpaceObjectCollection()
val scoreKeeper = ScoreKeeper(0)
coll.scoreKeeper = scoreKeeper
val trans = Transaction()
trans.addScore(15)
trans.addScore(27)
coll.applyChanges(trans)
assertThat(scoreKeeper.totalScore).isEqualTo(42)
}
This demands addScore on Transaction. Shall we do this in tiny steps? OK.
class Transaction
fun addScore(i: Int) {
}
This will fail on the assert.
expected: 42
but was: 0
Make it work:
class Transaction {
val adds = mutableSetOf<SpaceObject>()
val removes = mutableSetOf<SpaceObject>()
private var shouldClear = false
private var score = 0
fun addScore(scoreToAdd: Int) {
score += scoreToAdd
}
This will still not pass, until we do this:
fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
if (shouldClear ) spaceObjectCollection.clear()
spaceObjectCollection.addScore(score)
spaceObjectCollection.removeAndFinalizeAll(removes)
spaceObjectCollection.addAll(adds)
}
This demands a method on SpaceObjectCollection. Forgive me, I’m going to implement it.
class SpaceObjectCollection
fun addScore(score: Int) {
scoreKeeper.addScore(score)
}
ScoreKeeper already knows addScore so we expect a win. We are green.
Now what about the clear flag? Should it clear the scoreKeeper? It need not, because we actually create a new one when we clear:
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls
) {
cancelAllOneShots()
trans.clear()
saucer.initialize()
scoreKeeper = ScoreKeeper(shipCount)
knownObjects.scoreKeeper = scoreKeeper
val shipPosition = U.CENTER_OF_UNIVERSE
ship = Ship(shipPosition, controls)
}
However, I can imagine someone forgetting that, and it’s clear (!) that we intend the score to drop to zero, so let’s add another test.
@Test
fun `clears score on clear`() {
val coll = SpaceObjectCollection()
val scoreKeeper = ScoreKeeper(0)
coll.scoreKeeper = scoreKeeper
val trans = Transaction()
trans.addScore(15)
trans.addScore(27)
coll.applyChanges(trans)
assertThat(scoreKeeper.totalScore).isEqualTo(42)
val newTrans = Transaction()
newTrans.clear()
coll.applyChanges(newTrans)
assertThat(scoreKeeper.totalScore).isEqualTo(0)
}
This will fail expecting zero, with 42.
expected: 0
but was: 42
Fix clear:
class SpaceObjectCollection
fun clear() {
scoreKeeper.clear()
for ( coll in allCollections()) {
coll.clear()
}
}
ScoreKeeper does not know clear yet:
class ScoreKeeper
fun clear() {
totalScore = 0
}
Should be green. Green it is. Commit: Transaction, SpaceObjectCollection and ScoreKeeper accept addScore call to add to the score and clear to clear it.
Now we can use the new capability and, in case you hadn’t noticed, the regular Score objects work as well … for now. We can now replace those and we can do it one at a time if we care to.
There are six references to Score:
class SpaceObjectCollection
fun add(spaceObject: SpaceObject) {
when (spaceObject) {
is Score -> scoreKeeper.addScore(spaceObject.score)
is DeferredAction -> deferredActions.add(spaceObject)
else -> addActualSpaceObjects(spaceObject as InteractingSpaceObject)
}
We’ll rid ourselves of that soon.
In Missile, we call asteroid.getScore:
class Missile
override val subscriptions = Subscriptions(
interactWithAsteroid = { asteroid, trans ->
if (checkCollision(asteroid)) {
if (missileIsFromShip) trans.add(asteroid.getScore())
terminateMissile(trans)
}
},
class Asteroid
fun getScore(): Score {
return Score(
when (splitCount) {
2 -> 20
1 -> 50
0 -> 100
else -> 0
}
)
}
Let’s change getScore to return the Int and fix Missile:
class Asteroid
fun getScore(): Int {
return when (splitCount) {
2 -> 20
1 -> 50
0 -> 100
else -> 0
}
}
class Missile
interactWithAsteroid = { asteroid, trans ->
if (checkCollision(asteroid)) {
if (missileIsFromShip) trans.addScore(asteroid.getScore())
terminateMissile(trans)
}
},
This should run green. It does. Commit: missile-asteroid collision uses addScore instead of Score object.
We have the same situation in missile-saucer:
class Missile
interactWithSaucer = { saucer, trans ->
if (checkCollision(saucer)) {
if (missileIsFromShip) trans.add(saucer.getScore())
terminateMissile(trans)
}
},
I’ll change it from this side:
interactWithSaucer = { saucer, trans ->
if (checkCollision(saucer)) {
if (missileIsFromShip) trans.addScore(saucer.getScore())
terminateMissile(trans)
}
},
That demands this change:
class Saucer
fun getScore() = 200
Should be green. Yes. Commit: missile-saucer collision uses addScore instead of Score object.
Who else uses Score?
Just that reference in SpaceObjectCollection, and two tests that use Score objects. Not a very busy object, was it? I remove this test:
@Test
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)
}
With this test:
@Test
fun `clear clears all sub-collections`() {
val s = SpaceObjectCollection()
s.add(Missile(Ship(U.CENTER_OF_UNIVERSE)))
s.add(Asteroid(Point.ZERO))
s.add(Score(666))
val deferredAction = DeferredAction(3.0, Transaction()) {}
s.add(deferredAction)
s.clear()
for ( coll in s.allCollections()) {
assertThat(coll).isEmpty()
}
}
We don’t need to add the score, we were just testing to be sure it didn’t wind up in some collection. Remove that line only. Now in SpaceObjectCollection:
fun add(spaceObject: SpaceObject) {
when (spaceObject) {
is Score -> scoreKeeper.addScore(spaceObject.score)
is DeferredAction -> deferredActions.add(spaceObject)
else -> addActualSpaceObjects(spaceObject as InteractingSpaceObject)
}
}
Remove the is Score line. Score is never referenced. Safe Delete it. Tests all green. Commit: Remove Score class using Transaction.addScore instead.
There are now no direct implementors of SpaceObject. Everyone implements InteractingSpaceObject, and SpaceObject has no contents. Disconnect InteractingSpaceObject.
interface InteractingSpaceObject {
val subscriptions: Subscriptions
fun callOther(other: InteractingSpaceObject, trans: Transaction)
fun update(deltaTime: Double, trans: Transaction)
}
Should be green. No, we can’t quite do that because people are expecting SpaceObject. I was going to remove and rename. I’ll do it in one step. Don’t know if IDEA knows how to do this, I’ll just manually rename the SpaceObject and then rename Interacting.
interface SpaceObject {
val subscriptions: Subscriptions
fun callOther(other: SpaceObject, trans: Transaction)
fun update(deltaTime: Double, trans: Transaction)
}
That satisfies everyone and the tests are green. Commit: Simplify hierarchy to SpaceObject only, including subscriptions, callOther and update.
Some warnings arise. Here:
fun add(spaceObject: SpaceObject) {
when (spaceObject) {
is DeferredAction -> deferredActions.add(spaceObject)
else -> addActualSpaceObjects(spaceObject as SpaceObject)
}
}
We don’t need the cast any more. And here:
fun removeAndFinalizeAll(moribund: Set<SpaceObject>) {
moribund.forEach {
if ( it is SpaceObject) addAll(it.subscriptions.finalize())
}
removeAll(moribund)
}
We don’t need to check: everything is a SpaceObject now:
fun removeAndFinalizeAll(moribund: Set<SpaceObject>) {
moribund.forEach {
addAll(it.subscriptions.finalize())
}
removeAll(moribund)
}
Retest and commit. Nice.
This is a good stopping point. Let’s sum up.
Summary
Following our nose, we first changed Score’s inheritance to reflect that it had no behavior. That led to the idea of accumulating scores more directly, putting them as a separate field in Transaction. Test-driving, we implemented that feature, then applied that feature to the (only two) uses of the Score object. We removed redundant tests and the Score object.
Then we cleaned up the hierarchy, eliminating the unneeded empty layer. Along the way we removed a few rough edges.
We now have only one object that is “special”, the DeferredAction. DeferredAction goes into the transaction as if it were a standard mix object, but it is intercepted by SpaceObjectCollection, into its own sub-collection there.
Even as things stand, the Missile, Ship, Saucer, and Asteroids are nicely broken out into attacker and target collections, as well as showing up in the general list spaceObjects. We could remove that collection and iterate the other two, if we cared to. I think we’re moving in a different direction however.
The DeferredAction objects go into the deferredActions sub-collection, and they are separately updated by Game:
fun tick(deltaTime: Double) {
val trans = Transaction()
knownObjects.deferredActions.forEach { it.update(deltaTime, trans)}
knownObjects.forEachInteracting { it.update(deltaTime, trans) }
knownObjects.applyChanges(trans)
}
This is done as part of the overall idea of getting down to just the flying objects when we do interactions, which is coming up on the calendar pretty soon now.
I think we have to count DeferredObject as an “issue”, something that may need further breakout. We could certainly pull it out of the hierarchy and add it more directly, as we did with Score. Maybe that’s worth doing, maybe it isn’t.
Wandering?
I feel a bit like we’re wandering. We don’t have a solid concrete plan for exactly what object goes where and when we’ll do it. And I think that’s OK. What we’re doing is making small changes that seem to make sense, and seem to move us in the general direction of where we want to be, which is with as many as possible of the special objects removed, so that we come down just to the objects that interact on screen.
But we’re not here to simulate a big design change all planned out. We’re simulating, instead, making the design better (in the sense of more centralized, with fewer special objects), over time. Everything we do is a step in the general direction of that overall design theory, but every step is made relative to the situation as we find it, not some imagined future wonderful situation.
We are incrementally improving the design toward a more desirable structure, keeping the program working and maintainable every day.
We haven’t added any features lately, but we could. The system has always been well-structured for what it does and would be accepting of any reasonable change. We’re not redesigning or rewriting. We’re refactoring in a direction.
It’s going well. What will we do next? I’m not sure. Maybe DeferredObject? Bill Wake has an idea for something that might be better. I don’t agree with the whole idea … but I might like part of it.
Stop by next time and find out.