Czy to użycie warunkowe jest anty-wzorem?

14

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.

Kricket
źródło
2
Jak to refaktoryzujesz?
Tamás Szelei,
13
Zakładając, że todo jest ustawione w kilku miejscach, zgodnie z kilkoma nietrywialnymi, niewyłącznymi warunkami, nie mogę myśleć o refaktoryzacji, która ma najmniejszy sens. Jeśli nie ma refaktoryzacji, nie ma antypatternu. Z wyjątkiem nazewnictwa zmiennej todo; należy nazwać bardziej wyrazistym, na przykład „doSecurityCheck”.
user281377,
3
@ammoQ: +1; jeśli sprawy są skomplikowane, to właśnie takie są. Zmienna flagowa może mieć w niektórych okolicznościach znacznie więcej sensu, ponieważ wyjaśnia, że ​​decyzja została podjęta, i możesz ją wyszukać, aby dowiedzieć się, gdzie ta decyzja została podjęta.
Donal Fellows,
1
@Donal Fellows: Jeśli konieczne jest poszukiwanie przyczyny, zmienię zmienną na listę; dopóki jest pusty, jest „fałszywy”; gdziekolwiek flaga jest ustawiona, kod przyczyny jest dodawany do listy. Możesz więc skończyć z taką listą ["blacklisted-domain","suspicious-characters","too-long"]pokazującą, że zastosowano kilka powodów.
user281377,
2
Nie sądzę, że to anty-wzór, ale zdecydowanie jest to zapach
Binary Worrier,

Odpowiedzi:

23

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.

Bill Michell
źródło
Podział na 2 funkcje nadal wymagałby todozmiennej i prawdopodobnie byłby trudniejszy do zrozumienia.
Pubby
Tak, też bym to zrobił, ale moje pytanie dotyczyło raczej użycia flagi „todo”.
Kricket,
2
Jeśli skończysz z if (check_if_needed ()) do_whatever ();, nie ma tam widocznej flagi. Myślę, że może to spowodować zbyt duże uszkodzenie kodu i potencjalnie zaszkodzić czytelności, jeśli kod jest dość prosty. W końcu szczegóły tego, co robisz, do_whatevermogą mieć wpływ na sposób testowania check_if_needed, dlatego warto trzymać cały kod razem na tym samym ekranie. Ponadto nie gwarantuje to, że check_if_neededmożna uniknąć użycia flagi - a jeśli tak, prawdopodobnie użyje returndo tego wielu instrukcji, co może denerwować ścisłych zwolenników pojedynczego wyjścia.
Steve314,
3
@ Pubby8 powiedział „wyciągnij z tego 2 metody” , co dało 3 metody. 2 metody wykonujące rzeczywiste przetwarzanie i oryginalna metoda koordynująca przepływ pracy. Byłby to znacznie czystszy projekt.
MattDavey,
Pomija to ... // some other code hereprzypadek wczesnego powrotu
Caleth
6

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.

Péter Török
źródło
4

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);
pdr
źródło
Dlaczego nie pójść o krok dalej: if (BuildList (list)) SortList (list);
Phil N DeBlanc
2

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

ptyx
źródło
Jestem pewien, że to tylko dla ilustracji.
Loren Pechtel,
1
Zgoda. Próbowałem przekazać, że dobre wzorce utopione w złym kodzie nie powinny być obwiniane za jakość kodu.
ptyx
1

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.

Donal Fellows
źródło
To oczywiście nie jest porządek - nie zawsze działa. Trafiłem już takie przypadki - podczas przetwarzania czegoś może się okazać, że unieważniasz jakiś wstępnie obliczony wynik. Jeśli obliczenia są drogie, chcesz je uruchomić tylko w razie potrzeby.
Loren Pechtel
1

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ść.

Bill K.
źródło
1

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.

Ian
źródło
I oczywiście następnym krokiem jest pytanie, dlaczego jest to proces dwuetapowy. Gdziekolwiek widzę taki kod, refaktoryzuję metodę o nazwie BuildAndSortList (list)
Ian
To nie jest odpowiedź. Zmieniłeś zachowanie kodu.
D Drmmr
Nie całkiem. Znowu nie mogę uwierzyć, że odpowiadam na coś, co opublikowałem 7 lat temu, ale do diabła :) Argumentowałem, że SortList zawiera warunek. Jeśli miałeś test jednostkowy, który wykazał, że lista jest posortowana tylko wtedy, gdy spełniony jest warunek x, nadal by się udał. Przenosząc warunek do SortList, unikniesz konieczności pisania (jeśli (coś), a następnie SortList (...))
Ian
0

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.

java_mouse
źródło
0

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.

Andrzej Pasztet
źródło
co dodaje ten post, że brakuje istniejących odpowiedzi?
esoterik
@esoterik Czasami możliwość dodania małego CQRS jest często pomijana, jeśli chodzi o flagi ... logika podjęcia decyzji o zmianie flagi reprezentuje zapytanie, podczas gdy wykonywanie pracy oznacza polecenie. Czasami oddzielenie dwóch może sprawić, że kod będzie wyraźniejszy. Warto także wskazać w powyższym kodzie, że można to uprościć, ponieważ flaga jest ustawiona tylko w jednej gałęzi. Wydaje mi się, że flagi nie są antypatternem, a jeśli ich nazwa sprawia, że ​​kod jest bardziej wyrazisty, to dobrze. Uważam, że gdzie są tworzone, ustawiane i używane flagi powinny znajdować się blisko siebie, jeśli to możliwe.
andrew pasztet
0

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.

Neil
źródło
W podanym przez Ciebie przykładzie myślę, że wolałbym mieć funkcję shouldIDoIt (fieldsValidated, valuesExist), która zwraca odpowiedź. Wynika to z faktu, że wszystkie decyzje typu tak / nie są podejmowane jednocześnie, w przeciwieństwie do kodu, który widzę tutaj w pracy, w którym decyzja o kontynuacji jest rozproszona na kilka różnych niesąsiadujących ze sobą miejsc.
Kricket,
@KelseyRider, właśnie o to chodziło. Oddzielenie sprawdzania poprawności od wykonania pozwala upchnąć logikę do metody, aby uprościć ogólną logikę programu w if (isValidated ()) doOperation ()
Neil
0

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.

D Drmmr
źródło
-1

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.

supercat
źródło