Le migliori pratiche per "continuare" dall'interno di un ciclo annidato?

7

Ecco un esempio semplificato. Fondamentalmente, controlla una stringa da una lista di stringhe. Se il controllo passa, rimuoverà quella stringa ( filterStringOut(i); ) e non sarà più necessario continuare altri controlli. Quindi continue alla stringa successiva.

void ParsingTools::filterStrings(QStringList &sl)
{
    /* Filter string list */
    QString s;
    for (int i=0; i<sl.length(); i++) {
        s = sl.at(i);

        // Improper length, remove
        if (s.length() != m_Length) {
            filterStringOut(i);
            continue; // Once removed, can move on to the next string
        }          
        // Lacks a substring, remove
        for (int j=0; j<m_Include.length(); j++) {
            if (!s.contains(m_Include.at(j))) { 
                filterStringOut(i); 
                /* break; and continue; */ 
            }
        }
        // Contains a substring, remove
        for (int j=0; j<m_Exclude.length(); j++) {
            if (s.contains(m_Exclude.at(j))) { 
                filterStringOut(i); 
                /* break; and continue; */ 
            }
        } 
    }
}

Come si dovrebbe continuare il ciclo esterno da un ciclo annidato?

La mia ipotesi migliore è usare goto e posizionare un'etichetta alla fine del ciclo esterno. Questo mi ha spinto a fare questa domanda, visto come può essere tabù goto .

Nella chat IRC c ++, è stato suggerito di posizionare il for loops nelle funzioni bool, che restituiscono true se è passato un controllo. quindi

if ( containsExclude(s)) continue;
if (!containsInclude(s)) continue;

o semplicemente creo un booleano locale, impostalo su true break , controlla bool e continua se vero.

Dato che sto usando questo in un parser, in realtà ho bisogno di dare priorità alle prestazioni in questo esempio. È una situazione in cui goto è ancora utile o è il caso in cui devo ristrutturare il mio codice?

    
posta Akiva 04.05.2018 - 06:11
fonte

7 risposte

16

Non nidificare: convertire invece in funzioni. E avere quelle funzioni restituire true se eseguono la loro azione e i passaggi successivi possono essere saltati; false altrimenti. In questo modo si evita completamente l'intero problema di come uscire da un livello, continuare in un altro, ecc. Come si concatena semplicemente le chiamate con || (ciò presuppone che C ++ interrompe l'elaborazione di un'espressione su true ; ).

Quindi il tuo codice potrebbe apparire come il seguente (non ho scritto C ++ in anni, quindi probabilmente contiene errori di sintassi, ma dovresti darti un'idea generale):

void ParsingTools::filterStrings(QStringList &sl)
{
    QString s;
    for (int i=0; i<sl.length(); i++) {
        s = sl.at(i);

        removeIfImproperLength(s, i) ||
        removeIfLacksRequiredSubstring(s, i) ||
        removeIfContainsInvalidSubstring(s, i);
    }
}

bool removeIfImproperLength(QString s, int i) {
    if (s.length() != m_Length) 
    {
        filterStringOut(i);
        return true;
    }
    return false;
}          

bool removeIfLacksSubstring(QString s, int i) {
    for (int j=0; j<m_Include.length(); j++) {
        if (!s.contains(m_Include.at(j))) { 
            filterStringOut(i);
            return true; 
        }
    }

    return false;
}

bool removeIfContainsInvalidSubstring(QString s, int i) {
    for (int j=0; j<m_Exclude.length(); j++) {
        if (s.contains(m_Exclude.at(j))) { 
            filterStringOut(i); 
            return true;
        }
    } 

    return false;
}
    
risposta data 04.05.2018 - 09:18
fonte
13

Da un punto di vista più a vista dell'uccello, vorrei rifattorizzare il codice in modo che assomigli a questo ... (in pseudo codice, è troppo tempo fa ho toccato C ++)

void filterStrings(sl)
{
    /* Filter string list */
    for (int i=0; i<sl.length(); i++) {
        QString s = sl.at(i);
        if(!isProperString(s)) {
           filterStringOut(i);
        }
     }
}

bool isProperString(s) {

        if (s.length() != m_Length)
            return false; // Improper length

        for (int j=0; j<m_Include.length(); j++) {
            if (!s.contains(m_Include.at(j))) { 
                return false; // Lacks a substring
            }
        }

        for (int j=0; j<m_Exclude.length(); j++) {
            if (s.contains(m_Exclude.at(j))) { 
                return false; // Contains a substring
            }
        }

        return true; // all tests passed, it's a proper string
}

Questo è IMHO più pulito perché separa chiaramente ciò che costituisce una stringa corretta e ciò che fai quando non lo è.

Potresti anche fare un ulteriore passo avanti e utilizzare metodi di filtro integrati come myProperStrings = allMyStrings.filter(isProperString)

    
risposta data 04.05.2018 - 10:33
fonte
10

Mi piace molto come inizia @dagnelies . Breve e dritto al punto. Un buon uso dell'astrazione di alto livello. Sto solo modificando la sua firma ed evitando un inutile errore.

void ParsingTools::filterStrings(QStringList &sl)
{
    for (int i=0; i<sl.length(); i++) {
        QString s = sl.at(i);
        if ( isRejectString(s) ) {
            filterStringOut(i);
        }
    }
}

Tuttavia, mi piace come @DavidArno espone i test dei requisiti come singole funzioni. Certo, tutto diventa più lungo, ma ogni funzione è meravigliosamente minuscola. I loro nomi evitano la necessità di commenti per spiegare cosa sono. Non mi piace che si assumano l'ulteriore responsabilità di chiamare filterStringOut() .

A proposito, sì C ++ interromperà la valutazione di una catena || su un true fino a quando non avrai sovraccaricato l'operatore || . Questa è chiamata valutazione del cortocircuito . Ma questa è una micro ottimizzazione banale che sei libero di ignorare mentre leggi il codice fintanto che le funzioni sono prive di effetti collaterali (come quelle che seguono).

Quanto segue dovrebbe rendere chiara la definizione di una stringa di rifiuto senza trascinarti attraverso dettagli inutili:

bool isRejectString(QString s) {
    return isDifferentLength(s, m_Length) 
        || sansRequiredSubstring(s, m_Include)
        || hasForbiddenSubstring(s, m_Exclude)
    ;
}

Sollevati dalla necessità di chiamare filterStringOut() , le funzioni di test dei requisiti diventano più brevi ei loro nomi sono molto più semplici. Ho anche messo tutto ciò di cui sono dipendenti nella loro lista dei parametri per renderne più facile la comprensione senza guardarci dentro.

bool isDifferentLength(QString s, int length) {
    return ( s.length() != length );
}

bool sansRequiredSubstring(QString s, QStringList &include) {
    for (int j=0; j<include.length(); j++) {
        QString requiredSubstring = include.at(j);
        if ( !s.contains(requiredSubstring) ) { 
            return true; 
        }
    }
    return false;
}

bool hasForbiddenSubstring(QString s, QStringList &exclude) {
    for (int j=0; j<exclude.length(); j++) {
    QString forbiddenSubstring = exclude.at(j);
        if ( s.contains(forbiddenSubstring) ) { 
            return true; 
        }
    }
    return false;
}

Ho aggiunto requiredSubstring e forbiddenSubstring per gli umani. Ti rallenteranno? Prova e scoprilo. È più facile rendere il codice leggibile effettivamente veloce per rendere leggibile il codice prematuramente ottimizzato o in realtà veloce.

Se le funzioni risultano rallentare, esamina le funzioni inline prima di sottoporre gli umani a illeggibili codice. Ancora una volta, non dare per scontato che questo ti darà velocità. Test.

Penso che troverai uno di questi più leggibile di loop annidati. Quelli, combinati con if , stavano iniziando a darti un vero anti-pattern arrow . Penso che la lezione qui sia che le piccole funzioni sono una buona cosa.

    
risposta data 04.05.2018 - 21:54
fonte
4

Basta usare un lambda per il predicato e quindi usare la potenza di algoritmi standard e short- circuito. Non c'è bisogno di alcun flusso di controllo contorto o esotico:

void ParsingTools::filterStrings (QStringList& list)
{
    for (int i = list.size(); i--;) {
        const auto& s = list[i];
        auto contains = [&](const QString& x) { return s.contains(x); };
        if (s.size() != m_Length
                || !std::all_of(m_Include.begin(), m_Include.end(), contains)
                || std::any_of(m_Exclude.begin(), m_Exclude.end(), contains))
            filterStringOut(i);
    }
}
    
risposta data 15.05.2018 - 05:13
fonte
1

C'è anche l'opzione per rendere il contenuto del ciclo esterno (quello che vuoi continuare) un lambda e semplicemente usare return .
È sorprendentemente facile se conosci lambda; in pratica inizia il tuo ciclo interno con [&]{ e termina con }() ; all'interno puoi utilizzare return; in qualsiasi momento per lasciarlo:

void ParsingTools::filterStrings(QStringList &sl)
{
    /* Filter string list */
    QString s;
    for (int i=0; i<sl.length(); i++) {

      [&]{    // start a lamdba defintion

        s = sl.at(i);

        // Improper length, remove
        if (s.length() != m_Length) {
            filterStringOut(i);
            // continue; // Once removed, can move on to the next string
            return; // happily return here, this will continue 
        }          
        // Lacks a substring, remove
        for (int j=0; j<m_Include.length(); j++) {
            if (!s.contains(m_Include.at(j))) { 
                filterStringOut(i); 
                /* break; and continue; */  return;  // happily return here, this will continue the i-loop
            }
        }
        // Contains a substring, remove
        for (int j=0; j<m_Exclude.length(); j++) {
            if (s.contains(m_Exclude.at(j))) { 
                filterStringOut(i); 
                /* break; and continue; */  return; // happily return here, this will continue the i-loop
            }
        } 

      }()   // close/end the lambda definition and call it

    }
}
    
risposta data 05.05.2018 - 05:14
fonte
1

Penso che @dganelies abbia l'idea giusta come punto di partenza, ma penso che prenderei in considerazione un ulteriore passo avanti: scrivere una funzione generica in grado di eseguire lo stesso schema per (quasi) qualsiasi contenitore, criterio e azione:

template <class Container, class Action, class Condition>
void map_if(Container &container, Action action, Condition cond) {
    for (std::size_t i = 0; i < container.length(); i++) {
        auto s = container.at(i);
        if (cond(s))
            action(i);
    }
}

Il tuo filterStrings dovrebbe quindi solo definire i criteri e passare l'azione appropriata:

void ParsingTools::filterStrings(QStringList const &sl)
{
    auto isBad = [&](QString const &s) {

        if (s.length() != m_Length)
            return true;

        for (int j = 0; j < m_Include.length(); j++) {
            if (!s.contains(m_Include.at(j))) {
                return true;
            }
        }

        for (int j = 0; j < m_Exclude.length(); j++) {
            if (s.contains(m_Exclude.at(j))) {
                return true;
            }
        }
        return false;
    };

    map_if(sl, filterStringOut, isBad);
}

Naturalmente, ci sono altri modi per affrontare anche questo problema di base. Ad esempio, utilizzando la libreria standard, sembra che tu voglia qualcosa sullo stesso ordine generale di std::remove_if .

    
risposta data 05.05.2018 - 20:13
fonte
1

Diverse risposte suggeriscono un importante refactoring del codice. Questo probabilmente non è un brutto modo di andare, ma volevo fornire una risposta che fosse più in linea con la domanda stessa.

Regola n. 1: profilo prima dell'ottimizzazione

Visualizza sempre i risultati prima di tentare un'ottimizzazione. Potresti trovarti a perdere un sacco di tempo se non lo fai.

Detto questo ...

Così com'è, ho personalmente testato questo tipo di codice su MSVC. I booleani sono la strada da percorrere. Assegna al valore booleano qualcosa di semantico come containsString .

    ...
    boo containsString = true; // true until proven false
    // Lacks a substring, remove
    for (int j=0; j<m_Include.length(); j++) {
        if (!s.contains(m_Include.at(j))) { 
            filterStringOut(i); 
            /* break; and continue; */ 
            containsString = false;
        }
    }
    if (!containsString)
        continue;

In MSVC (2008), in modalità di rilascio (impostazioni tipiche dell'ottimizzatore), il compilatore ha ottimizzato un ciclo simile fino allo stesso identico insieme di codici opzionali della versione goto . Era abbastanza intelligente da vedere che il valore del booleano era direttamente legato al flusso di controllo, ed eliminava tutto. Non ho provato gcc, ma presumo che possa fare simili tipi di ottimizzazione.

Questo ha il vantaggio su goto di semplicemente non sollevare alcuna preoccupazione dai puristi che considerano goto di essere dannosi, senza sacrificare il valore di una singola istruzione.

    
risposta data 09.05.2018 - 06:41
fonte

Leggi altre domande sui tag