이 글은 리포지터리의 README에서도 확인할 수 있습니다.

Gilded Rose Refactoring Kata 소개

Gilded Rose Refactoring Kata는 리팩터링 연습을 위한 애플리케이션으로, 실제 코드를 보게되면 왜 이 애플리케이션이 리팩터링이 필요한지 알 수 있습니다.

자세한 설명은 여기서 확인할 수 있습니다.

개요

이 포스트에서는 Gilded Rose 애플리케이션을 리팩터링한 첫 번째 시도에 대해 작성하려고 합니다.

저는 Java를 이용하였습니다.

살펴보기

Gilded Rose 애플리케이션은 2개의 클래스로 구성되어 있습니다. GildedRose와 Item이고 요구사항에 따라 Item클래스는 수정할 수 없습니다. GildedRose클래스는 1개의 updateQuality메서드를 갖고 있습니다. 약 50줄 정도의 길이의 updateQuality는 언뜻 봐도 중첩된 if문과 하드코딩된 문자열, 그리고 다양한 연산들이 섞여있습니다.

코드는 다음과 같습니다.

class GildedRose {
    Item[] items;

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

    public void updateQuality() {
        for (int i = 0; i < items.length; i++) {
            if (!items[i].name.equals("Aged Brie")
                && !items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                if (items[i].quality > 0) {
                    if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
                        items[i].quality = items[i].quality - 1;
                    }
                }
            } else {
                if (items[i].quality < 50) {
                    items[i].quality = items[i].quality + 1;

                    if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                        if (items[i].sellIn < 11) {
                            if (items[i].quality < 50) {
                                items[i].quality = items[i].quality + 1;
                            }
                        }

                        if (items[i].sellIn < 6) {
                            if (items[i].quality < 50) {
                                items[i].quality = items[i].quality + 1;
                            }
                        }
                    }
                }
            }

            if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
                items[i].sellIn = items[i].sellIn - 1;
            }

            if (items[i].sellIn < 0) {
                if (!items[i].name.equals("Aged Brie")) {
                    if (!items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                        if (items[i].quality > 0) {
                            if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
                                items[i].quality = items[i].quality - 1;
                            }
                        }
                    } else {
                        items[i].quality = items[i].quality - items[i].quality;
                    }
                } else {
                    if (items[i].quality < 50) {
                        items[i].quality = items[i].quality + 1;
                    }
                }
            }
        }
    }
}

코드를 이해하지 못한 상태에서 읽어도 정말 많은 수정 사항들이 눈에 띄는 것을 알 수 있습니다.

다음은 제한 사항으로 인해 수정은 할 수 없는 Item클래스의 코드입니다.

public class Item {

    public String name;

    public int sellIn;

    public int quality;

    public Item(String name, int sellIn, int quality) {
        this.name = name;
        this.sellIn = sellIn;
        this.quality = quality;
    }

    @Override
    public String toString() {
        return this.name + ", " + this.sellIn + ", " + this.quality;
    }
}

name, sellIn, quality라는 3개의 필드를 갖고 있습니다. 또한 이 애플리케이션에서는 쓰이지는 않는 toString메서드를 갖고 있습니다.

테스트 코드로는 GildeRoseTest가 있습니다. 초기 세팅으로는 동작하지 않는 테스트가 하나 준비되어 있습니다.

class GildedRoseTest {

    @Test
    void foo() {
        Item[] items = new Item[]{new Item("foo", 0, 0)};
        GildedRose app = new GildedRose(items);
        app.updateQuality();
        assertEquals("fixme", app.items[0].name);
    }

}

요구사항과 제약사항

애플리케이션을 아무렇게나 리팩터링 할 수는 없습니다.

이미 아이템의 유형과 특정 조건들에 의해서 이미 동작하고 있는 애플리케이션이기에 요구사항과 제약사항을 잘 지켜야 합니다.

Gilded Rose 클래스의 updateQuality메서드를 변경하거나 애플리케이션에 코드를 추가할 수 있다는 요구사항이 있습니다.

또한 제약 사항으로는 Item 클래스를 변경하지 않아야하고, Item 클래스에 새로운 property를 추가할 수 없습니다.

리팩터링

1단계 - 이해하기

이제 리팩터링을 시작해보겠습니다.

장황한 updateQuality메서드를 이해하기 위해서 주석을 통해, 메서드가 어떻게 동작하는지 분석합니다.

package com.gildedrose;

class GildedRose {
    Item[] items;

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

    public void updateQuality() {
        for (Item item : items) {
            // "Aged Brie"와 "Backstage passes"가 아닌 경우
            // 품질이 0보다 크면 "Sulfuras, Hand of Ragnaros" 제외 아이템 품질을 1 감소
            if (!item.name.equals("Aged Brie")
                && !item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                if (item.quality > 0) {
                    if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
                        item.quality = item.quality - 1;
                    }
                }
            } else {
                // "Aged Brie" 또는 "Backstage passes"인 경우 품질 50 미만이면 품질 1 증가
                // "Backstage passes"의 경우 품질이 50 미만이고 판매일이 11일 미만인 경우 추가로 1 증가
                // 판매일이 6일 미만인 경우 또한 추가로 1 증가
                if (item.quality < 50) {
                    item.quality = item.quality + 1;

                    if (item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                        if (item.sellIn < 11) {
                            if (item.quality < 50) {
                                item.quality = item.quality + 1;
                            }
                        }

                        if (item.sellIn < 6) {
                            if (item.quality < 50) {
                                item.quality = item.quality + 1;
                            }
                        }
                    }
                }
            }

            // "Sulfuras, Hand of Ragnaros"가 아닌 경우 판매일을 1 줄임
            if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
                item.sellIn = item.sellIn - 1;
            }

            // 판매일이 지난 경우
            // "Aged Brie"가 아니고, "Backstage passes"가 아닌 경우 품질이 0 보다 크면 줄임
            // "Backstage passes"가 아닌 경우 품질을 0으로 변경
            // "Aged Brie"인 경우 품질이 50 미만이면 증가
            if (item.sellIn < 0) {
                if (!item.name.equals("Aged Brie")) {
                    if (!item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
                        if (item.quality > 0) {
                            if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
                                item.quality = item.quality - 1;
                            }
                        }
                    } else {
                        item.quality = item.quality - item.quality;
                    }
                } else {
                    if (item.quality < 50) {
                        item.quality = item.quality + 1;
                    }
                }
            }
        }
    }
}

주석을 바로 작성하기 전에, 기존의 indexed for loop를 enhanced for loop로 변경하였습니다. 그냥.. 참지 못해버렸습니다.

주석을 통해 모든 동작을 완벽하게 이해할 수는 없지만, 어떤 동작을 하는지 대략적으로 이해할 수 있었습니다.

2단계 - 추가 테스트 작성하기

1단계를 통하여 updateQuality메서드가 어떤 동작을 하는지 대략적으로 이해하였습니다. 다음 단계로는 추가적인 테스트를 작성하여, updateQuality메서드가 어떤 동작을 하는지 더욱 자세히
알아보겠습니다.

package com.gildedrose;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

@DisplayName("Gilded Rose 테스트")
class GildedRoseTest {
    private Item[] items;
    private GildedRose app;

    private void setup(String name, int sellIn, int quality) {
        items = new Item[]{new Item(name, sellIn, quality)};
        app = new GildedRose(items);
    }

    private int itemSellIn() {
        return app.items[0].sellIn;
    }

    private int itemQuality() {
        return app.items[0].quality;
    }

    @Nested
    @DisplayName("일반 아이템은")
    class StandardItem {

        @Test
        @DisplayName("하루가 지나면 Quality와 sellIn이 1씩 감소한다")
        void qualityAndSellInDecrease() {
            GildedRoseTest.this.setup("Standard Item", 10, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(19);
            assertThat(itemSellIn()).isEqualTo(9);
        }
    }

    @Nested
    @DisplayName("'Aged Brie'는")
    class AgedBrie {
        @Test
        @DisplayName("하루가 지나면 Quality는 증가하지만 sellIn은 감소한다")
        void qualityIncreasesSellInDecreases() {
            GildedRoseTest.this.setup("Aged Brie", 10, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(21);
            assertThat(itemSellIn()).isEqualTo(9);
        }
    }

    @Nested
    @DisplayName("'Backstage passes'는")
    class BackstagePasses {
        @Test
        @DisplayName("10일 초과 남았을 때 하루가 지나면 품질이 1 씩 증가하고 sellIn은 감소한다")
        void quality1IncreasesSellInDecreasesMoreThan10Days() {
            GildedRoseTest.this.setup("Backstage passes to a TAFKAL80ETC concert", 15, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(21);
            assertThat(itemSellIn()).isEqualTo(14);
        }

        @Test
        @DisplayName("6일에서 10일 남았을 때 하루가 지나면 품질이 2 씩 증가하고 sellIn은 감소한다")
        void quality2IncreasesSellInDecreases6to10Days() {
            GildedRoseTest.this.setup("Backstage passes to a TAFKAL80ETC concert", 10, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(22);
            assertThat(itemSellIn()).isEqualTo(9);
        }

        @Test
        @DisplayName("5일 미만 남았을 때 하루가 지나면 품질이 3 씩 증가하고 sellIn은 감소한다")
        void quality3IncreasesSellInDecreasesLessThan6Days() {
            GildedRoseTest.this.setup("Backstage passes to a TAFKAL80ETC concert", 5, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(23);
            assertThat(itemSellIn()).isEqualTo(4);
        }

        @Test
        @DisplayName("콘서트가 끝나면(즉, sellIn이 0 이하가 되면) 품질은 0이된다.")
        void qualityDropsToZeroAfterTheConcert() {
            GildedRoseTest.this.setup("Backstage passes to a TAFKAL80ETC concert", 0, 20);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(0);
        }
    }

    @Nested
    @DisplayName("'Sulfuras'는")
    class Sulfuras {

        @Test
        @DisplayName("하루가 지나도 품질과 sellIn은 변하지 않는다")
        void qualityAndSellInUnchanged() {
            GildedRoseTest.this.setup("Sulfuras, Hand of Ragnaros", 10, 80);
            app.updateQuality();
            assertThat(itemQuality()).isEqualTo(80);
            assertThat(itemSellIn()).isEqualTo(10);
        }
    }
}

일단 각 아이템들에 따라 품질과 판매일이 어떻게 변하는지 확인해보았습니다.

기계인간 Jhon Grib님의 블로그 글 - JUnit5로 계층 구조의 테스트 코드 작성하기 글을
통하여 Describe - Context - It구조로 테스트 코드를 작성하였습니다.

위와 같은 방법을 이용한 이유는, 각 아이템 종류에 따라 update의 동작이 다르기 때문입니다. 물론 기존의 Junit5 방식으로 테스트를 작성하여도 전혀 문제될 건 없습니다.

위와 같이 작성된 테스트 코드의 실행 결과입니다. 즉, updateQuality메서드가 어떤 동작을 하는지 테스트로 확인할 수 있게 되었습니다.

향후에 리팩터링을 진행하면서, 코드 수정이 일어나더라도 테스트 코드를 통해 동작이 제대로 수행되는지 확인할 수 있습니다.

3단계 - 본격적인 리팩터링 시작

이 단계에서는 가벼운 리팩터링을 진행했습니다.

2단계의 코드에서도 볼 수 있듯이, indexed for loop를 enhanced for loop로 변경하였습니다. 큰 변화는 아니지만, 코드의 가독성과 명확성이 향상됐습니다.

다음으로 중복해서 나타나는 quality = quality + 1과 같은 증분, 감소 연산들을 quality += 1과 같은 연산자로 변경하였습니다.

코드의 가독성 뿐만 아니라, 기존 시스템에서는 1이 증가율이지만, 향후에는 2나 3과 같은 증가율로 변경될 수도 있기 때문에, +=연산자를 사용하여 변화율이 변경될 때에도 유연하게 대처할 수 있도록 하였습니다.

4단계 - 전략 패턴

updateQuality메서드가 아이템마다 다른 동작을 하는 것을 파악하고, 이를 전략 패턴을 이용하여 리팩터링을 진행했습니다.

package com.gildedrose;

public interface ItemUpdateStrategy {
    void update(Item item);
}

위 인터페이스는 아이템을 업데이트하는 전략을 정의한 인터페이스입니다.

이후 각각의 아이템마다 전략을 구현하여 update 메서드를 구현하였습니다.

이 작업 후의 GildedRose클래스는 다음과 같습니다.

package com.gildedrose;

import java.util.HashMap;

class GildedRose {
    public static final String DEFAULT_STRATEGY = "default";
    Item[] items;
    HashMap<String, ItemUpdateStrategy> strategies;

    public GildedRose(Item[] items) {
        this.items = items;
        this.strategies = new HashMap<>();
        this.strategies.put("Aged Brie", new AgedBrieUpdateStrategy());
        this.strategies.put("Backstage passes to a TAFKAL80ETC concert", new BackstagePassesUpdateStrategy());
        this.strategies.put("Sulfuras, Hand of Ragnaros", new SulfurasUpdateStrategy());
        this.strategies.put(DEFAULT_STRATEGY, new StandardItemUpdateStrategy());
    }

    public void updateQuality() {
        for (Item item : items) {
            getItemUpdateStrategy(item).update(item);
        }
    }

    private ItemUpdateStrategy getItemUpdateStrategy(Item item) {
        return strategies.getOrDefault(item.name, strategies.get(DEFAULT_STRATEGY));
    }
}

위 코드를 보면 알 수 있듯, GildedRose클래스는 아이템 이름과 해당 업데이트 전략을 매핑하는 HashMap을 통해 아이템의 업데이트 전략을 가져옵니다.

물론 각 아이템마다의 전략을 구현한 클래스에 기존의 로직들이 그대로
남아있습니다. 소스코드

5단계 - 중복 코드 제거

4단계를 통하여 기존 로직이 각각의 아이템 전략 클래스들로 분리되었습니다. 하지만 여전히 각 코드들을 뜯어보면 item.quality += 1;, item.sellIn -= 1;과 같은 코드들이 많이 중복되어
있습니다.

이를 제거하기 위해 기존 전략 인터페이스인 ItemUpdateStrategy를 구현하는 추상 클래스를 만들었습니다.

package com.gildedrose.strategy;

import com.gildedrose.Item;

public abstract class AbstractItemUpdateStrategy implements ItemUpdateStrategy {
    protected static final int MAX_QUALITY = 50;
    protected static final int MIN_QUALITY = 0;

    protected void incrementQualityIfLessThanMax(Item item) {
        if (item.quality < MAX_QUALITY) {
            item.quality += 1;
        }
    }

    protected void decrementSellIn(Item item) {
        item.sellIn -= 1;
    }
}

위 추상 클래스는 ItemUpdateStrategy를 구현하고, 공통적으로 사용되는 메서드들을 정의하였습니다.

6단계 - Conjured 아이템 추가

이번 단계에서는 Conjured 아이템이 추가되었다는 요구사항을 반영하였습니다.

`Conjured` 아이템은 일반 아이템(`StandardItem`)의 2배의 속도로 품질이 감소합니다.

추상 클래스 AbstractItemUpdateStrategy에서 품질을 감소시키는 메서드를 추가하여, Conjured 아이템과 일반 아이템의 코드 중복을 제거할 수 있었습니다.

7단계 - 팩토리 패턴

6단계처럼 새로운 아이템이 추가될 때마다 GildedRose클래스의 HashMap에 새로운 아이템과 전략을 추가해주어야하는 문제가 있습니다.

이를 해결하기 위해 팩토리 패턴을 이용하여 GildedRose클래스의 코드를 수정하였습니다.

package com.gildedrose;

import com.gildedrose.strategy.*;

import java.util.HashMap;

class GildedRose {
    public static final String DEFAULT_STRATEGY = "default";
    Item[] items;
    HashMap<String, ItemUpdateStrategy> strategies;

    public GildedRose(Item[] items, StrategyFactory strategyFactory) {
        this.items = items;
        this.strategies = strategyFactory.createStrategies();
    }

    public void updateQuality() {
        for (Item item : items) {
            getItemUpdateStrategy(item).update(item);
        }
    }

    private ItemUpdateStrategy getItemUpdateStrategy(Item item) {
        return strategies.getOrDefault(item.name, strategies.get(DEFAULT_STRATEGY));
    }
}

GildedRose클래스의 생성자에서 StrategyFactory를 주입받아 HashMap을 생성하도록 변경하였습니다. 팩토리 클래스는 다음과 같습니다.

package com.gildedrose.strategy;

import java.util.HashMap;

public class StrategyFactory {
    public static final String DEFAULT_STRATEGY = "default";

    public HashMap<String, ItemUpdateStrategy> createStrategies() {
        HashMap<String, ItemUpdateStrategy> strategies = new HashMap<>();
        strategies.put("Aged Brie", new AgedBrieUpdateStrategy());
        strategies.put("Backstage passes to a TAFKAL80ETC concert", new BackstagePassesUpdateStrategy());
        strategies.put("Sulfuras, Hand of Ragnaros", new SulfurasUpdateStrategy());
        strategies.put("Conjured Mana Cake", new ConjuredUpdateStrategy());
        strategies.put(DEFAULT_STRATEGY, new StandardItemUpdateStrategy());
        return strategies;
    }
}

이제 새로운 아이템이 추가될 때마다 StrategyFactory에 전략을 추가해주기만 하면 되게 되었습니다.

위 팩토리 클래스에 대한 테스트 코드는 다음과 같습니다.

package com.gildedrose.strategy;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.util.HashMap;

import static org.assertj.core.api.Assertions.assertThat;

class StrategyFactoryTest {

    StrategyFactory factory;

    @BeforeEach
    void setUp() {
        factory = new StrategyFactory();
    }

    @Test
    @DisplayName("Strategy Factory를 사용하여 생성된 전략이 올바른 클래스인지 테스트한다")
    void testStrategies() {
        HashMap<String, ItemUpdateStrategy> strategies = factory.createStrategies();

        assertThat(strategies.get("Aged Brie")).isInstanceOf(AgedBrieUpdateStrategy.class);
        assertThat(strategies.get("Backstage passes to a TAFKAL80ETC concert")).isInstanceOf(BackstagePassesUpdateStrategy.class);
        assertThat(strategies.get("Sulfuras, Hand of Ragnaros")).isInstanceOf(SulfurasUpdateStrategy.class);
        assertThat(strategies.get("Conjured Mana Cake")).isInstanceOf(ConjuredUpdateStrategy.class);
        assertThat(strategies.get(StrategyFactory.DEFAULT_STRATEGY)).isInstanceOf(StandardItemUpdateStrategy.class);
    }
}

이를 통해, 팩토리 클래스가 올바른 전략 클래스를 생성하는지 확인할 수 있습니다.

결론

여전히 부족한 부분이 많지만, 기존의 updateQuality 메서드가 이름에 걸맞게 아이템의 품질을 업데이트하는 메서드로 변경되었습니다.

또한, 새로운 아이템이 추가될 때마다 GildedRose클래스의 코드를 수정해야하는 문제를 팩토리 패턴을 이용하여 해결하였습니다.

리포지터리에서 전체 코드를 확인할 수 있습니다.