Kotlin 80 - The point of a kata ...

UPDATE

I’ve gone over the code and tests again, in about ten different checked out versions, and my code always passes the legitimate tests. The bug was in the test, not the code. It has taken me nearly two hours to determine that, but I’m glad I did the work.

The point of a kata is that we do it again and again, practicing to get better. This isn’t really a kata, but it’s somewhat like one.

The point of a kata, if I’m not mistaken, and I assure you very strongly that I might possibly not be mistaken, is that there is a “right” way to do it, and one practices to get closer to that right way. Here, in the Gilded Rose “Kata”, there is no right way. There are, of course, many small moves that one might choose to make, and one might want to practice those moves toward perfection. Anyway, it’s a metaphor and we’re going to do the exercise again.

Since I’ve got those nice tests, I’ll keep them, but I’ll replace the update method with the original pre-refactoring one. that’ll save me a few steps and let me get right into the exploration of another way to go.

I cherry-pick back to the starting version. My tests won’t compile, I need the constants.

I’ll copy them over. I run my tests … and I get a failure in the passes scenario!

expected: <0> but was: <23>
Expected :0
Actual   :23

This is Very Bad, because that test passes on my “refactored” version, which suggests … a mistake on my part. It must also reflect a misunderstanding of the rules. I’m more interested in how the bug slipped through.

Nota Bene:

The main learning here, however, is that I should have written the tests sooner. Anyway we need to explore what’s going on.

UPDATE:

The bug was in the test. The code was always right, passed the same as the old code. I confused myself.

Here’s the test:

    @Test
    fun `passes scenario`() {
        checkUpdate(PROD_PASS, 11, 20, 21)
        checkUpdate(PROD_PASS, 10, 20, 22)
        checkUpdate(PROD_PASS, 5, 20, 23)
        checkUpdate(PROD_PASS, 1, 20, 0)
    }

The failure is on the last line. It says that at the end of the sellIn = 1 day, we drop the value of passes to zero. Of course, this depends on interpretation of sellIn and whether the ticket is still good on the zeroth day.

But how did I break it? If my refactorings were all machine-driven, that should be impossible, and I seriously doubt that there’s a refactoring bug in IDEA. But I did some things manually, and I may have messed up. Surely have messed up.

I vow to explore that, but for now, I think I’ll change the test and carry on, since clearly my refactoring must have changed something. We’re starting over today.

UPDATE:

I do explore it, at the cost of nearly two hours, to determine that I was mistaken when I thought I had made an error. I had not, unless one counts erroneously thinking that I’d made an error as an error. Which, indeed, one might do. The two hours was worth it.

So …

    @Test
    fun `passes scenario`() {
        checkUpdate(PROD_PASS, 11, 20, 21)
        checkUpdate(PROD_PASS, 10, 20, 22)
        checkUpdate(PROD_PASS, 5, 20, 23)
        checkUpdate(PROD_PASS, 1, 20, 23)
        checkUpdate(PROD_PASS, 0, 20, 0)
    }

That passes. So we’ll consider this a valid starting point. And I will explore where I introduced the mistake.

Aside

This error truly bothers me. I’m going to set it aside for now, spilt milk and all that, but it’s with me, and it’s going to affect how I feel during this exercise. Let’s hope it doesn’t do much harm, but I feel a bit shattered. Anyway let’s get to it.

I plan to approach the system this time via a “strangler” style. I propose to prefix the basic update with if statements or when statements, picking up products one by one until there are none going into the original update.

I’ll start with Sulfuras, because it’s trivial.

I’m looking at the beginning of all this, wondering just how to stick a wedge in here for a place to code:

    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
                }
            }
            ...

I’ll start by putting in an if that ends in a return. I expect that I’ll want to do it differently fairly soon. But I need to get started before I can improve things.

    private fun updateItem(item: Item) {
        with (item) {
            if (name== PROD_SULF) {
                return
            }
        }
        if (item.name != "Aged Brie" && item.name != "Backstage passes to a TAFKAL80ETC concert") {
            if (item.quality > 0) {
                if (item.name != "Sulfuras, Hand of Ragnaros") {
                    ...

I expect green for this, especially since analysis has told me that Sulfuras is never touched by the existing code. Green. Commit: Sulfuras strangled.

OK. The scheme here is to code what the spec says, and to test against the existing tests, which are satisfied by the old update. So let’s do a normal. That’s be an else, so let’s convert the if to a when.

This is a bit tricky. I want to end each completed item with a return. And normal items want to be the else case. So I do this:

        with (item) {
            when (name) {
                PROD_SULF -> {
                    return
                }
                PROD_BRIE -> {}
                PROD_PASS -> {}

                else -> {
                    quality -= 1
                    sellIn -= 1
                    if (sellIn<0 ) quality -= 1
                    return
                }
            }
        }
        // old update

This passes. However, I realize that my tests are weak: I do not check to be sure that quality is never negative. Improve the tests.

    fun `brie increases always`() {
        checkUpdate(PROD_BRIE, 20, 5, 6)
    }

That fails. Perfect. Let’s have a little decrementing method.

                else -> {
                    lowerQuality()
                    sellIn -= 1
                    if (sellIn<0 ) lowerQuality()
                    return
                }

    private fun Item.lowerQuality() {
        quality -= 1
        quality = max(0, quality)
    }

Test. Green. Commit: Normal products strangled.

Let’s do Brie.

                PROD_BRIE -> {
                    raiseQuality()
                    sellIn -= 1
                }

This demands raiseQuality:

    private fun Item.raiseQuality() {
        quality += 1
        quality = min(50, quality)
    }

I have high hopes for this. It fails. And why? Because I forgot the return. Should be:

                PROD_BRIE -> {
                    raiseQuality()
                    sellIn -= 1
                    return
                }

This is going to be a perennial problem for the dev team (me) forever. We will continually forget this. I don’t see an easy fix: I could put the old version in the else, but I really need the else to handle normal items. For now, I’ll live with it. After the strangling is done maybe it’ll be easier.

Now we have passes to do. I try this:

                PROD_PASS -> {
                    raiseQuality()
                    if (sellIn<=10) raiseQuality()
                    if (sellIn<=5) raiseQuality()
                    sellIn -= 1
                    if (sellIn<0) quality = 0
                }

I get an error:

expected: <19> but was: <18>

I don’t like that I have to look at the scenario to see which error it was.

    @Test
    fun `passes scenario`() {
        checkUpdate(PROD_PASS, 11, 20, 21)
        checkUpdate(PROD_PASS, 10, 20, 22)
        checkUpdate(PROD_PASS, 5, 20, 23)
        checkUpdate(PROD_PASS, 1, 20, 23)
        checkUpdate(PROD_PASS, 0, 20, 0)
    }

This is why people tell you to have one assert per test. Let’s do the right thing and break this up.

    @Test
    fun `passes increase by one before 10 days`() {
        checkUpdate(PROD_PASS, 20, 20, 21)
    }

    @Test
    fun `passes drop by one on day 11`() {
        checkUpdate(PROD_PASS, 11, 20, 21)
    }

    @Test
    fun `passes drop by two on day 10`() {
        checkUpdate(PROD_PASS, 10, 20, 22)
    }

    @Test
    fun `passes drop by two on day 5`() {
        checkUpdate(PROD_PASS, 5, 20, 23)
    }

    @Test
    fun `passes drop by three on day 1`() {
        checkUpdate(PROD_PASS, 1, 20, 23)
    }

    @Test
    fun `passes go to zero after day zero`() {
        checkUpdate(PROD_PASS, 0, 20, 0)
    }

Test again, but first on the old code. Green. Now on the new:

Four of them break:

passes drop by two on day 5
passes increase by one before 10 days
passes go to zero after day zero
passes drop by three on day 1

This is not good. Ah. I forgot the return AGAIN! Excuse me while I laugh at myself a bit.

With the return in, the tests all pass. Good news, that. We only have three special cases, so we should be done!

Since there is a return in each of these, we know something that Kotlin and IDEA also seem to know, that the code in the big block is unreachable. We can delete the whole thing.

    private fun updateItem(item: Item) {
        with (item) {
            when (name) {
                PROD_SULF -> {
                    return
                }
                PROD_BRIE -> {
                    raiseQuality()
                    sellIn -= 1
                    return
                }
                PROD_PASS -> {
                    raiseQuality()
                    if (sellIn<=10) raiseQuality()
                    if (sellIn<=5) raiseQuality()
                    sellIn -= 1
                    if (sellIn<0) quality = 0
                    return
                }
                else -> {
                    lowerQuality()
                    sellIn -= 1
                    if (sellIn<0 ) lowerQuality()
                    return
                }
            }
        }
    }

Nearly good. Commit: All products in new code, old code removed.

Now we don’t need the returns, so:

    private fun updateItem(item: Item) {
        with (item) {
            when (name) {
                PROD_SULF -> {
                    /* nothing */
                }
                PROD_BRIE -> {
                    raiseQuality()
                    sellIn -= 1
                }
                PROD_PASS -> {
                    raiseQuality()
                    if (sellIn<=10) raiseQuality()
                    if (sellIn<=5) raiseQuality()
                    sellIn -= 1
                    if (sellIn<0) quality = 0
                }
                else -> {
                    lowerQuality()
                    sellIn -= 1
                    if (sellIn<0 ) lowerQuality()
                }
            }
        }
    }

Commit: Remove redundant return statements.

Reflection

This went very smoothly. Makes you wish we did it that way the first time, doesn’t it? There are some lessons here to emphasize:

Strong Tests Help

We could have done these tests one by one, but it was nice to have them done. Had I done them one at a time, I might have put up with the long form. The fact that I had committed to do they all at once drove me to the easier form. At a guess …

Easier Tests Save Time

Even if I did them one at a time, creating the tests would have consumed more time, less noticeable, but still wasted. I would do well to respond sooner to tests being a bit hard to write.

Isolated Tests Help Isolate Concerns

Breaking out that scenario test actually helped with the “return” problem: the fact that all but one failed made me look for a larger problem, the lack of a return. Had I kept the scenario, only one test would have failed, and I’d likely have wasted time in the details, probably delaying my noticing the missing return.

The Characterization Test Failed Us

My microtests failed on the old code, which tells me that the characterization test must have left out some key elements. This is an important issue: no matter how big a characterization we run, we can’t be sure that it covers all the situations. This is why GeePaw’s doing directly to microtests was a better strategy than my reliance on a single run of a couple of days of output.

GeePaw’s approach was safer. Two points to GeePaw!

UPDATE

The characterization test could have been stronger, but it didn’t fail us. I did add a test that was not covered in the characterization and it did fail. But it was the test that was wrong, not the code.

Strangler Is Quite Powerful

One important thing about the strangler approach is that it’s easy to proceed incrementally, and we can pause in the improvement, returning next time to a better situation. One key aspect is that once the when is set up, we could readily do new features up there, making progress with the product, taking our time with the refactoring.

With my longer refactoring exercise, doing new features really needed to wait until the refactoring was done. That makes me think that when the refactoring is more than a few moments’ work, we should look seriously at a strangler kind of approach, allowing new features to be written in a clean space rather than merged into horrible code.

Should We Improve This Code Further?

We could imagine pulling the when blocks out into some separate structure, subclasses of Item, or at least separate functions. For me, at this scale, it’s just not worth it. Each one is just a few lines.

What might make sense would be some kind of DSL or other construct that made the code look more like the spec. We’d have to get creative for that. Maybe something will come to mind. Might be fun.

This Is Done

I’m calling this done and releasable.

UPDATE

The stricken comments below are mistaken, though it takes me two hours to find out.

But for now, we have a very clean improvement and it took almost no time. It’s still morning here, and I started at 0900.

See you next time!