dlaczego-duplikacja-kodu-jest-dobra

Dlaczego duplikacja kodu jest dobra?

Często słyszymy, że duplikacja kodu jest czymś złym. Niektórzy wręcz posuwają się do stwierdzenia, że to antywzorzec. W praktycznie każdym kursie autorzy mówią, że nie należy duplikować kodu tylko wydzielić funkcję/klasę, która będzie go zawierać. Często taka funkcja/klasa ląduje w module utils, common lub helpers. Dzięki temu nasz kod staje się reużywalny i łatwy do modyfikacji, ponieważ w przypadku kiedy będziemy chcieli go zmienić musimy to zrobić tylko w jednym miejscu! Brzmi świetnie prawda? Istnieje przecież wiele zasad dobrego programowania takich jak na przykład DRY, które mówią o tym aby nie powielać kodu. Ale czy aby na pewno dobrze je rozumiemy?

Problemy „utilsów” i „generyków”

Bazując na własnym doświadczeniu zauważyłem, że programiści często mają tendencję do zbyt wczesnego wydzielania powtarzającego się kodu do modułów takich jak utils/commons/helpers. Oczywiście nie ma w tym nic złego jeśli taki kod jest kodem narzędziowym wykorzystywanym w wielu miejscach i nie zawierającym logiki biznesowej. Taki moduł jest narzędziem z którego korzystamy podobnie jak z wbudowanych obiektów czy metod danego języka. Ważne jest natomiast to aby w takim module rzeczywiście znajdował się kod, który jest generyczny oraz narzędziowy (pomagający nam w rozwiązywaniu powszechnych problemów), a nie stanowiący esencje logiki biznesowej. Przykładem może być funkcja, która zwraca ostatni dzień miesiąca:

function getLastDayOfMonth(date: Date): Date {
  return new Date(date.getFullYear(), date.getMonth() + 1, 0);
}

lub funkcja generyczna sumująca długości dwóch tablic:

function sumArraysLength<T, K>(firstArray: T[], secondArray: K[]): number {
  return firstArray.length + secondArray.length;
}

Jak widzisz powyższe funkcje nie zawierają żadnej logiki biznesowej, a jedynie są pomocnym narzędziem, które możemy wykorzystać w naszej aplikacji. Nie zależą kompletnie od niczego. Ostatni dzień miesiąca zawsze będzie ostatnim dniem miesiąca bez względu na to gdzie wywołamy nasza funkcję, a suma długości dwóch tablic zawsze będzie sumą długości dwóch tablic bez względu na to jaki typ tablic przekażemy.

A więc gdzie leży problem? Problem pojawia się w momencie kiedy zbyt wcześnie i bez rozpoznania kontekstu biznesowego uznamy kod za generyczny/narzędziowy. Spójrzmy na poniższy przykład:

interface Product {
  id: string;
  price: number;
}

type BuyerType = 'user' | 'vip' | 'superVip' | 'company';

enum BuyerType {
  User = 'user',
  Vip = 'vip',
  SuperVip = 'superVip'
}

class Order {
  private id: string;
  private product: Product;
  private amount: number;
  private buyer: Buyer;

  constructor(id: string, product: Product, buyer: Buyer, amount: number) {
    this.id = id;
    this.product = product;
    this.amount = amount;
    this.buyer = buyer;
  }

  calculateFinalPrice() {
    return this.amount * this.product.price;
  }
}

Mamy tutaj przykład klasy Order, która posiada metodę calculateFinalPrice. Metoda zwraca nam cenę jaką musi zapłacić użytkownik za produkty. Na razie nie ma tutaj nic skomplikowanego. W pewnym momencie przychodzi do nas biznes i mówi, że musimy zaimplementować mechanizm, który sprawdzi czy użytkownik jest vipem lub super vipem i jeśli nim jest to zmniejszamy cenę o 10%, ale w przyszłości super vip może mieć inną zniżkę. Więc nie pozostaje nam nic innego jak zaimplementowanie tej funkcjonalności. Nasza metoda może wyglądać następująco:

  calculateFinalPrice() {
    if (this.buyer.type === BuyerType.User) {
      return this.amount * this.product.price;
    }

    if (this.buyer.type === BuyerType.Vip) {
      return (this.amount * this.product.price) * 0.90;
    }

    if (this.buyer.type === BuyerType.SuperVip) {
      return (this.amount * this.product.price) * 0.90;
    }
  }

Jak możemy zauważyć część kodu się powtarza. Co więcej nawet jeden if jest tutaj zbędny. Zmysł programisty oraz wiedza z kilkudziesięciu wcześniej obejrzanych kursów podpowiada nam, że skoro kod wygląda tak samo to możemy go wydzielić do osobnej metody!

class Order {
  private id: string;
  private product: Product;
  private amount: number;
  private buyer: Buyer;

  constructor(id: string, product: Product, buyer: Buyer, amount: number) {
    this.id = id;
    this.product = product;
    this.amount = amount;
    this.buyer = buyer;
  }

  calculateFinalPrice() {
    const price = this.amount * this.product.price;

    if (this.buyer.type === BuyerType.User) {
      return price;
    }

    if (this.buyer.type === BuyerType.Vip) {
      return this.calculatePriceWithDiscount(price, 0.10);
    }

    if (this.buyer.type === BuyerType.SuperVip) {
      return this.calculatePriceWithDiscount(price, 0.10);
    }
  }

  calculatePriceWithDiscount(price: number, discount: number): number {
    return price * (1 - discount);
  }
}

Tak też się stało! Nasza klasa przeszła dogłębną refaktoryzację! Wydzieliliśmy zduplikowany kod do osobnej metody. Nauczeni doświadczeniem zostawiliśmy jednak zbędnego if’a ze względu na rozszerzalność kodu ponieważ, mamy od biznesu informację, że w przyszłości super vip może mieć inną zniżkę. Według większości kursów nasz kod jest teraz czytelny, rozszerzalny oraz łatwy do modyfikacji. Ale czy aby na pewno?

Za jakiś czas biznes przychodzi do nas z informacją, że zniżka dla super vipa się zmienia. Od teraz super vip ma 10% zniżkę, ale jeśli kupi więcej niż 10 produktów to zniżka będzie wynosić 15%. Nie pozostaje nam nic innego jak zaimplementowanie tego wymagania:

class Order {
  private id: string;
  private product: Product;
  private amount: number;
  private buyer: Buyer;

  constructor(id: string, product: Product, buyer: Buyer, amount: number) {
    this.id = id;
    this.product = product;
    this.amount = amount;
    this.buyer = buyer;
  }

  calculateFinalPrice() {
    const price = this.amount * this.product.price;

    if (this.buyer.type === BuyerType.User) {
      return price;
    }

    if (this.buyer.type === BuyerType.Vip) {
      return this.calculatePriceWithDiscount(price, 0.10, false, this.amount);
    }

    if (this.buyer.type === BuyerType.SuperVip) {
      return this.calculatePriceWithDiscount(price, 0.10, true, this.amount);
    }
  }

  calculatePriceWithDiscount(price: number, discount: number, isSuperVip: boolean, amount: number): number {
    if (isSuperVip) {
      if (amount > 10) {
        return price * (1 - discount - 0.05);
      }

      return price * (1 - discount);
    }

    return price * (1 - discount);
  }
}

Dodaliśmy zatem brakujące parametry do naszej generycznej funkcji i wymagania zostały spełnione. Nasz wcześniej zostawiony if również się przydał! Po pewnym czasie znowu przychodzi do nas biznes i mówi, że nowa funkcjonalność znacząco zwiększyła obroty firmy więc trzeba zaimplementować podobny mechanizm dla vipa z tym, że vip musi kupić co najmniej 20 produktów o wartości większej niż 200zł aby otrzymać 15% zniżkę. Jeśli kupi mniej niż 20 produktów, ale ich wartość przekroczy 200zł to dajemy mu 13% zniżkę. Znamy wymagania więc nie pozostaje nam nic innego jak implementacja:

class Order {
  private id: string;
  private product: Product;
  private amount: number;
  private buyer: Buyer;

  constructor(id: string, product: Product, buyer: Buyer, amount: number) {
    this.id = id;
    this.product = product;
    this.amount = amount;
    this.buyer = buyer;
  }

  calculateFinalPrice() {
    const price = this.amount * this.product.price;

    if (this.buyer.type === BuyerType.User) {
      return price;
    }

    if (this.buyer.type === BuyerType.Vip) {
      return this.calculatePriceWithDiscount(price, 0.10, false, this.amount, true);
    }

    if (this.buyer.type === BuyerType.SuperVip) {
      return this.calculatePriceWithDiscount(price, 0.10, true, this.amount, false);
    }
  }

  calculatePriceWithDiscount(price: number, discount: number, isSuperVip: boolean, amount: number, isVip: boolean): number {
    if (isSuperVip) {
      if (amount > 10) {
        return price * (1 - discount - 0.05);
      }

      return price * (1 - discount);
    }

    if (isVip) {
      if (amount > 20 && price > 200) {
        return price * (1 - discount - 0.05);
      }

      if (price > 200) {
        return price * (1 - discount - 0.03);
      }
    }

    return price * (1 - discount);
  }
}

Jak możemy zauważyć im dalej w las tym bardziej nasza „generyczna” metoda przestaje być generyczna. Zaczyna się pojawiać coraz więcej argumentów co więcej wykluczających się na wzajem. Przecież nikt nie zabroni nam ustawić zarówno flagi isSuperVip jak i isVip na true. Co więcej w naszej „generycznej” metodzie kod również zaczyna się duplikować. Ktoś może wpaść na pomysł aby również wydzielić go do osobnej metody co na pewno nie poprawi czytelności. Co chwilę musimy dodawać kolejne argumenty oraz if’y. Część z Was szczególnie tych na początku swojej kariery może pomyśleć, że ten przykład jest mocno przekoloryzowany ale osobiście już nie raz spotkałem się z o wiele większymi „generycznymi” funkcjami.

Tutaj mamy jeszcze o tyle dobrą sytuację, że ta metoda jest częścią klasy Order i nikt nie wpadł na pomysł, żeby wrzucić ją do modułu utils. Taka sytuacja często następuje kiedy mamy dwie klasy na przykład Order i SuperOrder korzystające na pierwszy rzut oka z tego samego kodu.

class Order {
  calculatePriceWithDiscount(price: number, discount: number, isSuperVip: boolean, amount: number, isVip: boolean): number {
    if (isSuperVip) {
      if (amount > 10) {
        return price * (1 - discount - 0.05);
      }

      return price * (1 - discount);
    }

    if (isVip) {
      if (amount > 20 && price > 200) {
        return price * (1 - discount - 0.05);
      }

      if (price > 200) {
        return price * (1 - discount - 0.03);
      }
    }

    return price * (1 - discount);
  }
}

class SuperOrder {
  calculatePriceWithDiscount(price: number, discount: number, isSuperVip: boolean, amount: number, isVip: boolean): number {
    if (isSuperVip) {
      if (amount > 10) {
        return price * (1 - discount - 0.05);
      }

      return price * (1 - discount);
    }

    if (isVip) {
      if (amount > 20 && price > 200) {
        return price * (1 - discount - 0.05);
      }

      if (price > 200) {
        return price * (1 - discount - 0.03);
      }
    }

    return price * (1 - discount);
  }
}

Wtedy pojawia się pytanie gdzie wrzucić taki kod? W tutorialach mówili, że dziedziczenie jest złe, a kompozycja się nam nie przyda więc taki kod najczęściej ląduje w module utils. Co więcej z czasem pewnie okaże się, że te klasy różnią się miedzy sobą więc pewnie pojawią się flagi isOrder oraz isSuperOrder co jeszcze bardziej skomplikuje sprawę. Jest to poważny błąd, ponieważ w tym momencie logika biznesowa, która powinna być z enkapsulowana i chroniona nagle znajduje się w module utils. Co gorsza jeśli ktoś w przyszłości użyje tej funkcji i jej wynik nie będzie go satysfakcjonował to może zmienić algorytm nie będąc świadomym, że to ważna cześć logiki biznesowej bo przecież to tylko zwykły „utils” od którego przypadkowo zależy połowa naszej aplikacji.

Kod przestał być czytelny, rozszerzalny i modyfikowalny, a ta metoda ma dopiero 21 linii! W prawdziwych aplikacjach takie metody potrafią mieć po kilkadziesiąt lub nawet kilkaset linii kodu. Wyobraźcie sobie teraz sytuację w której nowy pracownik przychodzi do firmy i ma wprowadzić jakąś modyfikację dla super vipa. Istnieje duże prawdopodobieństwo, że wprowadzając taką zmianę niechcący zmodyfikuje również logikę dla vipa. Ponadto bez zaglądania do wnętrza tej metody nie jest w stanie powiedzieć za co odpowiadają poszczególne argumenty no bo co się stanie jeśli ustawimy zarówno isSuperVip jak i isVip na true? Nie wiemy tego. Ktoś może powiedzieć, że od tego są testy, ale osobiście nie wyobrażam sobie pisania i utrzymywania testów dla metody z taką ilością argumentów lub co gorsza argumentów zawierających jakieś złożone typy. Każda modyfikacja będzie prowadzić do rozwalenia testów. Co zatem poszło nie tak?

Przyczyny

Zbyt pochopna decyzja

Zbyt wcześnie uznaliśmy, że liczenie zniżek jest generyczne i będzie wyglądać tak samo dla każdego typu kupującego. Złamaliśmy zasadę Single Responsibilty Principle. Już na początku biznes powiedział nam, że liczenie zniżek dla super vipa będzie w przyszłości wyglądać inaczej. My jednak nie doprecyzowaliśmy co to znaczy „inaczej” i uznaliśmy, że tylko wielkość rabatu ulegnie zmianie. Ponadto prawdopodobnie nie bez powodu klasa Buyer posiada pole type. Powinniśmy to dostrzec i nie próbować na siłę tworzyć generycznej metody, tylko zostawić to jako dwa oddzielne chwilowo zduplikowane kawałki kodu.

Chwilowa i przypadkowa duplikacja

Jest to pułapka w którą często wpadają programiści. Polega ona na tym, że kod CHWILOWO wygląda tak samo. Programiści to zauważają i momentalnie robią z tego generyczna metodę/klasę, a co gorsza wyrzucają ją do modułu utils. Jest to błąd, ponieważ istnieje duże prawdopodobieństwo, że w przyszłości kod będzie wyglądał zupełnie inaczej, a nasz utils przestanie być narzędziowy i będzie posiadał zaawansowaną logikę biznesową.

Złe zrozumienie zasad dobrego programowania

Jeśli dobrze się przyjrzymy to wszystkie zasady mówiące o tym, że duplikacja kodu jest zła tak naprawdę mówią o tym, żeby nie duplikować logiki biznesowej, a nie samego kodu. Mają na myśli to, że na przykład jeśli w kilku modułach posiadamy kod odpowiedzialny za sprawdzanie dostępności danego produktu to powinniśmy ten kod wydzielić do osobnego modułu. W przeciwnym razie w przyszłości możemy mieć sprzeczne informacje na temat dostępności danego produktu, ponieważ logika jest rozrzucona po całej aplikacji. Część osób może teraz pomyśleć, że należy ten kod wrzucić do utils, common lub helpers. Jednak trzeba sobie wtedy zadać pytanie czy na pewno jest to kod narzędziowy? Przecież to jest ważna część naszego biznesu, która posiada logikę biznesową, którą należy chronić. Ten rodzaj logiki powinien być z enkapsulowany i wydzielony do osobnego modułu dostępności.

Po czym poznać, że kod nie jest generyczny?

Należy słuchać biznesu i zwracać uwagę na ich słownictwo. Jeśli biznes do opisywania rzeczy używa różnych zwrotów bądź wypowiada się o nich w inny sposób na przykład używa słów: vip i super vip to istnieje duże prawdopodobieństwo, że między tymi bytami są lub będą znaczące różnice o których aktualnie nie wiemy. Sami również powinniśmy zadbać o zadawanie właściwych pytań w celu pozyskania informacji o wymaganiach i dobrego zaprojektowania architektury. Jeśli chcecie wiedzieć więcej na ten temat to polecam książkę Oprogramowanie szyte na miarę. Jak rozmawiać z klientem, który nie wie, czego chce.

Ale co w przypadku kiedy nie mamy kontaktu z klientem lub nie wiemy jak pozyskać takie informacje? Najprostszym rozwiązaniem będzie duplikacja kodu. Zdecydowanie łatwiej jest coś potem scalić w jedną metodę/klasę niż rozbijać pseudo generyczną klasę/metodę na oddzielne byty. Łatwiej będzie również przetestować taki kod, ponieważ nawet jeśli będzie zduplikowany to na pewno będzie mniejszy niż „generyczna” metoda, którą stworzyliśmy wyżej. Warto również zostawić taki kod w jednym miejscu, ponieważ nawet jeśli nie jest on do końca dobry to nie zanieczyści nam reszty aplikacji i w przyszłości łatwiej będzie się go pozbyć. Oczywiście można również uznać, że kod rzeczywiście jest generyczny i wydzielić go do osobnej klasy/metody jednak wtedy trzeba być bardzo czujnym i jeśli zauważymy jakieś niepokojące sygnały, że kod może nie do końca jest kodem generycznym to należy go rozbić na niezależne od siebie byty.

Nie ma tutaj złotego środka, który powie nam czy kod jest generyczny lub narzędziowy czy też nie. Każdy przypadek jest inny. To co mogę polecić to lepsze zapoznanie się z wymaganiami biznesowymi oraz samą aplikacją. Jeśli będziemy znać kontekst biznesowy to zdecydowanie łatwiej będzie nam podjąć własciwą decyzję.

Co zatem należy zrobić?

  calculateFinalPrice() {
    const price = this.amount * this.product.price;

    if (this.buyer.type === BuyerType.User) {
      return price;
    }

    if (this.buyer.type === BuyerType.Vip) {
      if (amount > 20 && price > 200) {
        return price * (1 - discount - 0.05);
      }

      if (price > 200) {
        return price * (1 - discount - 0.03);
      }
    }

    if (this.buyer.type === BuyerType.SuperVip) {
      if (amount > 10) {
        return price * (1 - discount - 0.05);
      }

      return price * (1 - discount);
    }
  }

Już samo przywrócenie kodu do metody calculateFinalPrice znacząco poprawi jego czytelność. Część kodu jest zduplikowana jednak jest to chwilowe i przypadkowe. Co więcej kod znajduje się w jednym miejscu więc jego zmiana nie zajmie nam dużo czasu. W przyszłości możemy pokusić się o stworzenie oddzielnych metod do liczenia zniżek dla usera, vipa i super vipa lub nawet oddzielnych klas. Dzięki temu testowanie stanie się o wiele prostsze. Ktoś może powiedzieć, że przecież kod po zmianach również nie spełnia zasady Single Responsibilty Principle i tak to jest prawda, jednak teraz zdecydowanie łatwiej i szybciej będzie można go rozdzielić. We wszystkim należy być pragmatycznym.

Podsumowanie

Nie należy oceniać książki po okładce, a kodu bez dogłębnego zrozumienia biznesu. Lepiej spędzić dłuższą chwilę na rozmowach z klientem i planowaniu oraz pisaniu drugi raz „tego samego kodu” niż stworzyć potworka w postaci pseudo generycznej metody/klasy lub wrzucić logikę biznesową do utilsów. Tak pochopna decyzja może być potem ciężka do odwrócenia i tragiczna w skutkach. Oczywiście we wszystkim należy zachować pragmatyzm.