¿Hacer una tarea dentro de una condición se considera un olor de código?

7

Muchas veces tengo que escribir un bucle que requiere la inicialización de una condición de bucle, y una actualización cada vez que se ejecuta el bucle. Aquí hay un ejemplo:

List<String> currentStrings = getCurrentStrings();
while(currentStrings.size() > 0) {
  doThingsThatCanAlterCurrentStrings();
  currentStrings = getCurrentStrings();
}

Una cosa que no me gusta de este código es la llamada duplicada a getCurrentStrings() . Una opción es agregar la asignación a la condición de la siguiente manera:

List<String> currentStrings;
while( (currentStrings = getCurrentStrings()).size() > 0) {
  doThingsThatCanAlterCurrentStrings();
}

Pero aunque ahora tengo menos duplicación y menos código, siento que ahora es más difícil leer mi código. Por otro lado, es más fácil entender que estamos haciendo un bucle en una variable que puede ser cambiada por el bucle.

¿Cuál es la mejor práctica a seguir en casos como este?

    
pregunta vainolo 20.03.2014 - 08:25

5 respuestas

9

Primero, definitivamente enmarcaría la primera versión como un bucle for:

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

Lamentablemente, no existe una forma idiomática en C ++, Java o C # que conozca para eliminar la duplicación entre el inicializador y el incrementador. Personalmente, me gusta abstraer el patrón de bucle en un Iterable o Enumerable o lo que sea que proporcione su idioma. Pero al final, eso solo mueve la duplicación a un lugar reutilizable. Aquí hay un ejemplo de 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);
}

Ahora puedes hacer esto:

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

C # 's yield hace que escribir esto sea fácil; es más feo en Java o C ++.

La cultura C ++ acepta más la asignación en condición que los otros idiomas, y las conversiones booleanas implícitas se utilizan en algunos idiomas, por ejemplo. consultas de tipo:

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

Lo anterior se basa en la conversión implícita de punteros a bool y es idiomático. Aquí hay otro:

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

Esto se modifica dentro de la condición.

El patrón común, sin embargo, es que la condición en sí es trivial, usualmente confiando completamente en alguna conversión implícita a bool. Ya que las colecciones no hacen eso, poner una prueba vacía allí sería considerado menos idiomático.

La cultura C es aún más aceptada, con la expresión fgetc loop con este aspecto:

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

Pero en los lenguajes de nivel superior, esto está mal visto, porque con el nivel más alto por lo general viene una menor aceptación del código complicado.

    
respondido por el Sebastian Redl 20.03.2014 - 13:37
3

El problema fundamental aquí, me parece, es que tienes un N más un medio ciclo , y esos Siempre son un poco desordenado expresar. En este caso particular, usted podría izar la parte "media" del bucle en la prueba, como lo ha hecho, pero me parece muy incómodo. Dos formas de expresar ese bucle pueden ser:

Idiomatic C / C ++ en mi opinión:

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

"Programación estructurada" estricta, que tiende a fruncir el ceño en break :

for (bool done = false; !done; ) {
    List<String> currentStrings = getCurrentStrings();
    if (currentStrings.size() > 0) {
        doThingsThatCanAlterCurrentStrings();
    } else {
        done = true;
    }
}
    
respondido por el microtherion 20.03.2014 - 10:32
1

El código anterior me parece más racional y legible y todo el bucle también tiene mucho sentido. No estoy seguro sobre el contexto del programa, pero no hay nada malo con la estructura del bucle en esencia.

El ejemplo posterior parece intentar escribir un código Clever , lo cual es absolutamente confuso para mí a primera vista.

También estoy de acuerdo con la respuesta de Rob Y que, en primer lugar, Mire que podría pensar que debería ser una ecuación == en lugar de una asignación , sin embargo, si realmente lee la declaración while hasta el final se dará cuenta de que no es un error tipográfico o un error, sin embargo, el problema es que no puede entender claramente Por qué , hay una asignación dentro de la declaración while a menos que mantenga el nombre de la función exactamente igual que doThingsThatCanAlterCurrentStrings o agregue un comentario en línea que explique que la siguiente función es probable que cambie el valor de currentStrings .

    
respondido por el Mahdi 20.03.2014 - 11:22
0

Ese tipo de construcción aparece cuando se realiza algún tipo de lectura en búfer, donde la lectura llena un búfer y devuelve el número de bytes leídos, o 0. Es una construcción bastante familiar.

Existe el riesgo de que alguien piense que tienes = confundido con == . Puede recibir una advertencia si está utilizando un comprobador de estilo.

Honestamente, me molestaría más el (...).size() que la asignación. Eso parece un poco dudoso, porque no estás haciendo referencia a la asignación. ;)

No creo que haya una regla estricta, a menos que sigas estrictamente los consejos de un verificador de estilos & lo marca con una advertencia.

    
respondido por el Rob 20.03.2014 - 09:27
0

La duplicación es como la medicina. Es dañino en dosis altas, pero puede ser beneficioso cuando se usa apropiadamente en dosis bajas. Esta situación es uno de los casos útiles, ya que ya ha refaccionado la peor duplicación en la función getCurrentStrings() . Sin embargo, estoy de acuerdo con Sebastian en que es mejor escribirlo como un bucle for.

En la misma línea, si este patrón aparece todo el tiempo, podría ser una señal de que necesita crear mejores abstracciones o reorganizar las responsabilidades entre diferentes clases o funciones. Lo que hace que los bucles como estos sean problemáticos es que dependen en gran medida de los efectos secundarios. En ciertos dominios, esto no siempre es evitable, como la E / S, por ejemplo, pero aún así debe intentar empujar las funciones dependientes de los efectos secundarios tan profundamente en sus capas de abstracción como sea posible, por lo que no hay muchas de ellas.

En su código de ejemplo, sin conocer el contexto, lo primero que haría es intentar encontrar una manera de refactorizarlo para hacer todo el trabajo en mi copia local de currentStrings , luego actualizar el estado externo de una vez . Algo como:

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

Si las cadenas actuales están siendo actualizadas por otro hilo, un modelo de evento suele estar en orden:

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

Obtienes la imagen. Generalmente hay alguna forma de refactorizar su código para no depender tanto de los efectos secundarios. Si eso no es práctico, a menudo puede evitar la duplicación moviendo la responsabilidad a otra clase, como en:

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

Puede parecer una exageración crear una nueva clase CurrentStrings para algo que puede ser servido en su mayoría por un List<String> , pero a menudo abrirá toda una gama de simplificaciones en todo el código. Sin mencionar los beneficios de la encapsulación y la comprobación de tipos.

    
respondido por el Karl Bielefeldt 20.03.2014 - 16:46

Lea otras preguntas en las etiquetas