Kotlin 78 - Just a little bit more ...

Let’s see if we can wring a bit more out of refactoring the Gilded Rose (Some wiggling back and forth to slightly better code.).

I see some duplication that can probably be addressed. I’m not sure yet quite how to approach it.

    private fun updateBrie(item: Item) {
        increaseQualityUpTo50(item)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            increaseQualityUpTo50(item)
        }
    }

    private fun updateNormal(item: Item) {
        decrementQualityTowardZero(item)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            decrementQualityTowardZero(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 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
        }
    }

I see all those item.sellIn decrements that I wish could be consolidated. And then, based on sellIn, we increment, or decrement, or zero quality … always within limits (which are always zero to fifty, at least according to the current spec).

I’m envisioning something kind of like this:

Have an adjustQuality method that can adjust up or down, and always keeps quality between zero and fifty. (We don’t have negative quality, which is nice).
Have that method take an increment, +1 or -1, which way to go on quality. (I’m not sure about zeroing it, but of course adjust -item.quality would do it, albeit cryptically.)
Have a method that decrements sellIn and then calls the above method.

What I’d like to do, if I could see the path, is to get there by mechanized refactorings if possible, and by very very simple steps if IDEA can’t do it for me.

If we were allowed to edit Item, this might be a bit easier. So far, I’m abiding by that rule.

I think I’ll just try a couple of things.

First, let’s unconditionally change quality in the increment and decrement, and then force to the desired range:

    private fun decrementQualityTowardZero(item: Item) {
        item.quality = item.quality - 1
        item.quality = max(0,item.quality)
    }

    private fun increaseQualityUpTo50(item: Item) {
        item.quality = item.quality + 1
        item.quality = min(item.quality,50)
    }

Test. Green. Commit: refactor increase and decrement quality methods. (Odd choice of names.)

Now for one of them, let’s change signature to add a parameter. I can’t find a refactoring to do it. I’ll just type in what I want:

    private fun decrementQualityTowardZero(item: Item, change: Int) {
        item.quality = item.quality + change
        item.quality = max(0,item.quality)
    }

Note that I changed the adjustment from - to +. IDEA tells me that I have errors. In two places, I do this:

    decrementQualityTowardZero(item,-1)

I think I could have done this more cleverly, but this isn’t bad. Rename that method adjustQuality.

Now, before I forget, move the check from increase up into adjust:

    private fun adjustQuality(item: Item, change: Int) {
        item.quality = item.quality + change
        item.quality = max(0,item.quality)
        item.quality = min(item.quality,50)
    }

Test. Green. Commit: new adjustQuality method takes a change value.

Now find the increase calls and change them:

    private fun updateBrie(item: Item) {
        adjustQuality(item,1)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            adjustQuality(item,1)
        }
    }

    private fun updateNormal(item: Item) {
        adjustQuality(item,-1)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            adjustQuality(item,-1)
        }
    }

    private fun updatePasses(item: Item) {
        adjustQuality(item,1)
        if (item.sellIn < 11) {
            adjustQuality(item,1)
        }
        if (item.sellIn < 6) {
            adjustQuality(item,1)
        }
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            item.quality = 0
        }
    }

And increase is no longer used. Delete it. Test. Green. Commit: Using adjustQuality everywhere.

Now, do we like what we have there, or not? We used to say very specifically, increase or decrement, which we should have spelled decrease. Now the only clue as to which way we’re going is in the 1 or -1 parameter to adjustQuality. That’s not as clear and communicative as we might like.

Shall we fix that? We shall. Grab one of the increasing ones and extract method:

    private fun updatePasses(item: Item) {
        increaseQuality(item)
        if (item.sellIn < 11) {
            increaseQuality(item)
        }
        if (item.sellIn < 6) {
            increaseQuality(item)
        }
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            item.quality = 0
        }
    }

    private fun increaseQuality(item: Item) {
        adjustQuality(item, 1)
    }

Similarly for the decrease.

    private fun updateNormal(item: Item) {
        decreaseQuality(item)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            decreaseQuality(item)
        }
    }

    private fun decreaseQuality(item: Item) {
        adjustQuality(item, -1)
    }

I do think we might have moved more expeditiously here. I needed to see what I had before I could decide what I wanted. I’m OK with that, excess thinking can overheat the brain, and I’d rather see the code than imagine it.

Now about those sellIn decrements? Do we want to go after them?

    private fun updateBrie(item: Item) {
        increaseQuality(item)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            increaseQuality(item)
        }
    }

    private fun updatePasses(item: Item) {
        increaseQuality(item)
        if (item.sellIn < 11) {
            increaseQuality(item)
        }
        if (item.sellIn < 6) {
            increaseQuality(item)
        }
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            item.quality = 0
        }
    }

    private fun updateNormal(item: Item) {
        decreaseQuality(item)
        item.sellIn = item.sellIn - 1
        if (item.sellIn < 0) {
            decreaseQuality(item)
        }
    }

We can almost imagine wrapping those into a method that could call increase or decrease … or just the suitable adjust. But to give it a reasonable name, it’d have to be something like decreaseSellInAndAdjustQuality. It’s not cohesive: it does two things at a time.

Let’s commit this and then try it to see if we like what we get. Commit: increase decrease used everywhere.

No. Just looking at it, I don’t think it’s going to make me happy. If we were working in Item, we might then have better luck: we could have each item know its quality adjustment rules and we could run them automagically upon updating sellIn.

As it stands, right now, not that long before supper, I think this is better than at the top of the article, and perhaps it’s time to stop, at least for now.

I still owe some tests. Next time, I promise. But I’m still about 80% sure this is good, maybe even 90.

See you next time!