Kotlin 77 - Further Contrary
End Article Split
I continued directly from what’s below (which is the same code that the prior article ends with).
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
} else {
decrementNonSulfurasQuality(item)
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
private fun handleSellinDateLessThanZero(item: Item) {
if (item.name == "Sulfuras, Hand of Ragnaros") return
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = item.quality - item.quality
} else {
decrementQualityTowardZero(item)
}
}
}
private fun decrementNonSulfurasQuality(item: Item) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
decrementQualityTowardZero(item)
}
}
private fun decrementQualityTowardZero(item: Item) {
if (item.quality > 0) {
item.quality = item.quality - 1
}
}
private fun increaseQualityUpTo50(item: Item) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
Let’s inline the other call to decrementNonSulfurasQuality:
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
} else {
if (item.name != "Sulfuras, Hand of Ragnaros") {
decrementQualityTowardZero(item)
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
Remember that when I inverted the sense of that other if, I got a return, which I could move to the top. Let’s see if that happens again.
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
} else {
if (item.name == "Sulfuras, Hand of Ragnaros") {
} else {
decrementQualityTowardZero(item)
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
Let me fold up the else if:
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
} else if (item.name == "Sulfuras, Hand of Ragnaros") {
} else {
decrementQualityTowardZero(item)
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
I’d like to see that as a when.
private fun updateItem(item: Item) {
when (item.name) {
"Aged Brie" -> {
increaseQualityUpTo50(item)
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
"Sulfuras, Hand of Ragnaros" -> {
}
else -> {
decrementQualityTowardZero(item)
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
We can of course rearrange the order of the when:
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
else -> {
decrementQualityTowardZero(item)
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
Well …
It’s now 0830 on Thursday. I started this at 1500 Wednesday, and was interrupted for home events and an early supper … and didn’t come back until now. If I had any useful thoughts before I left, I didn’t get a chance to write them down. So we’ll be looking at the code above with fresh eyes.
That’s the main update method. Let’s continue with our small-scale thinking. That single line decrementing sell-in can be put inside each of the when-clauses, except for the Sulfuras one. I don’t think IDEA can automate that one for me. I’ll just do it:
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
item.sellIn = item.sellIn - 1
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
item.sellIn = item.sellIn - 1
}
else -> {
decrementQualityTowardZero(item)
item.sellIn = item.sellIn - 1
}
}
handleSellinDateLessThanZero(item)
}
(There is another way to do what I’m doing here, but this one, I believe, requires less care in thinking. I find value in making my thinking easy, especially first thing in the morning, before I’ve had my banana and chai.)
Now let’s look at that final method, handleSellInDateLessThan Zero:
private fun handleSellinDateLessThanZero(item: Item) {
if (item.name == "Sulfuras, Hand of Ragnaros") return
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = item.quality - item.quality
} else {
decrementQualityTowardZero(item)
}
}
}
That method guards itself against processing Sulfuras. We could move that return from this method up into the top level method. We have just discovered what we could have reasoned to: the code completely ignores Sulfuras, never modifying it in any way. It doesn’t adjust its quality or its sell in, ever. Fascinating!
It would certainly have been possible to trace through the code and work that out. Instead, by slowly improving it, we make it more clear what the code does. Or, in this case, doesn’t do. So we can do this, moving a return into the upper method and removing it from here. This may not be the best move I could make at this point, but it is, to me, the most obvious.
No, there’s a better move. Lets undo that last change, moving the sellIn code up.
Let’s look again at the situation:
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
else -> {
decrementQualityTowardZero(item)
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
private fun handleSellinDateLessThanZero(item: Item) {
if (item.name == "Sulfuras, Hand of Ragnaros") return
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = item.quality - item.quality
} else {
decrementQualityTowardZero(item)
}
}
}
The if on the sellIn update protects it from Sulfuras and nothing else. So we can move that code inside the handle... method, after the guard clause, removing the if. IDEA will remind us to do that if we don’t remember. I like this IDE …
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
else -> {
decrementQualityTowardZero(item)
}
}
handleSellinDateLessThanZero(item)
}
private fun handleSellinDateLessThanZero(item: Item) {
if (item.name == "Sulfuras, Hand of Ragnaros") return
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = item.quality - item.quality
} else {
decrementQualityTowardZero(item)
}
}
}
Test. Green. Commit: move sellIn reduction into handleSellin ...
IDEA has been hassling me frequently about that line
item.quality = item.quality - item.quality
I agree, finally.
private fun handleSellinDateLessThanZero(item: Item) {
if (item.name == "Sulfuras, Hand of Ragnaros") return
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = 0
} else {
decrementQualityTowardZero(item)
}
}
}
Test. Green. Commit: Backstage passes go to zero after event.
Why didn’t I change that earlier? I have the bad habit of checking the warnings during commit, and choosing Commit Anyway if they aren’t serious. (They never are: they’re generally interesting but not harmful.) I’ll see about changing my habits. I’m getting more adept with Kotlin and IDEA, so the warnings aren’t so distracting.
- Interesting Move
- Discussed further at the end, this increase in duplication results in a reduction of complexity.
Now let’s do something weird, in two steps, unless I get a better idea.
First, we can move the handleSellinDateLessThanZero call inside each of the when clauses. No point putting it into Sulfuras, so I won’t.
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
handleSellinDateLessThanZero(item)
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
handleSellinDateLessThanZero(item)
}
else -> {
decrementQualityTowardZero(item)
handleSellinDateLessThanZero(item)
}
}
}
Now, one by one, let’s inline it and take out what we don’t need:
"Aged Brie" -> {
increaseQualityUpTo50(item)
handleSellinDateLessThanZero(item)
}
Ah. IDEA can’t deal with extra returns in an inline. Good point, it might not work. Remove the guard clause, then inline.
Here’s the Brie:
"Aged Brie" -> {
increaseQualityUpTo50(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
item.quality = 0
} else {
decrementQualityTowardZero(item)
}
}
}
Now I just remove everything that isn’t Brie:
"Aged Brie" -> {
increaseQualityUpTo50(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
increaseQualityUpTo50(item)
}
}
Test. (Should have tested after the inlining, but I trust IDEA.) Green. Let’s continue and commit after all of them:
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> {
}
"Aged Brie" -> {
increaseQualityUpTo50(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
increaseQualityUpTo50(item)
}
}
"Backstage passes to a TAFKAL80ETC concert" -> {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
item.quality = 0
}
}
else -> {
decrementQualityTowardZero(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
decrementQualityTowardZero(item)
}
}
}
}
Test. Green. Commit: Everything done inside the top level when.
Now, with your kind permission, and if there are no objections, I plan to extract the contents of each when to a method with an appropriate name.
private fun updateItem(item: Item) {
when (item.name) {
"Sulfuras, Hand of Ragnaros" -> { /*nothing*/ }
"Aged Brie" -> { updateBrie(item) }
"Backstage passes to a TAFKAL80ETC concert" -> { updatePasses(item) }
else -> { updateNormal(item) }
}
}
We’ll look at the details in a moment, though they are surely obvious to us all. First, I want to borrow a trick from GeePaw, because I don’t like the look of the method above. I’m going to extract named constants.
private const val PROD_SULF = "Sulfuras, Hand of Ragnaros"
private const val PROD_BRIE = "Aged Brie"
private const val PROD_PASS = "Backstage passes to a TAFKAL80ETC concert"
private fun updateItem(item: Item) {
when (item.name) {
PROD_SULF -> { /*nothing*/ }
PROD_BRIE -> { updateBrie(item) }
PROD_PASS -> { updatePasses(item) }
else -> { updateNormal(item) }
}
}
Much nicer. Test. Green. Commit: Extract update methods by product.
Here are the detail methods:
private fun updateBrie(item: Item) {
increaseQualityUpTo50(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
increaseQualityUpTo50(item)
}
}
private fun updatePasses(item: Item) {
increaseQualityUpTo50(item)
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
item.quality = 0
}
}
private fun updateNormal(item: Item) {
decrementQualityTowardZero(item)
item.sellIn = item.sellIn - 1
if (item.sellIn < 0) {
decrementQualityTowardZero(item)
}
}
private fun decrementQualityTowardZero(item: Item) {
if (item.quality > 0) {
item.quality = item.quality - 1
}
}
private fun increaseQualityUpTo50(item: Item) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
Summary …
Let’s sum up the whole process, article #76 and this one, 77. Throughout these many steps, I just made simple local changes to make the code “better” in various ways. Mostly moving things together that belonged together, extracting chunks of code, especially duplicated ones, and giving them reasonable names. Generally reducing duplication.
But not always …
There were at least two cases where I increased duplication, moving the same pice of code into several branches of a when or the like. I did this in anticipation that inside, the code could be simpified. The Interesting Move above is the best example of this. This move is a bit less local than most of them. Looking at the two methods, the one with a near perfect breakout by product, and the otehr also breaking out by product, told me that if I put the latter into the former, I could, in a couple of steps, put just the relevant lines from the latter into the former.
Inverting ‘if’
I found that refactoring quite useful, since whoever wrote this was all about not this and that. Cursor on the if, Alt+Enter, Invert if condition.
Other paths? Certainly.
Could we have done that by judiciously cutting and pasting from the second method into the first? Yes, but that seemed to me to require more thinking. Just moving the whole thing into each context, and then, with all the relevant code there in front of my eyes, made the transformations easier and safer. YMMV. It seemed right to me, and still does.
Is it perfect? No way.
I don’t think this code is as clean and tight as it could be. We may return to it to see what else we can do. What I would ksay, however, is that the code now has a clean breakout by product, with all the code pertaining to each product right there in a single block. All that weird nested if stuff is gone.
And yet, it has gone nicely.
And, frankly I am impressed, not with myself, but with the process: small generally obvious steps toward improvement have led us to a rather clean, though still imperfect result.
I thought it entirely possible that I’d get part way through this and crash and burn, with a worse mess than I started with. That didn’t happen. I did lots of commits, and could have (and therefore “should” have) done probaby twice as many.
But testing?
Now … testing. I just relied on my single characterization test, and confidence in my mostly IDEA-driven refactoring. I declare very strongly that this is not sufficient. I have no reason to believe that the sample data includes all the interesting cases, and I know for a fact that a data entry error on Sulfuras wculd result in its entry being wrong—because we never touch it at all!
So before I pushed this to production, I would want to create at least a couple of microtests for each product, exercising it a bit. I’m about 80% sure that this code does exactly what the old code does, but 80 isn’t good enough.
On the other hand, the code is now simple enough that we could read it and determine mentally whether it follows the spec, and be pretty darn sure we were right. Tests would be better, in my view, and would serve as a platform for change.
We do have a feature we’re supposed to add, for Conjured items. We’ll use that as an opportunity for testing.
- Overall
- I am pleased and a bit surprised at how well this went. It seemed to me that trying to sort out that mess of code was a bad idea, and that “strangler” was a better strategy. And, in fact, it probably would be a better strategy … but this one wasn’t fatally bad.
-
It’s always good when we get out alive.
See you next time!