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

Код на:JavaC#PythonJavaScript

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

Проблем

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

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

static decimal CheckoutTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

static decimal InvoiceTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.05m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Каса: {CheckoutTotal(100, true):F2} €");
Console.WriteLine($"Фактура: {InvoiceTotal(100, true):F2} €");

Изход

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

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

Виж го

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

static decimal CheckoutTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

static decimal InvoiceTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.05m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Каса: {CheckoutTotal(100, true):F2} €");
Console.WriteLine($"Фактура: {InvoiceTotal(100, true):F2} €");

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

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

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

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

Идеята

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

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

static decimal OrderTotal(decimal subtotal, bool vip)
{
    decimal discount = vip ? subtotal * 0.10m : 0;
    decimal delivery = subtotal >= 50 ? 0 : 4.90m;
    return subtotal - discount + delivery;
}

Console.WriteLine($"Каса: {OrderTotal(100, true):F2} €");
Console.WriteLine($"Фактура: {OrderTotal(100, true):F2} €");

Изход

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

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

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

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

Виж го

Дълъг метод

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

static void PlaceOrder(string email, decimal subtotal, string city)
{
    // проверка
    if (!email.Contains("@"))
    {
        Console.WriteLine("Невалиден имейл: " + email);
        return;
    }
    if (subtotal <= 0)
    {
        Console.WriteLine("Празна кошница");
        return;
    }
    // сума
    decimal delivery = city == "София" ? 3.90m : 5.90m;
    if (subtotal >= 50)
    {
        delivery = 0;
    }
    decimal total = subtotal + delivery;
    // известие
    string text = $"Поръчката ти за {total:F2} € е приета.";
    Console.WriteLine("Към " + email + ": " + text);
    // лог
    Console.WriteLine($"[log] поръчка {email} {total:F2}");
}

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, decimal subtotal)
{
    if (!email.Contains("@"))
    {
        return "Невалиден имейл: " + email;
    }
    if (subtotal <= 0)
    {
        return "Празна кошница";
    }
    return null;
}

static decimal DeliveryFee(decimal subtotal, string city)
{
    if (subtotal >= 50)
    {
        return 0;
    }
    return city == "София" ? 3.90m : 5.90m;
}

static void NotifyCustomer(string email, decimal total)
{
    string text = $"Поръчката ти за {total:F2} € е приета.";
    Console.WriteLine("Към " + email + ": " + text);
}

static void Log(string email, decimal total)
{
    Console.WriteLine($"[log] поръчка {email} {total:F2}");
}

static void PlaceOrder(string email, decimal subtotal, string city)
{
    string problem = ProblemWith(email, subtotal);
    if (problem != null)
    {
        Console.WriteLine(problem);
        return;
    }
    decimal 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
{
    public string CustomerName { get; set; }
    public string CustomerEmail { get; set; }
    public string CustomerPhone { get; set; }
    public string DeliveryCity { get; set; }
    public string DeliveryStreet { get; set; }
    public string DeliveryPostcode { get; set; }
    public string CardNumber { get; set; }
    public string CardExpiry { get; set; }
    public decimal Subtotal { get; set; }
    public decimal Discount { get; set; }
    public bool SmsSent { get; set; }
    public bool EmailSent { get; set; }
}

var order = new Order();
order.DeliveryCity = "Варна";
Console.WriteLine("Доставка до " + 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
{
    public Customer Customer { get; set; }
    public Address Delivery { get; set; }
    public Card Card { get; set; }
    public decimal Subtotal { get; set; }
    public decimal Discount { get; set; }
    public bool SmsSent { get; set; }
    public bool EmailSent { get; set; }
}

var order = new Order();
order.Delivery = new Address("Варна", "ул. Морска 5", "9000");
Console.WriteLine("Доставка до " + order.Delivery.City);

Изход

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

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

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

Пробвай

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

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

double total = 0;
for (int i = 0; i < 3; i++)
{
    total += 0.10;
}
Console.WriteLine(total == 0.30);
Console.WriteLine(total);

Отговор

True / 0.3

True / 0.30

False / 0.30000000000000004

Провери

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

Виж го

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

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

decimal total = 0;
for (int i = 0; i < 3; i++)
{
    total += 0.10m;
}
Console.WriteLine(total == 0.30m);
Console.WriteLine(total);

Изход

True
0.30

В C# има decimal, който пази десетичните цифри точно, и затова в курса парите бяха decimal от началото. Литералът е с m: 0.10m. Без m числото е double и не се събира с decimal без изрично преобразуване.

decimal total = 0;
for (int i = 0; i < 3; i++)
{
    total += 0.10;
}
Console.WriteLine(total == 0.30m);
Console.WriteLine(total);

Провери

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

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

Виж го

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

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

using System.Text.RegularExpressions;

record Phone
{
    public string Value { get; }

    public Phone(string raw)
    {
        string digits = Regex.Replace(raw, "[ ()-]", "");
        if (digits.StartsWith("0"))
        {
            digits = "+359" + digits.Substring(1);
        }
        if (!Regex.IsMatch(digits, @"^\+359\d{9}$"))
        {
            throw new ArgumentException("Невалиден телефон: " + raw);
        }
        Value = digits;
    }
}

var a = new Phone("0888 123 456");
var b = new Phone("+359 888 123 456");
Console.WriteLine(a.Value);
Console.WriteLine(a == b ? "същият номер" : "различни номера");
try
{
    new Phone("0888 12");
}
catch (ArgumentException e)
{
    Console.WriteLine(e.Message);
}

Изход

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

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

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

record сравнява по стойност: a == b е истина, когато Value е еднакво.

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

1. if (!Regex.IsMatch(digits, @"^\+359\d{9}$"))↑↓
2. }↑↓
3. throw new ArgumentException("Невалиден телефон: " + raw);↑↓
4. Value = digits;↑↓
5. if (digits.StartsWith("0"))↑↓
6. }↑↓
7. {↑↓
8. {↑↓
9. string digits = Regex.Replace(raw, "[ ()-]", "");↑↓
10. digits = "+359" + digits.Substring(1);↑↓

Провери

Пробвай

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

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

var optedOut = new List<string> { "+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{3 if (optedOut.Contains(phone))4 {5 return;6 }7 Console.WriteLine("SMS към " + phone + ": Поръчката ти е на път.");8}

Провери

Виж го

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

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

static decimal Price(string kind)
{
    if (kind == "куриер")
    {
        return 5.90m;
    }
    else if (kind == "офис")
    {
        return 3.90m;
    }
    else if (kind == "автомат")
    {
        return 2.90m;
    }
    throw new ArgumentException("Непознат вид доставка: " + kind);
}

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

foreach (string kind in new[] { "куриер", "офис", "автомат" })
{
    Console.WriteLine($"{kind}: {Price(kind):F2} €, дни: {Days(kind)}");
}

Изход

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

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

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

record Delivery(decimal Price, int Days);

static readonly Dictionary<string, Delivery> Deliveries = new()
{
    ["куриер"] = new Delivery(5.90m, 1),
    ["офис"] = new Delivery(3.90m, 2),
    ["автомат"] = new Delivery(2.90m, 3),
};

foreach (string kind in new[] { "куриер", "офис", "автомат" })
{
    Delivery d = Deliveries[kind];
    Console.WriteLine($"{kind}: {d.Price:F2} €, дни: {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, decimal amount)
{
    if (channel == "sms")
    {
        string text = $"Платихте {amount:F2} €. Благодарим!";
        Console.WriteLine("SMS към " + contact.Replace(" ", "") + ": " + text);
    }
    else if (channel == "email")
    {
        string text = $"Платихте {amount:F2} €. Благодарим!";
        Console.WriteLine("Имейл към " + contact.Trim().ToLowerInvariant() + ": " + text);
    }
    else if (channel == "viber")
    {
        string text = $"Платихте {amount:F2} €. Благодарим!";
        Console.WriteLine("Viber към " + contact.Replace(" ", "") + ": " + text);
    }
}

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

Изход

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 вместо double

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

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

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

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

Провери

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

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

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