Kotlin 230 - Not So Bad
GitHub Decentralized Repo
GitHub Centralized Repo
Let’s simplify some tests. I think we can get
knownObjectsout of Game.
Wednesday 1700
Here’s one test that we can simplify:
@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)
assertThat(mix.missiles()).contains(missile)
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)
game.cycle(U.MISSILE_LIFETIME + 0.1)
assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
assertThat(mix.missiles()).doesNotContain(missile)
assertThat(mix.splats()).describedAs("splat should be present").isNotEmpty()
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.splats()).describedAs("splat should be gone").isEmpty()
}
I can see two ways to go here. I’ve mentioned the possibility of changing GameCycle.cycle to expect elapsed time rather than deltaTime. Could do that first, before changing this test and others like it.
Darn. I thought I was just going to adjust tests. OK, we’ll go ahead and adjust tests, deal with elapsed vs delta later.
I do this:
fun `missile and splat death`() {
val mix = SpaceObjectCollection()
val ship = Ship(
position = U.randomPoint()
)
val missile = Missile(ship)
mix.add(missile)
val cycler = GameCycler(mix)
assertThat(mix.contains(missile)).isEqualTo(true)
assertThat(mix.missiles()).contains(missile)
cycler.cycle(0.1)
assertThat(mix.deferredActions().any { it is DeferredAction }).describedAs("deferred action should be present").isEqualTo(true)
cycler.cycle(U.MISSILE_LIFETIME - 0.1)
cycler.cycle( 0.2)
assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
assertThat(mix.missiles()).doesNotContain(missile)
assertThat(mix.splats()).describedAs("splat should be present").isNotEmpty()
assertThat(mix.any { it is Splat }).describedAs("splat should be present").isEqualTo(true)
cycler.cycle(0.2) // needs a tick to init
cycler.cycle(2.3) // Splat lifetime is 2.0
assertThat(mix.splats()).describedAs("splat should be gone").isEmpty()
}
And default the drawer parameter in cycle to null, for convenience:
fun cycle(deltaTime: Double, drawer: Drawer?= null) {
tick(deltaTime)
beforeInteractions()
processInteractions()
U.AsteroidTally = knownObjects.asteroidCount()
createNewWaveIfNeeded()
createSaucerIfNeeded()
createShipIfNeeded()
drawer?.let { draw(drawer) }
}
We already accept null, so no issue there. That test is green. Commit: convert test using Game(mix) to use GameCycler.
That check for deferredActions is bizarre, now that I look at it.
assertThat(mix.deferredActions().any { it is DeferredAction }).describedAs("deferred action should be present").isEqualTo(true)
Kotlin says that’s always true, and I believe it. Remove that. Test again, green again, commit again: Remove useless assert.
Another test that sets the mix:
@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.spaceObjects().size).describedAs("mix size").isEqualTo(2)
assertThat(mix.deferredActions().size).describedAs("deferred size").isEqualTo(1)
val deferred = mix.deferredActions().find { it.delay == 7.0 }
assertThat(deferred).isNotNull
game.cycle(7.2) //ELAPSED seconds
val saucer = mix.saucers().first()
assertThat(saucer).isNotNull
}
That converts readily to this:
@Test
fun `game-centric NO MAKER saucer appears after seven seconds`() {
val mix = SpaceObjectCollection()
mix.add(Ship(U.CENTER_OF_UNIVERSE))
mix.add(Asteroid(Point(100.0,100.0)))
val cycler = GameCycler(mix)
cycler.cycle(0.1) // ELAPSED seconds
assertThat(mix.spaceObjects().size).describedAs("mix size").isEqualTo(2)
assertThat(mix.deferredActions().size).describedAs("deferred size").isEqualTo(1)
val deferred = mix.deferredActions().find { it.delay == 7.0 }
assertThat(deferred).isNotNull
cycler.cycle(7.2)
val saucer = mix.saucers().first()
assertThat(saucer).isNotNull
}
Nice enough.
You know, if we renamed Game to GameMaker and GameCycler to Game … but no.
It turns out that those are the only two tests that actually create Game with a mix. There are still ten that don’t, and other changes have reduced the original list that I thought was 25.
Commit that test.
Can we remove the Game’s starting mix parameter and create the mix internally?
I tried something, but must have made a mistake. Roll back. Also it is dinner time. We’ll do it again after. Or tomorrow.
Thursday 0800
OK, so “tomorrow”, i.e. today, has arrived. Before I look at removing the Game’s starting parameter, I want to do a bit of blue sky thinking, since I had an idea while trying not to wake up.
Duplication as Opportunity
Whenever the same, or very similar code is found in multiple places, it is a sign that there may be an idea in our head that isn’t well-expressed in our code. With a bit of thinking, we can often remove some or all of that duplication and wind up with code that is smaller, simpler, and more expressive. Let me give a tiny example before moving on the the larger blue sky idea.
The Asteroids game uses a Transaction object that allows the game’s objects to add or remove things from the game. The Transaction is used so that the mix of things in the game changes only after all the objects have had their say. If that were not the case, it might be possible for A to collide with B, remove itself, and then B would not detect a collision with A. So we retain the changes in a Transaction until everyone has had a look at the current picture.
The inevitable result of this design is that we tend to create a lot of code that looks like this:
val trans = Transaction()
doSomething(trans)
knownObjects.apply(trans)
After we do this a few times we notice the common pattern:
val trans = Transaction()
...
knownObjects.apply(trans)
Duplication. What is the idea? Well, the idea that I chose to express was performWithTransaction, and the code now looks like this:
knownObjects.performWithTransaction
{ doSomeething(trans) }
The idea is better expressed, the code is smaller and more clear, and we can’t ever forget to apply the transaction, because the perform handles it.
You may have seen a similar thing done with opening a file, processing it, and closing it all in one go. I like that pattern a lot, because I so often would forget to close the file.
So, the point now hammered viciously into the ground is that duplication can be an indicator of an idea we have that could be better expressed in the code, and it’s therefore an opportunity to make the code better.
Blue Sky
This may not pan out, but I want to get the idea down while it’s fresh, because if it does work out, I think it’ll be quite educational. In fact, I expect it to be educational even if it doesn’t work out, because I’ll learn how to do something wrong, which is useful in itself, so long as I keep it filed under “wrong” and not “things to do”.
First, observe duplication.
All our objects implement two member variables and five required methods, from the SpaceObject interface:
interface Collider {
val position: Point
val killRadius: Double
fun interact(asteroid: Asteroid, trans: Transaction)
fun interact(missile: Missile, trans: Transaction)
fun interact(saucer: Saucer, trans: Transaction)
fun interact(ship: Ship, trans: Transaction)
fun interactWith(other: Collider, trans: Transaction)
}
Some of them implement the base interact methods exactly the same way. In some cases there are similarities. Perhaps some of them are quite different: I’d have to review to be sure how much similarity there is. But there is similarity, for certain, and it is the similarity that goes with the notion “I am a Collider”.
Now in early days in the decentralized version of this program, I removed some of the duplication by using implementation inheritance from a concrete superclass. The delightful and charming GeePaw Hill objected to this and offered a really interesting object, Subscriptions, that allowed us to remove the duplication without implementation inheritance. It was really quite nifty, and I commend those articles to you if you haven’t already read them.
In this version, I’ve removed the Subscriptions, and at present we have the interface for Collider and some very limited duplication of implementation of those methods. It’s not bad at all and I really am not bothered by it. At least not much.
However, this morning while dozing I hatched an idea, again borrowed from GeePaw, of using Kotlin’s ability to delegate using its by keyword. We’ll explore in more detail in subsequent articles but the essence is this:
Suppose we could create a new object that could handle the common elements of Collider for some of the space objects. If we did that, we’d have to create that object, and each of the users of it would still have to implement all the methods, even if they just looked like this:
fun interact(...) {
helper.interact(...)
}
That might still be worth doing if there was a lot fo code to be moved over, as with any helper objects, but there would still be all those methods duplicated, even if all they did was forward the message.
The Kotlin by keyword helps us with that. I think we can apply it to Collider, and I propose to find out.
That will be for an upcoming article, probably the very next one. For now, I just wanted to get the blue sky thinking down on virtual paper, and to foreshadow what’s coming up.
We now return to your regularly scheduled refactoring.
Removing the Starting Mix
Game now only has a few members:
class Game(val knownObjects:SpaceObjectCollection = SpaceObjectCollection()) {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
private var cycler: GameCycler = GameCycler(knownObjects, 0, Controls())
I do think we can get rid of cycler if we make it optional but let’s stick to our plan here and try to get rid of the parameter. The good news is that no one is providing a mix when creating Game instances: we’ve changed all those tests to use GameCycler or by some other means. Let’s see about references to the parameter.
Ah! That’s where I got the idea of 25 uses. There are, in fact 25 accesses to knownObjects, most of them from tests. So we’ll make it a member variable, and I guess we’ll still have to init it, but at least it won’t be a construction parm any more.
class Game() {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
val knownObjects:SpaceObjectCollection = SpaceObjectCollection()
private var cycler: GameCycler = GameCycler(knownObjects, 0, Controls())
That should run green. Yes. Commit: remove knownObjects as construction parameter. Still public val.
What I’d really like to do would be to remove all external references to knownObjects, and to create a new one every time we create a GameCycler and pass it in. Game would have no institutional knowledge of knownObjects at all. To do that, I’ll have to tick through all 25 accesses to it. And there will surely be some cases where it’s difficult: no one could b that lucky.
@Test
fun `game creates asteroids after a while`() {
val game = Game()
val controls = Controls()
game.createInitialContents(controls)
assertThat(game.knownObjects.asteroidCount()).isEqualTo(0)
game.cycle(0.2)
game.cycle(0.3)
game.cycle(4.2)
assertThat(game.knownObjects.asteroidCount()).isEqualTo(4)
}
This isn’t even a Game responsibility any more. Let’s see whether we can recast this directly on GameCycler:
fun `game creates asteroids after a while`() {
val controls = Controls()
val mix = SpaceObjectCollection()
val cycler = GameCycler(mix, 4, controls)
assertThat(mix.asteroidCount()).isEqualTo(0)
cycler.cycle(0.2)
cycler.cycle(0.3)
cycler.cycle(4.2)
assertThat(mix.asteroidCount()).isEqualTo(4)
}
I think that may work. It does. Great. Commit: adjust test not to access game.knownObjects.
There are a few similar ones. This next one is harder:
@Test
fun `game creates asteroids even when quarter comes rapidly`() {
val game = Game()
val controls = Controls()
game.createInitialContents(controls)
assertThat(game.knownObjects.asteroidCount()).isEqualTo(0)
game.cycle(0.2)
game.cycle(0.3)
game.insertQuarter(controls)
game.cycle(0.2)
game.cycle(0.3)
assertThat(game.knownObjects.asteroidCount()).isEqualTo(0)
game.cycle(4.2)
assertThat(game.knownObjects.asteroidCount()).isEqualTo(4)
}
The issue is that we’re going to init the game. In my current plan, I was going to have that create a new mix, so that Game wouldn’t have to know about it. For this to work, I think we’ll need access to the mix from GameCycler.
Let’s skip this one. I think we’re going to change the rhythm of things between Game and Cycler. I’ll look for easier ones to pry away from Game and knownObjects.
I think that the Game will have to hold on to the GameCycler so as to forward to it, so let’s provide a new method that can be called on game to access the current mix from the cycler:
class Game() {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
val knownObjects:SpaceObjectCollection = SpaceObjectCollection()
private var cycler: GameCycler = GameCycler(knownObjects, 0, Controls())
fun currentMix(): SpaceObjectCollection = cycler.currentMix()
class GameCycler
fun currentMix(): SpaceObjectCollection = knownObjects
Now a lot of these tests accessing knownObjects should use this method instead. That works for a few. Not interesting. Commit: replace access to knownObjects with currentMix().
This is getting downright tedious.
What’s up when lots of tests need changing?
Thanks for asking. When lots of tests need changing, it is often a sign that we’re writing tests that are too large, like tests that check a whole story rather than tests that check the specific things that really should be tested. Now, sometimes, we really do want to test a story, and that may be OK, but more often, we’re testing at a higher level because our objects don’t help us test at a lower level.
In Asteroids, I’ve complained often in these articles that I’m writing a story test but can’t think of something better. So to a degree, I’ve done this to myself.
Another cause of test revision, however, can be design changes. Sometimes we might have one object that does some useful thing, and we later see a better design that divides up that useful thing into two smaller useful things. And sometimes, when we divide up those responsibilities, the original object no longer offers those capabilities, because they are now embedded in the other, smaller object.
That is part of what’s going on here. We have partitioned Game’s behavior into a new Game class, much like the old class, and GameCycler, a new class … and we’re changing Game’s knowledge of the details of GameCycler, because it doesn’t need to know. But our tests want to know things that Game doesn’t provide, and perhaps even GameCycler wouldn’t normally provide.
So we’re stuck with changing things. I attribute most of the issues here to having written tests that do test the right things, and test them well, but do not test them as compactly and directly as might be ideal. So while dividing up Game and GameCycler has shown these issues, I don’t think it has caused them so much as just shining a light on my less than ideal testing practice.
As we change the tests, I think we tend to see them getting simpler, showing, perhaps, how they might have been better done back then. But I’m not about condemning the past. I’m about observing it and being happy when I do better today than I did yesterday.
Let’s get back to it. We still have 15 accesses to knownObjects to trim down. We might not finish them today. No hurry, we’re always green and good to ship.
I find a bunch where I can just change knownObjects to currentMix(). That’s convenient and by the way what kind of fool doesn’t provide accessors from the get-go for things like that?
This one is a bit odd:
@Test
fun `how many asteroids per wave`() {
val game = Game()
game.insertQuarter(Controls())
val cycler = GameCycler(game.currentMix(), 4)
assertThat(cycler.howMany()).isEqualTo(4)
assertThat(cycler.howMany()).isEqualTo(6)
assertThat(cycler.howMany()).isEqualTo(8)
assertThat(cycler.howMany()).isEqualTo(10)
assertThat(cycler.howMany()).isEqualTo(11)
assertThat(cycler.howMany()).isEqualTo(11)
}
The test used to refer to knownObjects where it now says currentMix. No surprise there. But it’s using the call just to create a new cycler, and then just to check the number of asteroids it’ll create. We don’t need to do all that. We just want a valid cycler starting with 4.
@Test
fun `how many asteroids per wave`() {
val cycler = GameCycler(SpaceObjectCollection(), 4)
assertThat(cycler.howMany()).isEqualTo(4)
assertThat(cycler.howMany()).isEqualTo(6)
assertThat(cycler.howMany()).isEqualTo(8)
assertThat(cycler.howMany()).isEqualTo(10)
assertThat(cycler.howMany()).isEqualTo(11)
assertThat(cycler.howMany()).isEqualTo(11)
}
That was the last test reference to knosnObjects. We can make it private, and we do:
class Game() {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
private val knownObjects:SpaceObjectCollection = SpaceObjectCollection()
private var cycler: GameCycler = GameCycler(knownObjects, 0, Controls())
Much better. Test. Green. Commit: knownObjects now private in Game. Tests generally use currentMix().
Nice. Now, could we go to a local mix in Game?
class Game ...
private var cycler: GameCycler = GameCycler(knownObjects, 0, Controls())
private fun initializeGame(controls: Controls, shipCount: Int) {
numberOfAsteroidsToCreate = U.ASTEROID_STARTING_COUNT
knownObjects.performWithTransaction { trans ->
createInitialObjects(trans,shipCount, controls)
}
}
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls
) {
knownObjects.scoreKeeper = ScoreKeeper(shipCount)
cycler = GameCycler(knownObjects, numberOfAsteroidsToCreate, controls)
trans.clear()
}
If we didn’t feel the need to create the cycler immediately, we wouldn’t need that reference. In fact I guess we could remove the reference now by creating a new collection. Then we can make a local collection in initializeGame and pass it to createInitialObjects. Yes.
class Game() {
private var lastTime = 0.0
private var numberOfAsteroidsToCreate = 0
private var cycler: GameCycler = GameCycler(SpaceObjectCollection(), 0, Controls())
private fun initializeGame(controls: Controls, shipCount: Int) {
numberOfAsteroidsToCreate = U.ASTEROID_STARTING_COUNT
val knownObjects = SpaceObjectCollection()
knownObjects.performWithTransaction { trans ->
createInitialObjects(trans,shipCount, controls, knownObjects)
}
}
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls,
knownObjects: SpaceObjectCollection
) {
knownObjects.scoreKeeper = ScoreKeeper(shipCount)
cycler = GameCycler(knownObjects, numberOfAsteroidsToCreate, controls)
trans.clear()
}
This removes the knownObjects member entirely. Test. Green. Commit: remove knownObjects member entirely.
Now what about numberOfAsteroidsToCreate? Why is that even there? Let’s pass it as a parameter instead.
private fun initializeGame(controls: Controls, shipCount: Int) {
val knownObjects = SpaceObjectCollection()
knownObjects.performWithTransaction { trans ->
createInitialObjects(trans,shipCount, U.ASTEROID_STARTING_COUNT, controls, knownObjects)
}
}
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
numberOfAsteroidsToCreate: Int,
controls: Controls,
knownObjects: SpaceObjectCollection
) {
knownObjects.scoreKeeper = ScoreKeeper(shipCount)
cycler = GameCycler(knownObjects, numberOfAsteroidsToCreate, controls)
trans.clear()
}
Test. Green as my finger after wearing my Captain Midnight Secret Decoder Ring. Commit: remove numberOfAsteroidsToCreate member variable.
No, wait. A warning points out that I’m just passing a constant in as that last parameter. Can IDEA inline it or must I do it myself? I do it myself:
private fun initializeGame(controls: Controls, shipCount: Int) {
val knownObjects = SpaceObjectCollection()
knownObjects.performWithTransaction { trans ->
createInitialObjects(trans,shipCount, controls, knownObjects)
}
}
private fun createInitialObjects(
trans: Transaction,
shipCount: Int,
controls: Controls,
knownObjects: SpaceObjectCollection
) {
knownObjects.scoreKeeper = ScoreKeeper(shipCount)
cycler = GameCycler(knownObjects, U.ASTEROID_STARTING_COUNT, controls)
trans.clear()
}
Now the commit: remove numberOfAsteroidsToCreate member variable.
This is a fine stopping point. Let’s sum up:
Summary
After a moderately long period of changes, we have removed both the knownObjects and numberOfAsteroidsToCreate member variables from Game. It’s now down to two, and I think we could get it to one if we cared to. Almost all our changes were due to tests needing to change. I think that had I thought of creating an accessor for knownObjects sooner, the changes might have gone faster, but they were all just a minute or two for each test. Tedious but easy.
These changes simplify the Game class and they isolate most of the care and feeding of the knownObjects to GameCycler and the transactions it creates and applies.
Are these changes “worth it”? Well, that depends.
I think there’s no question that the class is simpler and therefore easier to change. However, one might argue that we have no plans to change it much and it was probably easy enough already. That might be true: I felt no great resistance to any work I wanted to do.
The real value here and now isn’t the improvement to this specific class. It is the demonstration that we can improve code when it needs improvement, in very small steps and without breaking things. That’s an important lesson because it applies to all of our work, not just to this small example.
Butler Lampson is credited with saying “All problems in computer science can be solved by another level of indirection”, and our addition of the currentMix() function is a perfect example of that. Prior to its introduction, we had all kinds of code referring to the knownObjects member variable in Game, and after we added it, all that code now neatly refers to the collection in GameCycler, a level of indirection beyond Game.
I think that if I were adept with IDEA’s magical pattern-matching replacement edit, I could have done them all in one big go. I am not adept with that feature. In fact I haven’t tried it even once. And I preferred to inspect the cases as I did them, just to be sure they weren’t doing something weird. Probably I could have trusted the tests to tell me that. I’ll try to learn more about the power tool. The simple manual changes worked well for us today.
Next time … tomorrow, not this afternoon, even if I do want to do a bit more work this afternoon … tomorrow we’ll take a look at that blue sky delegation idea.
See you then!