Kotlin 159: Simpler!
Now that all the SpaceObjects are broken out, we have duplication. Let’s see how much and how we might improve things.
Each kind of object in the system resides in its own class now. The closest to a multi-purpose object is the Asteroid, which comes in three convenient sizes. Today I plan some cleanup, of two kinds.
First, the interface of all these objects requires implementation of six functions and one value: update, beginInteraction, finishInteraction, draw, finalize, callOther, and interactions. the latter two are absolutely required, but most objects only have actual code in some of first five of these, which makes the object more complicated than it needs to be. I propose to use the interactions feature to allow objects to subscribe only to the messages they want to deal with.
(That reminds me, I am starting to think that interactions should be named subscriptions.)
Second, I think we’ll find that some of our objects can be created more simply than they currently are, and if we notice this, we’ll improve them.
Third1, We should get rid of the final companion method, the one for ship. The companion methods were useful when each object was a special configuration of a single class. That idea lasted longer than it deserved to, and it’s past time to get rid of the final vestiges.
Fourth2, we’ll probably see other opportunities to make things better, and we’ll take some of those opportunities.
The Companion
Let’s start with the companion object and its creation method:
companion object {
fun ship(pos: Point, control: Controls = Controls()): Ship {
return Ship(
position = pos,
velocity = Velocity.ZERO,
killRadius = 150.0,
controls = control,
)
}
}
Let’s chip away at this. We could just try a safe remove or something, but I think there are rather a bunch of calls to this function. Thirty, in fact.
We see that all our users are passing at most position and controls, except, I think, one that is passing in a different killRadius. If we change Ship’s constructor accordingly, this change should be pretty easy. We’ll let that one break.
Remove velocity from the constructor, put it inside.
class Ship(
var position: Point,
var killRadius: Double = -Double.MAX_VALUE,
val controls: Controls = Controls(),
) : ISpaceObject, InteractingSpaceObject {
var velocity: Velocity = Velocity.ZERO
var heading: Double = 0.0
val view = ShipView()
val finalizer = ShipFinalizer()
Let’s try to build the habit of going from green to green quickly, and committing each time. I find two direct creators using Velocity.ZERO. Fix them. Green. Commit: Ship creation no longer provides velocity.
Now killRadius.
class Ship(
var position: Point,
val controls: Controls = Controls(),
) : ISpaceObject, InteractingSpaceObject {
var velocity: Velocity = Velocity.ZERO
val killRadius = 150.0
companion object {
fun ship(pos: Point, control: Controls = Controls()): Ship {
return Ship(
position = pos,
controls = control,
)
}
}
Test and see who complains. I think one test will. Sure enough:
@Test
fun `stringent colliders`() {
val p1 = Vector2(100.0,100.0)
val p2 = Vector2(750.0, 100.0)
val game = Game()
val a0 = Asteroid(p1) // yes
game.add(a0)
val m1 = Ship(p1, 10.0) // yes
game.add(m1)
val s2 = Ship.ship(p1) // yes kr=150
game.add(s2)
val a3 = Asteroid(p2) // no
game.add(a3)
val a4 = Asteroid(p2) // no
game.add(a4)
val colliders = game.removalsDueToInteraction()
assertThat(colliders.size).isEqualTo(3)
}
That ship there, called m1, is trying to be a missile. Change it and the Ship.ship below it since we’re there.
Ah, a bit more nasty than I’d like. There are a couple of tests needing fixing. I hate to do this, but I’m going to make the killRadius a var, so that tests doing odd things can do them. We might do better to make a special test space object but that’s a bit too far.
Changing the tests to create the object and then set killRadius makes them green. Commit: killRadius no longer a parameter to Ship constructor.
But wait. Is that really better? I think we’ll be better to default it but allow the constructor to override. Let’s try that.
class Ship(
var position: Point,
val controls: Controls = Controls(),
val killRadius: Double = 150.0
) : ISpaceObject, InteractingSpaceObject {
var velocity: Velocity = Velocity.ZERO
companion object {
fun ship(pos: Point, control: Controls = Controls(), killRadius: Double = 150.0): Ship {
return Ship(
position = pos,
controls = control,
killRadius = killRadius
)
}
}
Yes, that makes the tests nicer, like this example:
val ship = Ship(
position=Vector2.ZERO,
killRadius = 100.0
)
Commit: controls and killRadius both optional parameters to ship creation, allowing for testing.
Now we can fix all the senders of Ship.ship and make them just call Ship. Here’s an example:
private fun newShip(controls: Controls): Ship {
return Ship.ship(U.CENTER_OF_UNIVERSE, controls)
}
I think IDEA might be willing to inline these for me. Might? I click on one and say to inline and it does them all, and removes the method and companion object for me.
Wow! There were 30 calls and it did them all. Even at ten seconds to find and edit each one, IDEA just saved me five minutes of tedium. Gets my vote!
Green. Commit: remove companion object with SHip.ship helper method.
- Small Commits
-
That task is done. Let’s talk about these small commits. I did four commits in less than a half hour. Each of them was only a change to a few lines, or a single IDEA refactoring operation. Surely I could have batched them up and saved time?
-
Well, except that in that half hour, I reverted once and if I had not committed the prior tiny changes, I’d have to figure them out again and do them over. It takes almost no time to commit. Command-K and type a message, click a button.
-
I think this is better, and that I’d benefit from improving my habits toward tiny commits. Once again Hill wins. Many More Much Smaller Steps. Good idea, and the steps can be smaller than one might think.
-
Is there a lower limit? Are there steps that are too small? Well, if we mean by a step as a move from green to green, it’s hard to see how too small would be an issue. A couple of my changes so far this morning were two lines. Didn’t feel too small. Felt good. I’m going to keep trying to take smaller steps.
-
What should you do? I’m not the boss of you: you should do whatever you think is best.
Other Constructors
I think we’ll review the other constructors: I noticed some odd code creating an Asteroid as I was fixing up those tests. We’ll just scan them quickly to see what we can see.
class Asteroid(
var position: Point,
val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
val killRadius: Double = 500.0,
private val splitCount: Int = 2
) : ISpaceObject, InteractingSpaceObject {
Why are we allowing people to set velocity? I notice in asSplit:
private fun asSplit(asteroid: Asteroid): Asteroid {
val newKr = asteroid.killRadius / 2.0
val newVel = asteroid.velocity.rotate(Math.random() * 360.0)
return Asteroid(
position = asteroid.position,
velocity = newVel,
killRadius = newKr,
splitCount = splitCount - 1
)
}
One random velocity is as good as another. We don’t need this setting. I’ll check other creators, maybe some tests are trying something tricky. Yes, there are a few checking the “randomness” and/or driving an asteroid from outside range to inside. We’ll leave it, but let’s remove that special stuff from asSplit:
private fun asSplit(asteroid: Asteroid): Asteroid {
return Asteroid(
position = asteroid.position,
killRadius = asteroid.killRadius / 2.0,
splitCount = splitCount - 1
)
}
I inlined the other temp as well. Test. One test needs adjustment because it was setting its own velocity. Green. Commit: simplified Asteroid.asSplit.
I do a bit of tidying in Missile, not worth comparing here. Test. Commit: tidying.
We have bigger fish to fry here. Let’s go after the general interface, those methods update, beginInteraction, finishInteraction, draw, and finalize. We’d like to make those optional, using our Interactions approach.
Take Missile, for example:
override fun beforeInteractions() {}
override fun afterInteractions(trans: Transaction) {}
If those were optional, via Interactions, we wouldn’t have to specify them, and they wouldn’t clutter the Missile’s interface.
Let’s start with one of those. We could do more than one but small steps innit. We’ll do beginInteractions. Here’s the Interactions class:
class Interactions(
val interactWithScore: (score: Score, trans: Transaction) -> Unit = { _, _, -> },
val interactWithShip: (ship: Ship, trans: Transaction) -> Unit = { _, _, -> },
val interactWithMissile: (missile: Missile, trans: Transaction) -> Unit = { _, _, -> },
val interactWithShipChecker: (checker: ShipChecker, trans: Transaction) -> Unit = { _, _, -> },
val interactWithShipMaker: (maker: ShipMaker, trans: Transaction) -> Unit = { _, _, -> },
val interactWithWaveMaker: (maker: WaveMaker, trans: Transaction) -> Unit = { _, _, -> },
val interactWithShipDestroyer: (destroyer: ShipDestroyer, trans: Transaction) -> Unit = { _, _, -> },
val interactWithAsteroid: (asteroid: Asteroid, trans: Transaction) -> Unit = { _, _, -> },
)
We’ll just do the one: we need to change all the subscribed classes each time.
class Interactions(
val beforeInteractions: () -> Unit = {},
Now we need to find all the implementors of beforeInteractions and sign up the ones with non-empty contents.
Remove the function from the interface.
Asteroids implementation is empty. Remove it. Missile: remove. Score: remove. ScoreKeeper, remove.
ShipChecker has an implementation:
override fun beforeInteractions() {
missingShip = true
}
Remove that and replace with this:
override val interactions: Interactions = Interactions(
beforeInteractions = {
missingShip = true
},
interactWithShip = { solid, _ ->
if ( solid == ship ) missingShip = false
}
)
ShipDestroyer, remove. ShipMaker:
override fun beforeInteractions() {
safeToEmerge = true
asteroidTally = 0
}
I find this in ShipMaker, and I think it’s wrong:
override val interactions: Interactions = Interactions { asteroid, trans ->
asteroidTally += 1
safeToEmerge = safeToEmerge && !tooClose(asteroid)
}
There’s no method named there. What should it be?
override val interactions: Interactions = Interactions (
beforeInteractions = {
safeToEmerge = true
asteroidTally = 0
},
interactWithAsteroid = { asteroid, _ ->
asteroidTally += 1
safeToEmerge = safeToEmerge && !tooClose(asteroid)
},
)
I think safe to emerge has not been checked for a while. Interesting. The above should be right, but I’d better make a test for it. Note made.
Clicking on …
Splat, remove. WaveChecker:
override fun beforeInteractions() {
sawAsteroid = false
}
Replace:
override val interactions: Interactions = Interactions (
beforeInteractions = { sawAsteroid = false},
interactWithAsteroid = { _, _ -> sawAsteroid = true }
)
That one looked weird as well.
WaveMaker’s method is empty, remove. We should be able to test. Well, after we change Game:
private fun beginInteractions() {
knownObjects.forEach { it.interactions.beforeInteractions() }
}
And after I change a thousand tests to refer through interactions, we’re green. Commit: moved beforeInteractions to Interactions instance.
That was interesting: there were two entries in the interactions implementation that we flat wrong. I’d best check them all … with I did something wrong or IDEA helped me …
Missile was wrong, in the same way. How can that be?
Fix it:
override val interactions: Interactions = Interactions (
interactWithAsteroid = { asteroid, trans ->
if (weAreInRange(asteroid)) {
trans.remove(this)
}
}
}
I frankly do not understand why Missiles worked with that malformed version in there. they’re working now. I’ll chalk it up to mystery, but I don’t like it.
Anyway we’ve moved beforeInteractions into the Interactions object.
Let’s do after now. Same drill:
Remove afterInteractions from the interface.
Add it to the Interactions object. It takes a transaction.
val beforeInteractions: () -> Unit = {},
val afterInteractions: (trans: Transaction) -> Unit = {_ -> },
Look for implementors of afterInteractions and remove them, adjusting the object’s interactions val.
Asteroid, remove. Missile, remove. Score, remove. ScoreKeeper, remove. ShipChecker is a live one:
override fun afterInteractions(trans: Transaction) {
if ( missingShip ) {
trans.add(ShipMaker(ship))
trans.remove(this)
}
}
Do this:
override val interactions: Interactions = Interactions(
beforeInteractions = {
missingShip = true
},
interactWithShip = { solid, _ ->
if ( solid == ship ) missingShip = false
},
afterInteractions = { trans ->
trans.add(ShipMaker(ship))
trans.remove(this)
}
)
ShipDestroyer, empty, remove. ShipMaker, yes:
override fun afterInteractions(trans: Transaction) {
if (elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip(trans)
}
}
Change:
override val interactions: Interactions = Interactions (
beforeInteractions = {
safeToEmerge = true
asteroidTally = 0
},
interactWithAsteroid = { asteroid, _ ->
asteroidTally += 1
safeToEmerge = safeToEmerge && !tooClose(asteroid)
},
afterInteractions = { trans->
if (elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip(trans)
}
}
)
Splat, empty, remove. WaveChecker:
override fun afterInteractions(trans: Transaction) {
if ( elapsedTime > 1.0 ) {
elapsedTime = 0.0
if (!sawAsteroid) {
elapsedTime = -5.0 // judicious delay to allow time for creation
trans.add(WaveMaker(4))
}
}
}
Change:
override val interactions: Interactions = Interactions (
beforeInteractions = { sawAsteroid = false},
interactWithAsteroid = { _, _ -> sawAsteroid = true },
afterInteractions = this::makeWaveInDueTime
)
private fun makeWaveInDueTime(trans: Transaction) {
if ( elapsedTime > 1.0 ) {
elapsedTime = 0.0
if (!sawAsteroid) {
elapsedTime = -5.0 // judicious delay to allow time for creation
trans.add(WaveMaker(4))
}
}
}
That was complicated enough that I broke it out as a function.
WaveMaker is empty, remove. Test. Game needs adjustment:
private fun finishInteractions() {
val buffer = Transaction()
knownObjects.forEach {
it.interactions.afterInteractions(buffer)
}
knownObjects.applyChanges(buffer)
}
Test. A bunch of tests fail because they need to call through interactions. But a couple actually fail.
Both tests failed for the same reason, finding a ShipMaker that wasn’t supposed to be there:
@Test
fun `ShipChecker does nothing if ship seen via withOther`() {
val ship = Ship(
position = U.randomPoint()
)
val checker = ShipChecker(ship)
checker.interactions.beforeInteractions()
val trans = Transaction()
checker.interactions.interactWithShip(ship, trans)
assertThat(trans.removes).isEmpty()
val emptyTransaction = Transaction()
checker.interactions.afterInteractions(emptyTransaction)
assertThat(emptyTransaction.adds).describedAs("something added").isEmpty()
assertThat(emptyTransaction.removes).isEmpty()
val alsoNothing = Transaction()
checker.update(0.01, alsoNothing)
assertThat(alsoNothing.adds).isEmpty()
assertThat(alsoNothing.removes).isEmpty()
}
Let’s see what ShipChecker is doing in afterInteractions.
override val interactions: Interactions = Interactions(
beforeInteractions = {
missingShip = true
},
interactWithShip = { solid, _ ->
if ( solid == ship ) missingShip = false
},
afterInteractions = { trans ->
trans.add(ShipMaker(ship))
trans.remove(this)
}
)
But the original method was this:
override fun afterInteractions(trans: Transaction) {
if ( missingShip ) {
trans.add(ShipMaker(ship))
trans.remove(this)
}
}
Daddy didn’t copy all the code. Bad daddy.
override val interactions: Interactions = Interactions(
beforeInteractions = {
missingShip = true
},
interactWithShip = { solid, _ ->
if ( solid == ship ) missingShip = false
},
afterInteractions = { trans ->
if (missingShip ) {
trans.add(ShipMaker(ship))
trans.remove(this)
}
}
)
Test. Green as fake gold. This is why we have tests. Commit: afterInteractions is ow handed in interactions.
I rather like how the interactions val tells the story of the object: set the flag, invert the flag if you see a ship, add a maker and remove yourself if there was no ship.
Let’s break and sum up. I’ve been here nearly three hours, with a few interruptions.
Summary
All the changes today have been simple, though some have been tedious, where we had tin insert .interactions a lot. We have ten commits in three hours, or one every 18 minutes, not too bad given that I have to write the article while I code. There were no real problems, except …
There were those malformed uses of interactions. I have a vague notion of why they worked, but am tempted to reinstall them and see why nothing breaks. I am pretty sure that any access to interactions in those cases would execute the block … but why that didn’t throw I really can’t guess. Maybe they didn’t look at the one parameter and the other was OK?
The actual changes allowed me to remove the empty implementations of beforeInteractions and afterInteractions from quite a few classes, with no added complexity to those classes. In the other cases, the same code was moved from the method into interaction, or, in one case, the method renamed and called. Net change to code complexity about the same, perhaps a minimal increase, but in a fashion that we’re familiar with because everyone’s doing it.
I will freely state that the interaction of the little objects in the mix is tricky, since each one does some atomic thing and it is the collection of all of them that makes the game work. I rather like that outcome but it is probably harder to grok than a more god-object procedural approach might be.
But I like it, and doing things like adding a new ship or a different kind of obstacle will probably be easier. Maybe we’ll try something weird. And we do have Saucer and its missiles to do.
Overall, a good outcome this morning. I am pleased and comfortable. See you next time!
-
I lied about the two, well, no, actually, often I go into a section with two ideas and come out with more. Rather than revise the past, it has become my practice to say that I lied about the “two” and to move on with the next idea. ↩
-
Oh, come on, Ron! Four when you said two? Are you even paying attention? ↩