Рефакторинг от души
Май 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»