La definizione di una variabile per denominare un argomento metodo è una buona pratica?

Spiegazione delle variabili

Il tuo caso è un esempio del introdurre la spiegazione del refactoring della variabile . In breve, una variabile esplicativa è una variabile che non è strettamente necessaria, ma consente di dare un nome chiaro a qualcosa, con l'obiettivo di aumentare la leggibilità.

Il codice di buona qualità comunica intento al lettore; e in quanto sviluppatore professionale, la leggibilità e la manutenibilità sono i tuoi obiettivi numero uno.

Pertanto, la regola generale che raccomanderei è questa: se lo scopo del tuo parametro non è immediatamente ovvio, sentiti libero di usare una variabile per dargli un buon nome. Penso che questo sia un buone pratiche in generale (a meno che non vengano abusate). Ecco un esempio rapido e forzato: prendi in considerazione:

editButton.Enabled = (_grid.SelectedRow != null && ((Person)_grid.SelectedRow).Status == PersonStatus.Active);

contro il leggermente più lungo, ma probabilmente più chiaro:

bool personIsSelected = (_grid.SelectedRow != null);
bool selectedPersonIsEditable = (personIsSelected && ((Person)_grid.SelectedRow).Status == PersonStatus.Active)
editButton.Enabled = (personIsSelected && selectedPersonIsEditable);

Parametri booleani

In realtà il tuo esempio evidenzia perché i i booleani nelle API sono spesso una cattiva idea - dal lato delle chiamate, non fanno nulla per spiegare cosa sta succedendo. Prendere in considerazione:

ParseFolder(true, false);

Dovresti cercare cosa significano questi parametri; se fossero enumerati, sarebbe molto più chiaro:

ParseFolder(ParseBehaviour.Recursive, CompatibilityOption.Strict);

Modifica

Aggiunti i titoli e scambiato l'ordine dei due paragrafi principali, perché troppe persone si stavano concentrando sulla parte dei parametri booleani (per essere onesti, era il primo paragrafo in origine). Inoltre, ho aggiunto un esempio alla prima parte.

Non scrivere codice che non ti serve.

Se trovi doSomething(true) difficile da comprendere, dovresti aggiungere un commento:

// Do something and prevent the undo.
doSomething(true);

oppure, se la lingua lo supporta, aggiungi il nome del parametro:

doSomething(preventUndo: true);

Altrimenti, affidati al tuo IDE per darti la firma del metodo chiamato:

Casi in cui è utile

L'inserimento di una variabile aggiuntiva può essere utile:

1. Per scopi di debug:

   var productId = this.Data.GetLastProductId();
   this.Data.AddToCart(productId);
   

   Lo stesso codice può essere scritto in una singola riga, ma se vuoi inserire un punto di interruzione prima di aggiungere un prodotto al carrello per vedere se l'id del prodotto è corretto, scrivere due righe anziché una è una buona idea.

2. Se hai un metodo con molti parametri e ogni parametro passa da una valutazione. In una riga, questo può diventare totalmente illeggibile.

   // Even with indentation, this is unreadable.
   var doSomething(
       isSomethingElse ? 0 : this.getAValue(),
       this.getAnotherOne() ?? this.default,
       (a + b + c + d + e) * f,
       this.hello ? this.world : (this.hello2 ? this.world2 : -1));
   

3. Se la valutazione di un parametro è troppo complicata. Un esempio di codice che ho visto:

   // Wouldn't it be easier to have several if/else's (maybe even in a separate method)?
   do(something ? (hello ? world : -1) : (programmers ? stackexchange : (com ? -1 : 0)));
   

Perché non in altri casi?

Perché non dovresti creare variabili aggiuntive in casi semplici?

1. Non a causa dell'impatto sulle prestazioni. Questa sarebbe una supposizione molto sbagliata di uno sviluppatore principiante che è micro-ottimizzazione della sua app. Non ci sono impatti sulle prestazioni nella maggior parte delle lingue, poiché il compilatore inline la variabile. In quelle lingue in cui il compilatore non lo fa, potresti guadagnare qualche microsecondo inserendolo a mano, il che non vale la pena. Non farlo.

2. Ma a causa del rischio di dissociare il nome che date alla variabile al nome del parametro.

   Esempio:

   Supponiamo che il codice originale sia:

   void doSomething(bool preventUndo)
   {
       // Does something very interesting.
       undoHistory.removeLast();
   }
   
   // Later in code:
   var preventUndo = true;
   doSomething(preventUndo);
   

   Successivamente, uno sviluppatore che lavora su doSomething nota che recentemente la cronologia degli annullamenti ha introdotto due nuovi metodi:

   undoHistory.clearAll() { ... }
   undoHistory.disable() { ... }
   

   Ora, preventUndo sembra non molto chiaro. Significa che verrà impedita solo l'ultima azione? O forse significa che l'utente non sarà più in grado di utilizzare la funzione di annullamento? O che la cronologia degli annullamenti verrà cancellata? I nomi più chiari sarebbero:

   doSomething(bool hideLastUndo) { ... }
   doSomething(bool removeAllUndo) { ... }
   doSomething(bool disableUndoFeature) { ... }
   

   Quindi ora hai:

   void doSomething(bool hideLastUndo)
   {
       // Does something very interesting.
       undoHistory.removeLast();
   }
   
   // Later in code, very error prone, while the signature of the method is clear:
   var preventUndo = true;
   doSomething(preventUndo);
   

E riguardo le enumerazioni?

Alcune altre persone hanno suggerito di usare le enumerazioni. Mentre risolve il problema immediato, ne crea uno più grande. Diamo un'occhiata:

enum undoPrevention
{
    keepInHistory,
    prevent,
}

void doSomething(undoPrevention preventUndo)
{
    doTheJob();
    if (preventUndo == undoPrevention.prevent)
    {
        this.undoHistory.discardLastEntry();
    }
}

doSomething(undoPrevention.prevent);

Problemi con questo:

1. È troppo codice. %codice%? Sul serio?! Non voglio scrivere tale if (preventUndo == undoPrevention.prevent) s ogni volta.

2. Aggiungere un elemento all'enum è molto allettante in seguito, se sto usando lo stesso enum da qualche altra parte. Cosa succede se lo modifico in questo modo:

   enum undoPrevention
   {
       keepInHistory,
       prevent,
       keepButDisable, // Keeps the entry in the history, but makes it disabled.
   }
   

   Cosa accadrà ora? Il metodo if funzionerà come previsto? Per evitare ciò, sarebbe necessario scrivere questo metodo sin dall'inizio:

   void doSomething(undoPrevention preventUndo)
   {
       if (![undoPrevention.keepInHistory, undoPrevention.prevent].contains(preventUndo))
       {
           throw new ArgumentException('preventUndo');
       }
   
       doTheJob();
       if (preventUndo == undoPrevention.prevent)
       {
           this.undoHistory.discardLastEntry();
       }
   }
   

   La variante che usa i booleani inizia ad apparire così bella!

   void doSomething(bool preventUndo)
   {
       doTheJob();
       if (preventUndo)
       {
           this.undoHistory.discardLastEntry();
       }
   }
   

Né, non dovresti scrivere metodi con flag booleani.

Per citare Robert C. Martin dice nel suo libro Clean Code (ISBN-13 978-0-13-235088-4), "Passare un booleano in una funzione è una pratica veramente terribile."

Per parafrasarlo, il ragionamento è che il fatto che tu abbia un interruttore vero / falso significa che molto probabilmente il tuo metodo fa due cose diverse (es. "Fai qualcosa con l'annullamento" e "Fai qualcosa senza annullare"), e dovrebbe quindi essere diviso in due metodi diversi (che potrebbero chiamare internamente la stessa cosa).

DoSomething()
{...

DoSomethingUndoable()
{....

Quando guardi due classi per vedere come sono accoppiate, una delle categorie è < em> accoppiamento dati , che si riferisce al codice in una classe che chiama i metodi di un'altra classe e passa solo dati, come il mese in cui si desidera creare un report, e un'altra categoria è controllo accoppiamento , chiamare metodi e passare qualcosa che controlla il comportamento del metodo. L'esempio che uso in classe è un verbose flag o un reportType flag, ma preventUndo è anche un ottimo esempio. Come hai appena dimostrato, l'accoppiamento di controllo rende difficile leggere il codice di chiamata e capire cosa sta succedendo. Rende anche il codice chiamante vulnerabile alle modifiche in doSomething() che utilizzano lo stesso flag per controllare sia l'annullamento che l'archiviazione, oppure aggiungere un altro parametro a doSomething() per controllare l'archiviazione, quindi rompere il codice e così via.

Il problema è che il codice è troppo strettamente accoppiato. Passare a un bool per controllare il comportamento è, a mio avviso, un segno di una cattiva API. Se possiedi l'API, modificala. Due metodi, doSomething() e doSomethingWithUndo() , sarebbero migliori. se non lo possiedi, scrivi tu stesso due metodi wrapper nel codice che possiedi, e chiedi a uno di loro di chiamare doSomething(true) e l'altro chiama doSomething(false) .

Non è il ruolo del chiamante definire il ruolo degli argomenti. Vado sempre per la versione inline e controllo la firma della funzione chiamata se ho un dubbio.

La variabile dovrebbe essere chiamata dopo il loro ruolo nell'ambito corrente. Non l'ambito in cui vengono inviati.

Quello che farei in JavaScript è la funzione prendere un oggetto come l'unico parametro qualcosa del genere:

function doSomething(settings) {
    var preventUndo = settings.hasOwnProperty('preventUndo') ? settings.preventUndo : false;
    // Deal with preventUndo in the normal way.
}

Quindi chiamalo con:

doSomething({preventUndo: true});

Sono appassionato di sfruttare le funzionalità del linguaggio per aiutare me stesso a fare chiarezza. Ad esempio in C # è possibile specificare i parametri per nome:

 CallSomething(name: "So and so", age: 12, description: "A random person.");

In JavaScript, di solito preferisco utilizzare un oggetto opzioni come argomento per questo motivo:

function doSomething(args) { /*...*/ }

doSomething({ name: 'So and so', age: 12, description: 'A random person.' });

Questa è solo la mia preferenza. Suppongo che dipenderà molto dal metodo che viene chiamato e da come l'IDE aiuta lo sviluppatore a capire la firma.

Trovo che questo sia il più semplice e facile da leggere:

enum Undo { ALLOW, PREVENT }

doSomething(Undo u) {
    if (Undo.ALLOW == u) {
        // save stuff to undo later.
    }
    // do your thing
}

doSomething(Undo.ALLOW);

MainMa non gli piace. Potremmo dover accettare di non essere d'accordo, oppure possiamo usare una soluzione che Joshua Bloch propone:

enum Undo {
    ALLOW {
        @Override
        public void createUndoBuffer() {
            // put undo code here
        }
    },
    PREVENT {
        @Override
        public void createUndoBuffer() {
            // do nothing
        }
    };

    public abstract void createUndoBuffer();
}

doSomething(Undo u) {
    u.createUndoBuffer();
    // do your thing
}

Ora, se aggiungi mai un Undo.LIMITED_UNDO, il tuo codice non verrà compilato a meno che non implementi il metodo createUndoBuffer (). E in doSomething () non c'è se (Undo.ALLOW == u). L'ho fatto in entrambi i modi e il secondo metodo è abbastanza pesante e un po 'difficile capire quando l'enumerazione di Undo si espande a pagine e pagine di codice, ma ti fa pensare. Di solito passo con il metodo più semplice per sostituire un semplice booleano con enum a 2 valori finché non ho motivo di cambiare. Quando aggiungo un terzo valore, utilizzo il mio IDE in "Trova-usi" e risolvo tutto allora.

Penso che la tua soluzione renda il tuo codice un po 'più leggibile, ma eviterei di definire una variabile extra solo per rendere più chiara la tua funzione. Inoltre, se si desidera utilizzare una variabile extra, la contrassegnerei come costante se il linguaggio di programmazione lo supporta.

Posso pensare a due alternative che non implicano una variabile extra:

1. Utilizza un commento extra

doSomething(/* preventUndo */ true);

2. Utilizza un enum a due valori anziché il valore booleano

enum Options
{
    PreventUndo,
    ...
}

...

doSomething(PreventUndo);

Puoi utilizzare l'alternativa 2 se la tua lingua supporta le enumerazioni.

Modifica

Naturalmente, usare gli argomenti con nome è anche un'opzione, se la lingua li supporta.

Riguardo la codifica aggiuntiva necessaria per l'enumerazione, mi sembra davvero trascurabile. Invece di

if (preventUndo)
{
    ...
}

hai

if (undoOption == PreventUndo)
{
    ...
}

o

switch (undoOption)
{
  case PreventUndo:
  ...
}

E anche se è un po 'più di digitazione, ricorda che il codice viene scritto una volta e letto molte volte, quindi può valere la pena di digitare un po' di più ora per trovare un codice più leggibile sei mesi più tardi.

Sono d'accordo con ciò che @GlenPeterson ha detto:

doSomething(Undo.ALLOW); // call using a self evident enum

ma anche l'intestazione sarebbe la seguente poiché mi sembra che ci siano solo due possibilità (vero o falso)

//Two methods    

doSomething()  // this method does something but doesn't prevent undo

doSomethingPreventUndo() // this method does something and prevents undo

Dato che stai chiedendo principalmente di javascript, usando il coffeescript puoi avere un argomento con nome come la sintassi, super facile:

# declare method, ={} declares a default value to prevent null reference errors
doSomething = ({preventUndo} = {}) ->
  if preventUndo
    undo = no
  else
    undo = yes

#call method
doSomething preventUndo: yes
#or 
doSomething(preventUndo: yes)

compila in

var doSomething;

doSomething = function(_arg) {
  var preventUndo, undo;
  preventUndo = (_arg != null ? _arg : {}).preventUndo;
  if (preventUndo) {
    return undo = false;
  } else {
    return undo = true;
  }
};

doSomething({
  preventUndo: true
});

doSomething({
  preventUndo: true
});

Divertiti con link per provare le possibilità

Solo per una soluzione meno convenzionale e poiché l'OP ha menzionato Javascript, una possibilità è di usare un array associativo per mappare i valori. Il vantaggio su un singolo booleano è che puoi avere tutti gli argomenti che vuoi e non preoccuparti della loro sequenza.

var doSomethingOptions = new Array();

doSomethingOptions["PREVENTUNDO"] = true;
doSomethingOptions["TESTMODE"] = false;

doSomething( doSomethingOptions );

// ...

function doSomething( doSomethingOptions ){
    // Check for null here
    if( doSomethingOptions["PREVENTUNDO"] ) // ...
}

Mi rendo conto che questo è un riflesso di qualcuno con uno sfondo strongmente tipizzato e potrebbe non essere così pratico, ma consideralo come originale.

Un'altra possibilità è avere un oggetto ed è anche possibile in Javascript. Non sono sicuro al 100% che questo sia sintatticamente corretto. Dai un'occhiata a modelli come Factory per ulteriore ispirazione.

function SomethingDoer( preventUndo ) {
    this.preventUndo = preventUndo;
    this.doSomething = function() {
            if( this.preventUndo ){
                    // ...
            }
    };
}

mySomethingDoer = new SomethingDoer(true).doSomething();

Il focus della tua domanda e delle altre risposte sta migliorando la leggibilità in cui viene chiamata la funzione, che è un punto di messa a fuoco con cui sono d'accordo. Qualsiasi linea guida specifica, l'evento "nessun argomento booleano", dovrebbe sempre funzionare come mezzo per raggiungere questo scopo e non diventare e finire se stessi.

Penso sia utile notare che questo problema viene per lo più evitato quando il linguaggio di programmazione supporta argomenti argomenti / parole chiave, come in C # 4 e Python, o dove gli argomenti del metodo sono intercalati nel nome del metodo, come in Smalltalk o Obiettivo -C.

Esempi:

// C# 4
foo.doSomething(preventUndo: true);

# Python
foo.doSomething(preventUndo=True)

// Objective-C
[foo doSomethingWith:bar shouldPreventUndo:YES];

Si dovrebbe evitare di passare le variabili booleane come argomenti alle funzioni. Perché le funzioni dovrebbero fare una cosa alla volta. Passando la variabile booleana la funzione ha due comportamenti. Questo crea anche problemi di leggibilità in una fase successiva o per altri programmatori che tendono a vedere il tuo codice. Questo crea anche problemi nel testare la funzione. Probabilmente in questo caso devi creare due casi di test. Immagina che se hai una funzione che ha l'istruzione switch e ne modifica il comportamento in base al tipo di switch, devi avere così tanti diversi casi di test definiti per esso.

Ogni volta che si incontra un argomento booleano per la funzione, devono modificare il codice in modo tale da non scrivere affatto un argomento booleano.

int preventUndo = true;
doSomething(preventUndo);

Un buon compilatore per linguaggi a bassa lavel come C ++ ottimizzerà il codice macchina di sopra 2 linee come di seguito:

doSomething(true); 

quindi non ha importanza nelle prestazioni, ma aumenterà la leggibilità.

Inoltre è utile usare una variabile nel caso in cui diventi variabile più tardi e possa accettare anche altri valori.