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

Код на:JavaC#PythonJavaScript

Накратко: Миризмата в кода (code smell) е признак, че следващата промяна ще роди бъг: дълъг метод, голям клас, дублиране, примитиви вместо типове и дълга верига от if.

Проблем

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

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

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Каса: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Фактура: %.2f €%n", invoiceTotal(100, true));

Изход

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

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

Виж го

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

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Каса: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Фактура: %.2f €%n", invoiceTotal(100, true));

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

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

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

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

Идеята

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

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

static double orderTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Каса: %.2f €%n", orderTotal(100, true));
System.out.printf("Фактура: %.2f €%n", orderTotal(100, true));

Изход

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

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

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

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

Виж го

Дълъг метод

Методът приема поръчка и прави всичко:

static void placeOrder(String email, double subtotal, String city) {
    // проверка
    if (!email.contains("@")) {
        System.out.println("Невалиден имейл: " + email);
        return;
    }
    if (subtotal <= 0) {
        System.out.println("Празна кошница");
        return;
    }
    // сума
    double delivery = city.equals("София") ? 3.90 : 5.90;
    if (subtotal >= 50) {
        delivery = 0;
    }
    double total = subtotal + delivery;
    // известие
    String text = String.format("Поръчката ти за %.2f € е приета.", total);
    System.out.println("Към " + email + ": " + text);
    // лог
    System.out.printf("[log] поръчка %s %.2f%n", email, total);
}

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

Изход

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

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

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

static String problemWith(String email, double subtotal) {
    if (!email.contains("@")) {
        return "Невалиден имейл: " + email;
    }
    if (subtotal <= 0) {
        return "Празна кошница";
    }
    return null;
}

static double deliveryFee(double subtotal, String city) {
    if (subtotal >= 50) {
        return 0;
    }
    return city.equals("София") ? 3.90 : 5.90;
}

static void notifyCustomer(String email, double total) {
    String text = String.format("Поръчката ти за %.2f € е приета.", total);
    System.out.println("Към " + email + ": " + text);
}

static void log(String email, double total) {
    System.out.printf("[log] поръчка %s %.2f%n", email, total);
}

static void placeOrder(String email, double subtotal, String city) {
    String problem = problemWith(email, subtotal);
    if (problem != null) {
        System.out.println(problem);
        return;
    }
    double total = subtotal + deliveryFee(subtotal, city);
    notifyCustomer(email, total);
    log(email, total);
}

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

Изход

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

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

Виж го

Голям клас

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

class Order {
    String customerName;
    String customerEmail;
    String customerPhone;
    String deliveryCity;
    String deliveryStreet;
    String deliveryPostcode;
    String cardNumber;
    String cardExpiry;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.deliveryCity = "Варна";
System.out.println("Доставка до " + order.deliveryCity);

Изход

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

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

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

record Customer(String name, String email, String phone) {}

record Address(String city, String street, String postcode) {}

record Card(String number, String expiry) {}

class Order {
    Customer customer;
    Address delivery;
    Card card;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

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

Изход

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

record пише полетата, конструктора и методите за четене. Адресът вече е една стойност: подава се, сравнява се и се проверява на едно място.

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

Пробвай

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

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

double total = 0;
for (int i = 0; i < 3; i++) {
    total += 0.10;
}
System.out.println(total == 0.30);
System.out.println(total);

Отговор

true / 0.3

false / 0.30000000000000004

true / 0.30

Провери

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

Виж го

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

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

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal("0.10"));
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Изход

true
0.30

BigDecimal пази десетичните цифри точно. Създава се от низ: new BigDecimal(0.10) взима вече сгрешения double и пази грешката. Сравнява се с compareTo, защото equals смята 0.30 и 0.3 за различни.

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal());
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Провери

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

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

Виж го

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

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

record Phone(String value) {
    Phone {
        String digits = value.replaceAll("[ ()-]", "");
        if (digits.startsWith("0")) {
            digits = "+359" + digits.substring(1);
        }
        if (!digits.matches("\\+359\\d{9}")) {
            throw new IllegalArgumentException("Невалиден телефон: " + value);
        }
        value = digits;
    }
}

Phone a = new Phone("0888 123 456");
Phone b = new Phone("+359 888 123 456");
System.out.println(a.value());
System.out.println(a.equals(b) ? "същият номер" : "различни номера");
try {
    new Phone("0888 12");
} catch (IllegalArgumentException e) {
    System.out.println(e.getMessage());
}

Изход

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

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

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

Компактният конструктор на record получава value преди да се запише и може да го смени. equals идва от record и сравнява чистите стойности.

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

1. }↑↓
2. value = digits;↑↓
3. if (!digits.matches("\\+359\\d{9}")) {↑↓
4. if (digits.startsWith("0")) {↑↓
5. digits = "+359" + digits.substring(1);↑↓
6. throw new IllegalArgumentException("Невалиден телефон: " + value);↑↓
7. }↑↓
8. String digits = value.replaceAll("[ ()-]", "");↑↓

Провери

Пробвай

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

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

List<String> optedOut = List.of("+359888123456");
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

Изход

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

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

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

1static void notifyShipped(String phone, List<String> optedOut) {2 if (optedOut.contains(phone)) {3 return;4 }5 System.out.println("SMS към " + phone + ": Поръчката ти е на път.");6}

Провери

Виж го

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

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

static double price(String kind) {
    if (kind.equals("куриер")) {
        return 5.90;
    } else if (kind.equals("офис")) {
        return 3.90;
    } else if (kind.equals("автомат")) {
        return 2.90;
    }
    throw new IllegalArgumentException("Непознат вид доставка: " + kind);
}

static int days(String kind) {
    if (kind.equals("куриер")) {
        return 1;
    } else if (kind.equals("офис")) {
        return 2;
    } else if (kind.equals("автомат")) {
        return 3;
    }
    throw new IllegalArgumentException("Непознат вид доставка: " + kind);
}

for (String kind : List.of("куриер", "офис", "автомат")) {
    System.out.printf("%s: %.2f €, дни: %d%n", kind, price(kind), days(kind));
}

Изход

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

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

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

record Delivery(double price, int days) {}

static final Map<String, Delivery> DELIVERY = Map.of(
    "куриер", new Delivery(5.90, 1),
    "офис", new Delivery(3.90, 2),
    "автомат", new Delivery(2.90, 3));

for (String kind : List.of("куриер", "офис", "автомат")) {
    Delivery d = DELIVERY.get(kind);
    System.out.printf("%s: %.2f €, дни: %d%n", kind, d.price(), 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:

static void sendReceipt(String channel, String contact, double amount) {
    if (channel.equals("sms")) {
        String text = String.format("Платихте %.2f €. Благодарим!", amount);
        System.out.println("SMS към " + contact.replace(" ", "") + ": " + text);
    } else if (channel.equals("email")) {
        String text = String.format("Платихте %.2f €. Благодарим!", amount);
        System.out.println("Имейл към " + contact.trim().toLowerCase() + ": " + text);
    } else if (channel.equals("viber")) {
        String text = String.format("Платихте %.2f €. Благодарим!", amount);
        System.out.println("Viber към " + contact.replace(" ", "") + ": " + text);
    }
}

sendReceipt("sms", "0888 123 456", 24.5);
sendReceipt("email", " Ana@Example.com ", 24.5);
sendReceipt("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Закръгляне при всяко печатанеBigDecimal, създаден от низ

4. Една и съща верига от if по вида доставка има в цената, в срока и в етикета. Идва нов вид. Какво помага?

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

5. Модул за годишния отчет: 300 реда в един метод, никой не го е пипал от три години, няма тестове. Утре трябва да пуснеш друго. Какво правиш?

Изтриваш гоОставяш го. Ще го оправиш, когато се наложи да го пипаш, и първо ще му напишеш тестовеРазделяш го на функции още днес

Провери

Започни урока отначало

Свързани уроци

• Нужно отпреди: State · Имена в кода · Функции: една работа, малко параметри, без скрити странични ефекти · Коментари, които лъжат, и такива, които помагат
• Продължава в: Тестовете като предпазна мрежа · Refactoring на малки стъпки · SOLID без догма · Упражнение: разхвърлян клас, изчистен стъпка по стъпка
• Задачи по темата: Чист код и refactoring