Tuesday 2018-08-21

The Gilded Rose kata seems like an exercise in restraint.

Many people seem to take the refactoring challenge to the maximum -- complete rewrites, full SOLID, etc. -- when the requirements hint otherwise.

First, the requirements are written in-character in WoW, so while you could read that as a normie, that's not the impetus. Second, while Leeroy may have left, the goblin in the corner is also a developer for this code base. Third, the code for Sulfuras fails to retain its how-long-in-inventory information, so Allison has non-standard ideas about inventory. Altogether, these hints point towards finding a maximum viable change -- i.e. the most helpful changeset which won't be revert-rejected by the goblin.

So what definitely needs to happen to the codebase? The biggest win seems like moving the different items into their own code path. Every other change pales in comparison. That change can be perhaps be accompanied by some DRY improvements; anything else and the code begins to look out-of-character given the codebase.

class GildedRose(object):
    def __init__(self, items):
        self.items = items

    def update(self):
        for item in self.items:
            itemIs = lambda substr: re.compile( r'\b' + substr + r'\b').search(item.name)
            if itemIs('Sulfuras'):
            elif itemIs('Brie'):
                item.sell_in -= 1
                if item.quality < 50:
                    item.quality += 2 if item.sell_in < 0 else 1
                    item.quality = min(item.quality, 50)
            elif itemIs('Backstage passes'):
                item.sell_in -= 1
                if item.quality < 50 or item.sell_in < 0:
                    item.quality += sum([ item.sell_in >= 0, item.sell_in < 5, item.sell_in < 10 ])
                    item.quality = 0 if item.sell_in < 0 else min(50, item.quality)
            elif itemIs('Conjured'):
                item.sell_in -= 1
                if item.quality >= 0:
                    item.quality -= 4 if item.sell_in < 0 else 2
                    item.quality = max(item.quality, 0)
                item.sell_in -= 1
                if item.quality >= 0:
                    item.quality -= 2 if item.sell_in < 0 else 1
                    item.quality = max(0, item.quality)
        return self.items

This in-character refactoring has two advantages: 1) Most prominent is the tri-partite decomposition of the mapping from previous sell_in,quality to the next, and 2) it avoids the starchitect problem of rushing to the design without examining the transition mapping.

The downside of this approach -- and every other downstream refactoring -- is there is no obvious map from the rules-based requirements to the implementation.

For sanity-checks of the above code, generated tests seemed the way to go...

def isSame(a,b):
    a = a[0] if type(a) == type([]) else a
    b = b[0] if type(b) == type([]) else b
    return a.name == b.name and a.sell_in == b.sell_in and a.quality == b.quality

"Sulfuras, Hand of Ragnaros",
"Backstage passes to a TAFKAL80ETC concert",
"Aged Brie",

for item in items:
    for sellin in range(-10, 100):
        for quality in range(-10, 100):
            a = GildedRoseOrig([Item(item, sellin, quality)]).update()
            b = GildedRose([Item(item, sellin, quality)]).update()
                assert isSame(a, b)
                print(sellin, quality, a, b)