Kotlin 76 - Being Contrary
One course seems obviously better. I shall choose the other.
There are really just a few options for updates in the spec, a mere handful of items and perhaps four or five unusual ones. So the approach of writing special code for each one, and slowly obsoleting the big messy function, should be simple and quite obvious. As such, it seems that it isn’t really worth doing, unless there were a great public outcry to see how it could be done.
Accordingly, I think I’ll try the approach of refactoring the existing code, keeping it working, and ultimately, if we succeed, making it discernibly better.
Frankly, I find it a bit daunting: reading the code, I do not see a lot of clearly good ideas. So it’ll be fun trying not so good ideas and learning from them. Or, it’ll be terribly frustrating and embarrassing. Good either way.
- The Start
- Here’s the code for the update:
private fun updateItem(item: Item) {
if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
}
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
if (item.sellIn < 0) {
if (item.name != "Aged Brie") {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
item.quality = item.quality - item.quality
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
}
}
It does appear that there are some sections that can be pulled out as separate methods, although they don’t have obvious meaning. I’ll start with that.
Removing the last big if, about sellIn, I get this:
...
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
sellInLessThanZero(item)
}
private fun sellInLessThanZero(item: Item) {
if (item.sellIn < 0) {
if (item.name != "Aged Brie") {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
item.quality = item.quality - item.quality
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
}
}
My elementary test remains green. I will want to do some more detailed ones, but probably not yet. I notice that if with the negative condition. I think perhaps IDEA will invert that for me, if I can position the cursor just right.
Yes. Cursor on the if, Alt-Enter, select Invert ‘if’ condition.
private fun sellInLessThanZero(item: Item) {
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
if (item.quality < 50) {
item.quality = item.quality + 1
}
} else {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
item.quality = item.quality - item.quality
}
}
}
}
That little bit of code for the Brie makes some sense. Let’s pull it. We’ll get some random methods, but if all else fails, we can revert. I wonder if I should be committing. Let’s do, we’re sure to do something we’d like to get rid of.
private fun increaseQualityUpTo50(item: Item) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
IDEA found more places to use that method. The sellInLessThanZero function is now this …
private fun sellInLessThanZero(item: Item) {
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
item.quality = item.quality - item.quality
}
}
}
}
But the outer function is this:
private fun updateItem(item: Item) {
if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
sellInLessThanZero(item)
}
I’m finding the name sellInLessThanZero to be a bit confusing. First, commit: extract increaseQualityUpTo50.
Rename to handleSellinDateLessThanZero. Test, commit: rename method.
There at the end of the update method, there’s this:
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
I’d like to pull that and turn it onto a default, but there’s code above it that is looking at the sellInprior to it being updated. We’ll have to accommodate that. Set that aside, though it certainly could be pulled and named.
Up at the top of the update, we have this:
if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
} else { ...
Could we invert the order of those two inner ifs? If so, we have another bit of common code, because … no wait … this very block occurs twice, once here, and once in the handle... method. Let’s see if IDEA will discover and extract both.
Test. Green. Commit: rename method. decrementNonSulfurasQuality.
Let’s look at the big picture:
private fun updateItem(item: Item) {
if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
decrementNonSulfurasQuality(item)
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
}
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.sellIn = item.sellIn - 1
}
handleSellinDateLessThanZero(item)
}
private fun handleSellinDateLessThanZero(item: Item) {
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
decrementNonSulfurasQuality(item)
} else {
item.quality = item.quality - item.quality
}
}
}
}
private fun decrementNonSulfurasQuality(item: Item) {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
}
private fun increaseQualityUpTo50(item: Item) {
if (item.quality < 50) {
item.quality = item.quality + 1
}
}
This block looks to me to be able to be simplified:
} else {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
}
Can’t we close the initial if after incrementing quality, and then move the inner if out to the same level? Or, move that outside?
I decide to invert the if for a better look at things:
if (item.name == "Aged Brie" || item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
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)
}
Now, what if we were to create some duplication by splitting the if into two, one with each case? IDEA offers “Split if into two”, resulting in this:
if (item.name == "Aged Brie") {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
}
Test just for stability. Green. Commit: Split if. The warnings tell me what I already knew, the checks for passes are now both redundant. Here:
if (item.name == "Aged Brie") {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
}
It can be deleted. Will IDEA do that for us? Oddly, no, but I can do it.
And in the other case, the inner condition can be removed.
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
if (item.quality < 50) {
item.quality = item.quality + 1
}
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality < 50) {
item.quality = item.quality + 1
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)
}
Test that. Still green. I’m starting to wish for more tests, but I trust IDEA, I trust the few non-mechanized changes I’ve made, so I’m happy for now, but I will want to back fill with some tests.
Now right inside both those if branches, we have the same check, for quality < 50. Not that interesting. Instead let’s extract all the passes code, with a meaningful name.
No, first let’s replace the Brie one with increaseQualityUpTo50, which it matches.
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
Test. Green. Commit: Use available increaseQualityUpTo50.
Now here:
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality < 50) {
item.quality = item.quality + 1
if (item.sellIn < 11) {
increaseQualityUpTo50(item)
}
if (item.sellIn < 6) {
increaseQualityUpTo50(item)
}
}
}
Can we close the top if and move the other two outward? It seems to me that we can.
The tests agree. We have this:
private fun updateItem(item: Item) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else if (item.name == "Backstage passes to a TAFKAL80ETC concert") {
if (item.quality < 50)
item.quality = item.quality + 1
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)
}
Now I can replace that first bit:
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)
}
Now let’s look at decrementNonSulfurasQuality:
private fun decrementNonSulfurasQuality(item: Item) {
if (item.quality > 0) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
item.quality = item.quality - 1
}
}
}
We can reverse those two ifs, since they are anded. Not sure that helps.
How about flattening this:
private fun handleSellinDateLessThanZero(item: Item) {
if (item.sellIn < 0) {
if (item.name == "Aged Brie") {
increaseQualityUpTo50(item)
} else {
if (item.name != "Backstage passes to a TAFKAL80ETC concert") {
decrementNonSulfurasQuality(item)
} else {
item.quality = item.quality - item.quality
}
}
}
}
First, this:
private fun handleSellinDateLessThanZero(item: Item) {
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 {
decrementNonSulfurasQuality(item)
}
}
}
}
Then this:
private fun handleSellinDateLessThanZero(item: Item) {
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 {
decrementNonSulfurasQuality(item)
}
}
}
Then let’s reverse the decrement in place:
private fun decrementNonSulfurasQuality(item: Item) {
if (item.name != "Sulfuras, Hand of Ragnaros") {
if (item.quality > 0) {
item.quality = item.quality - 1
}
}
}
I’ve not been committing. Tests are green. Commit: various refactoring.
Now I want to inline the decrement and take a look at the result:
private fun handleSellinDateLessThanZero(item: Item) {
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 {
if (item.name == "Sulfuras, Hand of Ragnaros") return
if (item.quality > 0) {
item.quality = item.quality - 1
}
}
}
}
I inverted the if after inlining. Now that quality decrement is duplicated, let’s extract.
private fun handleSellinDateLessThanZero(item: Item) {
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 {
if (item.name == "Sulfuras, Hand of Ragnaros") return
decrementQualityTowardZero(item)
}
}
}
It seems that I can move that Sulfuras line to the top as a guard clause:
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)
}
}
}
I’ve just taken an unscheduled break to take out the trash, which gave me a moment to reflect. I think this is notable:
- No Grand Plan
- I’ve been making entirely local changes that I’ve spotted, aimed at making the code simpler, more clear, locally. I haven’t been thinking about any grand idea for where this will go. It’s kind of an act of faith, that if I keep making things better, they’ll get somewhere good.
-
Is this a good way to work? Well, it is for me, because I’m as interested in what happens if it is bad as I am if it is good, so no harm done either way. But it does seem to sort of work out.
Article Split
I took a brief pause to think right here, and by the time I was done, I had about twice the number of lines we have here, so let’s pause right here. Here’s the code as it stands. We’ll start here tomorrow as well.
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
}
}