Czy to użycie warunkowe jest anty-wzorem?

Widziałem to często w naszym starszym systemie w pracy - funkcje, które wyglądają mniej więcej tak:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

Innymi słowy, funkcja składa się z dwóch części. Pierwsza część wykonuje jakieś przetwarzanie (potencjalnie zawierające pętle, efekty uboczne itp.), A po drodze może ustawić flagę „todo”. Druga część jest wykonywana tylko wtedy, gdy ustawiono flagę „todo”.

Wydaje się to dość brzydkim sposobem robienia rzeczy i myślę, że większość przypadków, które właściwie poświęciłem czas na zrozumienie, można by zreorganizować, aby uniknąć używania flagi. Ale czy to faktyczny anty-wzór, zły pomysł, czy całkowicie akceptowalny?

Pierwszą oczywistą refaktoryzacją byłoby pocięcie go na dwie metody. Jednak moje pytanie dotyczy bardziej tego, czy kiedykolwiek trzeba (w nowoczesnym języku OO) utworzyć zmienną flagi lokalnej, potencjalnie ustawiając ją w wielu miejscach, a następnie używając jej później, aby zdecydować, czy wykonać następny blok kodu.

Nie wiem o anty-wzorach, ale wyciągnę z tego trzy metody.

Pierwszy wykonałby trochę pracy i zwrócił wartość logiczną.

Drugi wykona dowolną pracę wykonywaną przez „jakiś inny kod”

Trzeci wykonałby pracę pomocniczą, gdyby wartość logiczna była prawdziwa.

Wyodrębnione metody byłyby prawdopodobnie prywatne, gdyby ważne było, aby wywołać tylko drugą (i zawsze) drugą metodę, jeśli pierwsza metoda zwróci prawdę.

Dobrze nazywając metody, mam nadzieję, że dzięki temu kod będzie wyraźniejszy.

Coś takiego:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Oczywiście należy zastanowić się nad tym, czy wczesne zwroty są dopuszczalne, ale jest to szczegół implementacji (podobnie jak standard formatowania kodu).

Chodzi o to, że intencja kodu staje się jaśniejsza, co jest dobre ...

Jeden z komentarzy do pytania sugeruje, że ten wzór reprezentuje zapach , i zgodziłbym się z tym. Warto na to spojrzeć, aby sprawdzić, czy uda się wyjaśnić cel.

Myślę, że brzydota wynika z faktu, że w jednej metodzie jest dużo kodu i / lub zmienne są źle nazwane (oba z nich same pachną kodem - anty-wzory to bardziej abstrakcyjne i złożone rzeczy IMO).

Jeśli więc wyodrębnisz większość kodu do metod niższego poziomu, jak sugeruje @Bill, reszta stanie się czysta (przynajmniej dla mnie). Na przykład

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Możesz też całkowicie pozbyć się flagi lokalnej, ukrywając czek flagi w drugiej metodzie i używając formularza podobnego do

calculateTaxRefund(isTaxRefundable(...), ...)

Ogólnie rzecz biorąc, nie widzę, aby zmienna flagi lokalnej była zła jako taka. Która z powyższych opcji jest bardziej czytelna (= lepsza ode mnie) zależy od liczby parametrów metody, wybranych nazw i która forma jest bardziej spójna z wewnętrzną logiką kodu.

todo to naprawdę zła nazwa zmiennej, ale myślę, że to może być wszystko, co jest złe. Trudno być całkowicie pewnym bez kontekstu.

Powiedzmy, że druga część funkcji sortuje listę zbudowaną z pierwszej części. Powinno to być znacznie bardziej czytelne:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Jednak sugestia Billa jest również poprawna. Jest to nadal bardziej czytelne:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Wzór maszyny stanowej wygląda dla mnie dobrze. Występujące tam anty-wzory to „todo” (zła nazwa) i „dużo kodu”.

To zależy naprawdę. Jeśli kod chroniony przez todo(mam nadzieję, że nie używasz tej nazwy tak naprawdę, ponieważ jest ona całkowicie niemnemoniczna!) Jest kodem czyszczącym koncepcyjnie, oznacza to, że masz anty-wzorzec i powinieneś użyć czegoś takiego jak RAII C ++ lub C # usingzamiast tego konstruuj, aby obsługiwać różne rzeczy.

Z drugiej strony, jeśli koncepcyjnie nie jest to etap oczyszczania, a jedynie dodatkowe przetwarzanie, które czasami jest potrzebne i gdzie decyzja o tym musi zostać podjęta wcześniej, napisane jest w porządku. Zastanów się, czy poszczególne fragmenty kodu byłyby lepiej refaktoryzowane do ich własnych funkcji, a także czy uchwyciłeś znaczenie zmiennej flag w jej nazwie, ale ten podstawowy wzorzec kodu jest OK. W szczególności próba włożenia zbyt wiele w inne funkcje może sprawić, że to, co się dzieje, będzie mniej jasne, a to z pewnością byłoby anty-wzorem.

Wiele odpowiedzi tutaj miałoby problemy z zaliczeniem kontroli złożoności, niektóre z nich wyglądały na> 10.

Myślę, że jest to „anty-wzór” tego, na co patrzysz. Znajdź narzędzie, które mierzy cykliczność złożoności twojego kodu - istnieją wtyczki do zaćmienia. Zasadniczo jest to miara tego, jak trudny jest twój kod do przetestowania i obejmuje liczbę i poziomy gałęzi kodu.

Całkowicie zgadując możliwe rozwiązanie, układ kodu sprawia, że ​​myślę w „Zadaniach”, jeśli dzieje się to w wielu miejscach, być może to, czego naprawdę chcesz, to architektura zorientowana na zadania - każde zadanie jest własne obiekt, aw połowie zadania masz możliwość kolejkowania następnego zadania poprzez utworzenie kolejnego obiektu zadania i rzucenie go do kolejki. Mogą być niezwykle proste w konfiguracji i znacznie zmniejszają złożoność niektórych rodzajów kodu - ale, jak powiedziałem, jest to całkowite dźgnięcie w ciemność.

Korzystając z powyższego przykładu pdr, ponieważ jest to dobry przykład, pójdę o krok dalej.

On miał:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Uświadomiłem sobie, że następujące działania będą działać:

if(BuildList(list)) 
    SortList(list)

Ale nie jest tak jasne.

Więc na pierwotne pytanie, dlaczego nie mieć:

BuildList(list)
SortList(list)

I niech SortList zdecyduje, czy wymaga sortowania?

Widzisz, wydaje się, że twoja metoda BuildList wie o sortowaniu, ponieważ zwraca wartość bool wskazującą jako taką, ale nie ma to sensu w przypadku metody zaprojektowanej do zbudowania listy.

Tak, wydaje się to stanowić problem, ponieważ musisz śledzić wszystkie miejsca, w których zaznaczasz flagę ON / OFF. Lepiej jest włączyć logikę tylko wewnątrz jako zagnieżdżony warunek, zamiast wyjmować logikę.

Także bogate modele domen, w tym przypadku tylko jedna linijka zrobi wielkie rzeczy wewnątrz obiektu.

Jeśli flaga jest ustawiona tylko raz, przesuń
...
kod w górę bezpośrednio po
... // innym kodzie tutaj,
a następnie usuń flagę.

W przeciwnym razie podziel
... // dużo kodu tutaj
... // trochę innego kodu tutaj
...
jeśli to możliwe, go na osobne funkcje, więc jasne jest, że ta funkcja ma jedną odpowiedzialność, która jest logiką gałęzi.

Tam, gdzie to możliwe, oddziel kod wewnątrz
... // dużo kodu tutaj
na dwie lub więcej funkcji, niektóre, które wykonują pewną pracę (która jest komendą), a inne, które albo zwracają wartość todo (która jest zapytaniem), albo ją tworzą bardzo wyraźnie modyfikują to (zapytanie wykorzystujące efekty uboczne)

Sam kod nie ma tu miejsca na anty-wzorzec ... Podejrzewam, że mieszanie się logiki rozgałęziania z faktycznym wykonywaniem rzeczy (poleceń) jest anty-wzorkiem, którego szukasz.

Czasami muszę wdrożyć ten wzór. Czasami chcesz wykonać wiele kontroli przed kontynuowaniem operacji. Ze względów wydajnościowych obliczenia obejmujące niektóre kontrole nie są wykonywane, chyba że wydaje się to absolutnie konieczne. Więc zazwyczaj widzisz kod taki jak:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Można to uprościć, oddzielając sprawdzanie poprawności od rzeczywistego procesu wykonywania operacji, aby uzyskać więcej takich informacji:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Oczywiście różni się w zależności od tego, co próbujesz osiągnąć, chociaż napisane w ten sposób, zarówno kod „sukcesu”, jak i kod „awarii” są zapisywane jeden raz, co upraszcza logikę i utrzymuje ten sam poziom wydajności. Stąd dobrym pomysłem byłoby zmieścić całe poziomy walidacji w wewnętrznych metodach, które zwracają logiczne oznaki sukcesu lub niepowodzenia, które dodatkowo upraszczają sprawy, chociaż niektórzy programiści lubią bardzo długie metody z jakiegoś dziwnego powodu.

To nie jest wzór . Najbardziej ogólna interpretacja jest taka, że ​​ustawiasz zmienną boolowską i rozgałęziasz jej wartość później. To normalne programowanie proceduralne, nic więcej.

Teraz twój konkretny przykład można przepisać jako:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

To może być łatwiejsze do odczytania. Albo może nie. Zależy to od reszty kodu, który pominąłeś. Skoncentruj się na tym, aby kod był bardziej zwięzły.

Flagi lokalne używane w przepływie sterowania powinny być rozpoznawane jako forma gotomaskowania. Jeśli flaga jest używana tylko w obrębie funkcji, można ją wyeliminować, wypisując dwie kopie funkcji, oznaczając jedną jako „flaga jest prawdą”, a drugą jako „flaga jest fałszem”, i zastępując każdą operację ustawiania flagi gdy jest wyczyszczony lub usuwa go, gdy jest ustawiony, ze skokiem między dwiema wersjami funkcji.

W wielu przypadkach kod, który używa flagi, będzie czystszy niż jakakolwiek możliwa alternatywa, która gotozamiast tego używa , ale nie zawsze tak jest. W niektórych przypadkach użycie gotopominięcia fragmentu kodu może być czystsze niż użycie do tego flag [choć niektórzy ludzie mogą tu wstawić określoną kreskówkę raptor].

Myślę, że główną zasadą przewodnią powinno być to, że przepływ logiki programu powinien w możliwie największym stopniu przypominać opis logiki biznesowej. Jeśli wymagania dotyczące logiki biznesowej są opisane w kategoriach stanów, które dzielą się i łączą w dziwny sposób, wykonanie logiki programu może być czystsze niż próba użycia flag w celu ukrycia takiej logiki. Z drugiej strony, jeśli najbardziej naturalnym sposobem opisywania reguł biznesowych byłoby stwierdzenie, że akcja powinna zostać wykonana, jeśli zostały wykonane pewne inne akcje, najbardziej naturalnym sposobem wyrażenia może być użycie flagi ustawianej podczas wykonywania te ostatnie działania są poza tym jasne.