It seems to your faithful author that we’ve done nearly all we can with the Gilded Rose. We’ll take a final look. Or maybe not final. Who knows?

My vague plan is to cruise the code, pull out things to consider, discuss them, and possibly try to improve them. My starting guess is that things are quite good. If I bless something and you have a better, or just more interesting idea, please do tweet me up.

The Tests

Most of the tests are one-liners, using the checkUpdate method:

    @Test
    fun `conjured decrease two per day`() {
        checkUpdate(ITEM_CONJ, 10, 6, 4)
    }

    fun checkUpdate(prod: String, sellInStart: Int, qualityStart: Int, qualityEnd: Int, status:String="") {
        val item = Item(prod, sellInStart, qualityStart)
        val gr = GildedRose(arrayOf(item))
        if (true)
            gr.updateItems()
        else
            gr.oldUpdateItem(item)
        assertEquals(status, item.status())
        assertEquals(sellInStart-1, item.sellIn, "sellIn")
        assertEquals(qualityEnd, item.quality, "quality")
    }

As I mentioned a few articles back, the if(true) there allows me to run the original update method, renamed to oldUpdateItem, and check it against my test input values. I’ve used the switch a time or two when I wasn’t sure what the old code would do. As also mentioned before, we could do this check every time.

But if we did, the old code would flag errors on any new items, like “conjured”, that are not catered to in the now obsolete old function. We’ll skip this idea.

One idea down, what’s next?

Type Conversion

There’s the code for converting product (item) to type. Here, I do think we can do better.

The converter:

class ProductTypeConverter(val conversions: List<ProdToType>){
    private fun typeOf(name: String): TYPE {
        val types = conversions.filter { it.matches(name) }.map {it.type}
        return if (types.isNotEmpty()) types[0] else TYPE.ERROR
    }

    fun typeOf(item: Item): TYPE = typeOf(item.name)
}

I think this is pretty decent. There might be a more exotic collection function, but the filter and map are familiar to me, and to most of us here at my house, so I think they are OK. There is another interesting case, however. What if the size of types isn’t exactly one? That would mean that two different rules triggered. We might want to issue a warning if that were to happen.

I think that’s a good idea, and that as a new feature it is up to the customer, Allison. I make a note to discuss it the next time I put on my Allison hat for a product planning session.

Another issue here and elsewhere is the use of the word “product” in naming. I have begun saying “product” because of a very strange IDEA bug that hit me the other day, where it became confused and couldn’t find the Item class any more. But I think these names could be better.

Is this an ItemTypeConverter? Is it a converter at all? Is it a TypeFinder? Aside from tests, I think we only use this once:

val PRODUCT = ProductTypeConverter(listOf(
    ExactProdToType("Aged Brie", TYPE.CHEESE),
    ExactProdToType("Elixir of the Mongoose", TYPE.NORMAL),
    ExactProdToType("Sulfuras, Hand of Ragnaros", TYPE.AGELES),
    ExactProdToType("+5 Dexterity Vest", TYPE.NORMAL),
    PartialProdToType("conjured", TYPE.CONJUR),
    PartialProdToType("passes", TYPE.PASSES),
    PartialProdToType("backstage", TYPE.PASSES),
    PartialProdToType("", TYPE.ERROR)
    )
)

Those class names should also be subject to renaming. In addition, the file arrangement for this stuff is a bit strange, although you never notice it because IDEA finds everything for you. I do think that putting all of the type-related stuff in one file might be a good compromise. Of course in a larger product, we tend to get things in more and more individual files. I’m not sure that I prefer that, but in a team situation it’s surely right.

Let’s rename these:

interface ProdToType {
    val prodName: String
    val type: TYPE
    fun matches(name: String): Boolean
}

class ExactProdToType(override val prodName: String, override val type: TYPE) : ProdToType {
    override fun matches(name: String): Boolean 
    	= prodName.equals(name,ignoreCase = true)
}

class PartialProdToType(override val prodName: String, override val type: TYPE): ProdToType {
    override fun matches(name: String): Boolean 
    	= name.contains(prodName, ignoreCase = true)
}

I think we’ll basically replace “prod” with “item” throughout, including the variable prodName

OK, this is amazing. I used IDEA’s “rename” to rename ProdToType to ItemNameToType, and it renamed the implementing classes and even the test and the file it was in. That’s just plain marvelous!

interface ItemNameToType {
    val itemName: String
    val type: TYPE
    fun matches(name: String): Boolean
}

class ExactItemNameToType(override val itemName: String, override val type: TYPE) : ItemNameToType {
    override fun matches(name: String): Boolean 
    	= itemName.equals(name,ignoreCase = true)
}

class PartialItemNameToType(override val itemName: String, override val type: TYPE): ItemNameToType {
    override fun matches(name: String): Boolean 
    	= name.contains itemName, ignoreCase = true)
}

One slightly less than marvelous thing is that there is a paren missing between “contains” and “itemName” above. I don’t know whether IDEA did that, or I did. Anyway, replaced, green, commit rename …Prod… to …Item…

This code is nicer, and also shows the need for an additional renaming:

val PRODUCT = ProductTypeConverter(listOf(
    ExactItemNameToType("Aged Brie", TYPE.CHEESE),
    ExactItemNameToType("Elixir of the Mongoose", TYPE.NORMAL),
    ExactItemNameToType("Sulfuras, Hand of Ragnaros", TYPE.AGELES),
    ExactItemNameToType("+5 Dexterity Vest", TYPE.NORMAL),
    PartialItemNameToType("conjured", TYPE.CONJUR),
    PartialItemNameToType("passes", TYPE.PASSES),
    PartialItemNameToType("backstage", TYPE.PASSES),
    PartialItemNameToType("", TYPE.ERROR)
    )
)

Let’s rename to ItemNameToTypeConverter. Green. Commit: rename ProductTypeConverter to ItemNameToTypeConverter.

As part of all this, we let IDEA create lots of extension methods to Item. Since the goblin can’t see them as part of his class (they are in fact not part of the class), we felt that didn’t violate the letter of the law about not modifying Item. I think I’d like to see them as a separate file, however. I’ve not found a way for IDEA to help me with this but it should be easy enough.

Ah. Live and learn. Belay that order. If we keep those extensions inside the GildedRose class, they belong to that class. Otherwise we’d have to declare them all public. Probably better to keep them inside GildedRose.

In for a penny …

That said, here are the only actual member functions in GildedRose class:

class GildedRose(var items: Array<Item>) {

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

    private fun updateItem(item: Item) {
        with (item) {
            adjustSellIn()
            updateQuality()
        }
    }

And that updateItem? I scent some feature envy there. The other two functions are already extensions to Item, and that one should be as well:

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

    private fun Item.update() {
        adjustSellIn()
        updateQuality()
    }

Even better, this way is shorter and, in my view, more clear. What a righteous result for a simple change! It just goes to show we need to watch closely for feature envy. I’ll update the habits right now.

The naming stuff …

I still think that the naming constants and enums and converters belong in their own file. I’ll try moving them over to the ItemNameToTypeConverter file. That goes smoothly. Commit: Move constants and types and such over to ItemNameToTypeConverter file.

I’ve left the PRODUCTS table inside the GildedRose file: it’s part of the shop’s data, not part of the code’s definition of operations. I do think I might like to rename it to ITEMS, but I am concerned because of what happened to me the other day.

Works, though. Commit: Rename PRODUCTS to ITEMS.

Done, for now.

We’ve looked at all the operational files and classes, and other than a few renames, a few moves to a different file, and one function moved from GildedRose to Item extensions, we’ve not found much needing improvement.

We might just be done with GildedRose for a while. Back to the ADVENTure game next time? At least until the Zoom gang comes up with something new.

See you next time!