5
\$\begingroup\$

I've just completed the Gilded Rose Refactoring Kata in Java. I took the approach that I would fully characterise the existing behaviour with 100% unit test coverage before refactoring.

Although this got me there, it took me about 2 hours and got me thinking - is there another approach to testing that would facilitate a more iterative "green-refactor" cycle? Please share your suggestions for better test and production code.

Test code:

package com.gildedrose;

import static org.junit.Assert.*;

import org.junit.Test;

public class GildedRoseTest {

    @Test
    public void updateQuality_decrementsItemsQualityAndSellInBy1() {
        Item[] items = new Item[] { new Item("foo", 1, 1) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("foo");
        assertEquals("foo", item.name);
        assertEquals(0, item.quality);
        assertEquals(0, item.sellIn);
    }

    @Test
    public void onceTheSellByDateHasPassed_QualityDegradesTwiceAsFast() throws Exception {
        Item[] items = new Item[] { new Item("bar", -1, 2) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("bar");
        assertEquals("bar", item.name);
        assertEquals(0, item.quality);
        assertEquals(-2, item.sellIn);
    }

    @Test
    public void qualityOfAnItemIsNeverNegative() throws Exception {
        Item[] items = new Item[] { new Item("foo", 2, 2) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        app.updateQuality();
        app.updateQuality();
        Item item = app.getItem("foo");
        assertEquals("foo", item.name);
        assertEquals(0, item.quality);
        assertEquals(-1, item.sellIn);
    }

    @Test
    public void theQualityOfAnItemIsNeverMoreThan50() throws Exception {
        Item[] items = new Item[] { new Item("Aged Brie", -1, 50), new Item("Backstage passes to a TAFKAL80ETC concert", 3, 49) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Aged Brie");
        assertEquals("Aged Brie", item.name);
        assertEquals(50, item.quality);
        assertEquals(-2, item.sellIn);

        Item item2 = app.getItem("Backstage passes to a TAFKAL80ETC concert");
        assertEquals("Backstage passes to a TAFKAL80ETC concert", item2.name);
        assertEquals(50, item2.quality);
        assertEquals(2, item2.sellIn); 
    }

    @Test
    public void briesQualityIncrementsByOneForEachDayPastItsSellByDate() throws Exception {
        Item[] items = new Item[] { new Item("Aged Brie", -1, 1) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Aged Brie");
        assertEquals("Aged Brie", item.name);
        assertEquals(3, item.quality);
        assertEquals(-2, item.sellIn);
    }

    @Test
    public void SulfurasBeingALegendaryItemNeverHasToBeSoldOrDecreasesInQuality() throws Exception {
        Item[] items = new Item[] { new Item("Sulfuras, Hand of Ragnaros", -1, 80) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Sulfuras, Hand of Ragnaros");
        assertEquals("Sulfuras, Hand of Ragnaros", item.name);
        assertEquals(80, item.quality);
        assertEquals(-1, item.sellIn);
    }

    @Test
    public void backstagePassesQualityIncrementsByOneWithEachDayPassing() throws Exception {
        Item[] items = new Item[] { new Item("Backstage passes to a TAFKAL80ETC concert", 11, 30) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Backstage passes to a TAFKAL80ETC concert");
        assertEquals("Backstage passes to a TAFKAL80ETC concert", item.name);
        assertEquals(31, item.quality);
        assertEquals(10, item.sellIn); 
    }

    @Test
    public void backstagePassesIncreaseInQualityBy2WhenThereAre10DaysOrLessRemaining() throws Exception {
        Item[] items = new Item[] { new Item("Backstage passes to a TAFKAL80ETC concert", 10, 30) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Backstage passes to a TAFKAL80ETC concert");
        assertEquals("Backstage passes to a TAFKAL80ETC concert", item.name);
        assertEquals(32, item.quality);
        assertEquals(9, item.sellIn); 
    }

    @Test
    public void backstagePassesIncreaseInQualityBy3WhenThereAre5DaysOrLessRemaining() throws Exception {
        Item[] items = new Item[] { new Item("Backstage passes to a TAFKAL80ETC concert", 5, 33) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Backstage passes to a TAFKAL80ETC concert");
        assertEquals("Backstage passes to a TAFKAL80ETC concert", item.name);
        assertEquals(36, item.quality);
        assertEquals(4, item.sellIn); 
    }

    @Test
    public void backstagePassesQualityDropsTo0AfterTheConcert() throws Exception {
        Item[] items = new Item[] { new Item("Backstage passes to a TAFKAL80ETC concert", -1, 30) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Backstage passes to a TAFKAL80ETC concert");
        assertEquals("Backstage passes to a TAFKAL80ETC concert", item.name);
        assertEquals(0, item.quality);
        assertEquals(-2, item.sellIn); 
    }

    @Test
    public void conjuredItemsDegradeInQualityTwiceAsFastAsNormalItems() throws Exception {
        Item[] items = new Item[] { new Item("Conjured", 2, 30) };
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        Item item = app.getItem("Conjured");
        assertEquals("Conjured", item.name);
        assertEquals(28, item.quality);
        assertEquals(1, item.sellIn); 
    }

    @Test(expected=RuntimeException.class)
    public void getItem_throwsRuntimeExceptionIfRequestedItemDoesntExist() throws Exception {
        GildedRose app = new GildedRose(new Item[0]);
        app.getItem("foobar");
    }
}

"Production" code:

package com.gildedrose;

import static java.lang.String.format;

class GildedRose {
    public static final int MAX_QUALITY = 50;
    public static final int MIN_QUALITY = 0;
    private final Item[] items;

    public GildedRose(Item[] items) {
        this.items = items;
    }

    public void updateQuality() {

        for (Item item : items) {

            if (degradesNaturally(item)) {
                decrementQuality(item);
            } else if (getsBetterWithAge(item)) {
                incrementQuality(item);

                if (isBackstagePass(item)) {
                    if (tenDaysOrLess(item)) {
                        incrementQuality(item);
                    }

                    if (fiveDaysOrLess(item)) {
                        incrementQuality(item);
                    }
                }
            }

            if (pastSellByDate(item)) {

                if (isAgedBrie(item)) {
                    incrementQuality(item);
                } else {
                    decrementQuality(item);
                }

                if (isBackstagePass(item)) {
                    setQualityToZero(item);
                }

            }

            decreaseSellIn(item);
        }
    }

    private void incrementQuality(final Item item) {
        if (item.quality < MAX_QUALITY) {
            item.quality = item.quality + 1;
        }
    }

    private void decrementQuality(Item item) {
        if (item.quality > MIN_QUALITY) {
            if (!isSulfuras(item)) {
                item.quality = item.quality - 1;
            }

            if (isConjured(item)) {
                item.quality = item.quality - 1;
            }
        }
    }

    private void setQualityToZero(Item item) {
        item.quality = 0;
    }

    private void decreaseSellIn(final Item item) {
        if (!isSulfuras(item)) {
            item.sellIn = item.sellIn - 1;
        }
    }

    private boolean degradesNaturally(Item item) {
        return !getsBetterWithAge(item);
    }

    private boolean getsBetterWithAge(final Item item) {
        return isAgedBrie(item) || isBackstagePass(item);
    }

    private boolean pastSellByDate(final Item item) {
        return item.sellIn < 0;
    }

    private boolean fiveDaysOrLess(Item item) {
        return item.sellIn < 6;
    }

    private boolean tenDaysOrLess(Item item) {
        return item.sellIn < 11;
    }

    private boolean isAgedBrie(Item item) {
        return item.name.equals("Aged Brie");
    }

    private boolean isBackstagePass(Item item) {
        return item.name.equals("Backstage passes to a TAFKAL80ETC concert");
    }

    private boolean isSulfuras(Item item) {
        return item.name.equals("Sulfuras, Hand of Ragnaros");
    }

    private boolean isConjured(Item item) {
        return item.name.equals("Conjured");
    }

    public Item getItem(String name) {
        Item[] clone = items.clone();
        for (Item item : clone) {
            if (name.equalsIgnoreCase(item.name)) {
                return item;
            }
        }

        throw new RuntimeException(format("Item '%s' not found", name));
    }
}
\$\endgroup\$
7
\$\begingroup\$

Your question would be improved if you posted (a link to) the original Gilded Rose code, or the problem statement, or whatever it was that you started with. Without knowing the problem statement — i.e., the constraints that you're stuck with and can't change — it's hard to know exactly which terrible parts of your code are fixable and which parts are irreducibly terrible due to the original constraints.

Since the Gilded Rose problem is phrased in pretty explicitly object-oriented terms (various hard-coded types of items, each with its own idiosyncratic "update quality" algorithm), and especially since you're working in Java (the most classical-OOP of languages), it would make sense to solve it using classical OOP polymorphism.

public class Item // the base class
{
    public string name;
    public int age;
    public int sellin;
    public int quality;
    public void updateQuality() { quality -= (age > sellin) ? 2 : 1; }
};

public class CheeseItem extends Item
{
    public void updateQuality() { quality += 1; }
};

public class TicketItem extends Item
{
    public void updateQuality() {
        int days_before_concert = sellin - age;
        quality += (days_before_concert < 5) ? 3 : (days_before_concert < 10) ? 2 : 1;
    }
};

public class LegendaryItem extends Item
{
    public void updateQuality() { }
};

Modulo some missing constructors, I think the above 26 lines of code addresses all the constraints of the Gilded Rose problem.

However, this system is unable to express the concept of a "legendary backstage pass", nor of "legendary cheese". In Java, you might solve that problem by making Legendary an adjective that wraps a noun; for example,

public class Legendary extends Item
{
    private Item t;
    public Legendary(Item i) { t = i; }
    public string name() { return t.name(); }
    public int quality() { return t.quality(); }
    public void updateQuality() { }
}

Then you could implement Conjured the same way:

public class Conjured extends Item
{
    Item t;
    public Conjured(Item i) { t = i; }
    public string name() { return t.name(); }
    public int quality() { return t.quality(); }
    public void updateQuality() { t.updateQuality(); t.updateQuality(); }
}

Notice that we have problems with the business logic here. Should legendary cheese actually still get better with age, or should it remain at the same quality forever? Should conjured cheese get better twice as fast?

The original problem statement is fuzzy on these concepts, because it's not really intended as a problem for working programmers; it's more about getting a learner comfortable with the idea of nested if statements.

if (Items[i].Name != "Aged Brie" && Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")

Real code wouldn't contain this kind of hard-coded string comparison, unless it was written by someone trying to end up on The Daily WTF. If you were really writing an inventory system, you'd try to make it general enough to handle any kind of item, because, well, that's the point of an inventory system. Otherwise all you've got is a handful of nested if statements.

In general, beware of "Test Driven Development"; as you can see, it's led you into a bad place. Just because you wrote some code that passes your tests doesn't mean that you've written good code.


Two specific comments on your code:

private boolean degradesNaturally(Item item) {
    return !getsBetterWithAge(item);
}

private boolean getsBetterWithAge(final Item item) {
    return isAgedBrie(item) || isBackstagePass(item);
}

Why provide x.degradesNaturally() when it would be clearer and simpler to say !x.getsBetterWithAge()? (Besides, cheese does degrade naturally.)

(Edited to add: On first reading, I completely missed that you made these functions non-static member functions of GildedRose instead of member functions of Item. So you don't even have x.degradesNaturally() — you have a monstrosity like (new GildedRose()).degradesNaturally(x). Don't do that.)


private boolean isConjured(Item item) {
    return item.name.equals("Conjured");
}

I believe this isn't what the problem statement intended. Maybe if you changed .equals to .startsWith, you'd have the right idea... but then conjured brie wouldn't pass your .equals("Aged Brie") comparison.

\$\endgroup\$
  • \$\begingroup\$ CheeseItem : Item isn't Java \$\endgroup\$ – abuzittin gillifirca Mar 21 '16 at 11:35
  • \$\begingroup\$ Good point. My C++ is showing. :) \$\endgroup\$ – Quuxplusone Mar 22 '16 at 3:57
  • \$\begingroup\$ i thought we were not allowed to change the item class. \$\endgroup\$ – Ray Tayek May 20 at 7:39
  • \$\begingroup\$ @RayTayek: Since the problem statement (to which I linked in my answer) is written in C#, and OP wanted to write in Java, changing the Item class was pretty much step 1! You're welcome to do the kata in C# yourself if you're looking to try it. :) (I do see that OP subsequently added a link to a Java version, sometime in the 3 years since I posted my answer. Good for OP!) \$\endgroup\$ – Quuxplusone May 20 at 16:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.