Refactoring на малки стъпки

Код на:JavaC#PythonJavaScript

Накратко: Refactoring е промяна на структурата на кода, без да се сменя поведението му, на малки назовани стъпки като Rename и Extract Method, със зелени тестове след всяка.

Проблем

Фактурата, която никой не иска да пипне

Онлайн магазинът печата фактури с метода Invoices.Inv. Работи от две години:

public static string Inv(string n, string street, string city, string zip, List<Item> items, string type)
{
    decimal t = 0;
    foreach (Item i in items)
    {
        t = t + i.Price * i.Qty;
    }
    decimal d = 0;
    if (type == "gold")
    {
        d = t * 0.05m;
    }
    else if (type == "company")
    {
        if (t >= 500)
        {
            d = t * 0.10m;
        }
    }
    string s = "Фактура: " + n + "\n";
    s = s + street + ", " + zip + " " + city + "\n";
    s = s + $"Сума: {t:F2} €\n";
    s = s + $"Отстъпка: {d:F2} €\n";
    s = s + $"Общо с ДДС: {AfterDiscount(t, d) * 1.20m:F2} €";
    return s;
}

static decimal AfterDiscount(decimal t, decimal d)
{
    return t - d;
}

Какво е t? А d? Шест параметъра подред, четири от тях текст: който размени града и пощенския код, получава фактура с „1504 София“ на грешното място, и компилаторът не казва нищо. Веригата от if по вида клиент расте: следващия месец идват партньори с 3%. А AfterDiscount прави едно изваждане, но те кара да скочиш на друго място, за да го видиш.

Искаш да я подредиш. Но фактурите отиват при счетоводството, и грешка в тях е грешка в данъците.

Идеята

Refactoring: формата се сменя, поведението не

Refactoring е промяна на формата на кода, при която поведението остава същото: същите входове дават същите изходи. Не е „пренаписване“. Не е и „оправих и една грешка, докато бях там“. Това са други промени, които се правят отделно.

Прави се на малки стъпки с имена. Всяка стъпка е толкова малка, че почти не може да се обърка, и след всяка пускаш тестовете (предпазната мрежа от предишния урок). Имената са от каталога на Мартин Фаулър и са едни и същи във всеки език и всяко IDE: Rename, Extract Method, Inline, Introduce Parameter Object, Replace Conditional with Polymorphism.

Първо мрежата. Четири теста заковават четирите вида фактура, които магазинът печата днес: обикновен клиент, златен клиент, фирма над и под 500 €.

public class InvoiceTest
{
    List<Item> coffee = [new("Кафе на зърна", 12.50m, 2), new("Чаша", 8.00m, 1)];
    List<Item> moreCoffee = [new("Кафе на зърна", 12.50m, 4)];
    List<Item> flour = [new("Брашно 25 кг", 30.00m, 20)];

    [Fact]
    public void Regular()
    {
        Assert.Equal("Фактура: Мария Петрова\nул. Шипка 5, 1504 София\nСума: 33.00 €\nОтстъпка: 0.00 €\nОбщо с ДДС: 39.60 €",
            Invoices.Inv("Мария Петрова", "ул. Шипка 5", "София", "1504", coffee, "regular"));
    }

    [Fact]
    public void Gold()
    {
        Assert.Equal("Фактура: Иван Георгиев\nул. Васил Левски 12, 4000 Пловдив\nСума: 50.00 €\nОтстъпка: 2.50 €\nОбщо с ДДС: 57.00 €",
            Invoices.Inv("Иван Георгиев", "ул. Васил Левски 12", "Пловдив", "4000", moreCoffee, "gold"));
    }

    [Fact]
    public void CompanyFrom500()
    {
        Assert.Equal("Фактура: Пекарна Изгрев ООД\nбул. Княз Борис I 80, 9000 Варна\nСума: 600.00 €\nОтстъпка: 60.00 €\nОбщо с ДДС: 648.00 €",
            Invoices.Inv("Пекарна Изгрев ООД", "бул. Княз Борис I 80", "Варна", "9000", flour, "company"));
    }

    [Fact]
    public void CompanyUnder500()
    {
        Assert.Equal("Фактура: Пекарна Изгрев ООД\nбул. Княз Борис I 80, 9000 Варна\nСума: 33.00 €\nОтстъпка: 0.00 €\nОбщо с ДДС: 39.60 €",
            Invoices.Inv("Пекарна Изгрев ООД", "бул. Княз Борис I 80", "Варна", "9000", coffee, "company"));
    }
}

Резултат

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Виж го

Стъпка 1: Rename

Имената от една буква стават думи: t е subtotal, d е discount, s е text, Inv е Invoice. zip става postcode, а type става customerType.

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = 0;
    foreach (Item item in items)
    {
        subtotal = subtotal + item.Price * item.Qty;
    }
    decimal discount = 0;
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
    string text = "Фактура: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {AfterDiscount(subtotal, discount) * 1.20m:F2} €";
    return text;
}

Тестовете

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Rename сменя и всяко място, което вика метода, включително тестовете. Очакваните фактури не се пипат.

Във Visual Studio е Ctrl+R, Ctrl+R (или F2), в Rider е Shift+F6. Компилаторът знае кой Inv е кой, затова се сменя точно той, а не всеки текст Inv.

Виж го

Стъпка 2: Extract Method

В метода има две отделни работи, които нямат нищо общо с текста на фактурата: сумата на редовете и отстъпката. Всяка отива в своя метод с име, което казва какво връща. Това е Extract Method:

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Фактура: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {AfterDiscount(subtotal, discount) * 1.20m:F2} €";
    return text;
}

static decimal ItemsTotal(List<Item> items)
{
    decimal sum = 0;
    foreach (Item item in items)
    {
        sum = sum + item.Price * item.Qty;
    }
    return sum;
}

static decimal DiscountFor(string customerType, decimal subtotal)
{
    decimal discount = 0;
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
    return discount;
}

Тестовете

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Сега Invoice се чете отгоре надолу като списък: колко е, колко е отстъпката, какъв е текстът. Подробностите са един скок надолу, само ако ти трябват.

Във Visual Studio: маркираш редовете и Ctrl+R, Ctrl+M (или Ctrl+. и „Extract method“). В Rider: Ctrl+Alt+M.

IDE-то гледа кои променливи влизат в маркираните редове и кои излизат, и прави параметрите и върнатата стойност. Ако правиш стъпката на ръка, точно там се бърка.

Пробвай

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

Някой прави Extract Method на ръка. Отстъпката отива в метод, който я записва в свой параметър, вместо да я върне:

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = 0;
    ApplyDiscount(customerType, subtotal, discount);
    string text = "Фактура: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {AfterDiscount(subtotal, discount) * 1.20m:F2} €";
    return text;
}

static void ApplyDiscount(string customerType, decimal subtotal, decimal discount)
{
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
}

Отговор

Минават и четирите

Падат Gold и CompanyFrom500

Пада само CompanyFrom500

Провери

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

Резултат

ОК    CompanyUnder500
ОК    Regular
ПАДА  CompanyFrom500
ПАДА  Gold
Тестове: 4, минали: 2, паднали: 2

discount вътре в ApplyDiscount е копие на стойността. Новата стойност остава там и изчезва, когато методът свърши, а отвън discount е пак 0. Където отстъпката и без това е 0 (обикновен клиент, фирма под 500 €), тестовете минават. Затова мрежата има тест за всеки клон на if. Извлеченото трябва да върне стойността, както прави IDE-то.

Виж го

Стъпка 3: Inline

Обратното на Extract Method. AfterDiscount прави едно изваждане и името му не казва нищо повече от subtotal - discount. Ползва се на едно място. Тялото отива там, където е извикването, а методът изчезва. Това е Inline:

static decimal AfterDiscount(decimal subtotal, decimal discount)
{
    return subtotal - discount;
}

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Фактура: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {(subtotal - discount) * 1.20m:F2} €";
    return text;
}

Тестовете

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Extract Method и Inline не си противоречат. Изваждаш, когато името обяснява нещо, което кодът не казва (ItemsTotal). Вкарваш обратно, когато името е само скок (AfterDiscount). В Rider е Ctrl+Alt+N, във Visual Studio е Ctrl+. и „Inline method“.

Виж го

Стъпка 4: Introduce Parameter Object

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

Invoices.Invoice("Мария Петрова", "ул. Шипка 5", "1504", "София", coffee, "regular")

Изход

Фактура: Мария Петрова
ул. Шипка 5, София 1504
Сума: 33.00 €
Отстъпка: 0.00 €
Общо с ДДС: 39.60 €

Четирите стават един обект: клиентът. Това е Introduce Parameter Object.

record пише свойствата и конструктора:

public record Customer(string Name, string Street, string City, string Postcode);

public static string Invoice(Customer customer, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Фактура: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {(subtotal - discount) * 1.20m:F2} €";
    return text;
}

Тестовете

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Извикванията в тестовете се сменят с формата, а очакваните фактури остават същите. Ето едно извикване през стъпките:

Invoices.Inv("Иван Георгиев", "ул. Васил Левски 12", "Пловдив", "4000", moreCoffee, "gold")
Invoices.Invoice("Иван Георгиев", "ул. Васил Левски 12", "Пловдив", "4000", moreCoffee, "gold")
Invoices.Invoice(new Customer("Иван Георгиев", "ул. Васил Левски 12", "Пловдив", "4000"), moreCoffee, "gold")
Invoices.Invoice(new Customer("Иван Георгиев", "ул. Васил Левски 12", "Пловдив", "4000"), moreCoffee, new Gold())

В C# конструкторът може да се вика и с имена: new Customer(Name: "...", City: "...", ...). Тогава редът не е важен.

Виж го

Стъпка 5: Replace Conditional with Polymorphism

Остава веригата от if по вида клиент в DiscountFor. Всеки нов вид е нов клон тук, а утре и на други места (имейлът, отчетът). Всеки вид става клас, който имплементира общ интерфейс, и знае своята отстъпка:

public interface ICustomerType
{
    decimal Discount(decimal subtotal);
}

public class Regular : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return 0;
    }
}

public class Gold : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return subtotal * 0.05m;
    }
}

public class Company : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        if (subtotal >= 500)
        {
            return subtotal * 0.10m;
        }
        return 0;
    }
}

public static string Invoice(Customer customer, List<Item> items, ICustomerType customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = customerType.Discount(subtotal);
    string text = "Фактура: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {(subtotal - discount) * 1.20m:F2} €";
    return text;
}

Тестовете

ОК    CompanyFrom500
ОК    CompanyUnder500
ОК    Gold
ОК    Regular
Тестове: 4, минали: 4, паднали: 0

Това е Strategy от модула за шаблоните: правилото е отделен обект, а фактурата само пита. Ако видът на клиента се сменя по време на работа (става златен след десетата поръчка), същият ход води до State. Текстът "gold" от базата се превръща в обект на едно място, на входа, с таблица като в урока Strategy.

public record Customer(string Name, string Street, string City, string Postcode);

public interface ICustomerType
{
    decimal Discount(decimal subtotal);
}

public static string Invoice(Customer customer, List<Item> items, ICustomerType customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = customerType.Discount(subtotal);
    string text = "Фактура: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Сума: {subtotal:F2} €\n";
    text = text + $"Отстъпка: {discount:F2} €\n";
    text = text + $"Общо с ДДС: {(subtotal - discount) * 1.20m:F2} €";
    return text;
}

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

1. Функцията преди първата стъпка: 23 реда в едно. Долу са четирите теста и сумите на четирите фактури.
2. Rename: t става subtotal, d става discount, s става text. Тестовете са зелени, сумите са същите.
3. Extract Method: цикълът отива в ItemsTotal, веригата от if в DiscountFor. На тяхно място остава по един ред.
4. Inline: AfterDiscount само изваждаше. Тялото му се връща в реда, а функцията изчезва.
5. Introduce Parameter Object: име, улица, град и пощенски код стават един клиент.
6. Replace Conditional with Polymorphism: if-ът изчезва. Всеки вид клиент е клас зад ICustomerType, а функцията вика customerType.Discount(subtotal).
7. Пет стъпки. Формата на Invoice е друга, а четирите теста и четирите фактури са същите като в началото.

Програмата, която печата четирите фактури, е пусната с кода отпреди първата стъпка и с кода след петата. Изходът е един и същ:

Изход преди и след

Фактура: Мария Петрова
ул. Шипка 5, 1504 София
Сума: 33.00 €
Отстъпка: 0.00 €
Общо с ДДС: 39.60 €

Фактура: Иван Георгиев
ул. Васил Левски 12, 4000 Пловдив
Сума: 50.00 €
Отстъпка: 2.50 €
Общо с ДДС: 57.00 €

Фактура: Пекарна Изгрев ООД
бул. Княз Борис I 80, 9000 Варна
Сума: 600.00 €
Отстъпка: 60.00 €
Общо с ДДС: 648.00 €

Фактура: Пекарна Изгрев ООД
бул. Княз Борис I 80, 9000 Варна
Сума: 33.00 €
Отстъпка: 0.00 €
Общо с ДДС: 39.60 €

Пробвай

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

Някой пише класа за фирма така:

public class Company : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return subtotal * 0.10m;
    }
}

Фирма поръчва кафе за 33 €:

Invoices.Invoice(new Customer("Пекарна Изгрев ООД", "бул. Княз Борис I 80", "Варна", "9000"), coffee, new Company())

Изход

Фактура: Пекарна Изгрев ООД
бул. Княз Борис I 80, 9000 Варна
Сума: 33.00 €
Отстъпка: 3.30 €
Общо с ДДС: 35.64 €

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

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

1public class Company : ICustomerType2{3 public decimal Discount(decimal subtotal)4 {5 return subtotal * 0.10m;6 }7}

Провери

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

Тестовете

ОК    CompanyFrom500
ОК    Gold
ОК    Regular
ПАДА  CompanyUnder500
Тестове: 4, минали: 3, паднали: 1

Идеята

Кога да спреш

Refactoring няма край, ако не му сложиш. Спираш, когато кодът е достатъчно ясен за промяната, която идва. Задачата е да добавиш партньори с 3%? Стъпка 5 я прави един нов клас. Задачата е друга? Може би стъпка 5 не ти трябва.

Не си струва, когато:

• кодът не се пипа и никой не го чете. Грозният код, който работи и не се сменя, не е спешен;
• вариантите са два и няма да растат. Един if е по-ясен от интерфейс и два класа;
• под кода няма мрежа. Тогава първата стъпка е мрежата, не refactoring.

И едно правило: не смесвай. Или сменяш формата, или поведението, никога и двете в една стъпка. Кент Бек казва „две шапки“: с едната пренареждаш и тестовете не се пипат, с другата добавяш и пишеш нов тест. Сменяш шапката съзнателно.

Упражнение

Партньори с 3%

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

Партньори. Магазинът добавя вид клиент „партньор“ с 3% отстъпка за всяка поръчка. Започни от кода след стъпка 5. Първо напиши тест за партньор с поръчката за 33 €, после класа Partner. Другите класове и Invoice не се пипат.

Изход

Фактура: Мария Петрова
ул. Шипка 5, 1504 София
Сума: 33.00 €
Отстъпка: 0.99 €
Общо с ДДС: 38.41 €

Още задачи

• Extract Method за текста. Изведи реда с адреса в метод AddressLine(Customer). Пускай тестовете след стъпката.
• Обратният ред. Направи петте стъпки в друг ред: първо Introduce Parameter Object, после Rename. Минават ли тестовете след всяка? Коя стъпка беше по-лесна по-рано?
• Видът от базата. Видът клиент идва като текст ("gold"). Напиши таблицата, която го превръща в обект, и реши какво става с непознат вид. Днешният код дава отстъпка 0 за непознат вид: запази това с тест.

Тест

Провери се

1. Кое от тези е refactoring?

Фактурата започва да показва и ДДС отделноЦикълът за сумата отива в отделен метод със същия резултатОтстъпката за златен клиент става 7%

2. След Introduce Parameter Object тестовете не се компилират, защото викат метода с шест параметъра. Какво правиш?

Сменяш и очакваните фактури, за да съвпаднатСменяш само извикванията в тестовете; очакваните фактури не се пипатИзтриваш тестовете и пишеш нови след края

3. Кога Inline е по-добро от Extract Method?

Когато методът е дълъгКогато малкият метод казва толкова, колкото и тялото му, и само добавя скок при четенеНикога: повече методи е винаги по-добре

4. Веригата от if по вида клиент става интерфейс и по клас за всеки вид. Как се казва това в модула за шаблоните?

SingletonObserverStrategy (или State, ако видът се сменя по време на работа)

5. Пет стъпки, тестовете след всяка. На четвъртата пада един тест. Какво значи това?

Тестът е грешенТрябва да се върнеш в началотоЧетвъртата стъпка е сменила поведението, а първите три са наред

Провери

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

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

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