c++ - guide - ¿Consideras que esta técnica es "MALA"?

google code style java (28)

En ocasiones, debe omitir la ejecución de parte de un método bajo ciertas condiciones de error no críticas. Puede usar excepciones para eso, pero las excepciones generalmente no se recomiendan en la lógica de aplicación normal, solo para situaciones anormales.

Así que hago un truco como este:

do { bool isGood = true; .... some code if(!isGood) break; .... some more code if(!isGood) break; .... some more code } while(false); ..... some other code, which has to be executed.

Utilizo un bucle "falso" que se ejecutará una vez, y puedo abortarlo por interrupción o continuar .

A algunos de mis colegas no les gustó eso, y lo llamaron "mala práctica". Personalmente encuentro ese enfoque bastante astuto. Pero ¿qué piensas?

Refactor La solución limpia en la mayoría de los casos será dividir este código en una función de ayuda más pequeña, desde la cual puede regresar, en lugar de salir de su no-realmente-un-bucle.

Luego puedes sustituir tus descansos por devolución, y ahora las personas pueden dar sentido a tu código de inmediato al leerlo, en lugar de tener que parar y preguntarse por qué hiciste este bucle que no funciona en bucle.

Sí, diría que simplemente porque no se comporta como el lector esperaría, es una mala práctica. El principio de menos sorpresa, y así sucesivamente. Cuando veo un ciclo, espero que se repita, y si no, tengo que parar y preguntarme por qué .

¿Por qué usar un bucle falso? Puede hacer lo mismo con un método y probablemente no será considerado una "mala práctica", ya que es más esperado.

someMethod() { .... some code if(!isGood) return; .... some more code if(!isGood) return; .... some more code }

¿Qué tal un enfoque funcional?

void method1() { ... some code if( condition ) method2(); } void method2() { ... some more code if( condition ) method3(); } void method3() { ... yet more code if( condition ) method4(); }

Básicamente acabas de describir goto. Uso goto en C todo el tiempo. No lo considero malo, a menos que lo use para emular un bucle (¡nunca lo haga!). Mi uso típico de goto en C es emular excepciones (C no tiene excepciones):

// Code if (bad_thing_happened) goto catch; // More code if (bad_thing_happened) goto catch; // Even more code finally: // This code is executed in any case // whether we have an exception or not, // just like finally statement in other // languages return whatever; catch: // Code to handle bad error condition // Make sure code tagged for finally // is executed in any case goto finally;

Excepto por el hecho de que atrapar y finalmente tener orden opuesto, no veo por qué este código debería ser MALO solo porque usa goto, si un verdadero código try / catch / finally funciona exactamente como este y simplemente no usa goto. Eso no tiene sentido. Y, por lo tanto, no veo por qué su código debe etiquetarse como MALO.

Consideraría esta mala práctica. Creo que sería más idiomático, y generalmente más claro si hicieras esto una función y cambiaras los saltos a devoluciones.

Creo que básicamente no hay nada malo con la técnica. Creo que me aseguraré de que el bool se llame algo más descriptivo que lo que es bueno. Aunque, ahora que lo veo, ¿por qué no funciona poner todo el código que está en el bucle en un único bloque if (! IsGood)? El ciclo solo se ejecuta una vez.

Creo que la gente no está siendo honesta aquí.

Si yo, como líder de tu equipo, vería un código como este, estarías listo para ser pequeño y marcado como un problema potencial para el equipo, ya que ese código es particularmente horrible.

Digo que debería escuchar a sus colegas y volver a escribir siguiendo cualquiera de las sugerencias publicadas aquí.

Creo que tendría que estar de acuerdo con sus colegas solo por la legibilidad, no está claro a primera vista lo que está tratando de lograr con este código.

¿Por qué no solo usar

if(isGood) { ...Execute more code }

Depende de cuáles sean las alternativas. Tienes que admitir que el código que publicaste es algo feo. No diría que está claro. Es una especie de truco. Entonces, si usar alguna otra solución de codificación sería peor, entonces está bien. Pero si tienes una mejor alternativa, no dejes que la excusa "es lo suficientemente buena" te consuele.

Divida su código en trozos más pequeños de elementos funcionales, de modo que pueda dividir lo anterior en una función que retorna en lugar de romperse.

No sé si lo anterior es una mala práctica, pero su legibilidad es un poco escasa y puede ser confusa para otros que podrían tener que mantener la fuente.

Es un idioma muy extraño. Utiliza un bucle para algo que no está destinado y puede causar confusión. Me imagino que esto abarcará más de una página, y sería una sorpresa para la mayoría de las personas que esto nunca se ejecute más de una vez.

¿Qué tal si usamos funciones de lenguaje más comunes como funciones?

bool success = someSensibleFunctionName(); if(success) { ... } someCommonCodeInAnotherFunction();

Esto es intrincado y confuso, lo descartaría de inmediato.

Considera esta alternativa:

private void DoSomething() { // some code if (some condition) { return; } // some more code if (some other condition) { return; } // yet more code }

También considere dividir el código anterior en más de un método.

Esto es para lo que son las excepciones. No puede continuar la lógica porque algo salió mal. Incluso si la recuperación es trivial, sigue siendo una excepción. Si tiene una buena razón para no usar excepciones, como cuando programa en un idioma que no las admite, utilice bloques condicionales, no bucles.

Has complicado el flujo de control no lineal dentro de una expresión idiomática difícil de reconocer. Entonces, sí, creo que esta técnica es mala.

Puede valer la pena gastar algún tiempo tratando de averiguar si esto puede escribirse un poco mejor.

He usado este idioma por años. Encuentro que es preferible a los ifs anidados o en serie similares, al "goto onError", o que tienen retornos múltiples. El ejemplo anterior con el ciclo anidado es algo a lo que hay que tener cuidado. Siempre agrego un comentario sobre el "hacer" para dejar en claro a cualquier persona nueva en el idioma.

Lo que intenta hacer es recuperación de fallas no locales. Esto es para lo que es goto . Úselo. (en realidad, este es el manejo de excepciones, pero si no puedes usarlo, ''goto'' o ''setjmp / longjmp'' son la mejor opción).

Este patrón, el patrón if(succeeded(..)) y ''goto cleanup'', los 3 son semánticamente y estructuralmente equivalentes. Usa el que sea más común en tu proyecto de código. Hay mucho valor en la coherencia.

Quisiera advertirme if(failed(..)) break; en un punto en el que está produciendo un resultado sorprendente si intenta anidar bucles:

do{ bool isGood = true; .... some code if(!isGood) break; .... some more code for(....){ if(!isGood) break; // <-- OOPS, this will exit the ''for'' loop, which // probably isn''t what the author intended .... some more code } } while(false); ..... some other code, which has to be executed.

Ni la goto cleanup ni if(succeeded(..)) tienen esta sorpresa, así que recomendaría usar uno de estos dos en su lugar.

Mala práctica, depende.

Lo que veo en este código es una forma muy creativa de escribir "goto" con menos palabras clave con olor a azufre.

Existen múltiples alternativas a este código, que puede o no ser mejor, según la situación.

Su solución do / while

Su solución es interesante si tiene un montón de código, pero evaluará la "salida" de este procesamiento en algunos puntos limitados:

do { bool isError = false ; /* some code, perhaps setting isError to true */ if(isError) break ; /* some code, perhaps setting isError to true */ if(isError) break ; /* some code, perhaps setting isError to true */ } while(false) ; // some other code

El problema es que no puedes usar tu "if (isError) break" fácilmente; es un bucle, ya que solo saldrá del bucle interno, no del bloque do / while.

Y, por supuesto, si la falla está dentro de otra función, la función debe devolver algún tipo de código de error, y su código no debe olvidar interpretar el código de error correctamente.

No discutiré alternativas utilizando ifs o incluso if anidados porque, después de pensar un poco, les encuentro soluciones inferiores a las suyas para su problema.

Llamar a un goto a ... goto

Quizás debería poner claramente sobre la mesa el hecho de que está usando un goto, y documentar las razones por las que elige esta solución sobre otra.

Al menos, mostrará que algo podría estar mal con el código, y pedirá a los revisores que validen o invaliden su solución.

Todavía debe abrir un bloque, y en lugar de romperlo, use un goto.

{ // etc. if(/*some failure condition*/) goto MY_EXIT ; // etc. while(/* etc.*/) { // etc. for(/* etc.*/) { // etc. if(/*some failure condition*/) goto MY_EXIT ; // etc. } // etc. if(/*some failure condition*/) goto MY_EXIT ; // etc. } // etc. } MY_EXIT: // some other code

De esta forma, al salir del bloque a través del goto, no hay forma de evitar un constructor de objetos con el goto (que está prohibido por C ++).

Este problema resuelve el proceso que sale del problema de bucles anidados (y usar goto para salir de bucles anidados es un ejemplo dado por B. Stroustrup como un uso válido de goto), pero no resolverá el hecho de que algunas llamadas a funciones podrían fallar y ser ignoradas (porque alguien no pudo probar correctamente su código de retorno, si corresponde).

Por supuesto, ahora, puede salir de su proceso desde múltiples puntos, desde la profundidad de anidamiento múltiple del bucle, por lo que si es un problema ...

trata de atraparlo

Si se supone que el código no debe fallar (por lo tanto, la falla es excepcional), o incluso si la estructura del código puede fallar, pero es demasiado complejo para salir, entonces el siguiente enfoque podría ser más claro:

try { // All your code // You can throw the moment something fails // Note that you can call functions, use reccursion, // have multiple loops, etc. it won''t change // anything: If you want to exit the process, // then throw a MyExitProcessException exception. if(/* etc. */) { // etc. while(/* etc.*/) { // etc. for(/* etc.*/) { // etc. if(/*some failure condition*/) throw MyExitProcessException() ; // etc. } // etc. callSomeFunction() ; // the function will throw if the condition is met // so no need to test a return code // etc. } // etc. } // etc. } catch(const MyExitProcessException & e) { // To avoid catching other exceptions, you should // define a "MyExitProcessException" exception } // some other code

Si no se cumple alguna condición en el código anterior, o dentro de algunas funciones llamadas por el código anterior, entonces arroje una excepción.

Esto es un poco más pesado que su solución do / while, pero tiene las mismas ventajas, e incluso puede abortar el procesamiento desde bucles internos o desde dentro de funciones llamadas.

Discusión

Su necesidad parece provenir del hecho de que puede tener un proceso complejo para ejecutar (código, llamadas a funciones, bucles, etc.), pero desea interrumpirlo por alguna condición (probablemente sea una falla o porque tuvo éxito antes de lo que se esperaba) . Si puede reescribirlo de otra manera, debe hacerlo. Pero tal vez, no hay otra manera.

Supongamos eso.

Si puede codificarlo con un try / catch, hágalo : para interrumpir una pieza compleja de código, lanzar una excepción es la solución correcta (no se debe subestimar el hecho de que puede agregar información de error / éxito dentro de su objeto de excepción). Tendrás un código más claro después de eso.

Ahora, si se encuentra en un cuello de botella de velocidad, resolver su problema con excepciones arrojadas como una salida no es la forma más rápida de hacerlo.

Nadie puede negar que tu solución es un goto glorificado. No habrá un código goto-spaghetti, porque el do / while no te permitirá hacer eso, pero sigue siendo un goto semántico. Esta puede ser la razón por la que algunos podrían encontrar este código "malo": huelen el goto sin encontrar su palabra clave claramente.

En este caso (y en este caso de rendimiento, verificado por perfil), su solución parece correcta y mejor que la alternativa que usa if), pero de menor calidad (IMHO) que la solución goto que, al menos, no se oculta detrás de un bucle falso.

Conclusión

En lo que a mí respecta, considero que su solución es creativa, pero me limitaría a la solución de excepción lanzada.

Entonces, en orden de preferencia:

Usa try / catch
Usa goto
Usa tu ciclo do / while
Use ifs / ifs anidados

No tengo ningún problema con eso, siempre y cuando el código sea legible.

Para bien o para mal, he usado la construcción en algunos lugares. El comienzo de esto está claramente documentado, sin embargo:

/* This is a one-cycle loop that simplifies error handling */ do { ...modestly complex code, including a nested loop... } while (0);

Esto está en C, en lugar de C ++, por lo que las excepciones no son una opción. Si estuviera usando C ++, consideraría seriamente usar excepciones para manejar excepciones. La repetida expresión idiomática sugerida por Jeremy también es razonable; Lo he usado con más frecuencia. RAII me ayudaría mucho, también; lamentablemente, C no lo admite fácilmente. Y usar más funciones ayudaría. El manejo de las pausas desde el interior del ciclo anidado se realiza mediante una prueba repetida.

No lo clasificaría como un gran estilo; No lo categorizaría automáticamente como "MALO".

Para mí, lo que estás haciendo es malo de muchas maneras. El ciclo se puede reemplazar poniendo ese código en un método.

Personalmente creo que si tienes que poner un! frente a sus condiciones, entonces está buscando algo equivocado. Para facilitar la lectura, haz que tu booleano coincida con lo que estás buscando. Realmente está verificando si hay un error o alguna mala condición, por lo que preferiría:

If (isError) { //Do whatever you need to do for the error and return; } encima

If (!isGood) { //Do something }

Por lo tanto, verifique lo que realmente desea verificar y mantenga las comprobaciones de excepción al mínimo. Tu objetivo debe ser legible antes que ser complicado. Piensa en la pobre alma que va a tener que venir y mantener tu código.

Una de las primeras cosas en las que trabajé hace 28 años fue un programa Fortran que siempre necesitaba verificar si había un dispositivo gráfico disponible. Alguien tomó la gran decisión de llamar al booleano para este LNOGRAF, por lo que si hubiera un dispositivo gráfico disponible, sería falso. Creo que se configuró de esta manera debido a una eficacia falsa, la verificación del dispositivo devolvió cero si había un dispositivo gráfico. A lo largo del código, todos los controles fueron para ver si el dispositivo gráfico estaba disponible. Estaba lleno de:

If (.NOT. LNOGRAF)

No creo que haya un solo:

Si (LNOGRAF)

en el programa. Esto fue utilizado en el software de planificación de misión para B-52 y misiles de crucero. Definitivamente me enseñó a nombrar mis variables para lo que realmente estoy buscando.

Primero, si solo quiere una respuesta a si esta estructura de código o modismo es "mala", creo que sí. Sin embargo, creo que esto es un síntoma de una mala descomposición en lugar de si el código que tiene es "bueno" o "malo".

Creo que será necesario realizar un análisis y una refactorización mucho mejores para poder seguir abordando el origen del problema, en lugar de mirar solo el código. Si puedes hacer algo como:

if (condition1(...) && condition2(...) && condition3(...) && ... && conditionN(...)) { // code that ought to run after all conditions }; // code that ought to run whether all conditions are met or not

Entonces creo que sería más "legible" y más idiomático. De esta forma, puedes hacer funciones como:

bool conditionN(...) { if (!real_condition) return false; // code that ought to run return true; };

Usted obtiene el beneficio de una mejor descomposición y ayuda del compilador para producir los cortocircuitos necesarios que traerá && ''s. El compilador incluso podría alinear el código en las funciones para producir un mejor código que si lo hiciera con el bucle codificado a mano.

Si se divide el código entre if (! IsGood); en funciones separadas, uno puede terminar con docenas de funciones que contienen solo un par de líneas, de modo que los hacedores no simplifiquen nada. No pude usar el retorno porque no estoy listo para dejar la función, todavía hay cosas que quiero hacer allí. Acepto que probablemente debería conformarme con una condición separada de if (isGood) {...} para cada parte del código que quiero ejecutar, pero a veces eso llevaría a MUCHO par de llaves. Pero supongo que acepto que a la gente realmente no le gusta ese tipo de construcción, ¡entonces las condiciones para todo son vientos!
Gracias por tus respuestas.

Si su código está haciendo algo diferente al significado simple de los constructos en su lugar, es una buena señal de que se ha aventurado en territorio "lindo".

En este caso, tiene un "bucle" que solo se ejecutará una vez. Cualquier lector del código tendrá que hacer una doble inspección para descubrir qué está pasando.

Si el caso en el que no es "bueno" es realmente excepcional, entonces lanzar excepciones sería la mejor opción.

Simplemente estás simplemente disfrazar un "goto" como un bucle falso. Ya sea que te gusten los gotos o no, estarías tan lejos usando un truco real no disimulado.

Personalmente, solo lo escribiría como

bool isGood = true; .... some code if(isGood) { .... some more code } if(isGood) { .... some more code }

Un comentario meta: cuando está codificando, su objetivo debe ser claro, el código que se puede mantener primero. No debe renunciar a la legibilidad en el altar de la eficiencia a menos que haga un perfil y demuestre que es necesario y que realmente mejora las cosas. Los trucos mentales Jedi deben evitarse. Siempre piense que el siguiente hombre en mantener su código es un gran psicópata con la dirección de su hogar. Yo, por ejemplo.

Yo diría que su solución puede ser la solución correcta, pero depende. Paul Tomblin ha publicado una respuesta que es mejor (una serie de si los tubos) ... si se puede utilizar.

La solución de Paul no se puede usar cuando hay costosas inicializaciones de objetos a lo largo del camino a través de su ciclo. Si los objetos creados se usan en pasos posteriores, la solución do while (0) es mejor.

Dicho esto, la denominación variable debería mejorarse. Además, ¿por qué reutilizar tanto la variable "escape"? En lugar de eso, atrape cada condición de error individual de manera explícita y rompa el ciclo de manera que sea obvio qué causa la ruptura en cada punto.

Otra persona sugirió usar llamadas a funciones. De nuevo, esto puede no ser una descomposición ideal si agrega complejidad innecesaria a las llamadas de función debido al número de argumentos que se pueden pasar en cualquier paso dado.

Otros han sugerido que este es un modismo difícil de entender. Bueno, primero podrías poner un comentario como se sugiere en la parte superior del ciclo. En segundo lugar, do-while (0) es un modismo normal y común en macros que todos los programadores C deben reconocer inmediatamente, así que simplemente no lo compro.

public bool Method1(){ ... } public bool Method2(){ ... } public void DoStuff(){ bool everythingWorked = Method1() && Method2(); ... stuff you want executed always ... }

La razón por la que esto funciona se debe a algo llamado lógica de cortocircuito. No se llamará a Method2 si Method1 devuelve falso.

Esto también tiene el beneficio adicional de que puede dividir su método en trozos más pequeños, que serán más fáciles de probar por unidad.

Puede usar excepciones para eso, pero las excepciones generalmente no se recomiendan en la lógica de aplicación normal, solo para situaciones anormales.

No hay nada malo con el uso de excepciones. Las excepciones son parte de la lógica de la aplicación. La directriz sobre excepciones es que deberían ser relativamente raras en tiempo de ejecución.