Questo uso di condizionali è un anti-modello?

Non so su anti-pattern, ma estrarrei tre metodi da questo.

Il primo eseguirà del lavoro e restituirà un valore booleano.

Il secondo eseguirà tutto il lavoro eseguito da "qualche altro codice"

Il terzo eseguirà il lavoro ausiliario se la booleana restituita fosse vera.

I metodi estratti sarebbero probabilmente privati se fosse importante chiamare il secondo (e sempre) se il primo metodo restituisse true.

Assegnando nomi ai metodi, spero che renderebbe il codice più chiaro.

Qualcosa del genere:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Ovviamente è necessario discutere se i ritorni anticipati siano accettabili, ma questo è un dettaglio di implementazione (come lo standard di formattazione del codice).

Il punto è che l'intento del codice diventa più chiaro, che è buono ...

Uno dei commenti sulla domanda suggerisce che questo modello rappresenta un odore , e sarei d'accordo. Vale la pena guardarlo per vedere se è possibile rendere più chiaro l'intento.

Penso che la bruttezza sia dovuta al fatto che c'è un sacco di codice in un singolo metodo, e / o le variabili sono malamente nominate (entrambe sono code odori a loro volta - gli antipattern sono cose più astratte e complesse IMO).

Quindi se estrai la maggior parte del codice in metodi di livello inferiore come suggerisce @Bill, il resto diventa pulito (almeno per me). Per es.

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Oppure puoi anche eliminare completamente il flag locale nascondendo il flag check nel secondo metodo e usando un modulo come

calculateTaxRefund(isTaxRefundable(...), ...)

Nel complesso, non vedo che una variabile flag locale sia necessariamente di per sé negativa. Quale opzione di cui sopra è più leggibile (= preferibile a me) dipende dal numero di parametri del metodo, dai nomi scelti e dalla forma che è più coerente con la logica interna del codice.

todo è davvero un brutto nome per la variabile, ma penso che potrebbe essere tutto ciò che è sbagliato. È difficile essere completamente sicuri senza il contesto.

Diciamo che la seconda parte della funzione ordina un elenco, creato dalla prima parte. Questo dovrebbe essere molto più leggibile:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Tuttavia, anche il suggerimento di Bill è corretto. Questo è più leggibile ancora:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Il modello della macchina di stato mi sembra a posto. Gli anti pattern in là sono "todo" (pessimo nome) e "molto codice".

Dipende davvero. Se il codice protetto da todo (spero che tu non stia usando quel nome per davvero come è completamente non-mnemonico!) È un codice concettualmente clean-up, allora hai un anti-pattern e dovresti usare qualcosa come C ++ Costrutto di using di RAII o C # per gestire invece le cose.

D'altra parte, se è concettualmente non una fase di pulizia ma piuttosto solo un'ulteriore elaborazione a volte necessaria e dove la decisione di farlo deve essere presa prima, ciò che è scritto va bene . Considerare se i singoli frammenti di codice dovrebbero essere meglio rifattorizzati nelle proprie funzioni, naturalmente, e anche se si è catturato il significato della variabile flag nel suo nome, ma questo modello di codice di base è OK. In particolare, provare a mettere troppo in altre funzioni potrebbe rendere meno chiaro ciò che sta accadendo, e che sarebbe sicuramente un anti-pattern.

Molte delle risposte qui avrebbero problemi nel superare un controllo di complessità, alcune guardate > 10.

Penso che questa sia la parte "anti-pattern" di ciò che stai guardando. Trova uno strumento che misuri la complessità ciclomatica del tuo codice: ci sono plugin per Eclipse. È essenzialmente una misura di quanto sia difficile testare il codice e coinvolge il numero e i livelli dei branch di codice.

Come ipotesi totale su una possibile soluzione, il layout del tuo codice mi fa pensare in "Compiti", se questo accade in molti punti, forse quello che vuoi veramente è un'architettura orientata al compito - ogni attività essendo esso stesso oggetto e nel medio-compito si ha la possibilità di accodare l'attività successiva istanziando un altro oggetto compito e gettandolo in coda. Questi possono essere incredibilmente semplici da configurare e riducono in modo significativo la complessità di alcuni tipi di codice, ma come ho detto questa è una pugnalata totale al buio.

Usando l'esempio di pdr sopra, dato che è un bell'esempio, farò un ulteriore passo avanti.

Aveva:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Quindi ho capito che quanto segue avrebbe funzionato:

if(BuildList(list)) 
    SortList(list)

Ma non è chiaro.

Quindi alla domanda originale, perché non avere:

BuildList(list)
SortList(list)

E lascia che SortList decida se richiede l'ordinamento?

Si vede che il metodo BuildList sembra sapere sull'ordinamento, poiché restituisce un valore bool indicato come tale, ma ciò non ha senso per un metodo progettato per creare un elenco.

Sì, questo sembra essere un problema perché devi tenere traccia di tutti i punti in cui stai contrassegnando la bandiera su ON / OFF. È meglio includere la logica appena dentro come condizione nidificata invece di escludere la logica.

Anche modelli di domini ricchi, in tal caso solo un liner farà grandi cose all'interno dell'oggetto.

Se la bandiera viene impostata solo una volta, sposta il simbolo ...
codice fino a subito dopo il
... // qualche altro codice qui
quindi elimina la bandiera.

Altrimenti dividi il campo   ... // sacco di codice qui
... // qualche altro codice qui
...
se possibile, codifica in funzioni separate, quindi è chiaro che questa funzione ha una sola responsabilità che è la logica di derivazione.

Ove possibile, separare il codice all'interno di
  ... // sacco di codice qui
in due o più funzioni, alcune che fanno un po 'di lavoro (che è un comando) e altre che restituiscono il valore di todo (che è una query) o rendono molto esplicito lo stanno modificando (una query che usa effetti collaterali)

Il codice stesso non è l'anti-pattern in corso qui ... Ho il sospetto che mescolare la logica della ramificazione con l'effettiva esecuzione di cose (comandi) sia l'anti-pattern che stai cercando.

A volte trovo necessario implementare questo modello. A volte si desidera eseguire più controlli prima di procedere con un'operazione. Per motivi di efficienza, i calcoli che comportano determinati controlli non vengono eseguiti a meno che non sia assolutamente necessario verificare. Quindi in genere vedi codice come:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Questo potrebbe essere semplificato separando la convalida dal processo effettivo di esecuzione dell'operazione, quindi vedresti più come:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Ovviamente varia in base a ciò che stai cercando di ottenere, anche se scritto in questo modo, sia il tuo codice "successo" che il tuo codice "fallimento" vengono scritti una volta, il che semplifica la logica e mantiene lo stesso livello di prestazioni. Da lì, sarebbe una buona idea adattare interi livelli di validazione all'interno di metodi interni che restituiscono indicazioni booleane di successo o fallimento che semplificano ulteriormente le cose, anche se alcuni programmatori amano i metodi estremamente lunghi per qualche strana ragione.

Questo non è uno schema . L'interpretazione più generale è che si imposta una variabile booleana e si ramificano sul suo valore in seguito. Questa è normale programmazione procedurale, niente di più.

Ora, il tuo esempio specifico può essere riscritto come:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Potrebbe essere più facile da leggere. O forse no. Dipende dal resto del codice che hai omesso. Concentrati a rendere il codice più sintetico.

I flag locali usati per il flusso di controllo dovrebbero essere riconosciuti come una forma di goto sotto mentite spoglie. Se un flag viene utilizzato solo all'interno di una funzione, può essere eliminato scrivendo due copie della funzione, etichettandone una come "flag is true" e l'altra come "flag is false" e sostituendo ogni operazione che imposta la flag quando è chiaro, o lo cancella quando è impostato, con un salto tra le due versioni della funzione.

In molti casi, il codice che utilizza l'utilizzo di un flag sarà più pulito di qualsiasi possibile alternativa che utilizza goto , ma non è sempre vero. In alcuni casi, usare goto per saltare un pezzo di codice potrebbe essere più pulito rispetto all'utilizzo di flag per farlo [anche se alcune persone potrebbero inserire qui un certo cartone animato di raptor].

Penso che il principio guida principale dovrebbe essere che il flusso della logica del programma dovrebbe assomigliare nella descrizione della logica aziendale per quanto possibile. Se i requisiti della logica aziendale sono descritti in termini di stati che si suddividono e si fondono in modi bizzarri, la logica del programma può anche essere più pulita di cercare di utilizzare i flag per nascondere tale logica. D'altra parte, se il modo più naturale di descrivere le regole di business sarebbe quello di dire che un'azione dovrebbe essere fatta se sono state fatte alcune altre azioni, il modo più naturale di esprimere che può essere quello di usare un flag che viene impostato durante l'esecuzione quest'ultima azione ed è altrimenti chiaro.