Kotlin 144
Kent Beck has me thinking about cohesion and coupling. Let’s look at the ShipMaker with that sensor turned on.
Kent is working on a series of books, and has recently been writing about coupling and cohesion, from a different angle than you may have heard before. I’ll not try to explain his ideas: it would likely be a favor to none of us. But I do have notions of my own about the coupling-cohesion axis and we can use those notions.
Here’s the ShipMaker as I left it yesterday
class ShipMaker(val ship: SolidObject) : ISpaceObject {
override var elapsedTime: Double = 0.0
var safeToEmerge = true
var asteroidTally = 0
override fun beginInteraction() {
safeToEmerge = true
asteroidTally = 0
}
override fun interactWith(other: ISpaceObject): List<ISpaceObject> {
if (other is SolidObject && other.isAsteroid) {
asteroidTally += 1
}
if (tooClose(other)) safeToEmerge = false
return emptyList()
}
override fun interactWithOther(other: ISpaceObject): List<ISpaceObject> = interactWith(other)
private fun tooClose(other:ISpaceObject): Boolean {
return if (other !is SolidObject) false
else (ship.position.distanceTo(other.position) < U.SAFE_SHIP_DISTANCE)
}
override fun finishInteraction(): Transaction {
return if (elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip()
} else {
Transaction()
}
}
override fun update(deltaTime: Double): List<ISpaceObject> {
elapsedTime += deltaTime
return emptyList()
}
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(possibleHyperspaceFailure())
}
}
private fun possibleHyperspaceFailure(): Transaction {
return if (hyperspaceFails()) destroyTheShip()
else Transaction()
}
private fun destroyTheShip(): Transaction {
return Transaction().also {
it.add(SolidObject.shipDestroyer(ship))
it.add(SolidObject.splat(ship))
}
}
private fun inHyperspace() = ship.position != U.CENTER_OF_UNIVERSE
private fun hyperspaceFails(): Boolean
= inHyperspace() && hyperspaceFailure(Random.nextInt(0, 63), asteroidTally)
// allegedly the original arcade rule
fun hyperspaceFailure(random0thru62: Int, asteroidCount: Int): Boolean
= random0thru62 >= (asteroidCount + 44)
}
Putting it vaguely, things are “cohesive” if they seem to belong together. While I’d freely grant that that’s a pretty fuzzy notion, as developers gain experience, they tend to come to a shared notion of what belongs together. Things that belong together are generally “coupled” through having something in common. Perhaps they need to change together. Perhaps they rely on the same information or happen at the same time … or must happen in a particular time order. Again, a vague fuzzy feeling kind of idea. I’m OK with that.
In our ShipMaker code, there are some separable notions that we could call out:
-
The
beginInteraction,interactWith,interactWithOtherandfinishInteractionfunctions work together to produceasteroidTallyandsafetoEmerge. They usetooCloseto make their determination, and they callreplaceTheShipwhen it’s safe and the necessary time has elapsed. -
The
updatemethod just updates elapsed time, so it is somewhat coupled to the functions above. -
The
replaceTheShipfunction produces a single Transaction that contains everything needed to replace the ship, including a possible hyperspace accident. It also produces a transaction element that will delete the ShipMaker itself and an element that creates a new ShipChecker.replaceTheShipusespossibleHyperspaceFailure,hyperspaceFails,hyperspaceFailure,destroyTheShip, andinHyperspace. -
The hyperspace logic is the bulk of what
replaceTheShipdoes, but the replace really does at least two things, the removal of the maker plus addition of ship and checker, and, separately, the possible placement of the hyperspace accident objects.
An added complication is this: ideally, we wouldn’t execute the interaction methods at all unless the elapsed time had expired. They are harmless before then, but they waste time and produce answers that no one wants.
We can sort the code above, as I have already tried to do, to show the related functions close together, but that 1,2,3 breakout is not obvious. That suggests some conclusions:
- The ShipMaker is probably harder to understand than it needs to be, because a reader has to workout that there are these nearly independent sections.
- When we change ShipMaker, it will likely take longer than it needs to, and we are more likely to make mistakes than if it were somehow simpler.
Now in the grand scheme of things, this is not a big problem and probably not worth even the time we’ve spent on it already. This is a toy game of no consequence. But as an example of a problem, it may be worth examining what we could do to make it easier to understand.
Once we’ve sorted the methods in some order, and written as many tiny functions as is reasonable, we’ve pretty much done everything that can be done in one object. Further improvements probably require more objects. Let’s try some options.
Planning
My “normal” approach here would be to extract one or more new classes from this code. It seems likely that we could have a hyperspace handler of some kind, that returns a possibly empty transaction (as the method does now). It seems likely that we could have the basic transaction built by a separate object as well.
- Aside
- This
objectidea does not work out. We do extract a new class. You can skim down to “HERE”.
Now Kotlin has an interesting capability, the object. Rather than create a public class, you can create an object, which is a single-use kind of thing, just an instance if you will. I don’t know whether this capability is useful, when it is useful, or whether it’ll be useful here. But I know how to find out: we can try it.
- My Guess
- My guess is that about all the
objectfeature will accomplish is to avoid polluting the program’s name space with lots of tiny special purpose classes. We’ll see, but the best way to see is to see it in action. So let’s.
Let’s have an object that makes the hyperspace part of the ship making activity. This will either be an empty transaction, if the ship makes it, or a small transaction containing a destroyer and a splat.
It turns out that an object declared in a scope can see the variables in its scope, which means that our object could see the asteroidTally if we wish.
Let’s see first how we might use the thing. Here:
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(possibleHyperspaceFailure())
}
}
We might say something like this:
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(HyperspaceOperation().execute())
}
}
We will probably add parameters to that call before we’re done. And if the HyperspaceOperation was a transaction, we could avoid the call to execute. We’ll think about that.
- HERE
- My attempt with
objectisn’t good enough even to show you. It’s like the class below, only messier. Another author would erase the mistake and start here. I am not another author.
I try the object and find that an object can’t have a constructor, which means that it has to pass variables all around. Perhaps there’s more that I don’t understand but I do this, extracting a class:
class HyperspaceOperation(val ship:SolidObject, val asteroidTally: Int) {
fun execute(): Transaction {
return if (hyperspaceFails()) {
destroyTheShip()
} else {
Transaction()
}
}
private fun destroyTheShip(): Transaction {
return Transaction().also {
it.add(SolidObject.shipDestroyer(ship))
it.add(SolidObject.splat(ship))
}
}
private fun inHyperspace()
= ship.position != U.CENTER_OF_UNIVERSE
private fun hyperspaceFails(): Boolean {
return inHyperspace() && hyperspaceFailure(Random.nextInt(0, 63), asteroidTally)
}
// allegedly the original arcade rule
private fun hyperspaceFailure(random0thru62: Int, asteroidTally: Int): Boolean {
return random0thru62 >= (asteroidTally + 44)
}
}
}
And use it:
class ShipMaker ...
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(HyperspaceOperation(ship,asteroidTally).execute())
}
}
That moves five method functions right out of ShipMaker. Now it looks like this:
class ShipMaker(val ship: SolidObject) : ISpaceObject {
override var elapsedTime: Double = 0.0
var safeToEmerge = true
var asteroidTally = 0
override fun update(deltaTime: Double): List<ISpaceObject> {
elapsedTime += deltaTime
return emptyList()
}
override fun beginInteraction() {
safeToEmerge = true
asteroidTally = 0
}
override fun interactWith(other: ISpaceObject): List<ISpaceObject> {
if (other is SolidObject && other.isAsteroid)
asteroidTally += 1
if (tooClose(other))
safeToEmerge = false
return emptyList()
}
override fun interactWithOther(other: ISpaceObject): List<ISpaceObject>
= interactWith(other)
private fun tooClose(other:ISpaceObject): Boolean {
return if (other !is SolidObject) false
else (ship.position.distanceTo(other.position) < U.SAFE_SHIP_DISTANCE)
}
override fun finishInteraction(): Transaction {
return if (elapsedTime > U.MAKER_DELAY && safeToEmerge) {
replaceTheShip()
} else {
Transaction()
}
}
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(HyperspaceOperation(ship,asteroidTally).execute())
}
}
}
We have a broken test, however. It needs to use the new thing.
@Test
fun `hyperspace failure checks`() {
val ship = SolidObject.ship(Point(10.0, 10.0))
val maker = ShipMaker(ship)
assertThat(maker.hyperspaceFailure(62, 19)).describedAs("roll 62 19 asteroids").isEqualTo(false)
assertThat(maker.hyperspaceFailure(62, 18)).describedAs("roll 62 18 asteroids").isEqualTo(true)
assertThat(maker.hyperspaceFailure(45, 0)).describedAs("roll 45 0 asteroids").isEqualTo(true)
assertThat(maker.hyperspaceFailure(44, 0)).describedAs("roll 44 0 asteroids").isEqualTo(true)
assertThat(maker.hyperspaceFailure(43, 0)).describedAs("roll 43 0 asteroids").isEqualTo(false)
}
I reckon those convert right into executing the same method in the new object. I’ll probably have to make it public.
@Test
fun `hyperspace failure checks`() {
val ship = SolidObject.ship(Point(10.0, 10.0))
val hyper = HyperspaceOperation(ship, 0)
assertThat(hyper.hyperspaceFailure(62, 19)).describedAs("roll 62 19 asteroids").isEqualTo(false)
assertThat(hyper.hyperspaceFailure(62, 18)).describedAs("roll 62 18 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(45, 0)).describedAs("roll 45 0 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(44, 0)).describedAs("roll 44 0 asteroids").isEqualTo(true)
assertThat(hyper.hyperspaceFailure(43, 0)).describedAs("roll 43 0 asteroids").isEqualTo(false)
}
And the following test is intermittent:
@Test
fun `makes with ship features unchanged`() {
val position = Point(123.0,456.0)
val velocity = Velocity(200.0,300.0)
val heading = 37.5
val ship = SolidObject.ship(position)
ship.heading = heading
ship.velocity = velocity
val maker = ShipMaker(ship)
maker.update(U.MAKER_DELAY + 0.01)
maker.beginInteraction()
// no obstacles
val trans = maker.finishInteraction()
assertThat(trans.adds.size).isEqualTo(2)
assertThat(trans.adds).contains(ship)
assertThat(ship.position).isEqualTo(position)
assertThat(ship.velocity).isEqualTo(velocity)
assertThat(ship.heading).isEqualTo(heading)
}
This will be due to rolling the dice. It has probably been intermittent for ages. Let’s bump up asteroidTally.
@Test
fun `makes with ship features unchanged`() {
val position = Point(123.0,456.0)
val velocity = Velocity(200.0,300.0)
val heading = 37.5
val ship = SolidObject.ship(position)
ship.heading = heading
ship.velocity = velocity
val maker = ShipMaker(ship)
maker.update(U.MAKER_DELAY + 0.01)
maker.beginInteraction()
// no obstacles
maker.asteroidTally = 60 // no possible hyperspace failure
val trans = maker.finishInteraction()
assertThat(trans.adds.size).isEqualTo(2)
assertThat(trans.adds).contains(ship)
assertThat(ship.position).isEqualTo(position)
assertThat(ship.velocity).isEqualTo(velocity)
assertThat(ship.heading).isEqualTo(heading)
}
Stays green. Let’s commit: Created new HyperspaceOperation class to deal with possible hyperspace failure.
I am slightly concerned about this:
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(HyperspaceOperation(ship,asteroidTally).execute())
}
}
It’d be nice if we didn’t need that call to execute, and nicer still if we didn’t have to know that class name. How about this:
private fun replaceTheShip(): Transaction {
return Transaction().also {
it.add(ship)
it.add(ShipChecker(ship))
it.remove(this)
it.accumulate(Transaction.hyperspaceEmergence(ship,asteroidTally))
}
}
And over in Transaction:
companion object {
fun hyperspaceEmergence(ship: SolidObject, asteroidTally: Int) :Transaction {
return HyperspaceOperation(ship,asteroidTally).execute()
}
I think I rather like that. If you are troubled by it, do tweet me up or mastodon me if you’re that kind of person, though I am not yet adept with mastodon, so might miss your message. I’m open to better ideas, so if you got ‘em, smoke ‘em over here.
Let’s sum up, the morning is wearing on.
Summary
By creating a sort of helper object, HyperspaceOperation, we’ve managed to remove five methods from ShipMaker, removing all its concerns about how to manage hyperspace. We have reduced the coupling in that code, and increased its cohesiveness.
We then created a companion object in Transaction. That companion is now coupled to our helper, but our ShipMaker is not. It seems a reasonable trade-off to me, in that it smooths the syntax a bit in ShipMaker.
I am a bit concerned about the added coupling between Transaction and the HyperspaceOperation object, in aid of giving Transaction the helper method hyperspaceEmergence, but I like the syntax that has resulted.
Overall, I think this is an improvement. What else have I “learned”? Well …
It seems this could be a common pattern:
- beginInteracton sets up some values
- interactWith updates them
- finishInteraction uses the values to create or destroy something
Often, it seems that it may also be common to use update to tick off elapsed time and to use that time in the above pattern.
As this becomes a pattern, if in fact it does, and that day may never come, it would be “better” if it could b abstracted into some kind of common scheme for doing what amounts to the same kind of thing again and again.
And … it starts to seem to me that things might be simpler with a separate tick function that is supported for all ISpaceObjects, whether they use it or not, because we could have a default tick and a default update, simplifying objects like ShipMaker.
We might try it just to see how it looks. For now, just an idea.
I do want to hammer home a notion, the notion of mindful practice.
Hammering Home
There is a possibly apocryphal story told of a pottery class where the professor told half the class that their grade would depend on one pot to be submitted at the end of the term, and told the other half that their grade would depend only on the total number of pots they made.
According to the story, the final pots made by the mass-pottery half of the class were generally better than the ones from the single-pot half. The difference is attributed to the effect of all the practice that the mass potters got, while the single pot folks just worked on very few pots, perhaps only one.
I don’t know if it works for pots, but I’m pretty sure that this practice effect works on code. I’m pretty sure that trying many ideas, slightly different ways of doing things, will improve our general skill at programming.
Whether you are “given “ time to practice or whether you have to steal it from your small pool of “free” time, it’s possible that practicing things like we do here would be of value to you.
I’m not the boss of you. I barely do what I tell myself to do. But it’s an idea …