Gilded Rose Kata

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) };

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.

StackExchange Code Review Q#123364, answer score: 7