Sta facendo un incarico all'interno di una condizione considerata un odore di codice?

Per prima cosa, definirò sicuramente la prima versione come un ciclo for:

for (List<String> currentStrings = getCurrentStrings();
     currentStrings.size() > 0; // if your List has an isEmpty() prefer it
     currentStrings = getCurrentStrings()) {
  ...
}

Sfortunatamente non c'è un modo idiomatico in C ++, Java o C # che io conosca per eliminare la duplicazione tra inizializzatore e incrementatore. Personalmente mi piace astrarre il pattern di loop in Iterable o Enumerable o qualunque sia la tua lingua. Ma alla fine, questo sposta semplicemente la duplicazione in un posto riutilizzabile. Ecco un esempio in C #:

IEnumerable<T> ValidResults<T>(Func<T> grab, Func<bool, T> validate) {
  for (T t = grab(); validate(t); t = grab()) {
    yield return t;
  }
}
// != null is a common condition
IEnumerable<T> NonNullResults<T>(Func<T> grab) where T : class {
  return ValidResults(grab, t => t != null);
}

Ora puoi fare questo:

foreach(var currentStrings in NonNullResults(getCurrentStrings)) {
  ...
}

% di% di C # co_de rende la scrittura facile; è più brutto in Java o C ++.

La cultura C ++ accetta più l'assegnazione-in-condition rispetto agli altri linguaggi e le conversioni booleane implicite sono effettivamente utilizzate in alcuni idiomi, ad es. digita le query:

if (Derived* d = dynamic_cast<Derived*>(base)) {...}

Quanto sopra si basa sulla conversione implicita di puntatori a bool ed è idiomatico. Eccone un altro:

std::string s;
while (std::getline(std::cin, s)) {...}

Questo modifica all'interno della condizione.

Il modello comune, tuttavia, è che la condizione stessa è banale, di solito si basa completamente su alcune conversioni implicite in bool. Dal momento che le raccolte non lo fanno, mettendo un test vuoto ci sarebbe da considerare meno idiomatico.

C culture è ancora più accettabile, con l'idioma del ciclo yield simile a questo:

int c;
while((c = fgetc(stream)) != EOF) {...}

Ma nei linguaggi di livello superiore, questo è disapprovato, perché con il livello più alto di solito viene meno l'accettazione del codice difficile.

Il problema fondamentale qui, mi sembra, è che tu abbia un N più un mezzo ciclo , e quelli sono sempre un po 'confusi da esprimere. In questo caso specifico, potrebbe issare la parte "mezza" del ciclo nel test, come hai fatto tu, ma mi sembra molto imbarazzante. Due modi per esprimere quel loop possono essere:

Idiomatic C / C ++ secondo me:

for (;;) {
    List<String> currentStrings = getCurrentStrings();
    if (!currentStrings.size())
        break;
    doThingsThatCanAlterCurrentStrings();
}

Rigorosa "programmazione strutturata", che tende a corrugare la fronte su break :

for (bool done = false; !done; ) {
    List<String> currentStrings = getCurrentStrings();
    if (currentStrings.size() > 0) {
        doThingsThatCanAlterCurrentStrings();
    } else {
        done = true;
    }
}

Il precedente codice mi sembra più razionale e leggibile e anche l'intero ciclo ha perfettamente senso. Non sono sicuro del contesto del programma, ma non c'è niente di sbagliato con la struttura del ciclo in sostanza.

L'esempio successivo sembra provare a scrivere un codice Clever , cosa che mi confonde a prima vista.

Sono anche d'accordo con Rob Y risposta che nel primissimo si potrebbe pensare che dovrebbe essere una equazione == piuttosto che un compito , tuttavia se in realtà leggi l'istruzione mentre alla fine capiremo che non è un errore o un errore, tuttavia il problema è che non puoi capire chiaramente Perché c'è un compito all'interno dell'istruzione mentre a meno che tu non mantenga esattamente il nome della funzione come doThingsThatCanAlterCurrentStrings o aggiungi un commento in linea che spiega che la funzione seguente probabilmente modificherà il valore di currentStrings .

Questo tipo di costruzione si presenta quando si esegue una sorta di lettura bufferizzata, in cui la lettura riempie un buffer e restituisce il numero di byte letti o 0. È un costrutto piuttosto familiare.

C'è il rischio che qualcuno pensi che tu abbia ottenuto = confuso con == . Potresti ricevere un avviso se stai utilizzando un controllo dello stile.

Onestamente, sarei più preoccupato del (...).size() rispetto al compito. Sembra un po 'incerto, perché stai rinunciando al compito. ;)

Non penso che ci sia una regola difficile, a meno che tu non stia seguendo rigorosamente il consiglio di un correttore di stili & lo segnala con un avvertimento.

La duplicazione è come una medicina. È dannoso in dosi elevate, ma può essere utile se usato in modo appropriato a basse dosi. Questa situazione è uno dei casi utili, in quanto hai già effettuato il refactoring della peggiore duplicazione nella funzione getCurrentStrings() . Sono d'accordo con Sebastian, tuttavia, che è meglio scritto come un ciclo for.

Lungo le stesse linee, se questo modello si presenta continuamente, potrebbe essere un segno che è necessario creare astrazioni migliori o riorganizzare le responsabilità tra diverse classi o funzioni. Ciò che rende gli anelli come questi problematici è che dipendono strongmente dagli effetti collaterali. In alcuni domini che non è sempre evitabile, come ad esempio l'I / O, ma dovresti comunque provare a spingere le funzioni dipendenti dall'effetto come più in profondità nei tuoi livelli di astrazione il più possibile, quindi non ce ne sono molte.

Nel tuo codice di esempio, senza conoscere il contesto, la prima cosa che farei è provare a trovare un modo per rifattorizzarlo per fare tutto il lavoro sulla mia copia locale di currentStrings , quindi aggiornare lo stato esterno tutto in una volta . Qualcosa come:

for(String string: getCurrentStrings()) {
    doSomething(string);
}
clearCurrentStrings();

Se le stringhe correnti vengono aggiornate da un altro thread, un modello di eventi è spesso in ordine:

void onCurrentStringAdd(String addedString) {
    doSomething(addedString);
}

Ottieni l'immagine. Di solito c'è un modo un po ' per rifattorizzare il tuo codice per non dipendere più dagli effetti collaterali. Se ciò non è pratico, puoi spesso evitare la duplicazione spostando alcune responsabilità in un'altra classe, come in:

CurrentStrings currentStrings = getCurrentStrings();
while (!currentStrings.empty()) {
    currentStrings.alter();
}

Potrebbe sembrare eccessivo creare una nuova classe CurrentStrings per qualcosa che può essere in gran parte servito da List<String> , ma spesso aprirà un'intera gamma di semplificazioni in tutto il codice. Per non parlare dei vantaggi di incapsulamento e controllo del tipo.