Миризми в кода (code smells) в Python

Проблем

Касата казва 90 €, фактурата 95 €

Сумата на поръчката се смята на две места: на касата и във фактурата. Преди месец VIP отстъпката стана 10%. Програмистът поправи касата:

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Каса: {checkout_total(100, True):.2f} €")
print(f"Фактура: {invoice_total(100, True):.2f} €")

Каса: 90.00 €
Фактура: 95.00 €

Клиентът плаща 90 €, а във фактурата пише 95 €. Счетоводството вижда разлика от 5 € във всяка VIP поръчка за месец назад. Никой ред тук не е „грешен“ сам по себе си: invoice_total прави точно това, за което е написана. Проблемът е, че едно правило живее на две места.

Виж го

Как се разминават две копия

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


print(f"Каса: {checkout_total(100, True):.2f} €")
print(f"Фактура: {invoice_total(100, True):.2f} €")

Стъпка по стъпка, с думи

invoice_total започна като copy-paste на checkout_total: същите три реда, и двете с 5% VIP отстъпка.
VIP отстъпката става 10%. Бъгът дойде от касата, затова поправката влиза в checkout_total, ред 2.
Никой не помни, че фактурата е копие. Ред 8 остава с 0.05: двете копия вече казват различни неща.
Поръчка за 100 € на VIP клиент минава през касата: 100 − 10 + 0 = 90 €.
Същата поръчка влиза във фактурата: 100 − 5 + 0 = 95 €.
Клиентът плаща 90 €, а фактурата пише 95 €. Едно правило на две места: поправяш едното, другото остава.

В деня на copy-paste двете функции бяха верни. Бъгът се роди по-късно, при поправка, която беше направена честно, но на едното място. Кодът, който е верен днес, но ще роди бъг при следващата промяна, има миризма (code smell). Терминът е на Кент Бек и Мартин Фаулър: миризмата не е грешка, а знак, че нещо до нея скоро ще се развали.

Този урок е за петте най-чести миризми и как да ги подушиш, преди да ти струват 5 € на поръчка.

Идеята

Дублиране: едно правило, едно място

Правилото за сумата става една функция, а касата и фактурата я викат:

def order_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Каса: {order_total(100, True):.2f} €")
print(f"Фактура: {order_total(100, True):.2f} €")

Каса: 90.00 €
Фактура: 90.00 €

Сега няма как да поправиш едното и да забравиш другото: другото го няма.

Как го подушваш. Поправяш бъг и се питаш: „Това копирано ли е някъде?“. Търсиш характерно число или израз (0.05) и го намираш на две места. Две функции с почти еднакви имена (checkout…, invoice…) и почти еднакви тела.

Внимавай. Не всичко еднакво е дублиране. Безплатна доставка от 50 € и връщане до 50 дни са две правила с едно число. Ако ги слееш в една константа, промяната на едното ще смени и другото. Дублиране е едно правило на две места. Много екипи чакат третото копие, преди да изнесат общото: две може да са случайност, три рядко са.

Виж го

Дълъг метод

Функцията приема поръчка и прави всичко:

def place_order(email, subtotal, city):
    # проверка
    if "@" not in email:
        print("Невалиден имейл: " + email)
        return
    if subtotal <= 0:
        print("Празна кошница")
        return
    # сума
    delivery = 3.90 if city == "София" else 5.90
    if subtotal >= 50:
        delivery = 0
    total = subtotal + delivery
    # известие
    text = f"Поръчката ти за {total:.2f} € е приета."
    print("Към " + email + ": " + text)
    # лог
    print(f"[log] поръчка {email} {total:.2f}")

place_order("ana@example.com", 34, "Пловдив")
place_order("ana.example.com", 34, "Пловдив")

Към ana@example.com: Поръчката ти за 39.90 € е приета.
[log] поръчка ana@example.com 39.90
Невалиден имейл: ana.example.com

Как го подушваш: скролваш, за да видиш края; коментари като заглавия на части („проверка“, „сума“); не можеш да кажеш какво прави, без да кажеш „и“; променлива, обявена горе, се ползва чак долу. Всяко заглавие-коментар е име на функция, която чака (виж урока Коментари, които лъжат).

Всяка част отива в своя функция с името от коментара:

def problem_with(email, subtotal):
    if "@" not in email:
        return "Невалиден имейл: " + email
    if subtotal <= 0:
        return "Празна кошница"
    return None


def delivery_fee(subtotal, city):
    if subtotal >= 50:
        return 0
    return 3.90 if city == "София" else 5.90


def notify_customer(email, total):
    text = f"Поръчката ти за {total:.2f} € е приета."
    print("Към " + email + ": " + text)


def log(email, total):
    print(f"[log] поръчка {email} {total:.2f}")

def place_order(email, subtotal, city):
    problem = problem_with(email, subtotal)
    if problem is not None:
        print(problem)
        return
    total = subtotal + delivery_fee(subtotal, city)
    notify_customer(email, total)
    log(email, total)

place_order("ana@example.com", 34, "Пловдив")
place_order("ana.example.com", 34, "Пловдив")

Към ana@example.com: Поръчката ти за 39.90 € е приета.
[log] поръчка ana@example.com 39.90
Невалиден имейл: ana.example.com

Изходът е буква по буква същият. Основната функция вече се чете като списък със стъпки, а таксата за доставка може да се тества сама. Тази стъпка се казва Extract Method и ще я правиш на малки стъпки в урока за refactoring.

Виж го

Голям клас

Поръчката е събрала всичко, което някога е трябвало на някого:

@dataclass
class Order:
    customer_name: str = ""
    customer_email: str = ""
    customer_phone: str = ""
    delivery_city: str = ""
    delivery_street: str = ""
    delivery_postcode: str = ""
    card_number: str = ""
    card_expiry: str = ""
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery_city = "Варна"
print("Доставка до " + order.delivery_city)

Доставка до Варна

Как го подушваш: групи полета с общо начало (customer…, delivery…, card…); методи, които ползват само едната група; име като OrderManager или Utils, което не казва нищо; всяка задача в екипа пипа този файл, и конфликтите при сливане в git са все в него.

Всяка група с общо начало е тип, който чака име:

@dataclass
class Customer:
    name: str
    email: str
    phone: str


@dataclass
class Address:
    city: str
    street: str
    postcode: str


@dataclass
class Card:
    number: str
    expiry: str


@dataclass
class Order:
    customer: Customer | None = None
    delivery: Address | None = None
    card: Card | None = None
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery = Address("Варна", "ул. Морска 5", "9000")
print("Доставка до " + order.delivery.city)

Доставка до Варна

@dataclass пише __init__, __repr__ и __eq__ от полетата. Адресът вече е една стойност: подава се, сравнява се и се проверява на едно място. Address | None казва, че адрес може и да няма.

Останалото (smsSent, emailSent) е следващата група. Не е нужно да оправиш всичко наведнъж.

Пробвай

Предвиди изхода

Три продукта по 0,10 €. Какво печата програмата?

total = 0
for _ in range(3):
    total += 0.10
print(total == 0.30)
print(total)

Отговор

False / 0.30000000000000004

True / 0.30

True / 0.3

0.10 няма точен двоичен запис, точно както 1/3 няма точен десетичен. double пази най-близкото двоично число, и малките грешки се трупат.

Виж го

Примитиви вместо типове: парите

Парите в double са най-честата форма на миризмата примитиви вместо типове (primitive obsession): стойност със свои правила живее в тип, който тези правила не знае. Досега в курса сумите бяха float, за да е кратко. В истински код:

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal("0.10")
print(total == Decimal("0.30"))
print(total)

True
0.30

Decimal от модула decimal пази десетичните цифри точно. Създава се от низ: Decimal(0.10) взима вече сгрешения float и пази грешката.

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal()
print(total == Decimal("0.30"))
print(total)

От низ: Decimal("0.10"). От float грешката вече е вътре.

По-точно, за любопитните

Още по-добре е тип Money, който пази сумата и валутата заедно и не позволява да събереш евро с долари по грешка. Това е същата идея като телефона на следващата карта.

Виж го

Примитиви вместо типове: телефонът

Телефонът като низ е друга форма на същата миризма. „0888 123 456“, „+359 888 123 456“ и „+359888123456“ са един номер, но три различни низа. Всяко място, което сравнява телефони, трябва само да помни как се чистят. Типът Phone чисти и проверява веднъж, когато се създава:

import re
from dataclasses import dataclass


@dataclass(frozen=True)
class Phone:
    value: str

    def __post_init__(self):
        digits = re.sub(r"[ ()-]", "", self.value)
        if digits.startswith("0"):
            digits = "+359" + digits[1:]
        if not re.fullmatch(r"\+359\d{9}", digits):
            raise ValueError("Невалиден телефон: " + self.value)
        object.__setattr__(self, "value", digits)

a = Phone("0888 123 456")
b = Phone("+359 888 123 456")
print(a.value)
print("същият номер" if a == b else "различни номера")
try:
    Phone("0888 12")
except ValueError as e:
    print(e)

+359888123456
същият номер
Невалиден телефон: 0888 12

Регулярният израз [ ()-] маха интервалите, скобите и тиретата, а \+359\d{9} проверява, че след +359 има точно девет цифри.

Невалиден телефон не може да съществува: конструкторът не го пуска. Всеки Phone в програмата вече е чист.

frozen=True забранява промяната след създаването. Затова чистият номер се записва с object.__setattr__ в __post_init__, единственото място, където това е допустимо. == идва от @dataclass.

Подреди редовете на конструктора така, че "0888 123 456" да стане "+359888123456":

raise ValueError("Невалиден телефон: " + self.value)
digits = re.sub(r"[ ()-]", "", self.value)
digits = "+359" + digits[1:]
if digits.startswith("0"):
object.__setattr__(self, "value", digits)
if not re.fullmatch(r"\+359\d{9}", digits):

Пробвай

Намери грешката

Клиентът се е отписал от SMS-ите, а магазинът му праща известие за всяка пратка:

opted_out = ["+359888123456"]
notify_shipped("0888 123 456", opted_out)
notify_shipped("0877 555 010", opted_out)

SMS към 0888 123 456: Поръчката ти е на път.
SMS към 0877 555 010: Поръчката ти е на път.

На кой ред е грешката?

Избери реда с грешката.

Проверката сравнява два низа: "0888 123 456" и "+359888123456". За човек това е един номер, за програмата са два различни текста, и проверката казва „не е отписан“. С типа Phone и двата номера се чистят при създаването и проверката сравнява едно и също:

def notify_shipped(phone, opted_out):
    if phone in opted_out:
        return
    print("SMS към " + phone.value + ": Поръчката ти е на път.")

opted_out = [Phone("+359888123456")]
notify_shipped(Phone("0888 123 456"), opted_out)
notify_shipped(Phone("0877 555 010"), opted_out)

SMS към +359877555010: Поръчката ти е на път.

Виж го

Дълга верига от if по вид

Цената и срокът на доставката зависят от вида. Веригата е в двете функции:

def price(kind):
    if kind == "куриер":
        return 5.90
    elif kind == "офис":
        return 3.90
    elif kind == "автомат":
        return 2.90
    raise ValueError("Непознат вид доставка: " + kind)


def days(kind):
    if kind == "куриер":
        return 1
    elif kind == "офис":
        return 2
    elif kind == "автомат":
        return 3
    raise ValueError("Непознат вид доставка: " + kind)

for kind in ["куриер", "офис", "автомат"]:
    print(f"{kind}: {price(kind):.2f} €, дни: {days(kind)}")

куриер: 5.90 €, дни: 1
офис: 3.90 €, дни: 2
автомат: 2.90 €, дни: 3

Как го подушваш: една и съща верига по едно и също поле (kind, type, status) на няколко места; нов вид значи да намериш всяка верига; забравеният клон се вижда чак когато клиент го избере.

Тук видовете се различават само по числа. Тогава веригата става таблица: вид → данни:

@dataclass(frozen=True)
class Delivery:
    price: float
    days: int


DELIVERY = {
    "куриер": Delivery(5.90, 1),
    "офис": Delivery(3.90, 2),
    "автомат": Delivery(2.90, 3),
}

for kind in ["куриер", "офис", "автомат"]:
    d = DELIVERY[kind]
    print(f"{kind}: {d.price:.2f} €, дни: {d.days}")

куриер: 5.90 €, дни: 1
офис: 3.90 €, дни: 2
автомат: 2.90 €, дни: 3

Нов вид е нов ред в таблицата. Когато видовете се различават по поведение, а не само по числа, всеки вид става свой клас или функция: това е Strategy. Ако видът се сменя с времето (поръчката е нова, после платена, после изпратена), това е State. Как една верига се превръща стъпка по стъпка в класове (Replace Conditional with Polymorphism), ще видиш в урока за refactoring.

Идеята

Миризмата не е бъг: кога да я оставиш

Петте миризми накратко:

дублиране: едно правило на няколко места → една функция или константа;
дълъг метод: скролваш и четеш коментари-заглавия → Extract Method;
голям клас: групи полета с общо начало → по клас за всяка група;
примитиви вместо типове: пари в double, телефон в низ → тип, който пази правилата си;
дълга верига if по вид: същата верига на много места → таблица, Strategy или State.

Миризмата не е бъг. Тя казва колко ще струва следващата промяна. Затова я оставяш, когато:

кодът не се сменя. Отчетът, който никой не е пипал от три години, не струва нищо, докато не го пипнеш;
няма тестове. Без тях, оправяйки миризмата, можеш да счупиш нещо, което работеше. Първо тестовете (следващият урок), после малките стъпки;
две неща само изглеждат еднакви. Две правила с едно число не са дублиране;
кодът е за веднъж: скрипт за миграция, който ще изтриеш утре.

Правилото на скаута: когато така или иначе пипаш файла, остави го малко по-чист, отколкото си го намерил. Не пренаписвай цели модули „за всеки случай“.

Упражнение

Подуши и оправи

Реши задачата в редактора си.

Разписката. Функцията праща разписка по SMS, имейл или Viber:

def send_receipt(channel, contact, amount):
    if channel == "sms":
        text = f"Платихте {amount:.2f} €. Благодарим!"
        print("SMS към " + contact.replace(" ", "") + ": " + text)
    elif channel == "email":
        text = f"Платихте {amount:.2f} €. Благодарим!"
        print("Имейл към " + contact.strip().lower() + ": " + text)
    elif channel == "viber":
        text = f"Платихте {amount:.2f} €. Благодарим!"
        print("Viber към " + contact.replace(" ", "") + ": " + text)

send_receipt("sms", "0888 123 456", 24.5)
send_receipt("email", " Ana@Example.com ", 24.5)
send_receipt("viber", "0877 555 010", 9.9)

SMS към 0888123456: Платихте 24.50 €. Благодарим!
Имейл към ana@example.com: Платихте 24.50 €. Благодарим!
Viber към 0877555010: Платихте 9.90 €. Благодарим!

Намери миризмите в нея (поне три) и ги назови. После ги оправи една по една, като пускаш програмата след всяка стъпка: изходът трябва да остане буква по буква същият.

Още задачи

Нов канал. Добави Telegram с контакт като SMS-а. На колко места пипаш преди поправката и на колко след нея?
Пари в центове. Смени сумата на разписката да е в цели центове (2450), а в евро да се превръща само при печатане.
Голямият клас. Довърши Order от картата „Голям клас“: изнеси smsSent и emailSent в своя тип и помисли как да се казва.

Тест

Провери се

1. Кодът има миризма. Значи ли това, че има бъг?Не. Кодът може да работи днес; миризмата казва, че следващата промяна е рискованаДа, ако компилаторът дава предупреждениеДа, миризмата е бъг с друго име

2. Безплатната доставка е от 50 €, а връщането на стока е до 50 дни. Да ги направиш ли една константа, за да няма дублиране?Да, но с име FIFTYДа, едно число трябва да е на едно мястоНе. Това са две различни правила, които случайно имат еднакво число

3. Сумите се трупат в double и 0.10 три пъти не дава 0.30. Какво помага?По-голям тип: float вместо doubleЗакръгляне при всяко печатанеDecimal, създаден от низ

4. Една и съща верига от if по вида доставка има в цената, в срока и в етикета. Идва нов вид. Какво помага?Таблица вид → данни, или по клас за всеки вид, ако поведението е различноОще един else if на трите местаКоментар над всяка верига кои са видовете

5. Модул за годишния отчет: 300 реда в един метод, никой не го е пипал от три години, няма тестове. Утре трябва да пуснеш друго. Какво правиш?Изтриваш гоОставяш го. Ще го оправиш, когато се наложи да го пипаш, и първо ще му напишеш тестовеРазделяш го на функции още днес

Миризмите: дълъг метод, голям клас, дублиране, примитиви вместо типове, дълга верига if

Бисквитки