Best Practice, um innerhalb einer verschachtelten Schleife fortzufahren?

8

Hier ist ein vereinfachtes Beispiel. Grundsätzlich werden Zeichenfolgen aus einer Zeichenfolgenliste überprüft. Wenn die Prüfung erfolgreich ist, wird diese Zeichenfolge ( filterStringOut(i);) entfernt, und es ist nicht mehr erforderlich, andere Prüfungen fortzusetzen. Also continuezum nächsten String.

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; */ 
            }
        } 
    }
}

Wie soll man die äußere Schleife innerhalb einer verschachtelten Schleife fortsetzen?

Meine beste Vermutung ist, gotoein Etikett am Ende der äußeren Schleife zu verwenden und zu platzieren. Das veranlasste mich, diese Frage zu stellen, da Tabu sein gotokann.

Im c ++ IRC-Chat wurde vorgeschlagen, die forSchleifen in Bool-Funktionen zu platzieren, die true zurückgeben, wenn eine Prüfung bestanden wurde. somit

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

oder dass ich einfach einen lokalen Booleschen breakWert erstelle, ihn auf true setze , bool überprüfe und fortfahre, wenn true.

Da ich dies in einem Parser verwende, muss ich in diesem Beispiel die Leistung priorisieren. Ist dies eine Situation, in der gotoes noch nützlich ist, oder ist es ein Fall, in dem ich meinen Code neu strukturieren muss?

Akiva
quelle
3
Mögliches Duplikat von Ist Mikrooptimierung beim Codieren wichtig?
Doc Brown
3
C ++ hat keine gekennzeichneten Pausen, daher besteht die kanonische und akzeptierte Praxis darin, sie gototrotz ihres schlechten Rufs über zu emulieren . Fürchte keine Namen - fürchte Konzepte.
Kilian Foth
2
@Akiva: Hast du den Leistungsunterschied tatsächlich gemessen? Und nur weil Sie gehört haben, dass "goto" ein akzeptabler Weg ist, aus einer verschachtelten Schleife auszubrechen, bedeutet dies nicht, dass die Alternative, eine Funktion mit einem klaren und prägnanten Namen einzuführen, nicht besser lesbar wäre.
Doc Brown
3
@Akiva: Das Benchmarking ist ziemlich einfach, Sie benötigen keine speziellen Tools oder Fähigkeiten: Richten Sie ein kleines Programm ein, das diese Funktion mit einigen Testdaten in einer Schleife mehrmals (möglicherweise mehrere Millionen Mal) aufruft, und messen Sie die Laufzeit mit eine Stoppuhr. Machen Sie dasselbe mit dem bereinigten Code. Ich wette, der Unterschied wird vernachlässigbar sein (vergessen Sie natürlich nicht, Compiler-Optimierungen zu verwenden).
Doc Brown
5
Warum erfindest du das Rad neu ?
Alexander - Reinstate Monica

Antworten:

16

Nicht verschachteln: stattdessen in Funktionen konvertieren. Lassen Sie diese Funktionen zurückkehren, truewenn sie ihre Aktion ausführen und die nachfolgenden Schritte übersprungen werden können. falseAndernfalls. Auf diese Weise vermeiden Sie das gesamte Problem, wie Sie aus einer Ebene ausbrechen, in einer anderen fortfahren usw., während Sie nur die Aufrufe verketten ||(dies setzt voraus, dass C ++ die Verarbeitung eines Ausdrucks auf a beendet true; ich denke, das tut es).

Ihr Code könnte also wie folgt aussehen (ich habe C ++ seit Jahren nicht mehr geschrieben, daher enthält er wahrscheinlich Syntaxfehler, sollte Ihnen aber die allgemeine Idee geben):

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;
}
David Arno
quelle
1
Tippfehler "reoveIfImproperLength". :)
Neil
5
Es ist besser, wenn die drei Bedingungsprüfungsfunktionen nebenwirkungsfrei bleiben (dh anstatt "remove-if" auszuführen , geben Sie einfach die boolesche Bedingung zurück und lassen Sie den Aufrufer ( ParsingTools::filterStrings) die filterStringOut(i)Funktion aufrufen , wie in der Antwort von
dagnelies
Sie verwenden also die Funktionsaufrufsemantik als Grundlage für die fehlenden break-Anweisungen von C ++. Sehr schlau.
Ryan Reich
13

Aus der Vogelperspektive würde ich den Code so umgestalten, dass er so aussieht ... (im Pseudocode ist es zu lange her, dass ich C ++ berührt habe)

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
}

Dies ist meiner Meinung nach sauberer, weil es klar trennt, was eine richtige Zeichenfolge ausmacht und was Sie tun, wenn dies nicht der Fall ist.

Sie können sogar noch einen Schritt weiter gehen und integrierte Filtermethoden wie verwenden myProperStrings = allMyStrings.filter(isProperString)

dagnelies
quelle
10

Mir gefällt sehr gut, wie @dagnelies anfängt . Kurz und auf den Punkt. Eine gute Verwendung der Abstraktion auf hoher Ebene. Ich optimiere nur die Signatur und vermeide ein unnötiges Negativ.

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

Mir gefällt jedoch, wie @DavidArno die Anforderungstests als einzelne Funktionen aufteilt . Sicher wird das Ganze länger, aber jede Funktion ist wunderbar klein. Ihre Namen vermeiden die Notwendigkeit von Kommentaren, um zu erklären, was sie sind. Ich mag es einfach nicht, dass sie die zusätzliche Verantwortung übernehmen, anzurufen filterStringOut().

Übrigens, ja, C ++ stoppt die Auswertung einer ||Kette true, solange Sie den ||Operator nicht überladen haben . Dies wird als Kurzschlussauswertung bezeichnet . Dies ist jedoch eine triviale Mikrooptimierung, die Sie beim Lesen des Codes ignorieren können, solange die Funktionen nebenwirkungsfrei sind (wie die folgenden).

Folgendes sollte die Definition einer Ablehnungszeichenfolge klar machen, ohne Sie durch unnötige Details zu ziehen:

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

Die Notwendigkeit, filterStringOut()die Anforderungstestfunktionen aufzurufen , wird kürzer und ihre Namen sind viel einfacher. Ich habe auch alles, wovon sie abhängig sind, in ihre Parameterliste aufgenommen, um es einfacher zu machen, sie zu verstehen, ohne nach innen zu schauen.

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

Ich fügte hinzu requiredSubstringund forbiddenSubstringfür die Menschen. Werden sie dich verlangsamen? Testen und herausfinden. Es ist einfacher, lesbaren Code tatsächlich schnell zu machen, als vorzeitig optimierten Code lesbar oder tatsächlich schnell zu machen.

Wenn sich herausstellt, dass die Funktionen Sie verlangsamen, schauen Sie sich Inline-Funktionen an, bevor Sie die Menschen unlesbarem Code aussetzen. Gehen Sie auch hier nicht davon aus, dass dies Ihnen Geschwindigkeit verleiht. Prüfung.

Ich denke, Sie werden feststellen, dass diese für Loops besser lesbar als verschachtelt sind. Diese, kombiniert mit den if's, gaben Ihnen ein echtes Pfeil-Anti-Muster . Ich denke, die Lehre hier ist, dass winzige Funktionen eine gute Sache sind.

candied_orange
quelle
1
Obwohl es sich um eine Kombination aus zwei anderen Antworten handelt, bietet dies einen großen Mehrwert. Machen Sie es "für Menschen", testen Sie die Leistung, bevor Sie sich entscheiden, den Code zu "verdecken", und machen Sie das Testen einfach. Tolles Zeug!
Carlossierra
1
Eigentlich war meine Verwendung ! isProperStringeher als isImproperStringbeabsichtigt. Ich neige dazu, Negationen in Funktionsnamen zu vermeiden. Stellen Sie sich vor, Sie müssen später überprüfen, ob es sich tatsächlich um eine richtige Zeichenfolge handelt. Sie benötigen diese, !isImproperStringdie meiner Meinung nach aufgrund der doppelten Negation eher zu Verwirrung neigt.
Dagnelies
@dagnelies besser?
candied_orange
4

Verwenden Sie einfach ein Lambda für das Prädikat und nutzen Sie dann die Leistung von Standardalgorithmen und Kurzschlüssen. Keine Notwendigkeit für einen verschlungenen oder exotischen Kontrollfluss:

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);
    }
}
Deduplikator
quelle
1

Es besteht auch die Möglichkeit, den Inhalt der äußeren Schleife (die Sie fortsetzen möchten) zu einem Lambda zu machen und einfach zu verwenden return.
Es ist überraschend einfach, wenn Sie Lambdas kennen. Sie beginnen Ihr Loop-Interieur im Grunde mit [&]{und beenden es mit }(); innen können Sie es jederzeit verwenden return;, um es zu verlassen:

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

    }
}
Aganju
quelle
3
(1) Um das Lambda am Ende der schließenden geschweiften Klammer sofort aufzurufen , muss der Aufruf ausgeführt werden (mit zwei Klammern, möglicherweise mit oder ohne Liste der Argumente). (2) alle drei Stellen, die verwendet werden und durch die ersetzt werden müssen . Ihr Code scheint den ersten Platz (der verwendet wird ) unverändert zu lassen, aber das muss auch geändert werden, da sich der Code im Lambda befindet und die Anweisung keinen Bereich finden konnte, der eine Schleife ist. continuebreakreturncontinuecontinue
Rwong
Ich habe das geschrieben, während ich auf rote Ampeln gewartet habe. Korrigiert.
Aganju
1

Ich denke, @dganelies hat die richtige Idee als Ausgangspunkt, aber ich denke, ich würde einen Schritt weiter gehen: Schreiben Sie eine generische Funktion, die für (fast) jeden Container, jedes Kriterium und jede Aktion dasselbe Muster ausführen kann:

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

Sie filterStringswürden dann einfach die Kriterien definieren und die entsprechende Aktion ausführen:

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

Natürlich gibt es auch andere Möglichkeiten, um dieses Grundproblem anzugehen. Wenn Sie beispielsweise die Standardbibliothek verwenden, möchten Sie anscheinend etwas in derselben allgemeinen Reihenfolge wie std::remove_if.

Jerry Sarg
quelle
1

Mehrere Antworten deuten auf einen wichtigen Refaktor des Codes hin. Dies ist wahrscheinlich kein schlechter Weg, aber ich wollte eine Antwort geben, die eher der Frage selbst entspricht.

Regel 1: Profil vor der Optimierung

Profilieren Sie die Ergebnisse immer, bevor Sie eine Optimierung versuchen. Wenn Sie dies nicht tun, verschwenden Sie möglicherweise viel Zeit.

Davon abgesehen ...

So wie es ist, habe ich diese Art von Code persönlich auf MSVC getestet. Die Booleschen Werte sind der richtige Weg. Nennen Sie den Booleschen etwas semantisch Bedeutendes 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;

Unter MSVC (2008) optimierte der Compiler im Release-Modus (typische Optimierungseinstellungen) eine ähnliche Schleife auf genau den gleichen Satz von Opcodes wie die gotoVersion. Es war klug genug zu sehen, dass der Wert des Booleschen Werts direkt mit der Steuerung des Flusses verbunden war und alles wegwarf. Ich habe gcc nicht getestet, aber ich gehe davon aus, dass es ähnliche Arten der Optimierung durchführen kann.

Dies hat den Vorteil goto, dass Puristen, die gotoes für schädlich halten, einfach keine Bedenken äußern , ohne die Leistung einer einzelnen Anweisung zu beeinträchtigen.

Cort Ammon
quelle