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

194

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.

Promień
źródło
1
To pytanie dotyczy tego, gdzie należą decyzje. Przenieś decyzje w centralne miejsce zamiast je zaśmiecać. Dzięki temu złożoność będzie niższa niż współczynnik dwóch ścieżek kodu za każdym razem, gdy masz if.
28
Martin Fowler ma artykuł na ten temat: martinfowler.com/bliki/FlagArgument.html
Christoffer Hammarström
@ ChristofferHammarström Nice link. Uwzględnię to w mojej odpowiedzi, ponieważ szczegółowo wyjaśnia ten sam pomysł mojego wyjaśnienia.
Alex
1
Nie zawsze zgadzam się z tym, co ma do powiedzenia Nick, ale w tym przypadku zgadzam się w 100%: Nie używaj parametrów boolowskich .
Marjan Venema

Odpowiedzi:

107

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 lub czeku dostępie do testowania zarówno dostęp dozwolonych i odmowa dostępu scenariuszy.

Ref: Spójność (informatyka)

Obrabować
źródło
Rob, czy mógłbyś wyjaśnić czym jest Spójność i jak się ona stosuje?
Ray
Ray, im więcej o tym myślę, tym bardziej myślę, że powinieneś sprzeciwić się kodowi opartemu na luce w zabezpieczeniach, którą wprowadza do aplikacji wartość logiczna, którą włącza kontrola dostępu. Poprawa jakości bazy kodu będzie fajnym efektem ubocznym;)
Rob
1
Bardzo ładne wyjaśnienie spójności i jej zastosowania. To naprawdę powinno uzyskać więcej głosów. Zgadzam się również z kwestią bezpieczeństwa ... chociaż wszystkie metody są prywatne, istnieje mniejsza potencjalna luka
Ray
1
Dzięki Ray. Wygląda na to, że łatwo będzie ponownie uwzględnić fakt, gdy pozwoli na to czas. Być może warto zamieścić komentarz do zrobienia TODO, aby podkreślić ten problem, zachowując równowagę między autorytetem technicznym a wrażliwością na presję, jaką czasami jesteśmy pod działaniem.
Rob
„nie do utrzymania”
aaa90210,
149

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 .

James Youngman
źródło
9
Naprawdę świetne konkretne przykłady, a także wgląd w naturę tego, z czym mamy do czynienia i jak na nas wpływa.
Ray
33
+1. Używam do tego w jak największym stopniu wyliczeń. Widziałem funkcje, w których dodatkowe boolparametry zostały dodane później, a wywołania zaczynają wyglądać DoSomething( myObject, false, false, true, false ). Nie można dowiedzieć się, co oznaczają dodatkowe argumenty boolowskie, podczas gdy przy znaczących nazwach wartości wyliczeniowych jest to łatwe.
Graeme Perrow
17
O rany, wreszcie dobry przykład, jak FrobnicateBackwards. Szukałem tego na zawsze.
Alex Pritchard
38

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.

Alex
źródło
1
Dzięki za wywołanie terminu „zapach kodu” - wiedziałem, że pachnie źle, ale nie mogłem zrozumieć, co to za zapach. Twój przykład jest bardzo trafny w to, na co patrzę.
Ray
24
Istnieje wiele sytuacji, w których if (flag) doThat()wnętrze foo()jest uzasadnione. Przekazanie decyzji o wywołaniu doThat()do każdego dzwoniącego wymusza powtórzenie, które będzie musiało zostać usunięte, jeśli później dowiesz się o niektórych metodach, flagzachowanie również musi zadzwonić doTheOther(). Wolałbym raczej umieścić zależności między metodami w tej samej klasie, niż później przeszukać wszystkie wywołujące.
Blrfl
1
@Blrfl: Tak, myślę, że bardziej prostymi refaktoryzacjami byłoby albo utworzenie metod doOnea doBoth(odpowiednio dla fałszywego i prawdziwego przypadku), albo użycie osobnego typu wyliczenia, jak zasugerował James Youngman
hugomg
@missingno: Można by jeszcze mieć ten sam problem pchanie nadmiarowy kod z dzwoniącym, aby doOne()lub doBoth()decyzję. Podprogramy / funkcje / metody mają argumenty, więc ich zachowanie można zmieniać. Używanie wyliczenia dla prawdziwie logicznego warunku przypomina powtarzanie się, jeśli nazwa argumentu już wyjaśnia, co robi.
Blrfl
3
Jeśli wywoływanie dwóch metod jedna po drugiej lub jedna metoda może być uznane za zbędne, to jest również zbędne, jak piszesz blok try-catch, a może jeśli nie. Czy to oznacza, że ​​napiszesz funkcję, aby wyodrębnić je wszystkie? Nie! Bądź ostrożny, tworzenie jednej metody, którą wywołuje tylko dwie inne metody, niekoniecznie stanowi dobrą abstrakcję.
Alex
29

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.

kmote
źródło
4
Doceniam zastrzeżenie z pierwszego akapitu. Byłem celowo prowokujący, mówiąc „źle”, i z pewnością potwierdzam, że mamy do czynienia z „najlepszymi praktykami” i zasadami projektowania oraz że tego rodzaju rzeczy są często sytuacyjne i trzeba ważyć wiele czynników
Ray
13
Twoja odpowiedź przypomina mi cytat, że nie pamiętam źródła - „jeśli twoja funkcja ma 17 parametrów, prawdopodobnie brakuje jednego”.
Joris Timmermans
Bardzo się z tym zgodzę i odniosę się do pytania, aby powiedzieć, że tak, często złym pomysłem jest przekazywanie flagi logicznej, ale nigdy nie jest tak proste jak złe / dobre ...
JohnB
15

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.

dreza
źródło
@ dreza, masz na myśli prawo Curlys .
MattDavey,
Oczywiście z doświadczeniem powinieneś wiedzieć, kiedy zignorować takie argumenty.
gnasher729
8

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.

Jorn
źródło
Używam tylko argumentów logicznych do kontrolowania zachowania w metodach prywatnych (jak pokazano tutaj). Ale problem: jeśli jakiś dufus zdecyduje się zwiększyć widoczność FooInternalw przyszłości, to co?
ADTC
Właściwie wybrałbym inną trasę. Kod wewnątrz FooInternal powinien być podzielony na 4 części: 2 funkcje do obsługi logicznych przypadków prawda / fałsz, jedna dla pracy, która ma miejsce wcześniej, a druga dla pracy, która nastąpi później. Następnie swój Foo1postać: { doWork(); HandleTrueCase(); doMoreWork() }. Idealnie, każda z funkcji doWorki doMoreWorkjest podzielona na (jedną lub więcej) znaczących części dyskretnych działań (tj. Jako osobne funkcje), a nie tylko dwie funkcje w celu podziału.
jpaugh
7

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.
okej
źródło
1
Jestem stronnikiem twojego rozwiązania jako wielkiego miłośnika i ewangelisty Guawy ... ale nie mogę dać ci +1, ponieważ pomijasz tę część, której tak naprawdę szukam, co jest złe (lub śmierdzi) o drugą stronę. Myślę, że w rzeczywistości jest to ukryte w niektórych twoich wyjaśnieniach, więc może gdybyś mógł to jednoznacznie wyjaśnić, lepiej by odpowiedział na pytanie.
Ray
Podoba mi się pomysł oddzielenia metod konfiguracji i działania!
Sher10ck,
Linki do biblioteki Guava są zepsute
Josh Noe
4

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

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 .

Eva
źródło
4

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.

Maarten Bodewes
źródło
3

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); 
BlueRaja - Danny Pflughoeft
źródło
1
Pytanie nie dotyczy tego, co jest lepsze od zachowania determinującego flagę, chodzi o to, czy taka flaga ma zapach, a jeśli tak, to dlaczego
Ray
2
@Ray: Nie widzę różnicy między tymi dwoma pytaniami. W języku, w którym można wymusić użycie nazwanych parametrów lub gdy można mieć pewność, że zawsze będą używane nazwane parametry (np. Metody prywatne) , parametry boolowskie są w porządku. Jeśli nazwane parametry nie mogą być wymuszone przez język (C #), a klasa jest częścią publicznego interfejsu API lub jeśli język nie obsługuje nazwanych parametrów (C ++), więc kod podobny do tego myFunction(true)może zostać napisany, jest to kod zapach.
BlueRaja - Danny Pflughoeft
Podane podejście z parametrami jest jeszcze bardziej błędne. Bez nazwy będzie zmuszony przeczytać dokument API. Jeśli masz imię, myślisz, że nie musisz: ale parametry mogą być błędne. Na przykład można go było użyć do zapisania (wszystkich) zmian, ale później implikacja zmieniła się nieco, aby zapisać tylko duże zmiany (dla pewnej wartości dużej).
Ingo
@ Ingo Nie zgadzam się. To ogólny problem z programowaniem; możesz łatwo zdefiniować inną SaveBignazwę. Każdy kod może zostać zepsuty, ten rodzaj zepsucia nie jest specyficzny dla nazwanych parametrów.
Maarten Bodewes,
1
@Ingo: Jeśli jesteś otoczony idiotami, idź i znajdź pracę gdzie indziej. Właśnie tego potrzebujesz recenzji kodu.
gnasher729
1

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 ).

BaBu
źródło
1

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.

Sunnyskyguy EE75
źródło
0

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”.

James Anderson
źródło
0

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.

RalphChapin
źródło
Wyliczenie WARN, SILENT miałoby większy sens, nawet przy użyciu klasy / struktury, jak napisałeś (tj. Kontekstu ). A może po prostu skonfiguruj rejestrowanie zewnętrzne - nie musisz niczego przekazywać.
Maarten Bodewes
-1

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?
Caleb
źródło
21
Właściwie nie mam pojęcia, co falseoznacza ten file.saveWithEncryptionprzykład. Czy to oznacza, że ​​oszczędza bez szyfrowania? Jeśli tak, to dlaczego na świecie metoda miałaby „z szyfrowaniem” w nazwie? Mogłem zrozumieć taką metodę save(boolean withEncryption), ale kiedy widzę file.save(false), na pierwszy rzut oka nie jest wcale oczywiste, że parametr wskazuje, że będzie to z szyfrowaniem lub bez. Myślę, że tak naprawdę to pierwszy punkt Jamesa Youngmana na temat używania wyliczenia.
Ray
6
Alternatywna hipoteza falseoznacza, że ​​nie zastępuj żadnego istniejącego pliku o tej samej nazwie. Przemyślany przykład Wiem, ale aby się upewnić, musisz sprawdzić dokumentację (lub kod) funkcji.
James Youngman
5
Nazwana metoda, saveWithEncryptionktóra czasami nie oszczędza na szyfrowaniu, jest błędem. Powinno być możliwe file.encrypt().save(), lub jak Java new EncryptingOutputStream(new FileOutputStream(...)).write(file).
Christoffer Hammarström
2
W rzeczywistości kod, który robi coś innego niż mówi, nie działa, a zatem jest błędem. Nie leci, aby utworzyć metodę o nazwie, saveWithEncryption(boolean)która czasami zapisuje bez szyfrowania, tak jak nie leci, aby utworzyć metodę , saveWithoutEncryption(boolean)która czasami zapisuje z szyfrowaniem.
Christoffer Hammarström
2
Metoda jest zła, ponieważ są oczywiście „wątpliwości co do znaczenia„ fałszu ”tutaj. W każdym razie nigdy nie napisałbym takiej metody. Zapisywanie i szyfrowanie to osobne działania, a metoda powinna zrobić jedną rzecz i zrobić to dobrze. Zobacz moje wcześniejsze komentarze, aby uzyskać lepsze przykłady, jak to zrobić.
Christoffer Hammarström