¿Mejor práctica para "continuar" desde dentro de un bucle anidado?

8

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 es necesario continuar ninguna otra verificación. Así continuea 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 se debe continuar el bucle externo desde el interior de un bucle anidado?

Mi mejor conjetura es usar gotoy colocar una etiqueta al final del bucle externo. Eso me llevó a hacer esta pregunta, dado lo tabú que gotopuede ser.

En el chat c ++ IRC, se sugirió que coloque los forbucles en las funciones bool, que devuelven verdadero si se pasa un cheque. así

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

o que simplemente creo un booleano local, lo configuro en 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 gotoque todavía es útil, o es un caso en el que necesito reestructurar mi código?

Akiva
fuente
3
C ++ no tiene roturas etiquetadas, por lo tanto, la práctica canónica y aceptada es emularlas a través de goto, a pesar de su mala reputación. No temas a los nombres, teme a los conceptos.
Kilian Foth
2
@ Akiva: ¿realmente midió la diferencia de rendimiento? Y el hecho de que haya escuchado que "goto" es una forma aceptable de salir de un bucle anidado no implica que la alternativa de introducir una función con un nombre claro y conciso no sería más legible.
Doc Brown
3
@ Akiva: comparar esto es bastante simple, no necesita herramientas o habilidades especiales: configure un pequeño programa que llame a esta función con algunos datos de prueba en un ciclo varias veces (tal vez varios millones de veces), y mida el tiempo de ejecución con Un cronómetro. Haga lo mismo con el código limpiado. Apuesto a que la diferencia será insignificante (por supuesto, no olvides usar las optimizaciones del compilador).
Doc Brown
55
¿Por qué estás reinventando la rueda ?
Alexander - Restablece a Mónica

Respuestas:

16

No anides: convierte a funciones en su lugar. Y truehaga que esas funciones regresen si realizan su acción y se pueden omitir los pasos siguientes; falsede otra manera. De esa manera, evita 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 terminar pareciéndose a lo siguiente (no he escrito C ++ en años, por lo que probablemente contenga errores de sintaxis, pero debería darle la 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;
}
David Arno
fuente
1
"reoveIfImproperLength" Typo. :)
Neil
55
Es mejor si las tres funciones de condición de comprobación permanecen libres (efecto secundario es decir, en lugar de hacer "remove-si", acaba de regresar la condición booleana y dejar que la persona que llama ( ParsingTools::filterStrings) llame a la filterStringOut(i)función, como se muestra en la respuesta de dagnelies.
rwong
Por lo tanto, está utilizando la semántica de llamadas de función como base para las declaraciones de interrupción faltantes de C ++. Muy inteligente.
Ryan Reich
13

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

Esto es más limpio en mi humilde opinión 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 utilizar métodos de filtro integrados como myProperStrings = allMyStrings.filter(isProperString)

dagnelies
fuente
10

Realmente me gusta cómo comienza @dagnelies . Corto y al grano. Un buen uso de la abstracción de alto nivel. Solo estoy ajustando 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 desglosa las pruebas de requisitos como funciones individuales. Claro 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 filterStringOut().

Por cierto, sí, C ++ detendrá la evaluación de una ||cadena truesiempre que no haya sobrecargado el ||operador. Esto se llama 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 aclarar la definición de una cadena de rechazo sin arrastrarlo 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()los requisitos, las funciones de prueba 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 entenderlos 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é requiredSubstringy forbiddenSubstringpara los humanos. ¿Te retrasarán? Prueba y descúbrelo. Es más fácil hacer que el código legible sea realmente rápido que hacer que el código optimizado prematuramente sea legible o realmente rápido.

Si las funciones te ralentizan, busca funciones en línea antes de someter a los humanos a un código ilegible. Nuevamente, no asumas que esto te dará velocidad. Prueba.

Creo que encontrarás cualquiera de estos más legibles que anidados para bucles. Esos, combinados con los if's, estaban comenzando a darle un verdadero antipatrón de flecha . Creo que la lección aquí es que las funciones pequeñas son algo bueno.

naranja confitada
fuente
1
Aunque es una combinación de otras dos respuestas, esto agrega mucho valor. Hágalo "para humanos", pruebe el rendimiento antes de decidir "oscurecer" el código y haga que sea fácil probarlo. ¡Buena cosa!
carlossierra
1
En realidad, mi uso de ! isProperStringmás que isImproperStringintencional. Tiendo a evitar negaciones en los nombres de funciones. Imagine que necesita verificar si es una cadena adecuada más adelante, necesitará !isImproperStringcuál es, en mi humilde opinión, más propenso a la confusión debido a la doble negación.
Dagnelies
@dagnelies mejor?
candied_orange
4

Simplemente use una lambda para el predicado, y luego use el poder de algoritmos estándar y cortocircuitos. No es necesario ningún flujo de control enrevesado 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);
    }
}
Deduplicador
fuente
1

También existe la opción de hacer que el contenido del bucle externo (el que desea continuar) sea lambda , y simplemente usar return.
Es sorprendentemente fácil si conoces lambdas; básicamente comienza su interior de bucle [&]{y termina con }(); dentro puedes usarlo 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

    }
}
Aganju
fuente
3
(1) Para llamar realmente a la lambda de inmediato , al final de la llave de cierre, se debe hacer la llamada (con un par de paréntesis, potencialmente con la lista de argumentos o sin ella). (2) los tres lugares que usan continuey breakdeben ser reemplazados por return. Parece que su código deja el primer lugar (que usa continue) sin cambios, pero eso también debe cambiarse, porque el código está dentro de la lambda y la continuedeclaración no pudo encontrar un alcance que sea un bucle.
rwong
Escribí eso mientras esperaba en las luces rojas. Corregido
Aganju
1

Creo que @dganelies tiene la idea correcta como punto de partida, pero creo que consideraría ir un paso más allá: escribir 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 filterStringsentonces simplemente definir los criterios, y pasar 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, usando la biblioteca estándar, parece querer algo en el mismo orden general que std::remove_if.

Jerry Coffin
fuente
1

Varias respuestas sugieren un refactor principal del código. Probablemente no sea un mal camino, 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 una gran cantidad de tiempo si no lo hace.

Habiendo dicho eso...

Tal como están las cosas, he probado personalmente este tipo de código en MSVC. Los booleanos son el camino a seguir. Nombra el booleano como algo semánticamente significativo 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 a exactamente el mismo conjunto de códigos de operación que la gotoversión. Fue lo suficientemente inteligente como para ver que el valor del booleano estaba directamente relacionado con el flujo de control, y eludió todo. No he probado gcc, pero supongo que puede hacer tipos similares de optimización.

Esto tiene la ventaja gotode no plantear ninguna preocupación por parte de los puristas que consideran gotoperjudicial, sin sacrificar el rendimiento de una sola instrucción.

Cort Ammon
fuente