Kotlin 176: Ship Emergence
I’d like to clean up ship emergence. I believe it can be more clear and if it can be, it should be. BONUS: A movie of the most interesting bug so far.
In a real software product effort, I think we’d be under a lot more pressure to deliver features, and my usual articles try to reflect that, showing how, for me, having good tests helps me go faster, and how well-crafted code keeps me going faster and less well-crafted code slows me down. I show how to take opportunities to improve the code while working on a feature.
It’s pretty clear that I’m not feeling the same feature pressure here in Asteroids as I usually do. The result is that we can try different approaches to things, building up a sense of the trade-offs, and learning a bit of Kotlin along the way. On your real efforts, you may have less opportunity to do code improvement and experiments, but since we can try something and learn i an hour or so, maybe you can find the time to keep your tools sharp as well as to work on features.
Be that as it may, what’s going on here at Ron’s Asteroids Factory is that we’re working more on how to build Asteroids, more than on building a specific one.
Today, we’re going to explore how the Ship works, in particular ship emergence, and we’ll see if we can improve it.
It’s no surprise that the Ship is one of the most complex objects in the system, vying with Asteroid for the title. In addition, there’s a lot of complexity around ship, with the ShipChecker object that notices when there’s no live ship, and the ShipMaker, which puts the ship back into the mix while dealing with timing, hyperspace, and bringing the ship back only when it’s relatively safe. The process also uses two additional classes. ShipDestroyer and HyperspaceEmergence
We may review Ship itself later on, and you can look at it in the repo, but I think it’s close to the right size given what it does, and Its code seems to me to be very straightforward and understandable. Not perfect, but honestly I think it’s good enough. I’ll define “good enough” to mean something like “we might see some things that could be better, but they can be left until we’re working in that area anyway”. Of course, to be honest, I think that’s always the most efficient approach: work on improving code when you have a reason to be working on it anyway. If we’re not working on it, it’s not bothering anyone. But I digress.
It’s ShipChecker, ShipMaker, and HyperspaceOperation that trouble me more than Ship itself. Let’s review them and see what we think.
ShipChecker is an object that is in the mix most of the time. It’s job is to check to see if the ship is present, and if the ship is gone, ShipChecker’s job is to bring it back, if it should be brought back: that is, if the ship is just in hyperspace, or it has been destroyed but there’s another ship remaining in the number you’re allowed before game over.
I think ShipChecker is pretty decent:
class ShipChecker(
val ship: Ship,
val scoreKeeper: ScoreKeeper = ScoreKeeper()
) : ISpaceObject, InteractingSpaceObject {
private var missingShip = true
override fun update(deltaTime: Double, trans: Transaction) {}
override val subscriptions = Subscriptions(
beforeInteractions = { missingShip = true },
interactWithShip = { _, _ -> missingShip = false },
afterInteractions = { trans ->
if ( missingShip && (ship.inHyperspace || scoreKeeper.takeShip())) {
trans.add(ShipMaker(ship, scoreKeeper))
trans.remove(this)
}
}
)
override fun callOther(other: InteractingSpaceObject, trans: Transaction) =
other.subscriptions.interactWithShipChecker(this, trans)
}
I’m curious whether anyone subscribes to interactWithShipChecker. The answer is that no one does. Let’s remove the reference here, and remove it from the subscriptions master list.
override fun callOther(other: InteractingSpaceObject, trans: Transaction) {}
And a deleted line in Subscriptions:
val interactWithShipChecker: (checker: ShipChecker, trans: Transaction) -> Unit = { _, _, -> },
Test, green, commit: remove unneeded interactWithShipChecker.
Anyway, we see that ShipChecker interacts with the ship, so that if an interaction occurs, the ship is alive. If the ship is missing then if we’re in hyperspace, or there’s a ship available from ScoreKeeper, ShipChecker adds a ShipMaker to the mix and removes itself.
It removes itself because it has started the process of creating a new ship and until that’s done, at some time in the future, we don’t want to check further. The ShipMaker will, as one of its final actions, remove itself and create a new ShipChecker, so that the cycle continues.
ShipChecker seems fine to me. Let’s turn to ShipMaker:
class ShipMaker(val ship: Ship, val scoreKeeper: ScoreKeeper = ScoreKeeper()) : ISpaceObject, InteractingSpaceObject {
var safeToEmerge = true
var asteroidTally = 0
private var elapsedTime = 0.0
override fun update(deltaTime: Double, trans: Transaction) {
elapsedTime += deltaTime
}
override val subscriptions = Subscriptions (
beforeInteractions = {
safeToEmerge = true
asteroidTally = 0
},
interactWithAsteroid = { asteroid, _ ->
asteroidTally += 1
safeToEmerge = safeToEmerge && !tooClose(asteroid)
},
interactWithSaucer = { saucer, _ ->
asteroidTally += 1
safeToEmerge = safeToEmerge && !tooClose(saucer)
},
afterInteractions = { trans->
if (ship.inHyperspace || elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip(trans)
}
}
)
override fun callOther(other: InteractingSpaceObject, trans: Transaction) =
other.subscriptions.interactWithShipMaker(this, trans)
private fun tooClose(collider: Collider): Boolean {
return ship.position.distanceTo(collider.position) < U.SAFE_SHIP_DISTANCE
}
private fun replaceTheShip(trans: Transaction) {
trans.add(ship)
ship.dropIn()
trans.add(ShipChecker(ship, scoreKeeper))
trans.remove(this)
HyperspaceOperation(ship, asteroidTally).execute(trans)
}
}
I see a bug: someone copy-pasted the code for Saucer and it’s counting as an asteroid. What is that number used for anyway? Ah, right, it is an input to HyperspaceOperation, which changes the odds of surviving hyperspace based on how many asteroids there are. Basically, if there aren’t many asteroids, your chance of surviving are lower, I suppose because you shouldn’t be getting into trouble if there are only a few asteroids. Anyway, that’s what we do with it. So we shouldn’t be counting the saucer as an asteroid.
Delete that line. Test, commit: saucer not counted as asteroid in tally for hyperspace check.
I can remove a bit of duplication by extracting a method isAnythingInTheWay:
override val subscriptions = Subscriptions (
beforeInteractions = {
safeToEmerge = true
asteroidTally = 0
},
interactWithAsteroid = { asteroid, _ ->
asteroidTally += 1
safeToEmerge = isAnythingInTheWay(asteroid)
},
interactWithSaucer = { saucer, _ ->
safeToEmerge = isAnythingInTheWay(saucer)
},
afterInteractions = { trans->
if (ship.inHyperspace || elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip(trans)
}
}
)
private fun isAnythingInTheWay(collider: Collider) = safeToEmerge && !tooClose(collider)
Test, commit: extract method to remove some duplication.
Now we’re left to consider replaceTheShip, which uses the HyperspaceOperation object.
private fun replaceTheShip(trans: Transaction) {
trans.add(ship)
ship.dropIn()
trans.add(ShipChecker(ship, scoreKeeper))
trans.remove(this)
HyperspaceOperation(ship, asteroidTally).execute(trans)
}
class HyperspaceOperation(val ship: Ship, private val asteroidTally: Int) {
fun execute(trans: Transaction) {
if (hyperspaceFails()) destroyTheShip(trans)
}
private fun destroyTheShip(trans: Transaction) {
trans.add(ShipDestroyer())
}
private fun hyperspaceFails(): Boolean
= inHyperspace() && hyperspaceFailure(Random.nextInt(0, 63), asteroidTally)
private fun inHyperspace() = ship.position != U.CENTER_OF_UNIVERSE
// allegedly the original arcade rule
fun hyperspaceFailure(random0thru62: Int, asteroidTally: Int): Boolean
= random0thru62 >= (asteroidTally + 44)
}
Now this requires some explanation, which probably proves that it’s not right. replaceTheShip adds the ship back to the mix, telling it to do its dropIn animation, puts the ShipCheckerBack, and removes itself. If we were to stop here, the ship would come back as intended.
Then we run the HyperspaceOperation and it rolls the dice and if the odds are not in your favor, the HyperspaceOperation throws a ShipDestroyer into the mix. The ship, perfectly happy a moment ago, interacts with the Destroyer and explodes. Hyperspace emergence failed.
I was kind of proud of this implementation. Rather than go through complicated code to decide whether to recreate the ship, I thought it was kind of elegant to throw a bomb in after it and destroy it again.
This is the only use of the ShipDestroyer, and despite that I think it’s rather a cool trick, it seems to me to be going around the horn. We do need to destroy the ship, or at least we need to draw down another one from the ScoreKeeper. And we need to ensure that the ship resets itself from being in hyperspace to emerging as a “new” ship at universe center. That’s done in ship.finalize.
I think we could get the same effect if we were to check the odds, and if they are in our favor, add the ship, and if not, finalize it, and add a Splat instead. The Maker exits, the Checker enters, finds that there is still not a ship, and draws another one from ScoreKeeper.
Let’s try that, by way of a spike. If it works, we can remove ShipDestroyer (kill your darlings?) and probably simplify HyperspaceOperation a lot.
I’ll just spike it, typing in what I mean, to see if I like it. We’ll revise this:
private fun replaceTheShip(trans: Transaction) {
trans.add(ship)
ship.dropIn()
trans.add(ShipChecker(ship, scoreKeeper))
trans.remove(this)
HyperspaceOperation(ship, asteroidTally).execute(trans)
}
TO something like this:
private fun replaceTheShip(trans: Transaction) {
trans.add(ShipChecker(ship, scoreKeeper))
trans.remove(this)
val hyp = HyperspaceOperation(ship, asteroidTally)
if (hyp.ok()) {
trans.add(ship)
ship.dropIn()
} else {
ship.finalize() // dead again
}
}
And I need the HyperspaceOperation to answer ok:
fun ok(): Boolean = !hyperspaceFails()
This doesn’t quite do the job. I need to clear the hyperspace flag on the ship, which can’t be done in finalize (because ShipMaker needs to see it later).
Without that, and in a testing mode, I got this charming effect:
I wind up with this code:
private fun replaceTheShip(trans: Transaction) {
trans.remove(this)
val hyp = HyperspaceOperation(ship, asteroidTally)
if (!ship.inHyperspace || hyp.ok()) {
trans.add(ship)
ship.dropIn()
} else {
ship.inHyperspace = false
trans.add(Splat(ship))
ship.finalize() // dead again
}
trans.add(ShipChecker(ship, scoreKeeper))
}
The game runs, as do the tests. Let’s clean up HyperspaceOperation. I’m now only calling it if the ship is in hyperspace, so it need not double check that.
class HyperspaceOperation(private val asteroidTally: Int) {
fun ok(): Boolean = !hyperspaceFailure(Random.nextInt(0, 63), asteroidTally)
// allegedly the original arcade rule
fun hyperspaceFailure(random0thru62: Int, asteroidTally: Int): Boolean
= random0thru62 >= (asteroidTally + 44)
}
I’ve removed the ship input parameter as we no longer need it. Test again. We’re not done, but I want to be sure that worked. Tests run except for the one that checks the function, which was also passing in the ship parameter. We’re good so far.
Let’s commit, even though I’d like to do more. Commit: HyperspaceOperation now just returns success or failure, no longer creates Destroyer.
The HyperspaceOperation is now so simple that it could be done inside ShipMaker. Let’s do that and then figure out how to wire the test up to it.
private fun replaceTheShip(trans: Transaction) {
trans.remove(this)
if (!ship.inHyperspace || hyperspaceOK()) {
trans.add(ship)
ship.dropIn()
} else {
ship.inHyperspace = false
trans.add(Splat(ship))
ship.finalize() // dead again
}
trans.add(ShipChecker(ship, scoreKeeper))
}
private fun hyperspaceOK(): Boolean = !hyperspaceFailure(Random.nextInt(0, 63), asteroidTally)
// allegedly the original arcade rule
fun hyperspaceFailure(random0thru62: Int, asteroidTally: Int): Boolean
= random0thru62 >= (asteroidTally + 44)
Now we need to rewire this test, but it’s easy:
@Test
fun `hyperspace failure checks`() {
val ignoredShip = Ship(U.CENTER_OF_UNIVERSE)
val hyper = ShipMaker(ignoredShip)
assertThat(hyper.hyperspaceFailure(62, 19)).describedAs("roll 62 19 asteroids").isEqualTo(false)
assertThat(hyper.hyperspaceFailure(62, 18)).describedAs("roll 62 18 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(45, 0)).describedAs("roll 45 0 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(44, 0)).describedAs("roll 44 0 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(43, 0)).describedAs("roll 43 0 asteroids").isEqualTo(false)
}
Now we can remove the HyperspaceOperation class. Safe delete rips it right out. Now let’s do ShipDestroyer. That goes smoothly. I got to remove interactWithShipDestroyer as well.
Test. Green. Game’s good. Commit: Refactor ShipMaker to remove ShipDestroyer and HyperspaceOperation entirely.
Marvelous. Let’s sum up.
Summary
I’ve been feeling that the process of removing and adding the ship was too much, but I hadn’t had a good idea about how to improve it. With the Ship, ShipChecker, ShipMaker, ShipDestroyer, and HyperspaceOperation all involved, it was just too hard to think about. Today, we just followed our nose through the classes, cleaning up little things, until it came down to ShipMaker and HyperspaceOperation (which created a ShipDestroyer).
At that point, I got the idea of adding the ship back in in Maker conditionally, either if we weren’t in hyperspace, or we were but survived the dice roll, and otherwise, not inserting the ship, putting a Splat down instead to show the failure.
That worked very nicely once I realized that I had to clear the hyperspace flag in the ship. Before I did that, as soon as you got a hyperspace failure you’d just explode, jump, explode, jump … it was very interesting on the screen, as in the movie above, but wasn’t much of a game.
Looking at the operational code here:
private fun replaceTheShip(trans: Transaction) {
trans.remove(this)
if (!ship.inHyperspace || hyperspaceOK()) {
trans.add(ship)
ship.dropIn()
} else {
ship.inHyperspace = false
trans.add(Splat(ship))
ship.finalize() // dead again
}
trans.add(ShipChecker(ship, scoreKeeper))
}
It’s a bit tricky there at the top of the if statement but I think we can live with it. The fact that we must clear the hyperspace flag in the else is not obvious and makes me wish for a better way of dealing with the hyperspace status but we can see here that it needs to follow the ship all the way down to here … and then be turned off.
Possibly we could make the determination of success at hyperspace entry, but we want the splat to show up on emergence. Although I suppose you could splat him where he is and say that it’s going into hyperspace that’s the problem. But I think the original showed you exploding on entry.
So it’s not perfect, but it is better. We’re 28 lines shorter, with two whole classes gone. A decent morning’s work.
This makes up for yesterday. Live in the moment, that’s my motto.
See you next time!