¿Se recomienda "continuar" desde dentro de un bucle anidado?

7

Aquí hay una muestra simplificada. Básicamente, verifica una cadena de una lista de cadenas. Si la verificación pasa, eliminará esa cadena ( filterStringOut(i); ) y ya no será necesario continuar con otras comprobaciones. Así, continue a la siguiente cadena.

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

¿Cómo debe uno continuar el bucle externo desde dentro de un bucle anidado?

Mi mejor suposición es usar goto y colocar una etiqueta al final del bucle externo. Eso me impulsó a hacer esta pregunta, dado cómo puede ser taboo goto .

En el chat de c ++ IRC, se sugirió que coloque for loops en funciones bool, que devuelven verdadero si se pasa una verificación. por lo tanto

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

o que simplemente cree un booleano local, configúralo como verdadero break , verifico bool y continúo si es verdadero.

Dado que estoy usando esto en un analizador, realmente necesito priorizar el rendimiento en este ejemplo. ¿Es esta una situación en la que goto sigue siendo útil, o es un caso en el que necesito reestructurar mi código?

    
pregunta Akiva 04.05.2018 - 06:11

7 respuestas

16

No anidar: convertir a funciones en su lugar. Y haga que esas funciones devuelvan true si realizan su acción y los pasos subsiguientes se pueden omitir; false de lo contrario. De esa manera, evitará por completo el problema de cómo salir de un nivel, continuar dentro de otro, etc., ya que simplemente encadena las llamadas con || (esto supone que C ++ deja de procesar una expresión en un true ; creo que sí ).

Por lo tanto, su código podría tener el siguiente aspecto (no he escrito C ++ en años, por lo que es probable que contenga errores de sintaxis, pero debería darle una idea general):

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;
}
    
respondido por el David Arno 04.05.2018 - 09:18
13

Desde una perspectiva más de vista de pájaro, refactorizaría el código para que se vea así ... (en pseudo código, hace mucho tiempo que toqué 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
}

Este es un limpiador IMHO porque separa claramente lo que constituye una cadena adecuada y lo que haces cuando no lo es.

Incluso podría ir un paso más allá y usar métodos de filtro integrados como myProperStrings = allMyStrings.filter(isProperString)

    
respondido por el dagnelies 04.05.2018 - 10:33
10

Realmente me gusta cómo comienza @dagnelies . Corto y al grano. Un buen uso de la abstracción de alto nivel. Solo estoy modificando su firma y evitando un negativo innecesario.

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

Sin embargo, me gusta cómo @DavidArno rompe las pruebas de requisitos como funciones individuales. Seguro que todo se vuelve más largo pero cada función es maravillosamente pequeña. Sus nombres evitan la necesidad de comentarios para explicar lo que son. Simplemente no me gusta que asuman la responsabilidad adicional de llamar a filterStringOut() .

Por cierto, sí, C ++ detendrá la evaluación de una cadena || en un true siempre y cuando no haya sobrecargado el operador || . Esto se denomina evaluación de cortocircuito . Pero esta es una micro optimización trivial que puede ignorar mientras lee el código siempre que las funciones no tengan efectos secundarios (como las que se muestran a continuación).

Lo siguiente debería dejar clara la definición de una cadena de rechazo sin arrastrarte a través de detalles innecesarios:

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

Aliviado de la necesidad de llamar a filterStringOut() , las funciones de prueba de requisitos se acortan y sus nombres son mucho más simples. También he puesto todo de lo que dependen en su lista de parámetros para que sea más fácil de entender sin mirar 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;
}

Agregué requiredSubstring y forbiddenSubstring para los humanos. ¿Te retrasarán? Prueba y averigua. Es más fácil hacer que el código legible sea realmente rápido que el código optimizado prematuramente como legible o realmente rápido.

Si las funciones resultan ser más lentas, mire las funciones en línea antes de someter a los humanos a ilegibles. código. De nuevo, no asumas que esto te dará velocidad. Prueba.

Creo que encontrarás cualquiera de estos más legibles que anidados para los bucles. Esos, combinados con los if 's, empezaron a darle un anti-patrón de flecha . Creo que la lección aquí es que las pequeñas funciones son una buena cosa.

    
respondido por el candied_orange 04.05.2018 - 21:54
4

Solo use un lambda para el predicado, y luego use el poder de algoritmos estándar y corto circulando No es necesario ningún control-flujo complicado o exótico:

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);
    }
}
    
respondido por el Deduplicator 15.05.2018 - 05:13
1

También existe la opción de hacer que el contenido del bucle externo (el que desea continuar) sea lambda , y simplemente use return .
Es sorprendentemente fácil si conoces las lambdas; básicamente, inicia el bucle interior con [&]{ y finaliza con }() ; dentro de usted puede usar return; en cualquier momento para dejarlo:

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

    }
}
    
respondido por el Aganju 05.05.2018 - 05:14
1

Creo que @dganelies tiene la idea correcta como punto de partida, pero creo que consideraría ir un paso más allá: escriba una función genérica que pueda llevar a cabo el mismo patrón para (casi) cualquier contenedor, criterio y acción:

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

Su filterStrings solo definiría los criterios y aprobaría la acción apropiada:

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

Por supuesto, también hay otras formas de abordar ese problema básico. Por ejemplo, al usar la biblioteca estándar, parece que quieres algo en el mismo orden general que std::remove_if .

    
respondido por el Jerry Coffin 05.05.2018 - 20:13
1

Varias respuestas sugieren un refactor importante del código. Probablemente esta no sea una mala manera de hacerlo, pero quería proporcionar una respuesta que esté más en línea con la pregunta en sí.

Regla # 1: perfil antes de optimizar

Siempre perfile los resultados antes de intentar una optimización. Puede que pierda mucho tiempo si no lo hace.

Dicho esto ...

Tal como está, personalmente he probado este tipo de código en MSVC. Los booleanos son el camino a seguir. Nombre el booleano algo semánticamente significativo como 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;

En MSVC (2008), en modo de lanzamiento (configuración típica del optimizador), el compilador optimizó un bucle similar hasta exactamente el mismo conjunto de códigos de operación que la versión goto . Era lo suficientemente inteligente como para ver que el valor del booleano estaba directamente relacionado con el flujo de control, y eliminaba todo. No he probado gcc, pero supongo que puede hacer tipos similares de optimización.

Esto tiene la ventaja de que goto simplemente no genera ninguna preocupación por parte de los puristas que consideran que goto es perjudicial, sin sacrificar el valor de una sola instrucción.

    
respondido por el Cort Ammon 09.05.2018 - 06:41

Lea otras preguntas en las etiquetas