GitHub Decentralized Repo
GitHub Centralized Repo

I’m working toward some small improvements to the delegation code. I’m not sure it’s worth it. I think we’ll have to try it to find out.

Last time, we completed the change to make collision delegation use the Kotlin by keyword. We removed the explicit forwarding through collisionStrategy, instead making the space objects all implement the Collider interface as well as SpaceObject. The interfaces now also inherit. All the flying objects implement SpaceObject directly and Collider by their CollisionStrategy.

I am far from certain that this is the best division of responsibility, but here’s what we have:

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)
}

interface SpaceObject: Collider {
    fun interactWith(other: SpaceObject, trans: Transaction)
    fun draw(drawer: Drawer)
    fun update(deltaTime: Double, trans: Transaction)
}

The way things stand now, the base class implements position and killRadius, and the delegate has getters for them:

class Ship(
    override var position: Point,
    val controls: Controls = Controls(),
    private val strategy: ShipCollisionStrategy = ShipCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.ship = this }
    override val killRadius = U.SHIP_KILL_RADIUS
    var velocity:  Velocity = Velocity.ZERO
    var heading: Double = 0.0
    ...

class ShipCollisionStrategy(): Collider {
    lateinit var ship: Ship
    override val position: Point
        get() = ship.position
    override val killRadius: Double
        get() = ship.killRadius

We have to do something like that, because both the base object and the Strategy need access to the position and killRadius of the object. Presently, the “primary” variables are in the base object, and the Strategies access them as shown above. An alternative is to allow the Strategy implement the variables and then the base would inherit them from the Strategy, just as it inherits the methods. That’s what we’re going to do.

To do that, we’ll need to change the interface to make the variable position a var rather than a val, because the base objects adjust position when they move.

Let’s try it. I guess I just have to change it in the interface. I don’t see a built-in refactoring that will do it. Let’s try it, we’re on a fresh commit.

I have to change all the definitions in the Strategy objects to look like this:

    override var position: Point = Point.ZERO
        get() = ship.position

The compiler required me to initialize them. Tests are green, but I don’t want to commit yet. Let’s see if this is going to work at all. Ship gives a good example of what I’m facing:

class Ship(
    override var position: Point,
    val controls: Controls = Controls(),
    private val strategy: ShipCollisionStrategy = ShipCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.ship = this }
    override val killRadius = U.SHIP_KILL_RADIUS
    var velocity:  Velocity = Velocity.ZERO

Ship is created with a position. I don’t see how we can get that to refer over to the Strategy, unless I give it a new name and save it explicitly, like this:

class Ship(
    var pos: Point,
    val controls: Controls = Controls(),
    private val strategy: ShipCollisionStrategy = ShipCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { 
        position = pos
        strategy.ship = this 
    }
    override val killRadius = U.SHIP_KILL_RADIUS

Now I’ll move the killRadius over to the Strategy and fix up the position.

class ShipCollisionStrategy(): Collider {
    lateinit var ship: Ship
    override var position: Point = Point.ZERO
    override val killRadius = U.SHIP_KILL_RADIUS

Will this pass the tests and run the game? Some compile errors arise:

    @Test
    fun `count interactions`() {
        val objects = SpaceObjectCollection()
        val n = 12
        for (i in 1..n) {
            objects.add(
                Ship(
                    position = Vector2.ZERO
                )
            )
        }
        val pairs = objects.pairsToCheck()
        assertThat(pairs.size).isEqualTo(n*(n-1)/2)
    }

That needs to say pos. I think I should have used rename refactoring. I had to change a bunch of lines like that and I know I didn’t expand the constructors like that. IDEA did it to save me. I am green. Game is good. I think this is going to work, so I’ll commit so far. Commit: Ship and Strategy move implementation of position and killRadius to Strategy.

I get some warnings, because the existence of a var and only a getter makes Kotlin worry. We’ll get past that. Let’s do Saucer.

class Saucer (
    private val strategy: SaucerCollisionStrategy = SaucerCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.saucer = this }

    override lateinit var position: Point
    override val killRadius = U.SAUCER_KILL_RADIUS

class SaucerCollisionStrategy(): Collider {
    lateinit var saucer: Saucer
    override var position: Point = Point.ZERO
        get() = saucer.position
    override val killRadius: Double
        get() = saucer.killRadius

We won’t need the lateinit when we move these, because we have to initialize over in Strategy anyway.

class Saucer (
    private val strategy: SaucerCollisionStrategy = SaucerCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.saucer = this }
    ...

class SaucerCollisionStrategy(): Collider {
    lateinit var saucer: Saucer
    override var position: Point = Point.ZERO
    override val killRadius: Double = U.SAUCER_KILL_RADIUS
    ...

Test. We are green. Commit: Saucer and Strategy move implementation of position and killRadius to Strategy.

So far so good. Missile.

class MissileCollisionStrategy(): Collider {
    lateinit var missile: Missile
    override lateinit var position: Point
    override val killRadius: Double = U.MISSILE_KILL_RADIUS

Corresponding deletes from Missile. Test. We’re good. Commit: Missile and Strategy move implementation of position and killRadius to Strategy.

OK, Asteroid. It’ll be a bit more tricky, but just a bit:

class Asteroid(
    override var position: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    private val splitCount: Int = 2,
    private val strategy: AsteroidCollisionStrategy = AsteroidCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.asteroid = this}
    override val killRadius: Double =
        when (splitCount) {
            2 -> U.ASTEROID_KILL_RADIUS
            1 -> U.ASTEROID_KILL_RADIUS/2
            else -> U.ASTEROID_KILL_RADIUS/4
        }

First of all, it takes position in its constructor. I’ve learned my lesson about renaming that with IDEA. That gets me here:

class Asteroid(
    var pos: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    private val splitCount: Int = 2,
    private val strategy: AsteroidCollisionStrategy = AsteroidCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { 
        position = pos
        strategy.asteroid = this
    }

I can change the Strategy to correspond:

class AsteroidCollisionStrategy(): Collider {
    lateinit var asteroid: Asteroid
    override var position: Point = Point.ZERO
    override val killRadius: Double
        get() = asteroid.killRadius

I think we could leave it like this, allowing Asteroid to manage the killRadius logic. That might be better than another reference from Strategy back to the Asteroid. Test. A test fails though.

Expecting LinkedHashSet:
  []
to contain:
  [Missile Vector2(x=249.0, y=0.0) (1.0)]
but could not find the following element(s):
  [Missile Vector2(x=249.0, y=0.0) (1.0)]

The test:

    @Test
    fun `missile asteroid collision`() {
        val asteroid = Asteroid(Point.ZERO)
        asteroid.pos = Point(249.0, 0.0)
        val missile = Missile(Point.ZERO)
        missile.position = asteroid.pos
        val trans = Transaction()
        missile.interact(asteroid, trans)
        assertThat(trans.removes).contains(missile)
        asteroid.interact(missile, trans)
        assertThat(trans.removes).contains(asteroid)
    }

Appears we didn’t register the collision. But we didn’t change the missile. Weird. A quick check tells me that we didn’t remove the asteroid either. So in both cases, the Collision object must have said they weren’t colliding.

A print tells me that in this test, the Asteroid position is still at zero, not 249. Game confirms, asteroids no longer collide.

I see that the test is setting asteroid.pos. I think I know the problem … kotlin renamed that member a bit too aggressively.

I think I should revert. There are lots of rippled changes of position to pos. Let’s back out and do this another way.

One more automated try. First change the signature to have the name pos:

class Asteroid(
    override var pos: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    private val splitCount: Int = 2,
    private val strategy: AsteroidCollisionStrategy = AsteroidCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init { strategy.asteroid = this}
    override val killRadius: Double =
        when (splitCount) {
            2 -> U.ASTEROID_KILL_RADIUS
            1 -> U.ASTEROID_KILL_RADIUS/2
            else -> U.ASTEROID_KILL_RADIUS/4
        }

Now we will be in trouble because the Strategy isn’t ready but that’s a simple change:

class AsteroidCollisionStrategy(): Collider {
    lateinit var asteroid: Asteroid
    override var position: Point = Point.ZERO
    override val killRadius: Double
        get() = asteroid.killRadius

Could it be this easy? No, I need to set position in init. And the rename was still too aggressive, so I have to find all the senders of .pos and fix them. Grr.

class Asteroid(
    private var pos: Point,
    val velocity: Velocity = U.randomVelocity(U.ASTEROID_SPEED),
    private val splitCount: Int = 2,
    private val strategy: AsteroidCollisionStrategy = AsteroidCollisionStrategy()
) : SpaceObject, Collider by strategy {
    init {
        position = pos
        strategy.asteroid = this
    }

We are green now. Game runs. Finally! Commit: Asteroid and Strategy move implementation of position and killRadius to Strategy.

Summary

We’ve done what we set out to do, moving the implementation of position to the Strategy objects, and moving killRadius over for all but Asteroid, having decided to leave that one where it is, to avoid a reference back from the Strategy. There might be a better way to approach that, but it works correctly and I’m not bothered by it.

We’ve removed a few lines of code and, rather than fetching values, we’re sharing the same declarations. (No telling how Kotlin really does it, of course.)

We might be able to avoid the explicit initialization with lateinit, but I think an explicit init is better anyway.

The big question to me is whether it was worth doing.

Yes
The code is simpler, and is a bit closer to standard usage, I think.
No
While it wouldn’t have taken longer to do it originally this way, the refactoring was problematical, mostly because of the tests referring to the innards of the objects in a few cases. I think that the time between where I started this morning and now, about 90 minutes, was reasonable but more than I’d have wanted to spend.

Part of that time was spent learning. That’s fine. Part was spent debugging. That’s chalked up to the difficulty of the refactoring, and to the fact that even though I tried automated refactorings, neither of the ways I went about it did what I needed.

Net Net
I’m not sorry that I did it, because it’s the first time I have done quite that sort of thing, so the learning has high value. But it did take longer than it might have, and I’m not sure how I’d go faster next time. I’ll think on it and ask my pals.

I’m more troubled by the need to pass the original receiver (e.g. Asteroid) in to the Strategy, but they need to be able to remove the object. That need has required me to put an init on each of the base objects, to pass itself over to its Strategy. Ideally a real Strategy wouldn’t know its client. Maybe these aren’t real Strategy objects. Maybe I still have something to learn about how to do this. (That seems certain.)

Net Net Net
Worth doing for the learning, maybe not something you’d do on a real product if you were where we were last time. I do think I’d do the conversion to delegation by, since that’s the official Kotlin way. This last improvement is pretty thin.
Concern
I may be just about out of things to do here. What should I do next?