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

Код на:JavaC#PythonJavaScript

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

Проблем

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

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

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Каса: ${checkoutTotal(100, true).toFixed(2)} €`);
console.log(`Фактура: ${invoiceTotal(100, true).toFixed(2)} €`);

Изход

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

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

Виж го

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

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Каса: ${checkoutTotal(100, true).toFixed(2)} €`);
console.log(`Фактура: ${invoiceTotal(100, true).toFixed(2)} €`);

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

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 € на поръчка.

Идеята

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

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

function orderTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Каса: ${orderTotal(100, true).toFixed(2)} €`);
console.log(`Фактура: ${orderTotal(100, true).toFixed(2)} €`);

Изход

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

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

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

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

Виж го

Дълъг метод

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

function placeOrder(email, subtotal, city) {
  // проверка
  if (!email.includes("@")) {
    console.log("Невалиден имейл: " + email);
    return;
  }
  if (subtotal <= 0) {
    console.log("Празна кошница");
    return;
  }
  // сума
  let delivery = city === "София" ? 3.9 : 5.9;
  if (subtotal >= 50) {
    delivery = 0;
  }
  const total = subtotal + delivery;
  // известие
  const text = `Поръчката ти за ${total.toFixed(2)} € е приета.`;
  console.log("Към " + email + ": " + text);
  // лог
  console.log(`[log] поръчка ${email} ${total.toFixed(2)}`);
}

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

Изход

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

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

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

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

function deliveryFee(subtotal, city) {
  if (subtotal >= 50) {
    return 0;
  }
  return city === "София" ? 3.9 : 5.9;
}

function notifyCustomer(email, total) {
  const text = `Поръчката ти за ${total.toFixed(2)} € е приета.`;
  console.log("Към " + email + ": " + text);
}

function log(email, total) {
  console.log(`[log] поръчка ${email} ${total.toFixed(2)}`);
}

function placeOrder(email, subtotal, city) {
  const problem = problemWith(email, subtotal);
  if (problem !== null) {
    console.log(problem);
    return;
  }
  const 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 {
  customerName = "";
  customerEmail = "";
  customerPhone = "";
  deliveryCity = "";
  deliveryStreet = "";
  deliveryPostcode = "";
  cardNumber = "";
  cardExpiry = "";
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.deliveryCity = "Варна";
console.log("Доставка до " + order.deliveryCity);

Изход

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

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

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

class Order {
  customer = null;
  delivery = null;
  card = null;
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.delivery = { city: "Варна", street: "ул. Морска 5", postcode: "9000" };
console.log("Доставка до " + order.delivery.city);

Изход

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

В JavaScript групата често е обикновен обект { city, street, postcode }. Клас си заслужава, когато групата има свои правила, като телефона по-долу.

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

Пробвай

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

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

let total = 0;
for (let i = 0; i < 3; i++) {
  total += 0.1;
}
console.log(total === 0.3);
console.log(total);

Отговор

false / 0.30000000000000004

true / 0.30

true / 0.3

Провери

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

Виж го

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

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

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += 10;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Изход

true
0.30

В JavaScript всяко число е double и десетичен тип няма. Затова парите се пазят в цели центове: 10 + 10 + 10 е точно 30. В евро се превръщат чак при печатане.

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += ;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Провери

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

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

Виж го

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

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

class Phone {
  #value;

  constructor(raw) {
    let digits = raw.replace(/[ ()-]/g, "");
    if (digits.startsWith("0")) {
      digits = "+359" + digits.slice(1);
    }
    if (!/^\+359\d{9}$/.test(digits)) {
      throw new Error("Невалиден телефон: " + raw);
    }
    this.#value = digits;
  }

  get value() {
    return this.#value;
  }

  equals(other) {
    return other instanceof Phone && other.#value === this.#value;
  }
}

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

Изход

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

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

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

В JavaScript === между два обекта сравнява дали са един и същ обект, не дали са равни. Затова Phone има свой equals, а #value е частно поле, което никой отвън не може да смени.

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

1. throw new Error("Невалиден телефон: " + raw);↑↓
2. digits = "+359" + digits.slice(1);↑↓
3. this.#value = digits;↑↓
4. if (digits.startsWith("0")) {↑↓
5. if (!/^\+359\d{9}$/.test(digits)) {↑↓
6. let digits = raw.replace(/[ ()-]/g, "");↑↓
7. }↑↓
8. }↑↓

Провери

Пробвай

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

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

const optedOut = ["+359888123456"];
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

Изход

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

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

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

1function notifyShipped(phone, optedOut) {2 if (optedOut.includes(phone)) {3 return;4 }5 console.log("SMS към " + phone + ": Поръчката ти е на път.");6}

Провери

Виж го

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

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

function price(kind) {
  if (kind === "куриер") {
    return 5.9;
  } else if (kind === "офис") {
    return 3.9;
  } else if (kind === "автомат") {
    return 2.9;
  }
  throw new Error("Непознат вид доставка: " + kind);
}

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

for (const kind of ["куриер", "офис", "автомат"]) {
  console.log(`${kind}: ${price(kind).toFixed(2)} €, дни: ${days(kind)}`);
}

Изход

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

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

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

const DELIVERY = new Map([
  ["куриер", { price: 5.9, days: 1 }],
  ["офис", { price: 3.9, days: 2 }],
  ["автомат", { price: 2.9, days: 3 }],
]);

for (const kind of ["куриер", "офис", "автомат"]) {
  const d = DELIVERY.get(kind);
  console.log(`${kind}: ${d.price.toFixed(2)} €, дни: ${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:

function sendReceipt(channel, contact, amount) {
  if (channel === "sms") {
    const text = `Платихте ${amount.toFixed(2)} €. Благодарим!`;
    console.log("SMS към " + contact.replaceAll(" ", "") + ": " + text);
  } else if (channel === "email") {
    const text = `Платихте ${amount.toFixed(2)} €. Благодарим!`;
    console.log("Имейл към " + contact.trim().toLowerCase() + ": " + text);
  } else if (channel === "viber") {
    const text = `Платихте ${amount.toFixed(2)} €. Благодарим!`;
    console.log("Viber към " + contact.replaceAll(" ", "") + ": " + 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Закръгляне при всяко печатанеЦели центове вместо евро

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

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

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

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

Провери

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

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

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