Omówię tutaj przykład niewielkiego refaktoringu, który ostatnio robiliśmy przy okazji review jednego z pull requestów.

Zalety refaktoringu można śmiało tłumaczyć na przykładzie Świąt Wielkanocnych. Kiedy masz większą rodzinę i musisz obrać 30 jajek, może to być zadanie upierdliwe. Ale nie musi. Zależy to od tego, jak zostały ugotowane. Jeśli w słonej wodzie, a potem zalane zimną wodą – to cała praca nie zajmie więcej niż 15 minut, bo skorupka łatwo zejdzie. Natomiast jeśli trzeba się z tym babrać po małych, klejących się do białka łupinkach, to wyjdzie i godzina.

Przykład kodu przed zmianami

Pomyślmy o prostym modelu danych. Mamy dokument, który posiada jedną lub więcej zgód. Żeby nie upychać wszystkie w pliku modelu, robimy klasycznie klasę serwisową, która w tym przypadku ma się zajmować usuwaniem zbędnych, bo nie podpisanych zgód dokumentów.

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def destroy
    destroy_agreements
  end

  private

  def destroy_agreements
    MultiJson.load(@agreements.to_json).each do |key, value|
      next if value.present?

      @document.granted_form_agreements.where(form_agreement_id: form_agreement_ids(key.to_s)).destroy_all
    end
  end

  def form_agreement_ids(agreement_type)
    FormAgreement.where(agreement_type: agreement_type).pluck(:id)
  end
end

Co jest nie tak z tym kodem?

1. Zbędne przetwarzanie JSON: W pierwszym przykładzie kodu, do przetwarzania obiektu @agreements użyto metody to_json i MultiJson.load. Jednakże, nie ma potrzeby serializować i deserializować danych w tym przypadku, ponieważ @agreements jest już tablicą. To zbędne przetwarzanie może wprowadzać dodatkowe koszty wydajnościowe.
2. Niepotrzebne metody pomocnicze: Użyto metody pomocniczej form_agreement_ids, która co prawda pomaga nam nazwać czym jest zmienna key, ale wcale nie ułatwia zadania. Do tego w linii 17 upchanych jest sporo metod, przez co gorzej się taki kod czyta i debuguje.
3. Brak optymalizacji zapytań do bazy danych: Każde wywołanie metody destroy_all powoduje osobne zapytanie do bazy danych. To może prowadzić do nadmiernego obciążenia bazy danych, szczególnie gdy operacja usuwania musi być wykonywana dla wielu elementów. Do tego mamy jeszcze dodatkowe zapytanie wykonujące się wielokrotnie na FormAgreement.
4. Niezwięzły kod: Kodu jest nieco nieczytelny w pętli, która iteruje po @agreements. Warunek if value.present? może być mylący, ponieważ kod wewnątrz pętli wykonuje działania na obiektach, które nie spełniają tego warunku. Nie wiadomo też czym to value jest, ponieważ nie jest nazwane odpowiednią zmienną.
5. Jedna linia jest za długa: Zauważmy, że nie widać jej nawet na podglądzie. Linie prawie zawsze da się skrócić.
6. Masło maślane: czyli nazewnictwo metody głównej destroy. Czasem się do zdarza przy robieniu builderów, że pisze się build. Lepiej jest po prostu robić call.

Przykład kodu po zmianach, podejście pierwsze

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def call
    destroy_agreements
  end

  private

  def destroy_agreements
    @agreements.each do |agreement_type, is_checked|
      next if is_checked

      @document
        .granted_form_agreements
        .joins(:form_agreement)
        .where(form_agreements: { agreement_type: agreement_type })
        .destroy_all
    end
  end
end

Zalety refaktoringu w tym przypadku obejmują:

1. Uproszczenie kodu: Refaktoring dokonuje uproszczeń w strukturze kodu, co prowadzi do większej czytelności i zrozumienia. Po refaktoringu, kod jest bardziej zwięzły i klarowny, co ułatwia jego utrzymanie i rozwój w przyszłości.
2. Usunięcie zbędnego przetwarzania: W oryginalnym kodzie używano metody MultiJson.load do serializacji i deserializacji obiektu @agreements. W zrewaktoryzowanym kodzie można bezpośrednio iterować po przekazanej tablicy @agreements, eliminując tym samym zbędne przetwarzanie.
3. Optymalizacja zapytania do bazy danych: W zrewaktoryzowanym kodzie użyto łączenia (.joins) i warunku (where) bezpośrednio w zapytaniu do bazy danych, zamiast wywoływać dodatkową metodę form_agreement_ids. To pozwala na zredukowanie liczby zapytań do bazy danych i bardziej efektywne przetwarzanie danych.

Przykład kodu po zmianach, podejście drugie

Czy można chcieć więcej? Jak najbardziej.

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def call
    document
      .granted_form_agreements
      .joins(:form_agreement)
      .where(form_agreements: { agreement_type: agreement_types_to_delete })
      .destroy_all
  end

  private
  
  attr_reader :document, :agreements
  
  def agreement_types_to_delete
    agreements.reject { |_type, is_checked| is_checked }.keys
  end
end

Oto kilka zalet tej wersji:

1. Prostsza logika: W tej wersji kodu, zamiast iterować po @agreements i sprawdzać wartość is_checked dla każdego typu umowy, użyto metody reject w celu odrzucenia umów, które nie są zaznaczone. To prowadzi do bardziej zwięzłej i czytelnej logiki.
2. Bezpośrednie pobieranie kluczy: Zamiast przekształcać @agreements do postaci tablicy kluczy, jak w drugim przykładzie, w tej wersji kodu klucze są bezpośrednio pobierane poprzez użycie metody keys na zwróconym wyniku z metody reject. To prowadzi do bardziej zwięzłego kodu.
3. Optymalizacja zapytań do bazy danych: Ta wersja kodu również wykorzystuje optymalizację zapytań do bazy danych, łącząc granted_form_agreements z form_agreements i wykonując jedno zapytanie z warunkiem where. To zapewnia bardziej efektywne usuwanie umów, zwłaszcza w przypadku, gdyby trzeba było usuwać je często i w dużych ilościach.
4. Korzystanie z metod zwracających atrybuty instancji: Odwoływanie się do zmiennych instancji przez @ jest czytelne w konstruktorze takiej klasy. Jednak robienie tego w dalszej części serwisu jest problematyczne, ponieważ literówka może zwrócić nam nil. Zresztą, można by i tutaj posunąć się o krok dalej i użyć metody attr_accessor, żeby ustawiać zmienne instancji w taki sposób: self.document = document. Jednak przyjęło się, że w konstruktorze używamy @.
5. Call robi robotę: Często zdarza się u nas w kodzie, że call niemalże aliasem do jakiejś prywatnej metody, w której jest logika serwisu. Wywoływanie poszczególnych metod w call ma sens, jeśli algorytm danej klasy jest obszerniejszy. W takim przypadku nie ma co mnożyć bytów. Z kolei prywatna metoda agreement_types_to_delete ma sens i niepotrzebnie wydłużałaby metodę call, gdyby jej wartość przypisywać do zmiennej.

Podsumowanie

Refaktoring kodu AgreementsDestroyer jest dobrym przykładem dbałości o szczegóły i rozwijania umiejętności programistycznych. Chociaż może się wydawać, że drobne zmiany nie mają znaczenia, to właśnie na takich szczegółach polega różnica między dobrym a doskonałym kodem.

Dbałość o szczegóły w kodzie jest kluczowa dla jego czytelności, skalowalności i wydajności. Małe zmiany mogą znacząco wpływać na ostateczny rezultat i jakość oprogramowania.

Kiedy jesteś początkującym, zwracanie uwagi na takie szczegóły jest niezwykle ważne. Nie chodzi tu o małostkowość, ale o rozwijanie umiejętności analitycznego myślenia, refaktoryzacji kodu i optymalizacji. Poprawianie struktury kodu, usuwanie zbędnych operacji czy optymalizacja zapytań to umiejętności, które będą ci towarzyszyć przez całą karierę programisty.

Pamiętaj, że jako programista tworzysz oprogramowanie, które będzie wykorzystywane przez innych ludzi. Dbałość o szczegóły jest dowodem Twojego profesjonalizmu. Wielkie rzeczy zaczynają się od małych detali, dlatego warto zwracać uwagę na każdy aspekt kodu i dążyć do doskonałości. Zwłaszcza w sobotnie poranki, kiedy kawa smakuje najlepiej.

(Uwielbiam jak ChatGPT podsumowuje przekaz o refaktoringu w swoim entuzjastycznym AI Duchu).