Czy niewłaściwe jest używanie parametru boolowskiego do określania zachowania?

Od czasu do czasu widziałem praktykę, która „czuje” się źle, ale nie jestem w stanie dokładnie wyartykułować, co jest w tym złego. A może to tylko moje uprzedzenia. Tutaj idzie:

Deweloper definiuje metodę z wartością logiczną jako jeden z jej parametrów, która wywołuje inną, i tak dalej, i ostatecznie ta wartość logiczna jest używana wyłącznie w celu ustalenia, czy podjąć pewne działanie. Może to być wykorzystane, na przykład, do zezwolenia na działanie tylko wtedy, gdy użytkownik ma określone prawa lub być może (lub nie jesteśmy) w trybie testowym, trybie wsadowym lub trybie na żywo, a może tylko wtedy, gdy system jest w trybie określony stan.

Cóż, zawsze istnieje inny sposób, aby to zrobić, pytając, kiedy należy podjąć działanie (zamiast przekazać parametr), lub stosując wiele wersji metody lub wiele implementacji klasy itp. Moje pytanie nie jest Tyle, jak to poprawić, ale raczej, czy tak naprawdę jest źle (jak podejrzewam), a jeśli tak, to co w tym złego.

Tak, prawdopodobnie jest to zapach kodu, który doprowadziłby do niemożliwego do utrzymania kodu, który jest trudny do zrozumienia i ma mniejsze szanse na łatwe ponowne użycie.

Jak zauważyli inni plakaty, kontekst jest wszystkim (nie idź z trudem, jeśli jest to jednorazowy przypadek lub jeśli praktyka została uznana za celowo zaciągnięty dług techniczny, który zostanie ponownie uwzględniony później), ale ogólnie mówiąc, jeśli parametr został przekazany w funkcję, która wybiera określone zachowanie do wykonania, wymagane jest dalsze udoskonalanie; Podział tej funkcji na mniejsze spowoduje uzyskanie bardziej spójnych.

Czym jest więc bardzo spójna funkcja?

Jest to funkcja, która wykonuje jedną i tylko jedną rzecz.

Problem z parametrem przekazanym podczas opisywania polega na tym, że funkcja wykonuje więcej niż dwie rzeczy; może, ale nie musi, sprawdzać prawa dostępu użytkowników w zależności od stanu parametru boolowskiego, a następnie w zależności od tego drzewa decyzyjnego wykona pewną funkcjonalność.

Lepiej byłoby oddzielić obawy związane z kontrolą dostępu od problemów związanych z zadaniem, działaniem lub dowództwem.

Jak już zauważyliście, przeplatanie się tych obaw wydaje się być wyłączone.

Pojęcie kohezyjności pomaga nam zidentyfikować, że dana funkcja nie jest bardzo spójna i że możemy zmienić kod w celu uzyskania zestawu bardziej spójnych funkcji.

Tak więc pytanie może zostać sformułowane ponownie; Biorąc pod uwagę, że wszyscy się zgadzamy, najlepiej unikać przekazywania parametrów selekcji behawioralnej, w jaki sposób poprawiamy sprawy?

Pozbyłbym się parametru całkowicie. Możliwość wyłączenia kontroli dostępu nawet w celu przetestowania stanowi potencjalne zagrożenie bezpieczeństwa. Dla celów testowych obu odgałęzienie lub udawaną czeku dostępie do testowania zarówno dostęp dozwolonych i odmowa dostępu scenariuszy.

Ref: Spójność (informatyka)

Dawno temu przestałem używać tego wzorca, z bardzo prostego powodu; koszty utrzymania. Kilka razy odkryłem, że miałem jakąś funkcję powiedzieć, frobnicate(something, forwards_flag)która była wywoływana wiele razy w moim kodzie i potrzebowałem zlokalizować wszystkie miejsca w kodzie, w których wartość falsezostała przekazana jako wartość forwards_flag. Nie możesz ich łatwo wyszukać, więc staje się to problemem z powodu konserwacji. A jeśli musisz naprawić błąd w każdej z tych stron, możesz mieć niefortunny problem, jeśli przegapisz jedną z nich.

Ale ten konkretny problem można łatwo naprawić bez zasadniczej zmiany podejścia:

enum FrobnicationDirection {
  FrobnicateForwards,
  FrobnicateBackwards;
};

void frobnicate(Object what, FrobnicationDirection direction);

Dzięki temu kodowi wystarczy wyszukać tylko wystąpienia FrobnicateBackwards. Chociaż jest możliwe, że istnieje kod, który przypisuje to do zmiennej, więc musisz podążać za kilkoma wątkami kontroli, w praktyce stwierdzam, że jest to na tyle rzadkie, że ta alternatywa działa OK.

Istnieje jednak inny problem z przekazaniem flagi w ten sposób, przynajmniej w zasadzie. Jest tak, że niektóre (tylko niektóre) systemy o takiej konstrukcji mogą ujawniać zbyt dużą wiedzę na temat szczegółów implementacji głęboko zagnieżdżonych części kodu (wykorzystujących flagę) zewnętrznym warstwom (które muszą wiedzieć, którą wartość przekazać pod tą flagą). Aby użyć terminologii Larry'ego Constantine'a, ten projekt może mieć zbyt silne sprzężenie między ustawiaczem a użytkownikiem flagi boolowskiej. Szczerze mówiąc, trudno jest z całą pewnością powiedzieć na ten temat, nie wiedząc więcej o bazie kodu.

Odnosząc się do konkretnych przykładów, które podajesz, chciałbym mieć pewne obawy w każdym z nich, ale głównie ze względu na ryzyko / poprawność. To znaczy, jeśli twój system musi przekazywać flagi, które wskazują, w jakim stanie jest system, może się okazać, że masz kod, który powinien był wziąć to pod uwagę, ale nie sprawdza parametru (ponieważ nie został przekazany do ta funkcja). Masz więc błąd, ponieważ ktoś pominął przekazanie parametru.

Warto również przyznać, że wskaźnik stanu systemu, który należy przekazać do prawie każdej funkcji, jest w rzeczywistości zmienną globalną. Zastosowanie będzie miało wiele wad zmiennej globalnej. Myślę, że w wielu przypadkach lepszym rozwiązaniem jest hermetyzowanie wiedzy o stanie systemu (lub poświadczeniach użytkownika lub tożsamości systemu) w obiekcie odpowiedzialnym za prawidłowe działanie na podstawie tych danych. Następnie przekazujesz odniesienie do tego obiektu w przeciwieństwie do surowych danych. Kluczową koncepcją tutaj jest enkapsulacja .

To niekoniecznie jest złe, ale może reprezentować zapach kodu .

Podstawowym scenariuszem, którego należy unikać w odniesieniu do parametrów logicznych jest:

public void foo(boolean flag) {
    doThis();
    if (flag)
        doThat();
}

Następnie dzwoniąc zwykle dzwonisz foo(false)i foo(true)zależy to od konkretnego zachowania, które chcesz.

To naprawdę problem, ponieważ jest to przypadek złej spójności. Tworzysz zależność między metodami, która nie jest tak naprawdę konieczna.

To, co powinieneś zrobić w tym przypadku, to odejście doThisi doThatjako osobne i publiczne metody, a następnie:

doThis();
doThat();

lub

doThis();

W ten sposób pozostawiasz poprawną decyzję dzwoniącemu (dokładnie tak, jakbyś przekazał parametr boolowski) bez tworzenia sprzężenia.

Oczywiście nie wszystkie parametry boolowskie są używane w tak zły sposób, ale zdecydowanie jest to zapach kodu i masz prawo podejrzewać, jeśli często widzisz to w kodzie źródłowym.

To tylko jeden przykład rozwiązania tego problemu na podstawie przykładów, które napisałem. Istnieją inne przypadki, w których konieczne będzie inne podejście.

Dobry artykuł Martina Fowlera wyjaśnia bardziej szczegółowo ten sam pomysł.

PS: jeśli metoda foozamiast wywoływania dwóch prostych metod miała bardziej złożoną implementację, wystarczy zastosować małe metody refaktoryzacji i wyodrębnić kod, aby wynikowy kod wyglądał podobnie do implementacji foo, którą napisałem.

Po pierwsze: programowanie nie jest nauką, lecz sztuką. Dlatego bardzo rzadko istnieje „zły” i „właściwy” sposób programowania. Większość standardów kodowania to jedynie „preferencje”, które niektórzy programiści uważają za przydatne; ale ostatecznie są raczej arbitralne. Dlatego nigdy nie nazwałbym wyboru parametru, który sam w sobie byłby „zły” - i na pewno nie byłby czymś tak ogólnym i użytecznym jak parametr boolowski. Użycie boolean(lub intw tym przypadku) do enkapsulacji stanu jest w wielu przypadkach całkowicie uzasadnione.

Ogólnie rzecz biorąc, decyzje dotyczące kodowania powinny opierać się przede wszystkim na wydajności i łatwości konserwacji. Jeśli wydajność nie jest zagrożona (a ja nie wyobrażam sobie, jak mogłaby być kiedykolwiek w twoich przykładach), następnym twoim rozważaniem powinno być: jak łatwe będzie to dla mnie (lub przyszłego redaktora)? Czy to jest intuicyjne i zrozumiałe? Czy to jest izolowane? Twój przykład wywołania funkcji łańcuchowych w rzeczywistości wydaje się potencjalnie kruchy pod tym względem: jeśli zdecydujesz się zmienić bIsUpna bIsDown, ile innych miejsc w kodzie będzie również wymagać zmiany? Ponadto, czy twoja lista parametrów jest balonowa? Jeśli twoja funkcja ma 17 parametrów, to czytelność stanowi problem i musisz ponownie rozważyć, czy doceniasz zalety architektury obiektowej.

Myślę, że artykuł Roberta C Martins Clean stwierdza, że ​​w miarę możliwości należy eliminować argumenty boolowskie, ponieważ pokazują one, że metoda robi więcej niż jedną rzecz. Metoda powinna zrobić jedną rzecz i tylko jedno myślę, że jest jednym z jego motta.

Myślę, że najważniejszą rzeczą jest praktyczność.

Gdy wartość logiczna określa całe zachowanie, po prostu zastosuj drugą metodę.

Gdy wartość logiczna określa tylko trochę zachowania w środku, możesz chcieć zachować ją w jednym, aby ograniczyć powielanie kodu. Tam, gdzie to możliwe, możesz nawet podzielić metodę na trzy: Dwie metody wywoływania dla każdej opcji boolean i jedna, która wykonuje większość pracy.

Na przykład:

private void FooInternal(bool flag)
{
  //do work
}

public void Foo1()
{
  FooInternal(true);
}

public void Foo2()
{
  FooInternal(false);
}

Oczywiście w praktyce zawsze będziesz miał punkt pomiędzy tymi skrajnościami. Zwykle po prostu idę z tym, co wydaje się właściwe, ale wolę się mylić po stronie mniejszego powielania kodu.

Podoba mi się podejście polegające na dostosowywaniu zachowania za pomocą metod konstruktora, które zwracają niezmienne wystąpienia. Oto jak GuavaSplitter go używa:

private static final Splitter MY_SPLITTER = Splitter.on(',')
       .trimResults()
       .omitEmptyStrings();

MY_SPLITTER.split("one,,,,  two,three");

Korzyści z tego są:

• Doskonała czytelność
• Wyraźne rozdzielenie konfiguracji od metod działania
• Promuje spójność, zmuszając do myślenia o tym, czym jest obiekt, co powinno, a czego nie powinno robić. W tym przypadku jest to Splitter. Nigdy nie wprowadziłbyś someVaguelyStringRelatedOperation(List<Entity> myEntities)klasy o nazwie Splitter, ale pomyślałbyś o umieszczeniu jej jako metody statycznej w StringUtilsklasie.
• Instancje są wstępnie skonfigurowane, dlatego można je łatwo wstrzykiwać w zależności. Klient nie musi się martwić o to, czy przejść truelub falseo metodę, aby uzyskać prawidłowe zachowanie.

Zdecydowanie zapach kodu . Jeśli nie narusza to zasady pojedynczej odpowiedzialności , prawdopodobnie narusza powiedz, nie pytaj . Rozważać:

• Czy potrafisz podsumować, co robi twoja metoda bez użycia spójników (i, ale itd.)?
• Czy są części twojej metody, które można łatwo zgrupować w inną mniejszą metodę ? (Obejmuje to złożone tworzenie zmiennej lokalnej ).
• Jeśli twoja flaga określa, którą metodę wywołać dla parametru, to czy parametr robi zbyt wiele ?
• Czy ta metoda należy do jednego z twoich parametrów? (Możesz zauważyć, że rozdziela zmienne instancji w twoim parametrze za pomocą getters / setters .)

Jeśli okaże się, że nie naruszasz jednej z tych dwóch zasad, nadal powinieneś użyć wyliczenia. Flagi boolowskie są logicznym odpowiednikiem liczb magicznych . foo(false)ma tyle samo sensu, co bar(42). Wyliczenia mogą być przydatne do Wzorca Strategii i pozwalają na dodanie kolejnej strategii. (Pamiętaj tylko, aby odpowiednio je nazwać .)

Twój szczególny przykład szczególnie mnie niepokoi. Dlaczego ta flaga przechodzi przez tak wiele metod? Wygląda na to, że musisz podzielić parametr na podklasy .

TL; DR: nie używaj argumentów boolowskich.

Zobacz poniżej, dlaczego są złe i jak je wymienić (pogrubioną twarzą).

Argumenty logiczne są bardzo trudne do odczytania, a zatem trudne do utrzymania. Głównym problemem jest to, że cel jest ogólnie jasny, gdy czytasz sygnaturę metody, w której argument jest nazwany. Jednak w większości języków nazwa parametru nie jest na ogół wymagana. Będziesz miał więc anty-wzorce, takie jak RSACryptoServiceProvider#encrypt(Byte[], Boolean)gdzie parametr boolowski określa, jakiego rodzaju szyfrowanie ma być użyte w funkcji.

Otrzymasz połączenie takie jak:

rsaProvider.encrypt(data, true);

gdzie czytelnik musi sprawdzić podpis metody, aby ustalić, co do cholery truemoże znaczyć. Przekazywanie liczby całkowitej jest oczywiście równie złe:

rsaProvider.encrypt(data, 1);

powiedziałbym ci tyle samo - a raczej: tak samo mało. Nawet jeśli zdefiniujesz stałe, które będą używane dla liczby całkowitej, użytkownicy funkcji mogą po prostu je zignorować i nadal używać wartości literalnych.

Najlepszym sposobem rozwiązania tego jest użycie wyliczenia . Jeśli musisz przekazać wyliczenie RSAPaddingz dwiema wartościami: OAEPlub PKCS1_V1_5wtedy od razu będziesz w stanie odczytać kod:

rsaProvider.encrypt(data, RSAPadding.OAEP);

Wartości logiczne mogą mieć tylko dwie wartości. Oznacza to, że jeśli masz trzecią opcję, musisz przefakturować swój podpis. Zasadniczo nie można tego łatwo wykonać, jeśli problemem jest kompatybilność wsteczna, dlatego trzeba rozszerzyć dowolną klasę publiczną za pomocą innej metody publicznej. To właśnie zrobił Microsoft, kiedy przedstawił, RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding)gdzie użył wyliczenia (lub przynajmniej klasy naśladującej wyliczenie) zamiast wartości logicznej.

Użycie pełnego obiektu lub interfejsu jako parametru może być nawet łatwiejsze na wypadek, gdyby sam parametr musiał zostać sparametryzowany. W powyższym przykładzie sam dopełnienie OAEP można sparametryzować za pomocą wartości skrótu do użycia wewnętrznego. Zauważ, że jest teraz 6 algorytmów mieszających SHA-2 i 4 algorytmy mieszające SHA-3, więc liczba wartości wyliczeniowych może wybuchnąć, jeśli użyjesz tylko jednego wyliczenia zamiast parametrów (jest to prawdopodobnie następna rzecz, którą Microsoft się dowie ).

Parametry boolowskie mogą również wskazywać, że metoda lub klasa nie została dobrze zaprojektowana. Podobnie jak w powyższym przykładzie: żadna biblioteka kryptograficzna inna niż .NET nie używa flagi wypełnienia w sygnaturze metody.

Prawie wszyscy guru oprogramowania, których lubię, ostrzegają przed argumentami logicznymi. Na przykład Joshua Bloch ostrzega przed nimi w bardzo cenionej książce „Effective Java”. Zasadniczo nie należy ich używać. Można argumentować, że można ich użyć, jeśli przypadek jest taki, że jeden parametr jest łatwy do zrozumienia. Ale nawet wtedy: Bit.set(boolean)prawdopodobnie lepiej jest go zaimplementować przy użyciu dwóch metod : Bit.set()i Bit.unset().

Jeśli nie możesz bezpośrednio refaktoryzować kodu, możesz zdefiniować stałe, aby przynajmniej uczynić je bardziej czytelnymi:

const boolean ENCRYPT = true;
const boolean DECRYPT = false;

...

cipher.init(key, ENCRYPT);

jest znacznie bardziej czytelny niż:

cipher.init(key, true);

nawet jeśli wolisz:

cipher.initForEncryption(key);
cipher.initForDecryption(key);

zamiast.

Dziwi mnie, że nikt nie wspomniał o nazwanych parametrach .

Problem, jaki widzę w przypadku flag boolean, polega na tym, że mają one negatywny wpływ na czytelność. Na przykład, co robi truew

myObject.UpdateTimestamps(true);

zrobić? Nie mam pojęcia. Ale co z:

myObject.UpdateTimestamps(saveChanges: true);

Teraz jasne jest, do czego służy parametr, który przekazujemy: mówimy funkcji, aby zapisała zmiany. W takim przypadku, jeśli klasa jest niepubliczna, myślę, że parametr boolowski jest w porządku.

Oczywiście nie możesz zmusić użytkowników swojej klasy do używania nazwanych parametrów. Z tego powodu enumzwykle preferowana jest jedna lub dwie oddzielne metody, w zależności od tego, jak często występuje przypadek domyślny. Właśnie to robi .Net:

//Enum used
double a = Math.Round(b, MidpointRounding.AwayFromZero);

//Two separate methods used
IEnumerable<Stuff> ascendingList = c.OrderBy(o => o.Key);
IEnumerable<Stuff> descendingList = c.OrderByDescending(o => o.Key); 

Nie potrafię do końca wyrazić, co jest w tym złego.

Jeśli wygląda jak zapach kodu, wygląda jak zapach kodu i - cóż - pachnie jak zapach kodu, prawdopodobnie jest to zapach kodu.

Co chcesz zrobić to:

1) Unikaj metod z efektami ubocznymi.

2) Obsługuj niezbędne stany za pomocą centralnej, formalnej maszyny stanów (jak ta ).

Zgadzam się ze wszystkimi obawami związanymi z używaniem parametrów boolowskich, aby nie określać wydajności w celu; poprawa, czytelność, niezawodność, obniżenie złożoności, obniżenie ryzyka związanego ze słabą enkapsulacją i spójnością oraz niższy całkowity koszt posiadania przy utrzymaniu.

Zacząłem projektować sprzęt w połowie lat 70-tych, które teraz nazywamy SCADA (kontrola nadzorcza i akwizycja danych). Były to precyzyjnie dostrojone urządzenia z kodem maszynowym w EPROMie, obsługujące makro-piloty zdalnego sterowania i zbierające szybkie dane.

Logika nazywa się maszynami Mealey & Moore , które nazywamy teraz Maszynami Skończonymi . Muszą być one również wykonane zgodnie z tymi samymi regułami, co powyżej, chyba że jest to maszyna czasu rzeczywistego ze skończonym czasem wykonania, a następnie należy wykonać skróty, aby spełnić ten cel.

Dane są synchroniczne, ale polecenia są asynchroniczne, a logika poleceń jest zgodna z logiką logiczną bez pamięci, ale z sekwencyjnymi poleceniami opartymi na pamięci poprzedniego, obecnego i pożądanego następnego stanu. Aby działało to w najbardziej wydajnym języku maszynowym (tylko 64kB), dołożono wszelkich starań, aby zdefiniować każdy proces w heurystyczny sposób IBM HIPO. Czasami oznaczało to przekazywanie zmiennych logicznych i wykonywanie indeksowanych rozgałęzień.

Ale teraz, z dużą ilością pamięci i łatwą obsługą OOK, enkapsulacja jest dziś niezbędnym składnikiem, ale karą za liczenie kodu w bajtach czasu rzeczywistego i kodu maszynowego SCADA.

Niekoniecznie jest to błędne, ale w konkretnym przykładzie działania zależnym od atrybutu „użytkownika” przekazałbym odwołanie do użytkownika, a nie flagę.

To wyjaśnia i pomaga na wiele sposobów.

Każdy, kto przeczyta instrukcję wywołującą, zauważy, że wynik zmieni się w zależności od użytkownika.

W funkcji, która jest ostatecznie wywoływana, można łatwo wdrożyć bardziej złożone reguły biznesowe, ponieważ można uzyskać dostęp do dowolnego atrybutu użytkownika.

Jeśli jedna funkcja / metoda w „łańcuchu” robi coś innego w zależności od atrybutu użytkownika, bardzo prawdopodobne jest, że podobne zależności od atrybutów użytkownika zostaną wprowadzone do niektórych innych metod w „łańcuchu”.

Przez większość czasu rozważałbym to złe kodowanie. Mogę jednak pomyśleć o dwóch przypadkach, w których może to być dobra praktyka. Ponieważ wiele odpowiedzi już mówi, dlaczego jest źle, oferuję dwa razy, gdy może być dobrze:

Po pierwsze, każde wywołanie w sekwencji ma sens samo w sobie. Sensowne byłoby, gdyby kod wywołujący mógł zostać zmieniony z true na false lub false na true, lub gdyby wywołana metoda mogła zostać zmieniona, aby zamiast parametru przekazywać bezpośrednio parametr boolean. Prawdopodobieństwo dziesięciu takich wywołań z rzędu jest niewielkie, ale może się zdarzyć, a jeśli tak, byłoby dobrą praktyką programistyczną.

Drugi przypadek jest trochę trudny, biorąc pod uwagę, że mamy do czynienia z wartością logiczną. Ale jeśli program ma wiele wątków lub zdarzeń, przekazywanie parametrów jest najprostszym sposobem śledzenia danych specyficznych dla wątku / zdarzenia. Na przykład program może uzyskać dane wejściowe z dwóch lub więcej gniazd. Kod działający dla jednego gniazda może wymagać wygenerowania komunikatów ostrzegawczych, a jeden działający dla drugiego może tego nie robić. Wtedy (w pewnym sensie) sensowne jest, aby zestaw wartości logicznych na bardzo wysokim poziomie był przekazywany przez wiele wywołań metod do miejsc, w których mogą być generowane komunikaty ostrzegawcze. Dane nie mogą być zapisywane (z wyjątkiem dużych trudności) w jakiejkolwiek globalnej, ponieważ wiele wątków lub przeplecionych zdarzeń potrzebowałoby własnej wartości.

Aby mieć pewność, w tym ostatnim przypadku pewnie bym utworzyć klasy / struktury, której treść była tylko logiczną i przekazać , że wokół zamiast. Byłbym prawie pewien, że będę potrzebował innych pól przed zbyt długim czasem - na przykład, gdzie wysłać ostrzeżenia.

Kontekst jest ważny. Takie metody są dość powszechne w iOS. Jako tylko jeden często używany przykład, UINavigationController podaje metodę -pushViewController:animated:, a animatedparametrem jest BOOL. Metoda wykonuje zasadniczo tę samą funkcję w obu przypadkach, ale animuje przejście z jednego kontrolera widoku do drugiego, jeśli podasz TAK, a nie, jeśli podasz NIE. Wydaje się to całkowicie uzasadnione; głupio byłoby podać dwie metody zamiast tej, żebyś mógł zdecydować, czy użyć animacji.

Może być łatwiej usprawiedliwić tego rodzaju rzecz w Objective-C, gdzie składnia nazewnictwa metod zapewnia więcej kontekstu dla każdego parametru niż w językach takich jak C i Java. Niemniej jednak sądzę, że metoda, która przyjmuje pojedynczy parametr, może z łatwością przyjąć wartość logiczną i nadal mieć sens:

file.saveWithEncryption(false);    // is there any doubt about the meaning of 'false' here?