I was glancing at yesterday’s article last night, and noticed some truly odd code that IDEA generated for me. We’ll start there, and I have some improvements in mind.

Here’s what I noticed, a patch of code that IDE wrote when I did an extract:

    fun updateItems() {
        for (i in items.indices) {
            val item = items[i]
            updateItem(item)
        }
    }

In my defense, I was more interested in the part it extracted, and that code scrolls off the top of my screen as I work on the updating code. And, of course, it’s harmless. It’s just not all that nice.

I wonder whether IDEA’s “intentions” will offer me a cleaner version. It seems not. OK, I still almost know how to code:

    fun updateItems() {
        items.forEach { updateItem(it) }
    }

I prefer that. I might even prefer this:

    fun updateItems() = items.forEach { updateItem(it) }

We’ll go with that for a while. I’m experimenting lately with writing very compact code. There’s no question that too compact is a problem, but so is code that is too open and spread out. I’m sure that the choice depends on what in the code needs to be emphasized. Anyway, I’m pushing toward compact recently, to see what my preferred balance will be.

So, we’re green. Commit: Tidying: use forEach.

Let’s Pretend …

Let’s pretend that we’re maintaining this Gilded Rose product, and that the goblin in the corner, who won’t let us work on his Item class, has moved on to a well-deserved position in another industry. We’ll do some work to improve the capability of the system, because Gilded Rose is really taking off, and we’ll need a more capable pricing system soon.

Or, let’s pretend that I enjoy working with code and that I plan to try some things that I think will be improvements.

The first thing I want to address is the issue of the product type. We have our constants that suggest the notion of a product type:

const val PROD_BRIE = "Aged Brie"
const val PROD_CONJ = "Conjured Item"
const val PROD_ELIX = "Elixir of the Mongoose"
const val PROD_PASS = "Backstage passes to a TAFKAL80ETC concert"
const val PROD_SULF = "Sulfuras, Hand of Ragnaros"
const val PROD_VEST = "+5 Dexterity Vest"

We use the constants throughout in the code, not that they occur very often. But they do help with readability, and are useful in tests.

    private fun Item.updateQuality() {
        when (name) {
            PROD_SULF -> { /*nothing*/ }
            PROD_BRIE -> raiseQuality(1)
            PROD_CONJ -> lowerQuality(2)
            PROD_PASS -> adjustPassQuality()
            else -> adjustNormalQuality()
        }
    }


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

The constants are particularly handy when they keep us from needing to type correctly:

if (item.name == "Backstage passes to a TAFKAL80ETC concert")

But, as indicated by the item above, the PROD_ things represent a particular product, not a particular type or kind of product. We need to change that, because we anticipate many different kinds of backstage passes, and conjured items, and of course there are many different kinds of “normal” items.

So we need to refactor (or improve?) the code to provide for a small number of specific product types, and as many product names as we may ever need.

I think we’ll continue to provide the short PROD_ names as constants naming the individual product, and have a new notion, TYPE_wzyz, that encompasses all products of a given, well, type.

We will want most tests to continue to be written in terms of “PROD_”, or, if need be, the product name, but the code in the update should be converted to the new TYPE_ notation.

And we’ll try to do this without breaking the system.

Is This Refactoring?
Refactoring is the process of improving the design without changing the behavior of the program. When we’re done, we’ll have some new … behavior? I’m not sure what we’ll call it. If things go as I intend (like that ever happens), we’ll have a new capability, and an improved design, and the same behavior as ever … and yet … something new.

Let’s do it.

Introduce TYPE_

We want the operational code, the code in class GildedRose to concern itself primarily with TYPE_, not PROD_.

Let’s start by defining new constants covering the old ones:

const val TYPE_AGELES = PROD_SULF
const val TYPE_CHEESE = PROD_BRIE
const val TYPE_CONJUR = PROD_CONJ
const val TYPE_PASSES = PROD_PASS

Now we can change all our references to the new names:

    private fun Item.updateQuality() {
        when (name) {
            TYPE_AGELES -> { /*nothing*/ }
            TYPE_CHEESE -> raiseQuality(1)
            TYPE_CONJUR -> lowerQuality(2)
            TYPE_PASSES -> adjustPassQuality()
            else -> adjustNormalQuality()
        }
    }

    private fun Item.adjustSellIn() {
        if(name!= TYPE_AGELES) sellIn -= 1
    }

Should be same as before. Test. Green. Commit: Create and use TYPE_ product types in update.

Now we have another issue … both of those bits of code refer to name, the item name. right now, that name is some string, whose name constant is PROD_something and whose type constant is TYPE_something. We want to be doing our checks, not against name, but against type.

    private fun Item.updateQuality() {
        when (type()) {
            TYPE_AGELES -> { /*nothing*/ }
            TYPE_CHEESE -> raiseQuality(1)
            TYPE_CONJUR -> lowerQuality(2)
            TYPE_PASSES -> adjustPassQuality()
            else -> adjustNormalQuality()
        }
    }

And …

    private fun Item.type(): String {
        return when (name) {
            PROD_BRIE -> TYPE_CHEESE
            PROD_CONJ -> TYPE_CONJUR
            PROD_PASS -> TYPE_PASSES
            PROD_SULF -> TYPE_AGELES
            else -> name
        }
    }

We’ll have to worry about that else -> name, but not yet.

Also:

    private fun Item.adjustSellIn() {
        if(type() != TYPE_AGELES) sellIn -= 1
    }

Test. Green. Commit: TYPE of product looked up in type() function.

Now let’s see whether we can convert our TYPE_ to an enum.

enum class TYPE {
    TYPE_AGELES, TYPE_CHEESE, TYPE_CONJUR, TYPE_PASSES, TYPE_NORMAL
}

    private fun Item.updateQuality() {
        when (type()) {
            TYPE.TYPE_AGELES -> { /*nothing*/ }
            TYPE.TYPE_CHEESE -> raiseQuality(1)
            TYPE.TYPE_CONJUR -> lowerQuality(2)
            TYPE.TYPE_PASSES -> adjustPassQuality()
            else -> adjustNormalQuality()
        }
    }

    private fun Item.adjustSellIn() {
        if(type() != TYPE.TYPE_AGELES) sellIn -= 1
    }

    private fun Item.type(): TYPE {
        return when (name) {
            PROD_BRIE -> TYPE.TYPE_CHEESE
            PROD_CONJ -> TYPE.TYPE_CONJUR
            PROD_PASS -> TYPE.TYPE_PASSES
            PROD_SULF -> TYPE.TYPE_AGELES
            else -> TYPE.TYPE_NORMAL
        }
    }

We test green. I see room for code improvement but first commit the function: Update now dispatches on product type, not name. Type “looked up” by name.

Now then. I don’t like the look of TYPE.TYPE_CHEESE. Let’s just remove all the TYPE_ from them, see how it looks:

enum class TYPE {
    AGELES, CHEESE, CONJUR, PASSES, NORMAL
}

    private fun Item.updateQuality() {
        when (type()) {
            TYPE.AGELES -> { /*nothing*/ }
            TYPE.CHEESE -> raiseQuality(1)
            TYPE.CONJUR -> lowerQuality(2)
            TYPE.PASSES -> adjustPassQuality()
            else -> adjustNormalQuality()
        }
    }

And so on. Test. Commit: Rename TYPE.TYPE_FOO to TYPE.FOO.

Kotlin will help us further: we can get rid of that else, because now everything has a TYPE, even normal items:

    private fun Item.updateQuality() {
        when (type()) {
            TYPE.AGELES -> { /*nothing*/ }
            TYPE.CHEESE -> raiseQuality(1)
            TYPE.CONJUR -> lowerQuality(2)
            TYPE.PASSES -> adjustPassQuality()
            TYPE.NORMAL -> adjustNormalQuality()
        }
    }

Test. Green. Commit: Replace else with TYPE.NORMAL in updateQuality.

There’s more good news. After a quick and smooth update, IDEA and Kotlin conspire to enforce completeness on a when like the one above. If I comment out one of those lines, the when gets red squiggles under it, and the program won’t compile, with the message:

'when' expression must be exhaustive, 
add necessary 'PASSES' branch or 'else' branch instead

So now, we’re required to include all the TYPEs that we have (or to use else, which would be a bad thing to do in my opinion. If I could flag that as an error or issue, I would).

I test and commit just to celebrate the new version.

Let’s reflect, since I broke my flow with the update.

Reflection

We now have a new enum of TYPE, covering all the types of items that we know how to support. Our conversion from name to TYPE is imperfect, but I think we almost like what it does. We’ll explore that in a bit more detail in a moment. What else?

The program is now safer, in the sense that we can’t implement a new TYPE and then forget to plug it into the updateQuality function. The compiler will catch us if we do. We would still want all our tests and such, but given the kinds of mistakes I make, this is a good check.

Let’s move on.

Moving On

It seems to me that our conversion from name to TYPE needs improvement:

    private fun Item.type(): TYPE {
        return when (name) {
            PROD_BRIE -> TYPE.CHEESE
            PROD_CONJ -> TYPE.CONJUR
            PROD_PASS -> TYPE.PASSES
            PROD_SULF -> TYPE.AGELES
            else -> TYPE.NORMAL
        }
    }

We can improve it a bit right now, because we do have those other PROD names:

    private fun Item.type(): TYPE {
        return when (name) {
            PROD_BRIE -> TYPE.CHEESE
            PROD_CONJ -> TYPE.CONJUR
            PROD_ELIX -> TYPE.NORMAL
            PROD_PASS -> TYPE.PASSES
            PROD_SULF -> TYPE.AGELES
//            PROD_VEST -> TYPE.NORMAL
            else -> { 
                println("Product $name is not found up in type(), NORMAL assumed")
                TYPE.NORMAL }
        }
    }

Now we will get an error message on the console for products not found in our little lookup. And in fact we do get some messages. I expected the vest ones, because I commented out the lines. But I also get this one:

Product Conjured Mana Cake is not found up in type(), NORMAL assumed

My new feature for Conjured items does not cover that item. Here’s my const:

const val PROD_CONJ = "Conjured Item"

I change that:

And guess what, the golden master test fails!

expected: <Conjured Mana Cake, 2, 5> but was: <Conjured Mana Cake, 2, 4>
Expected :Conjured Mana Cake, 2, 5
Actual   :Conjured Mana Cake, 2, 4

That’s because we suddenly started putting the mana cake through the TYPE.CONJUR case and computing it correctly. And the golden master has never done it correctly.

Interesting! My test for conjured ran for “Conjured Item”, which I was supposing they’d all be called, but I was mistaken. We’ve talked about the need for a pattern match or similar scheme for item type, and this points out how important that is.

I fix the golden master to reflect conjured items working. Commit: Accept Mana Cake as Conjured, Fix golden master.

I’m glad I added that error message. And clearly the priority is high for a more powerful scheme for figuring out types.

I think we’ll leave that for another day. Since an enum “constant” is actually an instance of the enum class (TYPE in our case), they can have behavior and other members. We might be able to leverage that somehow. It seems semi-clear that we’ll want some kind of pattern recognition that decides an item’s type, unless we want to list each product by name and type in a table someplace. It may come to that. I’ll need to discuss this with the voices in my head before I decide.

Summary

We’ve made nice progress. We’ve got an enum set up that enables us to require that our updateQuality function covers all the TYPEs. We have a rudimentary but not unreasonable conversion from name to TYPE. It needs improvement but does the job for now.

I think the code is a bit more clear and a bit more solid. I’m sure there’s more we can do as we move this code from a one-shop utility to something that could serve a larger chain with more products, but we are progressing all the time.

There is no “best”, but there is generally “better”.

See you next time!