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

Według Czy niewłaściwe jest używanie parametru boolowskiego do określania zachowania? , Wiem, jak ważne jest unikanie używania parametrów boolowskich do określania zachowania, np .:

orginalna wersja

public void setState(boolean flag){
    if(flag){
        a();
    }else{
        b();
    }
    c();
}

Nowa wersja:

public void setStateTrue(){
    a();
    c();
}

public void setStateFalse(){
    b();
    c();
}

Ale co z przypadkiem, w którym parametr boolowski jest używany do określania wartości zamiast zachowań? na przykład:

public void setHint(boolean isHintOn){
    this.layer1.visible=isHintOn;
    this.layer2.visible=!isHintOn;
    this.layer3.visible=isHintOn;
}

Próbuję wyeliminować flagę isHintOn i utworzyć 2 osobne funkcje:

public void setHintOn(){
    this.layer1.visible=true;
    this.layer2.visible=false;
    this.layer3.visible=true;
}

public void setHintOff(){
    this.layer1.visible=false;
    this.layer2.visible=true;
    this.layer3.visible=false;
}

ale zmodyfikowana wersja wydaje się mniej konserwowalna, ponieważ:

1. ma więcej kodów niż oryginalna wersja

2. nie może wyraźnie pokazać, że widoczność warstwy 2 jest przeciwna do opcji podpowiedzi

3. kiedy dodaje się nową warstwę (np .: layer4), muszę ją dodać

   this.layer4.visible=false;
   

   i

   this.layer4.visible=true;  
   

   na setHintOn () i setHintOff () oddzielnie

Więc moje pytanie brzmi: jeśli parametr boolowski jest używany tylko do określania wartości, ale nie zachowań (np .: brak parametru if-else tego parametru), czy nadal zaleca się wyeliminowanie tego parametru boolowskiego?

Projektowanie interfejsu API powinno koncentrować się na tym, co jest najbardziej użyteczne dla klienta interfejsu API, od strony wywołującej .

Na przykład, jeśli ten nowy interfejs API wymaga od dzwoniącego regularnego pisania takiego kodu

if(flag)
    foo.setStateTrue();
else
    foo.setStateFalse();

to powinno być oczywiste, że unikanie parametru jest gorsze niż posiadanie interfejsu API, który umożliwia programowi wywołującemu pisanie

 foo.setState(flag);

Poprzednia wersja powoduje tylko problem, który należy rozwiązać po stronie wywołującej (i prawdopodobnie więcej niż raz). Nie zwiększa to czytelności ani łatwości konserwacji.

Strona implementacyjna nie powinna jednak decydować o tym, jak wygląda publiczny interfejs API. Jeśli funkcja taka jak setHintparametr wymaga mniej kodu podczas implementacji, ale interfejs API w kategoriach setHintOn/ setHintOffwygląda na łatwiejszy w obsłudze dla klienta, można go zaimplementować w ten sposób:

private void setHint(boolean isHintOn){
    this.layer1.visible=isHintOn;
    this.layer2.visible=!isHintOn;
    this.layer3.visible=isHintOn;
}

public void setHintOn(){
   setHint(true);
}

public void setHintOff(){
   setHint(false);
}

Tak więc, chociaż publiczny interfejs API nie ma parametru logicznego, nie ma tutaj zduplikowanej logiki, więc tylko jedno miejsce można zmienić, gdy pojawi się nowe wymaganie (jak w przykładzie pytania).

Działa to również na odwrót: jeśli setStatepowyższa metoda wymaga przełączania między dwoma różnymi fragmentami kodu, fragmenty kodu można przekształcić w dwie różne metody prywatne. Zatem IMHO nie ma sensu szukać kryterium decydującego między „jednym parametrem / jedną metodą” a „zerowymi parametrami / dwiema metodami”, patrząc na elementy wewnętrzne. Spójrz jednak na sposób, w jaki chciałbyś zobaczyć API w roli jego konsumenta.

W razie wątpliwości spróbuj użyć „rozwoju opartego na testach” (TDD), co zmusi Cię do myślenia o publicznym interfejsie API i sposobie jego używania.

Martin Fowler cytuje Kent Beck, zalecając osobne setOn() setOff()metody , ale także mówi, że nie należy tego uważać za nienaruszalne:

Jeśli ciągnąc [sic] Dane z logicznym źródła, takie jak kontrola UI lub źródła danych, wolałbym mieć setSwitch(aValue)niż

if (aValue)
  setOn();
else
  setOff();

Jest to przykład, że interfejs API powinien zostać napisany, aby ułatwić dzwoniącemu, więc jeśli wiemy, skąd pochodzi dzwoniący, powinniśmy zaprojektować interfejs API z uwzględnieniem tych informacji. Argumentuje to również, że czasami możemy zapewnić oba style, jeśli otrzymamy osoby wywołujące na dwa sposoby.

Kolejnym zaleceniem jest użyć flagi wyliczone wartości lub wpisać dać truei falselepiej, nazwy kontekst specyficzne. W przykładzie, showHinta hideHintmoże być lepiej.

Myślę, że łączysz dwie rzeczy w swoim poście, API i implementacji. W obu przypadkach nie sądzę, aby istniała silna zasada, z której można korzystać cały czas, ale należy rozważyć te dwie rzeczy niezależnie (w miarę możliwości).

Zacznijmy od API, zarówno:

public void setHint(boolean isHintOn)

i:

public void setHintOn()
public void setHintOff()

są ważnymi alternatywami w zależności od tego, co twój obiekt ma do zaoferowania i jak klienci będą korzystać z interfejsu API. Jak zauważył Doc, jeśli Twoi użytkownicy mają już zmienną logiczną (z kontrolki interfejsu użytkownika, akcji użytkownika, interfejsu zewnętrznego, API itp.), Pierwsza opcja ma większy sens, w przeciwnym razie wymuszasz dodatkową instrukcję if w kodzie klienta . Jeśli jednak na przykład zmienisz wskazówkę na true podczas rozpoczynania procesu i na false na końcu, pierwsza opcja daje coś takiego:

setHint(true)
// Do your process
…
setHint(false)

podczas gdy druga opcja daje to:

setHintOn()
// Do your process
…
setHintOff()

który IMO jest znacznie bardziej czytelny, więc w tym przypadku wybiorę drugą opcję. Oczywiście nic nie stoi na przeszkodzie, aby zaoferować obie opcje (lub więcej, możesz użyć wyliczenia, jak powiedział Graham, jeśli ma to na przykład większy sens).

Chodzi o to, że powinieneś wybrać swój interfejs API na podstawie tego, co powinien zrobić obiekt i jak klienci będą go używać, a nie na podstawie tego, jak zamierzasz go wdrożyć.

Następnie musisz wybrać sposób implementacji publicznego interfejsu API. Powiedzmy, że wybraliśmy metody setHintOni setHintOffnasz publiczny interfejs API i mają one wspólną logikę, jak w twoim przykładzie. Możesz łatwo wyodrębnić tę logikę za pomocą prywatnej metody (kod skopiowany z Doc):

private void setHint(boolean isHintOn){
    this.layer1.visible=isHintOn;
    this.layer2.visible=!isHintOn;
    this.layer3.visible=isHintOn;
}

public void setHintOn(){
   setHint(true);
}

public void setHintOff(){
   setHint(false);
}

I odwrotnie, powiedzmy, że wybraliśmy setHint(boolean isHintOn)nasz interfejs API, ale odwróćmy twój przykład, z jakiegokolwiek powodu ustawienie podpowiedzi On jest zupełnie inne niż ustawienie jej na Wył. W takim przypadku możemy go zaimplementować w następujący sposób:

public void setHint(boolean isHintOn){
    if(isHintOn){
        // Set it On
    } else {
        // Set it Off
    }    
}

Lub nawet:

public void setHint(boolean isHintOn){    
    if(isHintOn){
        setHintOn()
    } else {
        setHintOff()
   }    
}

private void setHintOn(){
   // Set it On
}

private void setHintOff(){
   // Set it Off 
}

Chodzi o to, że w obu przypadkach najpierw wybraliśmy nasz publiczny interfejs API, a następnie dostosowaliśmy naszą implementację do wybranego interfejsu API (i ograniczeń, które mamy), a nie na odwrót.

Nawiasem mówiąc, myślę, że to samo dotyczy posta, który podłączyłeś, używając parametru boolean do określenia zachowania, tj. Powinieneś zdecydować na podstawie konkretnego przypadku użycia, a nie jakiejś twardej reguły (chociaż w tym konkretnym przypadku zwykle jest to właściwe rozbija go na wiele funkcji).

Po pierwsze: kod nie jest automatycznie mniej konserwowalny, tylko dlatego, że jest nieco dłuższy. Najważniejsza jest przejrzystość.

Teraz, jeśli tak naprawdę masz do czynienia z danymi, to ustawiasz właściwość boolean. W takim przypadku możesz po prostu zapisać tę wartość bezpośrednio i uzyskać widoczność warstwy, tj

bool isBackgroundVisible() {
    return isHintVisible;
}    

bool isContentVisible() {
    return !isHintVisible;
}

^{(Pozwoliłem nadać warstwom rzeczywiste nazwy - jeśli nie masz tego w swoim oryginalnym kodzie, zacznę od tego)}

To wciąż pozostawia pytanie, czy zastosować setHintVisibility(bool)metodę. Osobiście poleciłbym zastąpienie go metodą showHint()i hideHint()- oba będą naprawdę proste i nie będziesz musiał ich zmieniać podczas dodawania warstw. Nie jest to jednak jednoznaczne określenie „dobrze / źle”.

Teraz, jeśli wywołanie funkcji powinno faktycznie zmienić widoczność tych warstw, to faktycznie masz zachowanie. W takim przypadku zdecydowanie zaleciłbym osobne metody.

Parametry logiczne są poprawne w drugim przykładzie. Jak już się zorientowałeś, parametry boolowskie same w sobie nie stanowią problemu. Problematyczne jest zachowanie przełączania na podstawie flagi.

Pierwszy przykład jest jednak problematyczny, ponieważ nazewnictwo oznacza metodę ustawiającą, ale implementacje wydają się być czymś innym. Masz więc antipattern zmieniający zachowanie i wprowadzającą w błąd metodę. Ale jeśli metoda jest w rzeczywistości zwykłym ustawiaczem (bez przełączania zachowań), nie ma z tym problemu setState(boolean). Posiadanie dwóch metod setStateTrue()i setStateFalse()po prostu niepotrzebnie komplikuje rzeczy bez korzyści.

Innym sposobem rozwiązania tego problemu jest wprowadzenie obiektu reprezentującego każdą podpowiedź, który będzie odpowiedzialny za określenie wartości logicznych powiązanych z tą podpowiedź. W ten sposób możesz dodawać nowe permutacje zamiast po prostu mieć dwa stany logiczne.

Na przykład w Javie możesz:

public enum HintState {
    SHOW_HINT(true, false, true),
    HIDE_HINT(false, true, false);

    private HintState(boolean layer1Visible, boolean layer2Visible, boolean layer3Visible) {
         // constructor body and accessors omitted for clarity
    }
}

A wtedy twój kod dzwoniącego wyglądałby tak:

setHint(HintState.SHOW_HINT);

Twój kod implementacyjny wyglądałby tak:

public void setHint(HintState hint) {
    this.layer1Visible = hint.isLayer1Visible();
    this.layer2Visible = hint.isLayer2Visible();
    this.layer3Visible = hint.isLayer3Visible();
}

Dzięki temu kod implementacyjny i kod dzwoniącego są zwięzłe, w zamian za zdefiniowanie nowego typu danych, który wyraźnie odwzorowuje silnie typowane, nazwane intencje na odpowiednie zestawy stanów. Myślę, że tak jest lepiej.

Więc moje pytanie brzmi: jeśli parametr boolowski jest używany tylko do określania wartości, ale nie zachowań (np .: brak parametru if-else tego parametru), czy nadal zaleca się wyeliminowanie tego parametru boolowskiego?

Kiedy mam wątpliwości co do takich rzeczy. Lubię sobie wyobrazić, jak wyglądałby ślad stosu.

Przez wiele lat pracowałem nad projektem PHP, który używał tej samej funkcji co setter i getter . Jeśli wartość minę zero , zwróci wartość, w przeciwnym razie ustaw ją. To było okropne w pracy .

Oto przykład śledzenia stosu:

function visible() : line 440
function parent() : line 398
function mode() : line 384
function run() : line 5

Nie miałeś pojęcia o stanie wewnętrznym, co utrudniło debugowanie. Istnieje wiele innych negatywnych skutków ubocznych, ale spróbuj zobaczyć, że w pełnej nazwie funkcji i przejrzystości jest wartość, gdy funkcje wykonują pojedyncze działanie.

Teraz wyobrażam sobie pracę ze śledzeniem stosu dla funkcji, która ma zachowanie A lub B w oparciu o wartość logiczną.

function bar() : line 92
function setVisible() : line 120
function foo() : line 492
function setVisible() : line 120
function run() : line 5

To mylące, jeśli mnie pytasz. Te same setVisiblelinie dają dwie różne ścieżki śledzenia.

Wróćmy do twojego pytania. Spróbuj wyobrazić sobie, jak będzie wyglądał ślad stosu, jak komunikuje się z osobą, co się dzieje, i zadaj sobie pytanie, czy pomagasz przyszłej osobie w debugowaniu kodu.

Oto kilka porad:

• jasna nazwa funkcji, która sugeruje zamiar bez konieczności znajomości wartości argumentów.
• funkcja wykonuje jedną akcję
• nazwa sugeruje mutację lub niezmienne zachowanie
• rozwiązywać problemy związane z debugowaniem związane ze zdolnością do debugowania języka i narzędzi.

Czasami kod wygląda pod nadmiernym mikroskopem, ale kiedy cofniesz się do większego obrazu, minimalistyczne podejście sprawi, że zniknie. Jeśli potrzebujesz go wyróżnić, abyś mógł go utrzymać. Dodanie wielu małych funkcji może wydawać się zbyt szczegółowe, ale poprawia łatwość konserwacji, gdy jest szeroko stosowane w szerszym kontekście.

W prawie każdym przypadku, gdy przekazujesz booleanparametr do metody jako flagę, aby zmienić zachowanie czegoś, powinieneś rozważyć bardziej wyraźny i bezpieczny sposób, aby to zrobić.

Jeśli nie robisz nic więcej niż używasz tego, Enumktóry reprezentuje stan, poprawiłeś zrozumienie swojego kodu.

W tym przykładzie użyto Nodeklasy z JavaFX:

public enum Visiblity
{
    SHOW, HIDE

    public boolean toggleVisibility(@Nonnull final Node node) {
        node.setVisible(!node.isVisible());
    }
}

jest zawsze lepszy niż, jak na wielu JavaFXobiektach:

public void setVisiblity(final boolean flag);

ale myślę .setVisible()i .setHidden()są najlepszym rozwiązaniem dla sytuacji, w której flaga jest, booleanponieważ jest najbardziej wyraźna i najmniej szczegółowa.

w przypadku wielu opcji wybór jest jeszcze ważniejszy, aby zrobić to w ten sposób. EnumSetistnieje właśnie z tego powodu.

Ed Mann ma naprawdę dobry post na blogu na ten temat. Właśnie miałem sparafrazować to, co mówi, więc aby nie powielać wysiłku, po prostu opublikuję link do jego postu na blogu jako uzupełnienie tej odpowiedzi.

Przy podejmowaniu decyzji między podejściem do jakiegoś interfejsu, który przechodzi parametr (boolowski) a metody przeciążone bez tego parametru, spójrz na klientów zużywających się.

Jeśli wszystkie zastosowania przekazywałyby wartości stałe (np. Prawda, fałsz), to przemawia za tym przeciążenie.

Jeśli wszystkie zastosowania przekazałyby wartość zmiennej, to przemawia za tym metoda z podejściem parametrycznym.

Jeśli żadna z tych skrajności nie ma zastosowania, oznacza to, że istnieje mieszanka zastosowań klienta, więc musisz wybrać, czy wspierać obie formularze, czy też dostosować jedną kategorię klientów do drugiego (co jest dla nich bardziej nienaturalne).

Istnieją dwa czynniki do zaprojektowania:

• API: jaki interfejs przedstawiasz użytkownikowi,
• Wdrożenie: jasność, łatwość konserwacji itp.

Nie należy ich łączyć.

Zupełnie dobrze jest:

• mieć wiele metod w interfejsie API delegowanych do pojedynczej implementacji,
• mieć jedną metodę wysyłania API do wielu implementacji w zależności od warunku.

Jako taki, każdy argument, który próbuje zrównoważyć koszty / korzyści projektu interfejsu API z kosztami / korzyściami projektu wdrożenia, jest wątpliwy i należy go dokładnie zbadać.

Po stronie API

Jako programista ogólnie preferuję programowalne interfejsy API. Kod jest znacznie wyraźniejszy, gdy mogę przekazać wartość, niż gdy potrzebuję instrukcji if/ switchwartości, aby zdecydować, którą funkcję wywołać.

To ostatnie może być konieczne, jeśli każda funkcja oczekuje różnych argumentów.

W takim przypadku jedna metoda setState(type value)wydaje się lepsza.

Jednakże , nie ma nic gorszego niż Nameless true, false, 2, etc ... te wartości nie mają znaczenia magiczne na własną rękę. Unikaj prymitywnej obsesji i używaj silnego pisania.

Tak więc, z POV API, chcę: setState(State state).

Po stronie wdrożenia

Polecam robić wszystko, co jest łatwiejsze.

Jeśli metoda jest prosta, najlepiej trzymać ją razem. Jeśli przepływ kontrolny jest zawiły, najlepiej jest go rozdzielić na wiele metod, z których każda dotyczy podklucza lub kroku potoku.

Na koniec rozważ grupowanie .

W twoim przykładzie (z dodaną białą spacją dla czytelności):

this.layer1.visible = isHintOn;
this.layer2.visible = ! isHintOn;
this.layer3.visible = isHintOn;

Dlaczego layer2przełamywanie trendu? Czy to funkcja, czy błąd?

Czy to możliwe, aby mieć 2 listy [layer1, layer3]i [layer2], z wyraźną nazwą wskazującą powody są one zgrupowane razem, a następnie iteracyjne nad tymi listami.

Na przykład:

for (auto layer : this.mainLayers) { // layer2
    layer.visible = ! isHintOn;
}
for (auto layer : this.hintLayers) { // layer1 and layer3
    layer.visible = isHintOn;
}

Kod mówi sam za siebie, jasne jest, dlaczego istnieją dwie grupy, a ich zachowanie się różni.

Oddzielnie od pytania setOn() + setOff()vs set(flag)zastanowię się, czy typ boolowski jest tutaj najlepszy. Czy jesteś pewien, że nigdy nie będzie trzeciej opcji?

Może warto rozważyć wyliczenie zamiast wartości logicznej. Oprócz umożliwienia rozszerzalności, utrudnia to również uzyskanie niewłaściwej wartości logicznej, np .:

setHint(false)

vs

setHint(Visibility::HIDE)

Dzięki wyliczeniu będzie znacznie łatwiej rozszerzyć, gdy ktoś zdecyduje, że chce opcji „w razie potrzeby”:

enum class Visibility {
  SHOW,
  HIDE,
  IF_NEEDED // New
}

vs

setHint(false)
setHint(true)
setHintAutomaticMode(true) // New

Według ... wiem, jak ważne jest unikanie używania parametrów boolowskich do określania zachowania

Proponuję ponownie ocenić tę wiedzę.

Przede wszystkim nie widzę wniosku, który proponujecie w połączonym ze sobą pytaniu SE. Mówią głównie o przekazywaniu parametru przez kilka etapów wywołań metod, gdzie jest on oceniany bardzo daleko w dół łańcucha.

W twoim przykładzie oceniasz parametr bezpośrednio w swojej metodzie. Pod tym względem nie różni się wcale od żadnego innego parametru.

Ogólnie rzecz biorąc, nie ma absolutnie nic złego w stosowaniu parametrów boolowskich; i oczywiście jakikolwiek parametr determinuje zachowanie lub dlaczego miałbyś je mieć?

Definiowanie pytania

Twoje tytułowe pytanie brzmi: „Czy [...] jest źle?” - ale co masz na myśli mówiąc „źle”?

Według kompilatora C # lub Java, nie jest źle. Jestem pewien, że jesteś tego świadomy i nie o to pytasz. Obawiam się, że poza tym, nprogramiści mają n+1różne opinie. Ta odpowiedź przedstawia, co o tym mówi książka Clean Code .

Odpowiedź

Clean Code zdecydowanie uzasadnia argumenty funkcji w ogóle:

Argumenty są trudne. Biorą dużo mocy pojęciowej. [...] nasi czytelnicy musieliby ją interpretować za każdym razem, gdy ją widzieli.

„Czytnikiem” tutaj może być konsument interfejsu API. Może to być także kolejny program kodujący, który nie wie jeszcze, co robi ten kod - którym możesz być ty za miesiąc. Będą albo przechodzić przez 2 funkcje osobno, albo przez 1 funkcję dwa razy , raz pamiętając truei raz falsena uwadze.
Krótko mówiąc, używaj jak najmniej argumentów .

Konkretny przypadek argumentu flagi jest później adresowany bezpośrednio:

Argumenty dotyczące flagi są brzydkie. Przekazanie funkcji logicznej na funkcję jest naprawdę okropną praktyką. Natychmiast komplikuje to sygnaturę metody, głośno głosząc, że ta funkcja wykonuje więcej niż jedną rzecz. Robi jedną rzecz, jeśli flaga jest prawdziwa, a drugą, jeśli flaga jest fałszywa!

Aby odpowiedzieć bezpośrednio na pytania:
zgodnie z Clean Code zaleca się wyeliminowanie tego parametru.

Dodatkowe informacje:

Twój przykład jest raczej prosty, ale nawet tam widzisz prostotę propagującą się do twojego kodu: funkcje bez parametrów wykonują tylko proste przypisania, podczas gdy druga funkcja musi wykonywać arytmetykę logiczną, aby osiągnąć ten sam cel. W tym uproszczonym przykładzie jest to banalna arytmetyka logiczna, ale w prawdziwej sytuacji może być dość złożona.

Widziałem tutaj wiele argumentów, które powinieneś uzależnić od użytkownika interfejsu API, ponieważ robienie tego w wielu miejscach byłoby głupie:

if (isAfterSunset) light.TurnOn();
else light.TurnOff();

I nie zgadzam się, że coś nie jest optymalne tutaj dzieje. Być może jest to zbyt oczywiste, ale twoje pierwsze zdanie mówi o „znaczeniu unikania [sic] używania parametrów boolowskich do określania zachowania” i to jest podstawa do całego pytania. Nie widzę powodu, aby uczynić to, co jest złe, łatwiejsze dla użytkownika interfejsu API.

Nie wiem, czy wykonujesz testy - w takim przypadku weź także pod uwagę:

Argumenty są jeszcze trudniejsze z testowego punktu widzenia. Wyobraź sobie trudność napisania wszystkich przypadków testowych, aby upewnić się, że wszystkie kombinacje argumentów działają poprawnie. Jeśli nie ma argumentów, jest to trywialne.