Kotlin 225 - Transaction
GitHub Decentralized Repo
GitHub Centralized Repo
With the simpler SpaceObjecgCollection in place, it’s time for Transaction. Do we just check out the old version? No, we do not. But why not? Also: You are waffling, sir!
The Plan
SpaceObjectCollection no longer saves separate collections for asteroids, missiles, saucer, ship, and splats, so there is no point in Transaction doing so either. There is surely a version of the class that we could check out and use. But aside from my desire to talk about it here, I don’t even consider that. Why not?
My reasoning is simple. The whole system works now, and its tests support change nicely. The Transaction and SpaceObjectCollection work well in concert: it’s just that Transaction can be simpler. While it’s true that an earlier version of Transaction that would work probably exists, I’d have to look for it. And, having found a candidate and brought it back in, it would be from the past. It’s as if we replaced you with a past version from before the holidays: you wouldn’t remember the holiday parties and your presents and people would think you had blacked out. You wouldn’t quite fit in to your own life as it is now.
The same with an old Transaction. It won’t quite fit in, and I’ll have to examine it carefully and adjust it to fit. If instead of going backward, I go forward, I can adjust the existing version, which fit perfectly, moving in small steps that I choose to bring it to the shape I want.
In short, I think that if I bring back an old one, I’ll have to debug it and perhaps warp it in ways it doesn’t want to go, and adjusting the current one, I’m very unlikely to have to debug it, and I can shape it just as I need to.
In other words, I’m going to refactor Transaction, not replace it with some old version. Let’s get to it.
Review the Terrain
SpaceObjectCollection only has three add methods:
fun add(deferredAction: DeferredAction) {
deferredActions.add(deferredAction)
}
fun add (spaceObject: SpaceObject) {
spaceObjects.add(spaceObject)
if (spaceObject is Collider) colliders.add(spaceObject)
}
fun addScore(score: Int) {
scoreKeeper.addScore(score)
}
Its remove comes down to this:
fun remove(deferredAction: DeferredAction) {
deferredActions.remove(deferredAction)
}
fun remove(spaceObject: SpaceObject) {
deferredActions.remove(spaceObject)
spaceObjects.remove(spaceObject)
if (spaceObject is Collider ) colliders.remove(spaceObject)
}
And it only maintains these physical collections:
private val colliders = mutableListOf<Collider>()
private val deferredActions = mutableListOf<DeferredAction>()
private val spaceObjects = mutableListOf<SpaceObject>()
So all we need Transaction to do is to send us deferredActions without losing their type, and otherwise just send us plain old SpaceObject instances.
However, presently Transaction maintains a raft of collections, and I think we have tests that check them. So we’ll need to cater to that necessity. Let’s look at Transaction:
val asteroids = mutableListOf<Asteroid>()
val missiles = mutableListOf<Missile>()
val saucers = mutableListOf<Saucer>()
val ships = mutableListOf<Ship>()
val splats = mutableListOf<Splat>()
val removes = mutableSetOf<SpaceObject>()
val deferredActionAdds = mutableSetOf<DeferredAction>()
val deferredActionRemoves = mutableSetOf<DeferredAction>()
We want to get rid of all the lists down to splats, accumulating all the SpaceObjects there. The DeferredActions should be kept separately, because we want to see that type explicitly in SpaceObjectCollection. We do not need a special collection for the removals.
Design Waffling
As I write about these two classes, I realize that they are coupled together in a rather complicated way. I’m designing Transaction right now based on what another class, SpaceObjectCollection, happens to do inside itself. I’m saying to myself:
Well, I can get rid of all this information here, because I happen to know that over there, this other object doesn’t use the information anyway.
This kind of thinking is never really desirable. Sure, we do need to think about how objects work together, but having them know about each others’ internal operation is probably going too far. This is particularly true here, because less than a week ago, we broke all this information out in Transaction precisely because, on that day, SpaceObjectCollection wanted it. Now, suddenly, we’re all like “never mind”. What’s up with that?
Part of me says that we should probably hold on to type information as long as we can, throwing it away at the last possible moment, and only then for good reason.
This calls into question our whole effort for the past three, maybe four articles.
History
As I recall my reasoning, I felt that the Interaction object was not carrying its weight, with its handful of methods and its very explicit understanding of what class could interact with what other class, in what way. So I wanted to go back to the prior algorithm, which simply gives every object a chance to interact with every other object, and leaves it up to them to decide what to do about it.
I think that was a righteous change: We removed 150 lines of code with that change, adding perhaps 20 back in, and object interactions themselves scarcely changed at all: all we did was add some interactions that do nothing.
Then we observed that SpaceObjectCollection was accumulating lots of type information that was no longer needed, because the pairing style interactions do not, and cannot, sort out the individual types for each element of each pair. Well, I say “cannot”. We might be able to do it, but the point of the pairwise operation is that we do not have to write lots of cases there. We have a simple double dispatch that recovers the type information where it is needed, in the individual interactions.
Point is, SpaceObjectCollection doesn’t need to preserve that much type information. I’m not even sure that it needs to break out the DeferredAction instances. That’s convenient, though, because we don’t really want to run them through interactions. Some experts even think we might benefit from handling timers in some other way entirely.
We simplified SpaceObjectCollection slightly when we converted it back to having only those three collections. But the truth is, we only saved about ten lines of code. That’s not a truly fair comparison, however, because we added a number of specialized functions in support of tests. Probably we saved 20 lines out of 110. Still not impressive.
We’ll get a similar effect if we change Transaction, though I’d like to try something and I’ll do that right now.
We’re on a clean version, so I can spike this. I want to know whether I can provide the special collections that the tests need by using extensions rather than adding methods to the actual class.
Yes. If Transaction just provided one function, say:
fun spaceObjects() = asteroids+missiles+saucers+ships+splats
Then a test needing asteroids could do this:
fun Transaction.asteroids(): List<Asteroid> {
return spaceObjects().filterIsInstance<Asteroid>()
}
Naturally, if Transaction never broke the space objects apart, it could just return that collection, the tests would run, and everything inside Transaction could be private. I’ve just made the asteroids collection private.
For best results, I’d have to put those extensions into a separate file under the tests. Let me try that.
Yes! With that extension in a separate file, I was able to quickly make the asteroids collection private in Transaction: tests just changed to add (). Let’s do the other collections as well.
Missiles done. I should be committing. Commit: asteroids and missiles collections private.
Commit: saucers collection is private. Commit: ship collection is private. Commit: splat collection is private.
Now let’s review Transaction as a whole. I think the proposed change, despite the coupling objection, is worth doing:
class Transaction {
private val asteroids = mutableListOf<Asteroid>()
private val missiles = mutableListOf<Missile>()
private val saucers = mutableListOf<Saucer>()
private val ships = mutableListOf<Ship>()
private val splats = mutableListOf<Splat>()
val removes = mutableSetOf<SpaceObject>()
val deferredActionAdds = mutableSetOf<DeferredAction>()
val deferredActionRemoves = mutableSetOf<DeferredAction>()
var shouldClear = false
var score = 0
fun add(asteroid: Asteroid) {asteroids.add(asteroid)}
fun add(missile: Missile) {missiles.add(missile)}
fun add(saucer: Saucer) {saucers.add(saucer)}
fun add(ship: Ship) {ships.add(ship)}
fun add(splat: Splat) {splats.add(splat)}
fun add(deferredAction: DeferredAction) {
deferredActionAdds.add(deferredAction)
}
fun addScore(scoreToAdd: Int) {
score += scoreToAdd
}
fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
if (shouldClear ) spaceObjectCollection.clear()
spaceObjectCollection.addScore(score)
removes.forEach { spaceObjectCollection.remove(it)}
deferredActionRemoves.forEach { spaceObjectCollection.remove(it)}
asteroids.forEach { spaceObjectCollection.add(it)}
missiles.forEach { spaceObjectCollection.add(it)}
saucers.forEach { spaceObjectCollection.add(it)}
ships.forEach { spaceObjectCollection.add(it)}
splats.forEach { spaceObjectCollection.add(it)}
deferredActionAdds.forEach { spaceObjectCollection.add(it)}
}
fun clear() {
shouldClear = true
}
fun remove(deferredAction: DeferredAction) {
deferredActionRemoves.add(deferredAction)
}
fun remove(spaceObject: SpaceObject) {
removes.add(spaceObject)
}
fun spaceObjects() = asteroids+missiles+saucers+ships+splats
// testing
fun firstRemove(): SpaceObject = removes.toList()[0]
}
There’s all those collections, all those adds, and all those lines in applyChanges. We don’t need them. Yes, removing them loses type information, and the only way we know we don’t need it is by looking at SpaceObjectCollection. But I want to claim that these two objects are halves of a specific hole and that it’s OK if they are similar, even though in general this coupling would be bad.
Let’s change to having a single space object collection, but retaining the deferred one.
class Transaction {
private val spaceObjects = mutableListOf<SpaceObject>()
val removes = mutableSetOf<SpaceObject>()
val deferredActionAdds = mutableSetOf<DeferredAction>()
val deferredActionRemoves = mutableSetOf<DeferredAction>()
private var shouldClear = false
private var score = 0
fun add(deferredAction: DeferredAction) {
deferredActionAdds.add(deferredAction)
}
fun add(spaceObject: SpaceObject) { spaceObjects.add(spaceObject) }
fun addScore(scoreToAdd: Int) {
score += scoreToAdd
}
fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
if (shouldClear ) spaceObjectCollection.clear()
spaceObjectCollection.addScore(score)
removes.forEach { spaceObjectCollection.remove(it)}
deferredActionRemoves.forEach { spaceObjectCollection.remove(it)}
spaceObjects.forEach { spaceObjectCollection.add(it)}
deferredActionAdds.forEach { spaceObjectCollection.add(it)}
}
fun clear() {
shouldClear = true
}
fun remove(deferredAction: DeferredAction) {
deferredActionRemoves.add(deferredAction)
}
fun remove(spaceObject: SpaceObject) {
removes.add(spaceObject)
}
fun spaceObjects() = spaceObjects
// testing
fun firstRemove(): SpaceObject = removes.toList()[0]
}
Took just moments to do that. Tests are green. Game is good, except that I have those darn double splats on missiles again. Is this a new bug? I know I’ve seen it before. Let me commit these changes and then I’ll check yesterday’s version.
Commit: remove detailed collections from Transaction.
Good news, the bug was there yesterday. I didn’t just create it.
The Defect
We’re seeing a second splat created, at the point where a missile times out, a couple of seconds after the first splat. We do not see a third. I add a quick print and fire one missile:
private val timeOut = OneShot(U.MISSILE_LIFETIME) {
it.remove(this)
it.add(Splat(this))
println("timeout")
}
That tells me that timeout is happening twice for a single missile. Didn’t we trace this problem to something about finalize once upon a time?
We trigger the timeout OneShot every time we update:
override fun update(deltaTime: Double, trans: Transaction) {
timeOut.execute(trans)
position = (position + velocity * deltaTime).cap()
}
Because it’s a OneShot, it will only trigger once until it fires. When do we trigger the DeferredActions?
Nope!
fun tick(deltaTime: Double) {
with (knownObjects) {
performWithTransaction { trans ->
forEach { it.update(deltaTime, trans) }
}
}
}
It used to be that this code did the deferredActions before it did the rest. Now it’s doing them in whatever order SpaceObjectCollection does them:
fun forEach(action: (SpaceObject)->Unit) {
deferredActions().forEach(action)
spaceObjects().forEach(action)
}
Yep~
Well, that isn’t quite it, we’re still doing the deferred ones first. I bet I know something that will fix it, however. Let’s try it.
fun tick(deltaTime: Double) {
with (knownObjects) {
performWithTransaction { trans ->
deferredActions().forEach { it.update(deltaTime, trans) }
}
performWithTransaction { trans ->
spaceObjects().forEach { it.update(deltaTime, trans) }
}
}
}
I think we have to commit the deferred actions before we do the other update.
Yes, that fixes the bug. How can I avoid doing it again?
fun tick(deltaTime: Double) {
updateTimersFirst(deltaTime)
thenUpdateSpaceObjects(deltaTime)
}
private fun updateTimersFirst(deltaTime: Double) {
with (knownObjects) {
performWithTransaction { trans ->
deferredActions().forEach { it.update(deltaTime, trans) }
}
}
}
private fun thenUpdateSpaceObjects(deltaTime: Double) {
with (knownObjects) {
performWithTransaction { trans ->
spaceObjects().forEach { it.update(deltaTime, trans) }
}
}
}
That should bring any would-be optimizer to a dead stop.
Test. A test breaks!
@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)
assertThat(mix.contains(missile)).describedAs("missile should be dead").isEqualTo(false)
assertThat(mix.missiles()).doesNotContain(missile)
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 is failing when it doesn’t find the splat (“splat should be present”.) What even is the any operator doing? Let’s read out the splats, we have that nice method now.
Found it. Very obscure.
The test had a really obscure defect: because it just calls cycle once with a large time, the first splat’s first update had a large time and it immediately timed out. I think we were actually checking the second splat all this time. Anyway the new test is green and good:
@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()
}
Amusing defect, a very rare case of a test hiding a defect. Commit: fix problem causing two splats per missile timeout, and broken test that didn’t find the problem.
Let’s sum up, it’s nearly breakfast time.
Summary
After some waffling, I decided to go ahead with the simplification of Transaction to match that of SpaceObjectCollection. Yes, the two classes are coupled, but it should come as no surprise that a Transaction and the data set it applies to know about each other.
That said, there might be a way to provide a common object or structure that both could share that would make the coupling more explicit and harder to get wrong. I’ll think about that. I’ll even make a sticky note about it. “common something in Transaction and SpaceObjectCollection to deal with coupling”.
I think there’s something suspicious about the intentional loss of type information that we’ve now put back into the system after carefully preserving types all the way down. Now that we have both versions (again) we’ll compare them and decide which of these designs is better and why. It may just be a case of balancing conflicting goals of simplicity and completeness.
I freely grant that I’ve waffled completely back and forth between almost no type preservation, to fully preserving types, and back, at least twice now. I could make the determination based on some principle, in which case I’d probably say “try never to lose type information” and go to the fully exploded version. But I don’t think a single principle applies.
We removed 10 lines of code, replaced five collections with just one, and removed about four or five methods, replacing again with just one. There is less scope, fewer concepts in this type-compressing version.
I’ll see if I can get this looked at by the Zoom group to get their views, and if you have opinions or questions, please feel free to start a conversation with me on Mastodon.social or, for now, on Twitter.
Which way is best? I honestly am right on the fence. I lean toward the shorter code, but my familiarity with duck typing is pushing me that way, since in a duck-typing situation I could fold all the different types down into one collection and when they fan back out, they’d all come up with the correct types. Here, it costs me a double dispatch to get the types back. But that’s still pretty darn simple.
I’d love to hear from you. Next time, I don’t know what I’ll do. I hope I don’t flip back the other way.
No promises.