Заметки Романа Теличкина

Рефакторинг от души

Май 2018
Из японских боевых искусств в программирование пришло понятие "ката". Ката – это упражнение, которое помогает отточить навыки при помощи практики и повторений. Типичный программист читает, понимает и изменяет уже имеющийся код намного чаще, чем пишет новый. Чтобы сделать нечитаемое читаемым, непонятное понятным, и неизменяемое изменяемым, применяется рефакторинг. Если члены команды умеют рефакторить, в проект проще добавлять и новые фичи, и новых людей. Поэтому я решил отточить навык рефакторига с помощью соответствующей каты "Glided Rose", вот её код:
class GildedRose:
    @staticmethod
    def update_quality(items):
        for i in range(0, len(items)):
            if "Aged Brie" != items[i].name and "Backstage passes to a TAFKAL80ETC concert" != items[i].name:
                # TODO: Improve this code. Word.
                if items[i].quality > 0:
                    if "Sulfuras, Hand of Ragnaros" != items[i].name:
                        items[i].quality = items[i].quality - 1
            else:
                if items[i].quality < 50:
                    items[i].quality = items[i].quality + 1
                    if "Aged Brie" == items[i].name:
                        if items[i].sell_in < 6:
                            items[i].quality = items[i].quality + 1
                    # Increases the Quality of the stinky cheese if it's 11 days to due date.
                    if "Aged Brie" == items[i].name:
                        if items[i].sell_in < 11:
                            items[i].quality = items[i].quality + 1
                    if "Backstage passes to a TAFKAL80ETC concert" == items[i].name:
                        if items[i].sell_in < 11:
                            # See revision number 2394 on SVN.
                            if items[i].quality < 50:
                                items[i].quality = items[i].quality + 1
                        # Increases the Quality of Backstage Passes if the Quality is 6 or less.
                        if items[i].sell_in < 6:
                            if items[i].quality < 50:
                                items[i].quality = items[i].quality + 1
            if "Sulfuras, Hand of Ragnaros" != items[i].name:
                items[i].sell_in = items[i].sell_in - 1
            if items[i].sell_in < 0:
                if "Aged Brie" != items[i].name:
                    if "Backstage passes to a TAFKAL80ETC concert" != items[i].name:
                        if items[i].quality > 0:
                            if "Sulfuras, Hand of Ragnaros" != items[i].name:
                                items[i].quality = items[i].quality - 1
                    else:
                        # TODO: Fix this.
                        items[i].quality = items[i].quality - items[i].quality
                else:
                    if items[i].quality < 50:
                        items[i].quality = items[i].quality + 1
                    if "Aged Brie" == items[i].name and items[i].sell_in <= 0:
                        items[i].quality = 0
                        # of for.
            if "Sulfuras, Hand of Ragnaros" != items[i].name:
                if items[i].quality > 50:
                    items[i].quality = 50
        return items


class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def __repr__(self):
        return "%s, %s, %s" % (self.name, self.sell_in, self.quality)
Задачей каты является добавление нового вида Item'a – "Conjured". Возможно, у вас возник вопрос: "Нам нужно просто добавить новую фичу, каким боком здесь рефакторинг?". Новая фича в таком коде – это неминуемый костыль. Можно провести ни один час в поисках правильного места для костыля, а можно потратить час на рефакторинг и добавить фичу за несколько минут. Второй вариант сэкономит время и нервы при добавлении следующей фичи, первый – приведет к выпадению волос и язве. Добавляя новую фичу без рефакторинга, вы превращаете ваш код в такую кухню:
Начнем уборку. Сначала сгруппируем расчет для каждого типа Item'a в отдельном статическом методе:
class GildedRose:
    @staticmethod
    def update_quality(items):
        for item in items:
            if item.name == "Aged Brie":
                GildedRose.calc_brie(item)

            elif item.name == "Backstage passes to a TAFKAL80ETC concert":
                GildedRose.calc_backstage(item)

            elif item.name == "Sulfuras, Hand of Ragnaros":
                GildedRose.calc_sulfuras(item)

            else:
                GildedRose.calc_simple(item)

        return items

    @staticmethod
    def calc_simple(item):
        item.sell_in = item.sell_in - 1

        if item.quality > 0:
            item.quality = item.quality - 1

        if item.sell_in < 0:
            if item.quality > 0:
                item.quality = item.quality - 1

        if item.quality > 50:
            item.quality = 50

    @staticmethod
    def calc_brie(item):
        item.sell_in = item.sell_in - 1
        item.quality = item.quality + 1

        if item.quality < 50:
            if item.sell_in < 6:
                item.quality = item.quality + 1
            if item.sell_in < 11:
                item.quality = item.quality + 1

        if item.sell_in <= 0:
            item.quality = 0

        if item.quality > 50:
            item.quality = 50

    @staticmethod
    def calc_backstage(item):
        item.sell_in = item.sell_in - 1
        item.quality = item.quality + 1

        if item.quality < 50:
            if item.sell_in < 6:
                item.quality = item.quality + 1
            if item.sell_in < 11:
                item.quality = item.quality + 1

        if item.sell_in < 0:
            item.quality = 0

        if item.quality > 50:
            item.quality = 50

    @staticmethod
    def calc_sulfuras(item):
        pass


class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def __repr__(self):
        return "%s, %s, %s" % (self.name, self.sell_in, self.quality)
Этот код выглядит лучше исходного. Стоит только добавить еще один статический метод и еще один if, и задача решена. Но на данном этапе, наш код остается процедурным. 
Методы класса GlidedRose работают с Item'ами как со структурой данных. Объекты в ООП – это не структуры данных, объекты – это поведение. Вы перестаете писать процедурный код только тогда, когда перестаете копаться во внутренностях объектов. Перенесем большую часть поведения в Item:
class GildedRose:
    @staticmethod
    def update_quality(items):
        for item in items:
            if item.name == "Aged Brie":
                item.calc_brie()

            elif item.name == "Backstage passes to a TAFKAL80ETC concert":
                item.calc_backstage()

            elif item.name == "Sulfuras, Hand of Ragnaros":
                item.calc_sulfuras()

            else:
                item.calc_simple()

        return items


class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def calc_simple(self):
        self.sell_in = self.sell_in - 1

        if self.quality > 0:
            self.quality = self.quality - 1

        if self.sell_in < 0:
            if self.quality > 0:
                self.quality = self.quality - 1

        if self.quality > 50:
            self.quality = 50

    def calc_brie(self):
        self.sell_in = self.sell_in - 1
        self.quality = self.quality + 1

        if self.quality < 50:
            if self.sell_in < 6:
                self.quality = self.quality + 1
            if self.sell_in < 11:
                self.quality = self.quality + 1

        if self.sell_in <= 0:
            self.quality = 0

        if self.quality > 50:
            self.quality = 50

    def calc_backstage(self):
        self.sell_in = self.sell_in - 1
        self.quality = self.quality + 1

        if self.quality < 50:
            if self.sell_in < 6:
                self.quality = self.quality + 1
            if self.sell_in < 11:
                self.quality = self.quality + 1

        if self.sell_in < 0:
            self.quality = 0

        if self.quality > 50:
            self.quality = 50

    def calc_sulfuras(self):
        pass
Мне не нравится, что объект GlidedRose продолжает принимать решения в зависимости от состояния Item'а. Давайте перенесем диспатч правильного действия внутрь Item'a. А еще удалим метод calc_backstage, он делает тоже самое, что и calc_brie:
class GildedRose:
    @staticmethod
    def update_quality(items):
        for item in items:
            item.update_quality()

        return items


class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def update_quality(self):
        concrete_method = {
            "Aged Brie": self._calc_brie,
            "Backstage passes to a TAFKAL80ETC concert": self._calc_brie,
            "Sulfuras, Hand of Ragnaros": self._calc_sulfuras,
        }.get(self.name, self._calc_simple)

        concrete_method()

    def _calc_simple(self):
        self.sell_in = self.sell_in - 1

        if self.quality > 0:
            self.quality = self.quality - 1

        if self.sell_in < 0:
            if self.quality > 0:
                self.quality = self.quality - 1

        if self.quality > 50:
            self.quality = 50

    def _calc_brie(self):
        self.sell_in = self.sell_in - 1
        self.quality = self.quality + 1

        if self.quality < 50:
            if self.sell_in < 6:
                self.quality = self.quality + 1
            if self.sell_in < 11:
                self.quality = self.quality + 1

        if self.sell_in < 0:
            self.quality = 0

        if self.quality > 50:
            self.quality = 50

    def _calc_sulfuras(self):
        pass
Сделаем еще немного косметических изменений. Избавимся от всех if'ов с помощью вспомогательной функции cond, а вместо проверок на quality будем приводить его к минимальному или максимальному значению в самом конце расчета:
class GildedRose:
    @staticmethod
    def update_quality(items):
        for item in items:
            item.update_quality()

        return items


class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def update_quality(self):
        concrete_method = {
            "Aged Brie": self._calc_brie,
            "Backstage passes to a TAFKAL80ETC concert": self._calc_brie,
            "Sulfuras, Hand of Ragnaros": self._calc_sulfuras,
        }.get(self.name, self._calc_simple)

        concrete_method()

    def _calc_simple(self):
        self.sell_in = self.sell_in - 1

        self.quality = cond(
            (self.sell_in >= 0, self.quality - 1),
            (self.sell_in < 0, self.quality - 2))

        self.quality = max(0, self.quality)

    def _calc_brie(self):
        self.sell_in = self.sell_in - 1

        self.quality = cond(
            (self.sell_in < 0, 0),
            (self.sell_in < 6, self.quality + 3),
            (self.sell_in < 11, self.quality + 2),
            (self.sell_in >= 11, self.quality + 1))

        self.quality = min(50, self.quality)

    def _calc_sulfuras(self):
        pass


def cond(*args):
    for is_true, returnable in args:
        if is_true:
            return returnable
А теперь добавим вишенку на торт – полиморфизм. С помощью полиморфизма мы делегируем ответственность за выбор конкретного алгоритма расчета маленьким объектам. Возможно, звучит сложно, но код выглядит очень просто. Объект Item в данном случае становится фабрикой, чтобы не нарушать интерфейс и не ломать клиентов (например, тесты):
class GildedRose:
    @staticmethod
    def update_quality(items):
        for item in items:
            item.update_quality()

        return items


class Item:
    def __new__(cls, name, sell_in, quality):
        item_class = {
            "Aged Brie": SpecialItem,
            "Backstage passes to a TAFKAL80ETC concert": SpecialItem,
            "Sulfuras, Hand of Ragnaros": ImmutableItem,
        }.get(name, DefaultItem)

        return item_class(sell_in, quality)


class ImmutableItem:
    def __init__(self, sell_in, quality):
        self.sell_in = sell_in
        self.quality = quality

    def update_quality(self):
        pass


class DefaultItem(ImmutableItem):
    def update_quality(self):
        self.sell_in = self.sell_in - 1

        self.quality = cond(
            (self.sell_in >= 0, self.quality - 1),
            (self.sell_in < 0, self.quality - 2))

        self.quality = max(0, self.quality)


class SpecialItem(ImmutableItem):
    def update_quality(self):
        self.sell_in = self.sell_in - 1

        self.quality = cond(
            (self.sell_in < 0, 0),
            (self.sell_in < 6, self.quality + 3),
            (self.sell_in < 11, self.quality + 2),
            (self.sell_in >= 11, self.quality + 1))

        self.quality = min(50, self.quality)


def cond(*args):
    for is_true, returnable in args:
        if is_true:
            return returnable
Этот код объектно-ориентированный, в отличие от изначального процедурного, его можно переиспользовать. Добавление фичи в такой код – почти всегда добавление нового кода, а не изменение старого. И делается это просто!
class ConjuredItem(ImmutableItem):
    def update_quality(self):
        self.sell_in -= 1

        self.quality = cond(
            (self.sell_in >= 0, self.quality - 2),
            (self.sell_in < 0, self.quality - 4))

        self.quality = max(0, self.quality)
Напоследок оставляю слова мудрого программиста, которые нужно повторять каждый раз перед добавлением новой фичи: «for each desired change, make the change easy (warning: this may be hard), then make the easy change»
Открыть комментарии