Kotlin 218 - Better Idea
GitHub Decentralized Repo
GitHub Centralized Repo
I think that code in Missile is a bit too clever. I may have a better idea. Let’s find out. Also: remarks on Jira.
This code is clever, I think too clever:
fun interact(asteroid: Asteroid, trans: Transaction) {
checkCollisionAndDoIfColliding(asteroid, trans) {
if (missileIsFromShip) trans.addScore(asteroid.getScore())
}
}
fun interact(saucer: Saucer, trans: Transaction) {
checkCollisionAndDoIfColliding(saucer, trans) {
if (missileIsFromShip) trans.addScore(saucer.getScore())
}
}
fun interact(ship: Ship, trans: Transaction) {
checkCollisionAndDoIfColliding(ship, trans) {}
}
fun interact(missile: Missile, trans: Transaction) {
checkCollisionAndDoIfColliding(missile, trans) {}
}
private fun checkCollisionAndDoIfColliding(
other: Collider,
trans: Transaction,
action: () -> Unit){
if ( checkCollision(other)) {
terminateMissile(trans)
action()
}
}
private fun terminateMissile(trans: Transaction) {
timeOut.cancel(trans)
trans.remove(this)
}
private fun checkCollision(other: Collider) = Collision(other).hit(this)
All we’re really trying to do is to add the score to the transaction, if the missile is from the ship as indicated by the missileIsFromShip boolean. Yesterday, the best idea I had was to pass in a block of code that would either do nothing, or add the score. This morning, in a brief moment of clarity, it occurred to me to pass in the score, possibly zero. Let’s do that.
I’ll change the name of the clever method, and then its signature, which will require all the callers to change. Before I change the callers, I’ll make the method itself correct. It’ll go like this:
First the name:
private fun checkAndScoreCollision(
other: Collider,
trans: Transaction,
action: () -> Unit){
if ( checkCollision(other)) {
terminateMissile(trans)
action()
}
}
Then the signature:
private fun checkAndScoreCollision(
other: Collider,
trans: Transaction,
score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
action()
}
}
Then fix the code:
private fun checkAndScoreCollision(
other: Collider,
trans: Transaction,
score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
if ( missileIsFromShip ) trans.addScore(score)
}
}
Then fix up all the callers:
fun interact(asteroid: Asteroid, trans: Transaction) {
checkAndScoreCollision(asteroid, trans, asteroid.getScore())
}
fun interact(saucer: Saucer, trans: Transaction) {
checkAndScoreCollision(saucer, trans,saucer.getScore())
}
fun interact(ship: Ship, trans: Transaction) {
checkAndScoreCollision(ship, trans, 0)
}
fun interact(missile: Missile, trans: Transaction) {
checkAndScoreCollision(missile, trans, 0)
}
That should do the job. Test. Green, and I also tested scoring in the game. We’re good. Commit: refactor missile interaction and scoring for more clarity and less cleverness.
Let’s reflect.
Reflection
Clearly this solution is simpler than yesterday’s idea of executing a block. I can’t help wondering why it didn’t come to mind. Whatever the reason, it didn’t, and as I look back at history, I remember plenty of times that I’ve written, or encountered, code that was not as simple as it could be. Sometimes it was just overly generalized with some kind of eye to the future, but other times, like today, it was just not as simple as it could be. I’m really quite sure that I didn’t do the block yesterday “because someday we might want to do something really weird”. I just didn’t think to pass in the score and apply it inside.
Whatever the cause, in this case I don’t have a deep lesson to discover, but I think I would offer a shallow lesson: executing a block is fairly deep in the Kotlin bag of tricks, and executing it optionally is deeper still, and providing an empty block so that if it’s executed it doesn’t do anything anyway is even deeper.
So, let’s be sensitive to times when we do things that are deep in our bag of tricks, and look a bit harder for simpler solutions.
That’s the only change I actually had in mind for this morning. We’ll look for something that needs doing, and that brings me to Jira.
Jira
As constant readers know, I keep little yellow sticky notes of things that I might want to do on my keyboard tray, whose name is Jira:
As you can see, there are a lot of them. About twenty. One of them is crossed off as done:remove targets & attacker. Another, now that I look at them, should be crossed off. Interaction object. Some of them are just notes. Some are just random ideas, like “allow window size not equal to 1024”. Some are redundant or related: “More interaction tests” and “missile vs missile test etc”.
My Jira, of only about twenty items, is disorganized, contains things we’ll never do, contains things that are done and not marked done. It includes random notes regarding things already done: “4,5,8,10,11 / round”. Some of the items have no meaning, even to me, and I wrote them: “Speed 6..31 appears random not faster”. I think that may refer to giving asteroids random speeds. If so, it’s probably a good idea.
My Jira, of only about twenty items, doesn’t communicate to its owner. Imagine what would happen if some other developer on my team had to figure out what to do from my Jira. You know they’d show up at my house with a yellow sticky note in their hand, saying “What does this mean??” And, odds are, I’d say “Well, to tell you the truth, in all the excitement, I kind of lost track myself.1”
Maybe we’d figure it out, maybe we’d throw it away. Either way we would have wasted time.
I knew a team who had 30,000 items in Jira. Thirty thousand! And you know what they were doing? They were imposing more and more stringent rules on themselves, ensuring that everything that went into Jira was fully explained with lots of text, so that anyone who happened to be reading thirty thousand items could understand each one. Naturally, all defects had to be logged into Jira, and so it often took longer to make the Jira entry for the bug, and update it afterwards, than it did to actually find and fix the bug.
Madness, do you hear me, madness!
What should we do? Should we not keep notes? Of course we should keep notes. A nice little box of 3x5 cards will work. A keyboard tray of yellow sticky notes can work. But even those simple approaches get messy.
I know a team who made a very small square on one of their whiteboards, and whenever they found some code that needed improvement, if they didn’t have time to do it just then, they made a card and put it in the square. When the square got too full for another card — and usually before that — a pair would pick a card out of the square, or a few cards, and do the necessary refactoring.
Everything that has been entered into Jira is in a work queue whether we think of it that way or not. It is “work in process”, and it is waiting, often forever, to be done.
What should we do? I don’t do “should”. What I’m going to do here on my keyboard tray is two things:
First, I’m going to make a separate section for notes about the current thing I’m doing, made just so that I don’t forget them, but that I have to do in order to be done. Those aren’t the same as the longer-term possibly optional things in my Jira section. Keeping them separate will help ensure that they get done as part of my current task, and will keep Jira a bit less messy.
Second, I’m going to limit the area used for my Jira, and when it gets full, something has to come out before other things can go in. Things can come out in at least two ways: I can do them, or I can decide not to do them and throw them away.
But, you ask, what if you throw something away and it turns out to be important? Well, I answer, if it’s important, it will come back on its own, or someone will drag it back in.
Here’s one: “OneShotCollection?” What does that mean? It means that at some point, I was thinking of having a collection object for OneShot instances, because there’s at least one object that maintains two or three OneShot instances. I was thinking that I could just have the collection somehow help with the necessary handling. I’m going to toss that one: if and when we get back to whatever object deals with multiple OneShot instances, either it’ll come to mind, or it won’t, and we’ll do something else reasonable.
What about code comments?
I could imagine putting a comment in the code, but I generally do not do that. In my experience, when a team develops that habit, they wind up with lots of comments and nothing ever fixed. I’ll toss it. If it’s important, it’ll come back.
Here’s another one: “Rename ShipCheckerAndMakerTest?” I look at that file. It contains two tests, both having to do with hyperspace. I rename it “HyperspaceTests”. Done. I don’t go back to Jira and make an entry and put my initials on it. I throw the sticky away. I also wonder why I didn’t do it instead of making the sticky. It was quicker to make the change. Commit: rename tests.
I’m going to toss the one about non-square windows. I don’t intend to implement non-square windows. I’m going to toss the one about window size other than 1024. If we need it, we’ll find out that we need it, and we’ll do it. I’m down to 11 items in my Jira, counting as singles, a group of 3, and two groups of 2 items.
What should you do? You probably have no choice. If you use Jira, the decision was likely made well above you. What I’d recommend, if I were to make a recommendation, would be that as a team and as individuals, you try to communicate person to person rather than through Jira or any written medium. The written word loses more than it communicates. Yes, I, who writes thousands of words, am telling you that the written word barely works at all.
Enough. If it were me, I’d avoid Jira as often and as much as I could. You? Do you.
Now What?
I’m a bit concerned about if statements and the like. In really good OO code, if statements are often an indication that there’s something better that could be done. I do have a Jira note about this one, that we just worked on:
private fun checkAndScoreCollision(other: Collider, trans: Transaction, score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
if ( missileIsFromShip ) trans.addScore(score)
}
}
How does that flag get set? It’s done when the missile is created:
class Missile
...
val missileIsFromShip: Boolean = false
): SpaceObject, Collider {
constructor(ship: Ship): this(ship.position, ship.heading, ship.killRadius, ship.velocity, ColorRGBa.WHITE, true)
constructor(saucer: Saucer): this(saucer.position, Random.nextDouble(360.0), saucer.killRadius, saucer.velocity, ColorRGBa.GREEN)
Is there something we could do here to avoid this if? Should we even care? Is there an object to which we could send a message that would either be processed or not? If there is, or could be, would it be more clear?
We could have the member variable be a block to be executed. That’s nearly as tricky as what we just removed. Let’s try it, just for fun.
Just for purposes of seeing whether I like it:
class Missile(
shooterPosition: Point,
shooterHeading: Double = 0.0,
shooterKillRadius: Double = U.SHIP_KILL_RADIUS,
shooterVelocity: Velocity = Velocity.ZERO,
val color: ColorRGBa = ColorRGBa.WHITE,
private val missileIsFromShip: Boolean = false
): SpaceObject, Collider {
constructor(ship: Ship): this(ship.position, ship.heading, ship.killRadius, ship.velocity, ColorRGBa.WHITE, true)
constructor(saucer: Saucer): this(saucer.position, Random.nextDouble(360.0), saucer.killRadius, saucer.velocity, ColorRGBa.GREEN)
val applyScore: (Transaction, Int) -> Unit = if (missileIsFromShip) {
{trans, score -> trans.addScore(score)}
} else {
{_, _, -> }
}
I’ve created a new val, applyScore, that is either an empty transaction o one that applies a score to a transaction.
And in the method of interest:
private fun checkAndScoreCollision(other: Collider, trans: Transaction, score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
applyScore(trans, score)
}
}
This will surely work. And it does. With that variable name, applyScore, we’ve obscured the fact that the addition of score is optional. We could make the creation of the thing simpler, but even so we lose the notion of the condition. We could have two classes of Missile, but to avoid an if? That’s just not on.
We’ll roll that back, and live with the if. Remove the note. Since we’re in the mood, though, let’s look around for other ifs and whens.
I do find a lot of them. Let’s just review a few of them, since they are a bit of a code smell.
class Asteroid
private fun checkCollision(other: Collider, trans: Transaction) {
if (Collision(other).hit(this)) {
dieDueToCollision(trans)
}
}
private fun splitIfPossible(trans: Transaction) {
if (splitCount >= 1) {
trans.add(asSplit(this))
trans.add(asSplit(this))
}
}
class DeferredAction
override fun update(deltaTime: Double, trans: Transaction) {
elapsedTime += deltaTime
if (elapsedTime >= delay && cond() ) {
action(trans)
trans.remove(this)
}
}
class Game
fun canShipEmerge(): Boolean {
if (knownObjects.saucerIsPresent()) return false
if (knownObjects.missiles.size > 0) return false
for ( asteroid in knownObjects.asteroids ) {
val distance = asteroid.position.distanceTo(U.CENTER_OF_UNIVERSE)
if ( distance < U.SAFE_SHIP_DISTANCE ) return false
}
return true
}
private fun createNewWaveIfNeeded() {
if ( U.AsteroidTally == 0 ) {
val trans = Transaction()
waveOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
}
private fun createSaucerIfNeeded() {
if ( knownObjects.saucerIsMissing() ) {
val trans = Transaction()
saucerOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
}
private fun createShipIfNeeded() {
if ( knownObjects.shipIsPresent() ) return
val trans = Transaction()
shipOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
private fun howMany(): Int {
return numberOfAsteroidsToCreate.also {
numberOfAsteroidsToCreate += 2
if (numberOfAsteroidsToCreate > 11) numberOfAsteroidsToCreate = 11
}
}
private val shipOneShot = OneShot(U.SHIP_MAKER_DELAY, { canShipEmerge() }) {
if ( scoreKeeper.takeShip() ) {
startShipAtHome(it)
}
}
class Interaction
private fun missilesVsMissileShipSaucerAsteroids() {
missiles.forEach { missile ->
missiles.forEach { other ->
if (other != missile ) {
missile.interact(other, trans)
other.interact(missile, trans)
}
}
...
There are a lot of them. I think that last one is actually wrong. We want to check for identity, not equality.
if (other !== missile ) {
Commit that: missile vs missile check changed to not identical rather than equal.
We could do this in howMany:
private fun howMany(): Int {
return numberOfAsteroidsToCreate.also {
numberOfAsteroidsToCreate = min(11, numberOfAsteroidsToCreate + 2)
}
}
That might be better. But is it really more clear this way? Honestly, I don’t think so. Revert that.
This game is no fun. But I do see an improvement to some of that code in Game:
private fun createShipIfNeeded() {
if ( knownObjects.shipIsPresent() ) return
val trans = Transaction()
shipOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
SpaceObjectCollection is happy to help with this:
private fun createShipIfNeeded() {
if ( knownObjects.shipIsPresent() ) return
knownObjects.performWithTransaction {
shipOneShot.execute(it) }
}
And a guard clause for a one-line method isn’t great. We’ll reverse the sense of the if.
private fun createShipIfNeeded() {
if ( knownObjects.shipIsMissing() ) {
knownObjects.performWithTransaction {
shipOneShot.execute(it) }
}
}
Test. Green. Game also works. Commit: refactor createShipIfNeeded.
Similarly:
private fun createNewWaveIfNeeded() {
if ( U.AsteroidTally == 0 ) {
val trans = Transaction()
waveOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
}
That becomes:
private fun createNewWaveIfNeeded() {
if ( U.AsteroidTally == 0 ) {
knownObjects.performWithTransaction {
waveOneShot.execute(it) }
}
}
Similarly:
private fun createSaucerIfNeeded() {
if ( knownObjects.saucerIsMissing() ) {
val trans = Transaction()
saucerOneShot.execute(trans)
knownObjects.applyChanges(trans)
}
}
That becomes:
private fun createSaucerIfNeeded() {
if ( knownObjects.saucerIsMissing() ) {
knownObjects.performWithTransaction {
saucerOneShot.execute(it) }
}
}
I wonder about U.AsteroidTally. When and how does that get set? Game sets it. It’s used in a test for hyperspace. I’ll let it live for now, but it is an evil global variable.
I notice, in browsing, the Collision object, which we use frequently.
class Asteroid
private fun checkCollision(
other: Collider,
trans: Transaction) {
if (Collision(other).hit(this)) {
dieDueToCollision(trans)
}
}
class Missile
private fun checkAndScoreCollision(
other: Collider,
trans: Transaction,
score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
if ( missileIsFromShip ) trans.addScore(score)
}
}
private fun checkCollision(other: Collider) = Collision(other).hit(this)
class Saucer
private fun checkCollision(
collider: Collider,
trans: Transaction) {
if (Collision(collider).hit(this)) {
trans.add(Splat(this))
trans.remove(this)
}
}
class Ship
private fun checkCollision(
other: Collider,
trans: Transaction) {
if (weAreCollidingWith(other)) {
collision(trans)
}
}
private fun weAreCollidingWith(other: Collider): Boolean
= Collision(other).hit(this)
fun collision(trans: Transaction) {
trans.add(Splat(this))
trans.remove(this)
}
I think we can simplify these. Here’s Collision:
class Collision(private val collider: Collider) {
fun hit(other: Collider): Boolean
= collider.position.distanceTo(other.position) < collider.killRadius + other.killRadius
}
Let’s extend Collision thus:
class Collision(private val collider: Collider) {
fun hit(other: Collider): Boolean
= collider.position.distanceTo(other.position) < collider.killRadius + other.killRadius
fun executeOnHit(other: Collider, action: () -> Unit) {
if (hit(other)) action()
}
}
Then we can do this:
class Asteroid
private fun checkCollision(other: Collider, trans: Transaction) {
Collision(this).executeOnHit(other) {
dieDueToCollision(trans)
}
}
I reversed the order of this and other because it better matches the names in Collision. Test this. Green. Commit: use Collision.executeIfHit.
In Saucer:
class Saucer
private fun checkCollision(collider: Collider, trans: Transaction) {
Collision(collider).executeOnHit(this) {
trans.add(Splat(this))
trans.remove(this)
}
}
Test. Green. Commit use Collision.executeIfHit.
Ship and Missile are a bit more involved. Here’s Missile:
class Missile
private fun checkAndScoreCollision(other: Collider, trans: Transaction, score: Int){
if ( checkCollision(other)) {
terminateMissile(trans)
if ( missileIsFromShip ) trans.addScore(score)
}
}
private fun checkCollision(other: Collider) = Collision(other).hit(this)
Let’s inline checkCollision. IDEA will do that for us. Now it looks like the others:
private fun checkAndScoreCollision(other: Collider, trans: Transaction, score: Int) {
if (Collision(other).hit(this)) {
terminateMissile(trans)
if (missileIsFromShip) trans.addScore(score)
}
}
And we transform it:
private fun checkAndScoreCollision(other: Collider, trans: Transaction, score: Int) {
Collision(other).executeOnHit(this) {
terminateMissile(trans)
if (missileIsFromShip) trans.addScore(score)
}
}
Test. Green. Commit: use Collision.executeIfHit.
Now there’s just Ship:
class Ship
private fun checkCollision(other: Collider, trans: Transaction) {
if (weAreCollidingWith(other)) {
collision(trans)
}
}
private fun weAreCollidingWith(other: Collider): Boolean = Collision(other).hit(this)
Again, inline:
private fun checkCollision(other: Collider, trans: Transaction) {
if (Collision(other).hit(this)) {
collision(trans)
}
}
Do our transformation:
private fun checkCollision(other: Collider, trans: Transaction) {
Collision(other).executeOnHit(this) {
collision(trans)
}
}
I am inclined to inline collision:
private fun checkCollision(other: Collider, trans: Transaction) {
Collision(other).executeOnHit(this) {
trans.add(Splat(this))
trans.remove(this)
}
}
Nice. All the collision stuff looks much the same in style. That will reduce confusion as we browse the classes. Test. Green. Commit: use Collision.executeIfHit.
I just noticed that that commit showed mods to GameTest, but I had clicked the button before I realized it. What did it change?
Mostly it just reformatted my code but it actually changed this test, inlining collision. That’s fine. No problem.
I think we’ve had enough fun for the day. Let’s sum up.
Summary
We began the morning with a specific plan, removing excess cleverness and adding simplicity. That went just fine.
We then discussed Jira and the fact that even on my keyboard tray, it was becoming unmanageable, and you can be sure that I don’t have thousands of sticky notes on my keyboard tray.
Then, we had a little time and didn’t have anything big to pick up, so, starting from some notes on my tray, we looked at some if statements. We improved one, and then, having observed a number of similar ones, we enhanced the Collision object so that it contains an if statement, which allowed us to remove four if statements elsewhere, and to generally simplify the code around them.
So that was a net win for if statements and code clarity.
I find this to be quite common: I start out looking for some specific kind of thing. I may or may not find it and I may or may not improve it. But along the way, I spot other areas in the code that could use a little improvement2 Changes like that are quick to make, and especially with decent tests, quite safe to make.
Instead of slowly deteriorating, our code slowly improves. That has great value, because it preserves our ability to deliver more value rapidly.
So we had a morning of “little improvements”. Nice.
See you next time!