GitHub Decentralized Repo
GitHub Centralized Repo

Now that the Collision Strategies area all broken out, let’s have a look and see what we think about them. Am I reversing yesterday’s work? Yes, some of it. That’s OK. (Ninth decade? Really?)

I wish I had a decent way to display these Strategy objects side by side, but no, we’ll have to look at them in a vertical arrangement. They are similar, yet all a bit different:

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

    override fun interact(asteroid: Asteroid, trans: Transaction) {}
    override fun interact(missile: Missile, trans: Transaction) =
        checkCollision(missile, trans)
    override fun interact(saucer: Saucer, trans: Transaction) =
        checkCollision(saucer, trans)
    override fun interact(ship: Ship, trans: Transaction) =
        checkCollision(ship, trans)

    private fun checkCollision(other: Collidable, trans: Transaction) {
        Collision(asteroid).executeOnHit(other) {
            dieDueToCollision(trans)
        }
    }

    private fun dieDueToCollision(trans: Transaction) {
        trans.remove(asteroid)
        trans.add(Splat(asteroid))
        splitIfPossible(trans)
    }

    private fun splitIfPossible(trans: Transaction) {
        if (asteroid.splitCount >= 1) {
            trans.add(asSplit(asteroid))
            trans.add(asSplit(asteroid))
        }
    }

    private fun asSplit(asteroid: Asteroid): Asteroid =
        Asteroid(
            position = asteroid.position,
            killRadius = asteroid.killRadius / 2.0,
            splitCount = asteroid.splitCount - 1
        )
}

class MissileCollisionStrategy(val missile: Missile): Collider {
    override val position: Point
        get() = missile.position
    override val killRadius: Double
        get() = missile.killRadius

    override fun interact(asteroid: Asteroid, trans: Transaction) =
        checkAndScoreCollision(asteroid, trans, asteroid.getScore())
    override fun interact(missile: Missile, trans: Transaction) =
        checkAndScoreCollision(missile, trans, 0)
    override fun interact(saucer: Saucer, trans: Transaction) =
        checkAndScoreCollision(saucer, trans,saucer.getScore())
    override fun interact(ship: Ship, trans: Transaction) =
        checkAndScoreCollision(ship, trans, 0)

    private fun checkAndScoreCollision(other: Collidable, trans: Transaction, score: Int) {
        Collision(other).executeOnHit(missile) {
            terminateMissile(trans)
            if (missile.missileIsFromShip) trans.addScore(score)
        }
    }

    private fun terminateMissile(trans: Transaction) {
        missile.timeOut.cancel(trans)
        trans.remove(missile)
    }

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

    override fun interact(asteroid: Asteroid, trans: Transaction) =
        checkCollision(asteroid, trans)
    override fun interact(missile: Missile, trans: Transaction) {
        if (missile == saucer.currentMissile) saucer.missileReady = false
        checkCollision(missile, trans)
    }
    override fun interact(saucer: Saucer, trans: Transaction) { }
    override fun interact(ship: Ship, trans: Transaction) {
        saucer.sawShip = true
        saucer.shipFuturePosition = ship.position + ship.velocity * 1.5
        checkCollision(ship, trans)
    }

    private fun checkCollision(collider: Collidable, trans: Transaction) {
        Collision(collider).executeOnHit(saucer) {
            trans.add(Splat(saucer))
            trans.remove(saucer)
        }
    }
}

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

    override fun interact(asteroid: Asteroid, trans: Transaction) =
        checkCollision(asteroid, trans)
    override fun interact(missile: Missile, trans: Transaction) =
        checkCollision(missile, trans)
    override fun interact(saucer: Saucer, trans: Transaction) =
        checkCollision(saucer, trans)
    override fun interact(ship: Ship, trans: Transaction) { }

    private fun checkCollision(other: Collidable, trans: Transaction) {
        Collision(other).executeOnHit(ship) {
            trans.add(Splat(ship))
            trans.remove(ship)
        }
    }
}

One somewhat subtle difference among these is the handling of interaction of objects with other instances of the same class:

class AsteroidCollisionStrategy(val asteroid: Asteroid): Collider {
    override fun interact(asteroid: Asteroid, trans: Transaction) {}

class MissileCollisionStrategy(val missile: Missile): Collider {
    override fun interact(missile: Missile, trans: Transaction) =
        checkAndScoreCollision(missile, trans, 0)

class SaucerCollisionStrategy(val saucer: Saucer): Collider {
    override fun interact(saucer: Saucer, trans: Transaction) { }

class ShipCollisionStrategy(val ship: Ship): Collider {
    override fun interact(ship: Ship, trans: Transaction) { }

Three of the four classes don’t interact with themselves at all. Of course, there is never more than one ship, and never more than one saucer, so the code for those could be anything, but “nothing” seems best. Missiles, however, can shoot each other down1.

But the important point isn’t what they do, it is that each of the four Strategies currently has a unique combination of behavior for the four class-specific methods. Could we make them all the same? And if we did, would that let us reduce the duplication we see in these little classes? Well, we could. We could make each of the currently empty ones do the same checkCollision (or equivalent) that the other three do. In two of those cases, it would be benign, because it’s never going to be called. (I don’t really like providing code that will never be called, but we can imagine it.) But in the case of asteroids, we’d then have to do something special somewhere else, because asteroids simply do not collide with each other.

We could make these classes more similar in other ways. We could rename checkCollisionAndScore to checkCollision. Perhaps we’d even find a way to wedge the scoring in somewhere else, Still, scoring is only a missile kind of thing, so spreading that out among all the other objects is likely to be worse.

Feature Envy?

There’s another issue worth considering. Some of the methods in some of the Strategy objects seem to have a serious case of Feature Envy, for example:

class AsteroidCollisionStrategy
    private fun splitIfPossible(trans: Transaction) {
        if (asteroid.splitCount >= 1) {
            trans.add(asSplit(asteroid))
            trans.add(asSplit(asteroid))
        }
    }

    private fun asSplit(asteroid: Asteroid): Asteroid =
        Asteroid(
            position = asteroid.position,
            killRadius = asteroid.killRadius / 2.0,
            splitCount = asteroid.splitCount - 1
        )

Here we are, looking into the asteroid to get information from it as to whether it should split and then splitting it. We call this kind of code “Feature Envy”, because it seems to be wishing that the other object, Asteroid in this case, would be more helpful2.

We certainly could move that capability back into asteroid. In fact, let’s do that.

AsteroidCollisionStrategy
    private fun dieDueToCollision(trans: Transaction) {
        trans.remove(asteroid)
        trans.add(Splat(asteroid))
        asteroid.splitIfPossible(trans)
    }

class Asteroid
    private fun checkCollision(other: Collidable, trans: Transaction) {
        Collision(asteroid).executeOnHit(other) {
            dieDueToCollision(trans)
        }
    }

    private fun dieDueToCollision(trans: Transaction) {
        trans.remove(asteroid)
        trans.add(Splat(asteroid))
        asteroid.splitIfPossible(trans)
    }

That works just fine, of course. Commit: Strategy asks asteroid to split.

Let’s look for further feature envy issues while we’re at it.

SaucerCollisionStrategy is interesting:

class SaucerCollisionStrategy
    override fun interact(missile: Missile, trans: Transaction) {
        if (missile == saucer.currentMissile) saucer.missileReady = false
        checkCollision(missile, trans)
    }
    override fun interact(ship: Ship, trans: Transaction) {
        saucer.sawShip = true
        saucer.shipFuturePosition = ship.position + ship.velocity * 1.5
        checkCollision(ship, trans)
    }

There are three things going on here:

  1. If the saucer’s current missile is still in flight, saucer wants to know, because it only fires one missile at a time;
  2. Saucer has two concerns about the ship:
    • it only fires missiles when the ship is present
    • it always predicts the ship’s position, in case it wants to fire at it.

None of these have much to do with colliding: they are informational things that the saucer wants to know. Let’s send messages back to saucer for each of these:

    override fun interact(missile: Missile, trans: Transaction) {
        saucer.inspectMissile(missile)
        checkCollision(missile, trans)
    }

And in Saucer:

    fun inspectMissile(missile: Missile) {
        if (missile == currentMissile) missileReady = false
    }

Test. Green. Commit: defer missile checking back to saucer from strategy.

Now the other:

class SaucerCollisionStrategy
    override fun interact(ship: Ship, trans: Transaction) {
        saucer.inspectShip(ship)
        checkCollision(ship, trans)
    }

class Saucer
    fun inspectShip(ship: Ship) {
        sawShip = true
        shipFuturePosition = ship.position + ship.velocity * 1.5
    }

Should run green. Yes. Commit: defer ship inspection back to saucer from strategy.

Is there more feature envy to consider? We have this:

class MissileCollisionStrategy
    private fun checkAndScoreCollision(other: Collidable, trans: Transaction, score: Int) {
        Collision(other).executeOnHit(missile) {
            terminateMissile(trans)
            if (missile.missileIsFromShip) trans.addScore(score)
        }
    }

    private fun terminateMissile(trans: Transaction) {
        missile.timeOut.cancel(trans)
        trans.remove(missile)
    }

Well, in for a penny. Let’s do this:

class MissileCollisionStrategy
    private fun checkAndScoreCollision(other: Collidable, trans: Transaction, score: Int) {
        Collision(other).executeOnHit(missile) {
            missile.score(trans, score)
            terminateMissile(trans)
        }
    }

class Missile
    fun score(trans: Transaction, score: Int) {
        if (missileIsFromShip) trans.addScore(score)
    }

I considered lots of odd names for the method, like scoreIfShipMissile, and decided on score. The code seems clear.

And then

class MissileCollisionStrategy
    private fun terminateMissile(trans: Transaction) {
        missile.prepareToDie(trans)
        trans.remove(missile)
    }

class Missile
    fun prepareToDie(trans: Transaction) {
        timeOut.cancel(trans)
    }

Oops! I forgot to commit the first change. Bad Ron, no biscuit! Test. Green. Commit: remove feature envy from strategy.

I think we’ve done some creditable cleanup and we’ll sum up for now:

Summary

I suggest that we have improved the Strategy objects. They do seem quite similar, but each one is a bit different from the others. There could be a way to consolidate that similarity, but at this writing, I do not see how to do that in a way that seems better than what we have. Perhaps we’ll have an idea later. Maybe I can even get the Zoom crew to look at advise.

Careful readers will realize that in every case of Feature Envy here, I3 moved that code over to the Strategy only a day or two ago, creating the Envy that we removed today by moving it back. Should I feel badly about that? Let me assure you that I do not feel badly, and I do not think that I “should” feel badly, nor do I think you should feel badly when something like this happens.

Think about how we arrange our kitchen cabinets. Early on, we figure out where things probably belong, but as we work with things, we move them to better places. The glasses we use most often move to the front of the shelf. The spices we only use rarely move to the high shelf or toward the back. Our favorite knives go into the knife block and the ones we don’t prefer move to the rack behind.

We adjust things and we don’t always get them right the first time. Moving all the code over to the Strategy was easier than carefully dissecting it to get it just right. Either we’d be back looking at that code soon, in which case we’d observe things about it and adjust them, because we’re always refactoring … or we wouldn’t look at that code, perhaps for a long time, in which case, the Feature Envy wasn’t causing a problem.

I don’t strive for perfect. I’ve never been perfect and expect that here in my ninth decade I never will be. I strive for better, and I commend that notion to you as well.

The code is better now. Is it perfect? Quite unlikely.

Law of Demeter? Inappropriate Intimacy?

However, I’d be remiss not to mention a kind of probable mistake that I often make in this program. It seems that quite often I refer directly to a member variable of another object, as with the phrase missile.missileIsFromShip that occurred earlier today. That’s not a good habit to get into. And I’m not talking about just wrapping things like that with accessors.

When we start looking into the innards of an object, it is quite often a sign that the object isn’t quite serving our needs, or that responsibilities and knowledge aren’t divided up as well as they might be.

I feel that I do that too often in this program. Now most of my offenses are in tests, and I honestly think that tests are allowed to check the internals of an object. That’s quite often what they care about. But when other application objects are doing it, it’s suspicious and I wish I were more sensitive to it.

I’m not sure how to become more sensitive to something like this. One possibility is to make occasional sweeps to see why all the members of an object aren’t private. You’d think that they should4 be, in most cases.

Even with this concern, today’s code is better than yesterday’s, and there is an even better distribution of responsibilities than we had yesterday, and certainly better than a week ago.

Better. My favorite thing.

See you next time!


  1. This is almost impossible to do, but I’ve done it, in desperation, when the saucer’s aimed missile was about to get me. ↩

  2. OK, it’s not a perfect name. I didn’t name it. That’s just what we call it. Let’s stick to the point here. ↩

  3. I say “we” when things are going well, and “I” when there’s a mistake involved. I don’t want you to think I blame you for errors. I don’t. I blame Chet. It’s his job. ↩

  4. In Smalltalk, my favorite OO language, all member variables are private. You’d think I’d be good at that by now. Bad habits are easy to adopt, I guess. ↩