Kotlin 203 - Finishing? Ship Creation
GitHub Decentralized Repo
GitHub Centralized Repo
I plan to wrap up the centralized version of Ship creation this morning, with the concomitant removal of two classes from the mix and the system.
It could go without saying that my plans often go awry, but it won’t go without saying, because, well, my plans often go awry. Will today be one of those days? At this moment, I do not know.
- Added in Post
- No! Things go marvelously! Callooh callay!
I do know that I have left the code for the new scheme open on my machine, and that I have two broken tests. Let’s start with the tests.
@Test
fun `missile and splat death`() {
val mix = SpaceObjectCollection()
val ship = Ship(
position = U.randomPoint()
)
val missile = Missile(ship)
mix.add(missile)
val game = Game(mix)
assertThat(mix.contains(missile)).isEqualTo(true)
game.cycle(0.0)
assertThat(mix.deferredActions.any { it is DeferredAction }).describedAs("deferred action should be present").isEqualTo(true)
game.cycle(U.MISSILE_LIFETIME + 0.1)
assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
assertThat(mix.any { it is Splat }).describedAs("splat should be present").isEqualTo(true)
game.cycle(U.MISSILE_LIFETIME + 0.2) // needs a tick to init
game.cycle(U.MISSILE_LIFETIME + 2.3) // Splat lifetime is 2.0
assertThat(mix.any { it is Splat }).describedAs("splat should be gone").isEqualTo(false)
}
This one fails thus:
lateinit property scoreKeeper has not been initialized
The failure occurs updating a DeferredAction. If I want to know exactly what’s going on, that’ll make it a bit difficult, but It would be better, I think, not to have that variable be lateinit. We could just initialize it sooner, but I think there’s a trick to it. No, not really. Let’s just give it one. We’ll replace it when we set up the game.
One might consider using the same ScoreKeeper over and over. That might be a good idea, but not right now, as it would be a more extensive change. We’ll try this first:
private var scoreKeeper: ScoreKeeper = ScoreKeeper(-1)
Test expecting green on that test. Yes, good. The second failure is:
[deferred size]
expected: 2
but was: 3
From this test:
@Test
fun `game-centric NO MAKER saucer appears after seven seconds`() {
// cycle receives ELAPSED TIME!
val mix = SpaceObjectCollection()
val game = Game(mix) // makes game without the standard init
game.cycle(0.1) // ELAPSED seconds
assertThat(mix.size).describedAs("mix size").isEqualTo(0)
assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(2)
val deferred = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
assertThat(deferred).isNotNull
game.cycle(7.2) //ELAPSED seconds
val saucer = mix.targets.find { it is Saucer }
assertThat(saucer).isNotNull
}
I think the test needs updating, because we will have added a ship-making DeferredAction to the mix. We can fix this by changing the count, or by changing the initialization to avoid the problem. Let’s provide a ship and an asteroid, which I think should get the deferred size down to 1.
@Test
fun `game-centric NO MAKER saucer appears after seven seconds`() {
// cycle receives ELAPSED TIME!
val mix = SpaceObjectCollection()
mix.add(Ship(U.CENTER_OF_UNIVERSE))
mix.add(Asteroid(Point(100.0,100.0)))
val game = Game(mix) // makes game without the standard init
game.cycle(0.1) // ELAPSED seconds
assertThat(mix.size).describedAs("mix size").isEqualTo(2) // changed
assertThat(mix.deferredActions.size).describedAs("deferred size").isEqualTo(1) // changed
val deferred = mix.deferredActions.find { (it as DeferredAction).delay == 7.0 }
assertThat(deferred).isNotNull
game.cycle(7.2) //ELAPSED seconds
val saucer = mix.targets.find { it is Saucer }
assertThat(saucer).isNotNull
}
I had to change the mix size to reflect the two new objects, and the deferred size drops to 1 as advertised. We are green.
Since most of the work was done so long ago, yesterday, I think I’ll review the diffs to see how things look.
Let’s quickly review the details.
The “big” change was in Game.cycle, calling and implementing the new method createShipIfNeeded:
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) }
}
private fun createShipIfNeeded() {
if ( knownObjects.attackers.find { it is Ship } != null ) return
val trans = Transaction()
shipOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
That first line of createShipIfNeeded could be better. I’m not sure how to make IDEA do this for me, so I’ll just type it in:
class Game
private fun createShipIfNeeded() {
if ( knownObjects.shipIsPresent() ) return
val trans = Transaction()
shipOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
class SpaceObjectCollection
fun shipIsPresent(): Boolean {
return attackers.filterIsInstance<Ship>().isNotEmpty()
}
Test. Still green. We could reverse the sense to shipIsMissing but I am OK with guard clauses like that, and prefer them over nesting, so we won’t do that.
What other changes are of note?
We promoted both ScoreKeeper and Ship instance to member variables, and added a new OneShot:
class Game(val knownObjects:SpaceObjectCollection = SpaceObjectCollection()) {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
private val saucer = Saucer()
private lateinit var ship: Ship
private var scoreKeeper: ScoreKeeper = ScoreKeeper(-1)
private val waveOneShot = OneShot(4.0) { makeWave(it) }
private val saucerOneShot = OneShot( 7.0) {it.add(saucer)}
private val shipOneShot = OneShot(U.SHIP_MAKER_DELAY, { canShipEmerge() }) {
if ( scoreKeeper.takeShip() ) it.add(ship)
}
// all OneShot instances go here:
private val allOneShots = listOf(waveOneShot, saucerOneShot, shipOneShot)
That OneShot uses the new ability to check a condition after the time expires, to call back to canShipEmerge. We tested and committed that a while back:
fun canShipEmerge(): Boolean {
for ( target in knownObjects.targets ) {
if ( target is Saucer ) return false
if ( target is Asteroid ) {
val distance = target.position.distanceTo(U.CENTER_OF_UNIVERSE)
if ( distance < U.SAFE_SHIP_DISTANCE ) return false
}
}
for ( attacker in knownObjects.attackers ) {
if ( attacker is Missile ) return false
if ( attacker is Saucer ) return false // already checked but hey
}
return true
}
Based on game play, I am inclined to increase SAFE_SHIP_DISTANCE, but the code is tested and works. What else?
In the game setup, we store the ScoreKeeper and Ship instances in member variables instead of local vars:
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls
) {
cancelAllOneShots()
trans.clear()
scoreKeeper = ScoreKeeper(shipCount)
knownObjects.scoreKeeper = scoreKeeper
val shipPosition = U.CENTER_OF_UNIVERSE
ship = Ship(shipPosition, controls)
// val shipChecker = ShipChecker(ship, scoreKeeper)
// trans.add(shipChecker)
}
We can remove those commented lines now.
That’s all we changed. Can we now remove ShipChecker and ShipMaker and their tests? I’ll look for references.
There is one test that we might like to retain, outside the Checker/Maker tests:
fun `ScoreKeeper provides ships to be made`() {
val keeper = ScoreKeeper(2)
val ship = Ship(U.CENTER_OF_UNIVERSE)
val checker = ShipChecker(ship, keeper)
val t1 = Transaction()
checker.subscriptions.beforeInteractions()
checker.subscriptions.afterInteractions(t1)
assertThat(t1.adds.size).isEqualTo(1)
val t2 = Transaction()
checker.subscriptions.beforeInteractions()
checker.subscriptions.afterInteractions(t2)
assertThat(t2.adds.size).isEqualTo(1)
val t3 = Transaction()
checker.subscriptions.beforeInteractions()
checker.subscriptions.afterInteractions(t3)
assertThat(t3.adds.size).isEqualTo(0)
}
No, that’s useless. I’ll make a note to write a new test for that if needed, but I’m not sure it is.
All references to ShipChecker are either in the tests, or the one in ShipMaker that ping=-pongs a new checker into existence. How about references to ShipMaker?
There’s this interaction defined:
val interactWithShipMaker: (maker: ShipMaker, trans: Transaction) -> Unit = { _, _, -> },
I remove that and the reference to it in ShipMaker.callOther. No one is using it. I think I can Safe Delete the ShipCheckerShipMakerTests. However … there are some tests in there that look useful, if they could be recast not to use Checker and Maker. Let’s review a few of them.
@Test
fun `ship randomizes position on hyperspace entry`() {
val ship = Ship(U.CENTER_OF_UNIVERSE)
val trans = Transaction()
U.AsteroidTally = 100 // hyperspace never fails
ship.enterHyperspace(trans)
assertThat(ship.position).isNotEqualTo(U.CENTER_OF_UNIVERSE)
}
@Test
fun `ship goes to center on collision death`() {
val ship = Ship(U.randomPoint())
val trans = Transaction()
ship.collision(trans)
assertThat(trans.firstRemove()).isEqualTo(ship)
ship.finalizeObject()
assertThat(ship.position).isEqualTo(U.CENTER_OF_UNIVERSE)
}
These have nothing to do with Checker and Maker. I think we’ll wind up removing the Checker/Maker-related tests and keeping these. We’ll rename the class of course.
@Test
fun `debits ScoreKeeper if ship is dead`() {
val scoreKeeper = ScoreKeeper(1)
val ship = Ship(U.CENTER_OF_UNIVERSE)
val checker = ShipChecker(ship, scoreKeeper)
checker.subscriptions.beforeInteractions()
val trans = Transaction()
checker.subscriptions.afterInteractions(trans)
assertThat(scoreKeeper.shipCount).isEqualTo(0)
}
This is a valid test but I’ve already made that note to write a new one checking this. This can go.
@Test
fun `saucer clears safeToEmerge`() {
val ship = Ship(U.CENTER_OF_UNIVERSE)
val saucer = Saucer()
saucer.position = U.CENTER_OF_UNIVERSE
val maker = ShipMaker(ship)
val ignored = Transaction()
maker.subscriptions.beforeInteractions()
assertThat(maker.safeToEmerge).isEqualTo(true)
maker.subscriptions.interactWithSaucer(saucer, ignored)
assertThat(maker.safeToEmerge).isEqualTo(false)
}
@Test
fun `missile clears safeToEmerge`() {
val ship = Ship(U.CENTER_OF_UNIVERSE)
val missile = Missile(Point.ZERO)
val maker = ShipMaker(ship)
val ignored = Transaction()
maker.subscriptions.beforeInteractions()
assertThat(maker.safeToEmerge).isEqualTo(true)
maker.subscriptions.interactWithMissile(missile, ignored)
assertThat(maker.safeToEmerge).isEqualTo(false)
}
Don’t we have new tests for this feature? Yes, using the new canShipEmerge in Game. These can go.
The rest are uninteresting, and I remove them. I think ShipChecker may now be referenced only by ShipMaker and vice versa.
Yes. They can both go, but because they reference each other, I’ll remove one of those references and remove the other class. Quickly done, classes removed. Test. Green as grass. Test the game for fun. Game plays fine, I am still terrible at it.
Let’s commit: Ship rezzing now done in Game, ShipChecker, ShipMaker, and related tests removed.
IDEA informs me that there are two unused methods in Subscriptions:
val interactWithScore: (score: Score, trans: Transaction) -> Unit = { _, _, -> },
val interactWithScoreKeeper: (keeper: ScoreKeeper, trans: Transaction) -> Unit = { _, _, -> },
Remove those. They’ve been unused a while, since scoring was changed. This was just the first time that we committed Subscriptions, so the first time the warning came out. Remove. Test. Commit: Remove unused interactions for Score and ScoreKeeper.
I think we’ll call it here. Let’s sum up.
Summary
We began by checking our two red tests, which serve to remind us where we are. We fixed the one by improving the code, removing a lateinit. We still have one lateinit left, for the ship. Let me make a note in Jira for that.
A second test was failing and we improved and corrected the test. The code had changed and the test needed adjustment.
So we began with two small improvements, one to code, one to tests. Then we reviewed the diff to see how extensive the changes were and then ticked through them.
We made one small improvement by pushing the question of whether the ship is present over to knownObjects, which knows about such things.
The rest was ticking through the tests to see if there were some worth preserving, and there were. Which reminds me, that test class needs a new name. Added to Jira.
That’s really all we did, and we did a couple of commits to ship the new implementation of ship rezzing.
The bigger picture …
The bigger picture is that, again, it was easy. The core idea is the same:
- To notice the need to create something, Game asks
knownObjectsto find out. - If something does need creation, Game fires a OneShot to ask for creation.
- The OneShot defers an action for the suitable timeout,
- The DeferredAction, when due, calls back to Game to create what’s needed.
Asteroid waves, the saucer, and the ship are all created this same way. Because the ship’s creation requires a safety check, we made a simple enhancement to OneShot and DeferredAction to check a condition in addition to the elapsed time check.
I believe we are down to the point where the only “Space Objects” are the ones visible on screen, Asteroid, Saucer, Ship, and Splat, plus the DeferredAction, which never enters the mix but is instead saved separately by the knownObjects collection. I’ll have to triple check, but a quick double check tells me that all the “special” objects have been removed from the system.
The main phase of converting to a centralized model is done and we have done a fairly marvelous thing, for which I choose to congratulate all of us, especially myself:
We have radically changed the design of this working game, from one where all the decisions were made in small separate objects interacting, to one where all the decisions (except collisions) are made in a centralized Game object.
Whether this is a good change remains to be evaluated, but it is a very substantial design change, done over the course of two years (well, OK, December 28 2022 through January 7 2023 so far) while keeping the game running after every release, essentially releasing every working day.
Why is this good? Well, because it suggests, but of course does not prove, that if we have a decent design and some refactoring skill, we can adjust to a new design over time, without ever undergoing a major rewrite of our system.
Just suggests … but in those cases where it’s true, we can deliver more value while we improve the design, and it is delivering value that keeps the wolf from the door.
I’m pleased with that aspect. I’m reserving judgment on which design I prefer. We’ll assess that after we decide what to do about collisions.
That’ll be soon. See you next time!