È sbagliato utilizzare un parametro booleano per determinare i valori?

La progettazione dell'API dovrebbe concentrarsi su ciò che è più utilizzabile per un client dell'API, dal lato chiamante .

Ad esempio, se questa nuova API richiede al chiamante di scrivere regolarmente un codice come questo

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

allora dovrebbe essere ovvio che evitare il parametro è peggio di avere un'API che consente al chiamante di scrivere

 foo.setState(flag);

La precedente versione produce solo un problema che deve essere risolto dal lato chiamante (e probabilmente più di una volta). Ciò non aumenta né la leggibilità né la manutenibilità.

Il lato dell'implementazione , tuttavia, non dovrebbe dettare l'aspetto dell'API pubblica. Se una funzione come setHint con un parametro richiede meno codice nell'implementazione, ma un'API in termini setHintOn / setHintOff sembra più facile da usare per un client, è possibile implementarla in questo modo:

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);
}

Quindi, se l'API pubblica non ha parametri booleani, qui non esiste una logica duplicata, quindi solo un luogo da modificare quando arriva un nuovo requisito (come nell'esempio della domanda).

Questo funziona anche al contrario: se il metodo setState di cui sopra ha bisogno di passare tra due diversi pezzi di codice, tali parti di codice possono essere refactored a due diversi metodi privati. Quindi IMHO non ha senso cercare un criterio per decidere tra "un parametro / un metodo" e "zero parametri / due metodi" osservando gli interni. Guarda, comunque, come vorresti vedere l'API nel ruolo di un consumatore.

In caso di dubbi, prova a utilizzare "test driven development" (TDD), che ti costringerà a pensare all'API pubblica ea come utilizzarla.

Martin Fowler cita Kent Beck in raccomandando metodi separati setOn() setOff() , ma dice anche che questo non dovrebbe essere considerato inviolabile:

If you pulling[sic] data from a boolean source, such as a UI control or data source, I'd rather have setSwitch(aValue) than

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

This is an example that an API should be written to make it easier for the caller, so if we know where the caller is coming from we should design the API with that information in mind. This also argues that we may sometimes provide both styles if we get callers in both ways.

Un altro consiglio è utilizzare un valore enumerato o un tipo di flag per dare true e false migliori, nomi specifici del contesto. Nel tuo esempio, showHint e hideHint potrebbero essere migliori.

Penso che stai mescolando due cose nel tuo post, l'API e l'implementazione. In entrambi i casi non penso che ci sia una regola strong che puoi usare sempre, ma dovresti considerare queste due cose in modo indipendente (il più possibile).

Iniziamo con l'API, entrambi:

public void setHint(boolean isHintOn)

e

public void setHintOn()
public void setHintOff()

sono valide alternative a seconda di cosa l'oggetto dovrebbe offrire e di come i tuoi clienti utilizzeranno l'API. Come sottolineato da Doc, se i tuoi utenti hanno già una variabile booleana (da un controllo dell'interfaccia utente, un'azione dell'utente, un'estensione, un'API, ecc.) La prima opzione ha più senso, altrimenti devi solo forzare un'istruzione extra se sul codice del client . Tuttavia, se ad esempio stai cambiando il suggerimento su true all'inizio di un processo e su false alla fine, la prima opzione ti dà qualcosa del tipo:

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

mentre la seconda opzione ti dà questo:

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

quale IMO è molto più leggibile, quindi vado con la seconda opzione in questo caso. Ovviamente, nulla ti impedisce di offrire entrambe le opzioni (o più, potresti usare un enum come diceva Graham se per esempio ha più senso).

Il punto è che dovresti scegliere la tua API in base a ciò che l'oggetto deve fare e in che modo i client la useranno, non in base a come la implementerai.

Quindi devi scegliere come implementare la tua API pubblica. Diciamo che abbiamo scelto i metodi setHintOn e setHintOff come nostra API pubblica e condividono questa logica comune come nell'esempio. Si potrebbe facilmente astrarre questa logica attraverso un metodo privato (codice copiato da 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);
}

Al contrario, diciamo che abbiamo selezionato setHint(boolean isHintOn) una nostra API, ma invertiamo il tuo esempio, a causa di qualsiasi motivo l'attivazione del suggerimento è completamente diversa per impostarlo su Off. In questo caso potremmo implementarlo come segue:

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

O anche:

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

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

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

Il punto è che, in entrambi i casi, abbiamo prima scelto la nostra API pubblica e poi adattato la nostra implementazione per adattarla all'API scelta (e ai vincoli che abbiamo), non viceversa.

A proposito, penso che lo stesso vale per il post che hai linkato sull'uso di un parametro booleano per determinare il comportamento, ovvero dovresti decidere in base al tuo particolare caso d'uso piuttosto che a qualche regola difficile (anche se in quel caso specifico di solito la cosa corretta da fare è suddividerla in più funzioni).

Per prima cosa: il codice non è automaticamente meno gestibile, solo perché è un po 'più lungo. La chiarezza è ciò che conta.

Ora, se sei veramente che si occupa solo di dati, allora quello che hai è un setter per una proprietà booleana. In tal caso, potresti voler semplicemente memorizzare quel valore direttamente e derivare la visibilità del livello, cioè

bool isBackgroundVisible() {
    return isHintVisible;
}    

bool isContentVisible() {
    return !isHintVisible;
}

^{(Mi sono preso la libertà di fornire i nomi effettivi dei livelli - se non lo hai nel tuo codice originale, inizierei con quello)}

Questo ti lascia ancora la domanda se avere un metodo setHintVisibility(bool) . Personalmente, consiglierei di sostituirlo con un metodo showHint() e hideHint() - entrambi saranno veramente semplici e non dovrai cambiarli quando aggiungi dei livelli. Tuttavia, non è chiaro o sbagliato.

Ora se chiamare la funzione dovrebbe effettivamente cambiare la visibilità di quei livelli, in realtà hai un comportamento. In tal caso, consiglierei sicuramente metodi separati.

I parametri booleani vanno bene nel secondo esempio. Come hai già capito, i parametri booleani non sono di per sé problematici. Sta cambiando il comportamento basato su un flag, che è problematico.

Il primo esempio è tuttavia problematico, perché la denominazione indica un metodo setter, ma le implementazioni sembrano essere qualcosa di diverso. Quindi hai il metodo di commutazione del comportamento, e un metodo chiamato in modo fuorviante. Ma se il metodo in realtà è un setter regolare (senza cambio di comportamento), allora non ci sono problemi con setState(boolean) . Avere due metodi, setStateTrue() e setStateFalse() complica inutilmente le cose senza alcun beneficio.

Un altro modo per risolvere questo problema è introdurre un oggetto per rappresentare ogni suggerimento e far sì che l'oggetto sia responsabile della determinazione dei valori booleani associati a quel suggerimento. In questo modo puoi aggiungere nuove permutazioni invece di avere semplicemente due stati booleani.

Ad esempio, in Java, puoi fare:

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
    }
}

Quindi il codice del chiamante sarà simile a questo:

setHint(HintState.SHOW_HINT);

E il tuo codice di implementazione sarebbe simile a questo:

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

In questo modo il codice di implementazione e il codice del chiamante vengono concisi, in cambio della definizione di un nuovo tipo di dati che mappa in modo chiaro e strongmente definito, intenzioni nei corrispondenti gruppi di stati. Penso che sia meglio tutto intorno.

Definizione della domanda

La domanda per il titolo è "È sbagliato [...]?" - ma cosa intendi con "sbagliato"?.

Secondo un compilatore C # o Java, non è sbagliato. Sono certo che ne sei consapevole e non è quello che stai chiedendo. Temo che a parte questo, abbiamo solo n programmatori n+1 opinioni diverse. Questa risposta presenta ciò che il libro Pulisci codice ha da dire su questo.

risposta

Pulisci codice crea una valida argomentazione contro gli argomenti della funzione in generale:

Arguments are hard. They take a lot of conceptual power. [...] our readers would have had to interpret it each time they saw it.

Un "lettore" qui può essere il consumatore dell'API. Può anche essere il prossimo programmatore, che non sa ancora cosa fa questo codice - che potresti essere tu tra un mese. Passeranno attraverso 2 funzioni separatamente o tramite 1 funzione due volte , tenendo in mente true e una volta false in mente.
In breve, utilizza il minor numero possibile di argomenti .

Il caso specifico di un argomento flag viene successivamente indirizzato direttamente:

Flag arguments are ugly. Passing a boolean into a function is a truly terrible practice. It immediately complicates the signature of the method, loudly proclaiming that this function does more than one thing. It does one thing if the flag is true and another if the flag is false!

Per rispondere direttamente alle tue domande:
In base a Clean Code , si consiglia di eliminare tale parametro.

Ulteriori informazioni:

Il tuo esempio è piuttosto semplice, ma anche lì puoi vedere la semplicità che si propaga nel tuo codice: le funzioni senza parametri eseguono solo assegnazioni semplici, mentre l'altra funzione deve eseguire l'aritmetica booleana per raggiungere lo stesso obiettivo. È banale aritmetica booleana in questo esempio semplificato, ma potrebbe essere piuttosto complessa in una situazione reale.

Ho visto molti argomenti qui che dovresti renderlo dipendente dall'utilizzatore dell'API, perché doverlo fare in molti posti sarebbe stupido:

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

I do sono d'accordo che qualcosa di non-ottimale sta accadendo qui. Forse è troppo ovvio da vedere, ma la tua primissima frase menziona "l'importanza di evitare [sic] usando parametri booleani per determinare un comportamento" e questa è la base dell'intera domanda. Non vedo un motivo per rendere quella cosa che è male fare più facilmente per l'utente API.

Non so se esegui test, in questo caso considera anche questo:

Arguments are even harder from a testing point of view. Imagine the difficulty of writing all the test cases to ensure that all the various combinations of arguments work properly. If there are no arguments, this is trivial.

So my question is, if the boolean parameter is used to determine values only, but not behaviours (eg:no if-else on that parameter), is it still recommended to eliminate that boolean parameter?

Quando ho dei dubbi su queste cose. Mi piace immaginare come dovrebbe apparire la traccia dello stack.

Per molti anni ho lavorato a un progetto PHP che utilizzava la stessa funzione di setter e getter . Se hai passato null , restituirebbe il valore, altrimenti impostalo. È stato orribile lavorare con .

Ecco un esempio di una traccia dello stack:

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

Non avevi idea dello stato interno e ha reso più difficile il debug. Ci sono un sacco di altri effetti collaterali negativi, ma prova a vedere che c'è value in un verbose nome della funzione e chiarezza quando le funzioni eseguono < em> singola azione .

Ora lavoriamo con la traccia dello stack per una funzione che ha un comportamento A o B basato su un valore booleano.

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

Questo è confuso se me lo chiedi. Le stesse righe setVisible producono due diversi percorsi di tracciamento.

Quindi torna alla tua domanda. Prova a immaginare quale sarà la traccia dello stack, come comunicherà a una persona cosa sta succedendo e chiediti se stai aiutando una persona futura a eseguire il debug del codice.

Ecco alcuni suggerimenti:

• un nome di funzione chiaro che implica l'intento senza dover conoscere i valori dell'argomento.
• una funzione esegue una singola azione
• il nome implica una mutazione o un comportamento immutabile
• risolvere le sfide di debug relative alla capacità di debug del linguaggio e degli strumenti.

A volte il codice sembra eccessivo al microscopio, ma quando si tira indietro l'immagine più grande un approccio minimalista lo farà scomparire. Se ne hai bisogno per distinguerti in modo da poterlo mantenere. L'aggiunta di molte piccole funzioni potrebbe sembrare eccessivamente prolissa, ma migliora la gestibilità se utilizzata in senso ampio in un contesto più ampio.

In quasi tutti i casi in cui stai passando un parametro boolean a un metodo come flag per modificare il comportamento di qualcosa, dovresti prendere in considerazione un esplicito e modo tipicamente sicuro per farlo.

Se non fai altro che usare un Enum che rappresenta lo stato in cui hai migliorato la comprensione del tuo codice.

Questo esempio utilizza la classe Node da JavaFX :

public enum Visiblity
{
    SHOW, HIDE

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

è sempre migliore di, come trovato su molti oggetti JavaFX :

public void setVisiblity(final boolean flag);

ma penso che .setVisible() e .setHidden() siano la soluzione migliore per la situazione in cui il flag è un boolean in quanto è il più esplicito e il meno dettagliato.

nel caso di qualcosa con selezioni multiple, è ancora più importante farlo in questo modo. EnumSet esiste solo per questo motivo.

Ed Mann has a really good blog post on just this subject. I was about to paraphrase just what he says, so not to duplicate effort I will just post a link to his blog post as an addendum to this answer.

Quando decidi tra un approccio per alcune interfacce che passa un parametro (booleano) rispetto a metodi sovraccaricati senza detto parametro, guarda ai client che consumano.

Se tutti gli usi dovessero passare valori costanti (ad es. true, false), allora questo argomenta per i sovraccarichi.

Se tutti gli utilizzi superassero un valore variabile, allora questo argomenta per il metodo con approccio ai parametri.

Se nessuno di questi estremi si applica, significa che c'è un mix di usi del client, quindi devi scegliere se supportare entrambe le forme o fare in modo che una categoria di client si adatti all'altro (ciò che per loro è più innaturale ) stile.

Ci sono due considerazioni da progettare:

API
• : quale interfaccia si presenta all'utente,
• Implementazione: chiarezza, manutenibilità, ecc.

Non dovrebbero essere confusi.

Va perfettamente bene a:

• avere più metodi nel delegato dell'API in una singola implementazione,
• hanno un unico metodo nella distribuzione dell'API a più implementazioni a seconda della condizione.

Pertanto, qualsiasi argomento che cerchi di bilanciare i costi / benefici di un progetto API con i costi / benefici di un progetto di implementazione è dubbio e dovrebbe essere esaminato attentamente.

Dal lato dell'API

Come programmatore, generalmente preferisco le API programmabili. Il codice è molto più chiaro quando posso inoltrare un valore rispetto a quando ho bisogno di un'istruzione if / switch sul valore per decidere quale funzione chiamare.

Quest'ultimo può essere necessario se ogni funzione si aspetta argomenti diversi.

Nel tuo caso, quindi, un solo metodo setState(type value) sembra migliore.

Tuttavia , non c'è niente di peggio di% senza nome true , false , 2 , ecc ... quei valori magici non hanno alcun significato da soli. Evita ossessione primitiva e accetta una strong digitazione.

Quindi, da un punto di vista dell'API, desidero: setState(State state) .

Sul lato dell'implementazione

Raccomando di fare qualsiasi cosa sia più semplice.

Se il metodo è semplice, è meglio tenerlo insieme. Se il flusso di controllo è convoluto, è meglio separarlo in più metodi, ognuno dei quali riguarda un sottocaso o un passaggio della pipeline.

Infine, considera il raggruppamento .

Nel tuo esempio (con spazio bianco aggiunto per la leggibilità):

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

Perché layer2 contrasta con la tendenza? È una funzionalità o è un bug?

Potrebbe essere possibile avere 2 liste [layer1, layer3] e [layer2] , con un nome esplicito che indica il motivo per cui sono raggruppati e quindi scorrere su quegli elenchi.

Ad esempio:

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

Il codice parla da solo, è chiaro perché ci sono due gruppi e il loro comportamento è diverso.

Separatamente dalla domanda setOn() + setOff() vs set(flag) , considererei attentamente se un tipo booleano è il migliore qui. Sei sicuro che non ci sarà mai una terza opzione?

Potrebbe valere la pena considerare un enum invece di un booleano. Oltre a consentire l'estensibilità, anche questo rende più difficile riportare il valore booleano al verso sbagliato, ad esempio:

setHint(false)

vs

setHint(Visibility::HIDE)

Con l'enum, sarà molto più facile estendere quando qualcuno decide di volere un'opzione "se necessario":

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

vs

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

According to ..., I know the importance of avoid using boolean parameters to determine a behaviour

Suggerirei di rivalutare questa conoscenza.

Prima di tutto, non vedo la conclusione che stai proponendo nella domanda SE che hai collegato. Si tratta principalmente di inoltrare un parametro su più passaggi di chiamate di metodo, dove viene valutato molto in basso nella catena.

Nel tuo esempio, stai valutando il parametro giusto nel tuo metodo. A tale riguardo, non differisce affatto da nessun altro tipo di parametro.

In generale, non c'è assolutamente nulla di sbagliato nell'usare i parametri booleani; e ovviamente qualsiasi parametro determinerà un comportamento, o perché lo avresti in primo luogo?