Maneras elegantes de manejar si (si no) más

161

Esta es una queja menor, pero cada vez que tengo que codificar algo como esto, la repetición me molesta, pero no estoy seguro de que ninguna de las soluciones sea peor.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • ¿Hay un nombre para este tipo de lógica?
  • ¿Soy un poco demasiado TOC?

Estoy abierto a sugerencias de códigos malvados, aunque solo sea por curiosidad ...

Benjol
fuente
8
@Emmad Kareem: dos DefaultActionllamadas violan el principio DRY
Abyx
Gracias por su respuesta, pero creo que está bien, excepto por no usar try / catch ya que puede haber errores que no devuelven resultados y podrían causar anulaciones (dependiendo de su lenguaje de programación).
NoChance
20
Creo que el problema principal aquí es que estás trabajando en niveles inconsistentes de abstracción . El nivel de abstracción más alto es: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Los detalles esenciales de asegurarse de tener los datos para DoSomething () están en un nivel de abstracción más bajo y, por lo tanto, deberían estar en una función diferente. Esta función tendrá un nombre en el nivel superior de abstracción, y su implementación será de bajo nivel. Las buenas respuestas a continuación abordan este problema.
Gilad Naor
66
Por favor, especifique un idioma. Las posibles soluciones, expresiones idiomáticas estándar y normas culturales de larga data son diferentes para diferentes idiomas y darán lugar a diferentes respuestas a su Q.
Caleb
1
Puede hacer referencia a este libro "Refactorización: Mejora del diseño del código existente". Hay varias secciones sobre la estructura if-else, práctica realmente útil.
Vacker

Respuestas:

96

Extraerlo para separar la función (método) y usar la returndeclaración:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

O, tal vez mejor, separe los contenidos y su procesamiento:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Upd:

Por qué no excepciones, por qué OpenFileno arroja una excepción IO:
creo que es una pregunta realmente genérica, en lugar de una pregunta sobre el archivo IO. Nombres como FileExists, OpenFilepuede ser confuso, pero si para sustituirlos por Foo, Bar, etc, - Sería más claro que DefaultActionse puede llamar las veces que sean DoSomething, por lo que puede ser el caso no excepcional. Péter Török escribió sobre esto al final de su respuesta.

Por qué hay un operador condicional ternario en la segunda variante:
si hubiera una etiqueta [C ++], escribiría una ifdeclaración con declaración de contentsen su parte de condición:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Pero para otros lenguajes (tipo C), if(contents) ...; else ...;es exactamente lo mismo que la declaración de expresión con operador condicional ternario, pero más larga. Debido a que la parte principal del código era la get_contentsfunción, simplemente utilicé la versión más corta (y también omití el contentstipo). De todos modos, está más allá de esta pregunta.

Abyx
fuente
93
+1 para devoluciones múltiples : cuando los métodos se hacen lo suficientemente pequeños , este enfoque funcionó mejor para mí
mosto
No soy un gran admirador de los retornos múltiples, aunque lo uso ocasionalmente. Es bastante razonable en algo simple, pero simplemente no escala bien. Nuestro estándar es evitarlo para todos los métodos simples pero locos porque los métodos tienden a crecer en tamaño más de lo que se reducen.
Brian Knoblauch el
3
Múltiples rutas de retorno pueden tener implicaciones negativas de rendimiento en programas C ++, anulando los esfuerzos del optimizador para emplear RVO (también NRVO, a menos que cada ruta devuelva el mismo objeto).
Functastic
Recomiendo invertir la lógica en la segunda solución: {if (el archivo existe) {establecer contenido; if (sometest) {devolver contenido; }} return null; } Simplifica el flujo y reduce el número de líneas.
Wedge
1
Hola Abyx, noté que incorporaste algunos de los comentarios de los comentarios aquí: gracias por hacerlo. He limpiado todo lo que se abordó en su respuesta y otras respuestas.
56

Si el lenguaje de programación que está utilizando (0) hace cortocircuitos en las comparaciones binarias (es decir, si no llama SomeTestsi FileExistsdevuelve falso) y (1) la asignación devuelve un valor (el resultado de OpenFilese asigna contentsy luego ese valor se pasa como argumento a SomeTest), se puede usar algo como lo siguiente, pero aún se le recomienda que le permite comentar el código señalar que la única =es intencional.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Dependiendo de cuán complicado sea el if, podría ser mejor tener una variable de bandera (que separa la prueba de las condiciones de éxito / falla con el código que maneja el error DefaultActionen este caso)

frozenkoi
fuente
Así es como lo haría.
Anthony
13
Bastante asqueroso poner tanto código en una ifdeclaración, en mi opinión.
moteutsch 01 de
15
Yo, por el contrario, me gusta este tipo de declaración "si algo existe y cumple con esta condición". +1
Gorpik
¡Yo también! Personalmente, no me gusta la forma en que las personas usan devoluciones múltiples. Algunas premisas no se cumplen. ¿Por qué no se invierte esos IFS y ejecutar su código si se cumplen?
klaar
"Si algo existe y cumple con esta condición" está bien. "Si algo existe y hace algo tangencialmente relacionado aquí y cumple con esta condición", OTOH, es confuso. En otras palabras, no me gustan los efectos secundarios en una condición.
Piskvor
26

Más en serio que la repetición de la llamada a DefaultAction es el estilo en sí mismo porque el código está escrito no ortogonal (vea esta respuesta por buenas razones para escribir ortogonalmente).

Para mostrar por qué el código no ortogonal es malo, considere el ejemplo original, cuando se introduce un nuevo requisito de que no debemos abrir el archivo si está almacenado en un disco de red. Bueno, entonces podríamos actualizar el código a lo siguiente:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Pero también viene el requisito de que tampoco debemos abrir archivos grandes de más de 2 Gb. Bueno, acabamos de actualizar nuevamente:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Debe quedar muy claro que ese estilo de código será un gran problema de mantenimiento.

Entre las respuestas aquí que están escritas correctamente ortogonalmente están el segundo ejemplo de Abyx y la respuesta de Jan Hudec , por lo que no repetiré eso, solo señale que agregar los dos requisitos en esas respuestas sería simplemente

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(o en goto notexists;lugar de return null;), sin afectar a ningún otro código que las líneas agregadas . Por ejemplo, ortogonal.

Al realizar la prueba, la regla general debe ser probar las excepciones, no el caso normal .

hlovdal
fuente
8
+1 para mí Los primeros retornos ayudan a evitar el patrón anti punta de flecha. Consulte codinghorror.com/blog/2006/01/flattening-arrow-code.html y lostechies.com/chrismissal/2009/05/27/… Antes de leer sobre este patrón, siempre me suscribí a la 1 entrada / salida por función teoría debido a lo que me enseñaron hace 15 años más o menos. Creo que esto hace que el código sea mucho más fácil de leer y, como mencionas, más fácil de mantener.
Mr Moose
3
@MrMoose: su mención del antipatrón de punta de flecha responde a la pregunta explícita de Benjol: "¿Hay un nombre para este tipo de lógica?" Publíquelo como respuesta y tendrá mi voto.
outis
Esta es una gran respuesta, gracias. Y @MrMoose: "el patrón anti punta de flecha" posiblemente responde mi primera viñeta, así que sí, publíquelo. No puedo prometer que lo aceptaré, ¡pero merece votos!
Benjol
@outis. Gracias. He añadido respuesta. El patrón anti punta de flecha ciertamente es relevante en la publicación de hlovdal y sus cláusulas de guardia funcionan bien para sortearlas. No sé cómo podrías responder el segundo punto a esto. No estoy calificado para diagnosticar eso :)
Mr Moose
44
+1 para "excepciones de prueba, no el caso normal".
Roy Tinker
25

Obviamente:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Dijiste que estás abierto incluso a soluciones malvadas, así que usar el mal goto cuenta, no?

De hecho, dependiendo del contexto, esta solución podría ser menos malvada que el mal haciendo la acción dos veces o la variable extra malvada. Lo envolví en una función, porque definitivamente no estaría bien en el medio de una función larga (no menos importante debido al retorno en el medio). Pero que la función larga no está bien, punto.

Cuando tenga excepciones, serán más fáciles de leer, especialmente si puede tener OpenFile y DoSomething simplemente lanzar una excepción si no se cumplen las condiciones, por lo que no necesita verificaciones explícitas en absoluto. Por otro lado, en C ++, Java y C # lanzar una excepción es una operación lenta, por lo que desde el punto de rendimiento, el goto sigue siendo preferible.


Nota sobre "mal": la Pregunta frecuente C ++ 6.15 define "mal" como:

Significa que tal y tal es algo que debe evitar la mayor parte del tiempo, pero no es algo que deba evitar todo el tiempo. Por ejemplo, terminarás usando estas cosas "malvadas" siempre que sean "la menos malvada de las alternativas malvadas".

Y eso se aplica gotoen este contexto. Las construcciones de control de flujo estructuradas son mejores la mayoría de las veces, pero cuando te encuentras en la situación en la que acumulan demasiados de sus propios males, como la asignación en condiciones, anidando más de aproximadamente 3 niveles de profundidad, duplicando el código o condiciones largas, gotosimplemente puede terminar siendo menos malvado.

Jan Hudec
fuente
11
Mi cursor está sobre el botón aceptar ... solo para molestar a todos los puristas. Oooohh la tentación: D
Benjol
2
¡Sí Sí! Esta es la forma absolutamente "correcta" de escribir código. La estructura del código ahora es "Si es un error, maneje el error. Acción normal. Si es un error, maneje el error. Acción normal", que es exactamente como debería ser. Todo el código "normal" se escribe con un solo nivel de sangría, mientras que todo el código relacionado con el error tiene dos niveles de sangría. Entonces, el código normal Y MÁS IMPORTANTE obtiene el lugar visual más prominente y es posible leer el flujo de manera muy rápida y fácil secuencialmente hacia abajo. Por supuesto acepte esta respuesta.
hlovdal
2
Y otro aspecto es que el código escrito de esta manera es ortogonal. Por ejemplo, las dos líneas "if (! FileExists (file)) \ n \ tgoto notexists;" ahora SÓLO están relacionados con el manejo de este aspecto de error único (KISS) y, lo que es más importante, no afecta a ninguna de las otras líneas . Esta respuesta stackoverflow.com/a/3272062/23118 enumera varias buenas razones para mantener el código ortogonal.
hlovdal
55
Hablando de las soluciones malas: Puedo tener su solución sin Goto:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
Herby
44
@herby: Su solución es más malvada que goto, porque está abusando breakde una manera que nadie espera que se abuse, por lo que las personas que leen el código tendrán más problemas para ver hacia dónde los lleva la ruptura que con goto, que lo dice explícitamente. Además, está utilizando un bucle infinito que solo se ejecutará una vez, lo que será bastante confuso. Desafortunadamente, do { ... } while(0)tampoco es exactamente legible, porque solo ves que es un bloque divertido cuando llegas al final y C no admite la ruptura de otros bloques (a diferencia de Perl).
Jan Hudec
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
fuente
o vaya al macho extra y cree un método adicional FileExistsAndConditionMet (archivo) ...
UncleZeiv
@herby SomeTestpuede tener la misma semántica que la existencia del archivo si SomeTestverifica el tipo de archivo, por ejemplo, comprueba que .gif es realmente un archivo GIF.
Abyx
1
Si. Depende @Benjol lo sabe mejor.
herby
3
... por supuesto, quise decir "ir más allá" ... :)
UncleZeiv
2
Eso está llevando los raviolis a las extremidades, aunque yo no vaya (y soy extremo en esto) ... Creo que ahora es muy fácil de leer teniendo en cuenta contents && f(contents). ¿Dos funciones para salvar una más?
herby
12

Una posibilidad:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Por supuesto, esto hace que el código sea un poco más complejo de una manera diferente. Por lo tanto, es en gran medida una pregunta de estilo.

Un enfoque diferente sería utilizar excepciones, por ejemplo:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Esto parece más simple, sin embargo, solo es aplicable si

  • sabemos exactamente qué tipo de excepciones esperar y se DefaultAction()ajusta a cada
  • esperamos que el procesamiento del archivo SomeTest()tenga éxito, y un archivo faltante o una falla es claramente una condición errónea, por lo tanto, es adecuado lanzar una excepción sobre él.
Péter Török
fuente
19
Noooo ~! No es una variable de indicador, definitivamente es un camino equivocado, porque conduce a un código complejo, difícil de entender (dónde-ese-indicador-se convierte en verdadero) y difícil de refactorizar.
Abyx
No si lo restringe al alcance más local posible. (function () { ... })()en Javascript, { flag = false; ... }en C-like etc.
herby
+1 para la lógica de excepción, que bien podría ser la solución más adecuada según el escenario.
Steven Jeuris
44
+1 Este mutuo '¡Nooooo!' es divertido. Creo que la variable de estado y el retorno temprano son razonables en ciertos casos. En rutinas más complejas, elegiría la variable de estado porque, en lugar de agregar complejidad, lo que realmente hace es hacer explícita la lógica. Nada de malo con eso.
Grossvogel
1
Este es nuestro formato preferido donde trabajo. Las 2 principales opciones utilizables parecen ser "retornos múltiples" y "variables de marca". Ninguno de los dos parece tener ningún tipo de verdadera ventaja en promedio, pero ambos se ajustan a ciertas circunstancias mejor que otras. Tienes que ir con tu caso típico. Solo otra guerra religiosa "Emacs" vs. "Vi". :-)
Brian Knoblauch
11

Esto está en un nivel más alto de abstracción:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

Y esto completa los detalles.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Jeanne Pindar
fuente
11

Las funciones deberían hacer una cosa. Deberían hacerlo bien. Solo deberían hacerlo.
- Robert Martin en Clean Code

Algunas personas encuentran que este enfoque es un poco extremo, pero también es muy limpio. Permítame ilustrar en Python:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Cuando dice funciones deben hacer una cosa, se refiere a una cosa. processFile()elige una acción basada en el resultado de una prueba, y eso es todo lo que hace. fileMeetsTest()combina todas las condiciones de la prueba, y eso es todo lo que hace. contentsTest()transfiere la primera línea a firstLineTest(), y eso es todo lo que hace.

Parecen muchas funciones, pero se lee prácticamente como el inglés directo:

Para procesar el archivo, verifique si cumple con la prueba. Si es así, entonces haz algo. De lo contrario, tome la acción predeterminada. El archivo cumple con la prueba si existe y pasa la prueba de contenido. Para probar el contenido, abra el archivo y pruebe la primera línea. La prueba para la primera línea ...

De acuerdo, eso es un poco prolijo, pero tenga en cuenta que si un mantenedor no se preocupa por los detalles, puede dejar de leer después de solo las 4 líneas de código processFile(), y aún tendrá un buen conocimiento de alto nivel de lo que hace la función.

Karl Bielefeldt
fuente
55
+1 Es un buen consejo, pero lo que constituye "una cosa" depende de la capa actual de abstracción. processFile () es "una cosa", pero dos cosas: fileMeetsTest () y doSomething () o defaultAction (). Me temo que el aspecto de "una cosa" puede confundir a los principiantes que a priori no entienden el concepto.
Caleb
1
Es un buen objetivo ... Eso es todo lo que tengo que decir al respecto ... ;-)
Brian Knoblauch
1
No me gusta pasar argumentos implícitamente como variables de instancia como esa. Te llenas de variables de instancia "inútiles" y hay muchas maneras de estropear tu estado y romper las invariantes.
hugomg
@Caleb, ProcessFile () de hecho está haciendo una cosa. Como Karl dice en su publicación, está usando una prueba para decidir qué acción tomar y diferir la implementación real de las posibilidades de acción a otros métodos. Si tuviera que agregar muchas más acciones alternativas, los criterios de un solo propósito para el método aún se cumplirían siempre que no se produzca un anidamiento de la lógica en el método inmediato.
S.Robins
6

Con respecto a cómo se llama esto, puede convertirse fácilmente en el anti patrón de punta de flecha a medida que su código crece para manejar más requisitos (como se muestra en la respuesta proporcionada en https://softwareengineering.stackexchange.com/a/122625/33922 ) y luego cae en la trampa de tener grandes secciones de códigos con declaraciones condicionales anidadas que se asemejan a una flecha.

Ver enlaces como;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

Hay mucho más sobre este y otros patrones anti que se encuentran en Google.

Algunos buenos consejos que Jeff brinda en su blog con respecto a esto son;

1) Reemplace las condiciones con cláusulas de protección.

2) Descomponer bloques condicionales en funciones separadas.

3) Convertir cheques negativos en cheques positivos

4) Regrese siempre oportunista tan pronto como sea posible de la función.

Vea algunos de los comentarios en el blog de Jeff sobre las sugerencias de Steve McConnells sobre devoluciones anticipadas también;

"Utilice un retorno cuando mejore la legibilidad: en ciertas rutinas, una vez que sepa la respuesta, desea devolverla a la rutina de llamada inmediatamente. Si la rutina se define de tal manera que no requiera más limpieza una vez que detecta un error, no volver inmediatamente significa que tienes que escribir más código ".

...

"Minimice el número de devoluciones en cada rutina: es más difícil entender una rutina cuando, al leerla en la parte inferior, no se da cuenta de la posibilidad de que haya regresado en algún lugar por encima. Por esa razón, use las devoluciones con prudencia, solo cuando mejoran legibilidad ".

Siempre me suscribí a la teoría de 1 entrada / salida por función debido a lo que me enseñaron hace 15 años más o menos. Creo que esto hace que el código sea mucho más fácil de leer y, como mencionas, más fácil de mantener

Mr Moose
fuente
6

Esto se ajusta a las reglas DRY, no-goto y no-multiple-return, es escalable y legible en mi opinión:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
fuente
1
Sin embargo, cumplir con los estándares no necesariamente equivale a un buen código. Actualmente estoy indeciso sobre este fragmento de código.
Brian Knoblauch el
esto solo reemplaza 2 defaultAction (); con 2 condiciones if idénticas y agrega una variable flag que es mucho peor.
Ryathal
3
El beneficio de usar una construcción como esta es que a medida que aumenta el número de pruebas, el código no comienza a anidar más ifdentro de otros if. Además, el código para manejar el caso fallido ( DefaultAction()) solo está en un lugar y para fines de depuración, el código no salta alrededor de las funciones auxiliares y al agregar puntos de interrupción a las líneas donde successse cambia la variable puede mostrar rápidamente qué pruebas pasaron (por encima de las activadas punto de interrupción) y cuáles no se han probado (a continuación).
frozenkoi
1
Sí, me está gustando, pero creo que cambiaría el nombre successa ok_so_far:)
Benjol
Esto es muy similar a lo que hago cuando (1) el proceso es muy lineal cuando todo va bien y (2) de lo contrario tendría el antipatrón de flecha. Sin embargo, trato de evitar agregar una variable adicional, que generalmente es fácil si piensa en términos de los requisitos previos para el siguiente paso (que es sutilmente diferente de preguntar si un paso anterior falló). Si el archivo existe, ábralo. Si el archivo está abierto, lea el contenido. Si tengo contenido, procésalos, de lo contrario, realiza la acción predeterminada.
Adrian McCarthy el
3

Lo extraería a un método separado y luego:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

que también permite

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

entonces posiblemente podría eliminar las DefaultActionllamadas y dejar la ejecución DefaultActionde la llamada:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

También me gusta el enfoque de Jeanne Pindar .

Konrad Morawski
fuente
3

Para este caso particular, la respuesta es bastante fácil ...

Hay una condición de carrera entre FileExistsy OpenFile: ¿qué sucede si se elimina el archivo?

La única forma sensata de lidiar con este caso particular es omitir FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Esto resuelve perfectamente este problema y hace que el código sea más limpio.

En general: intente repensar el problema e idee otra solución que evite el problema por completo.

Martin Wickman
fuente
2

Otra posibilidad si no le gusta ver demasiados es dejar el uso de else por completo y agregar una declaración de devolución adicional. Lo demás es algo superfluo a menos que requiera una lógica más compleja para determinar si hay más de dos posibilidades de acción.

Por lo tanto, su ejemplo podría convertirse en:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Personalmente, no me importa usar la cláusula else, ya que establece explícitamente cómo se supone que funciona la lógica y, por lo tanto, mejora la legibilidad de su código. Sin embargo, algunas herramientas de embellecimiento de código prefieren simplificar a una sola instrucción if para desalentar la lógica de anidamiento.

S.Robins
fuente
2

El caso que se muestra en el código de muestra generalmente se puede reducir a una sola ifdeclaración. En muchos sistemas, la función de abrir archivo devolverá un valor no válido si el archivo aún no existe. A veces este es el comportamiento predeterminado; otras veces, debe especificarse mediante un argumento. Esto significa que la FileExistsprueba se puede descartar, lo que también puede ayudar con las condiciones de carrera resultantes de la eliminación del archivo entre la prueba de existencia y la apertura del archivo.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Esto no aborda directamente el problema de la mezcla de niveles de abstracción, ya que evita completamente el problema de múltiples pruebas imposibles de encadenar, aunque eliminar la prueba de existencia de archivos no es incompatible con separar los niveles de abstracción. Suponiendo que los identificadores de archivos no válidos son equivalentes a "falsos" y los identificadores de archivos se cierran cuando salen del alcance:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
descuento
fuente
2

Estoy de acuerdo con frozenkoi, sin embargo, para C # de todos modos, pensé que sería útil seguir la sintaxis de los métodos TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
fuente
1

Su código es feo porque está haciendo demasiado en una sola función. Desea procesar el archivo o realizar la acción predeterminada, así que comience diciendo que:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Los programadores de Perl y Ruby escriben processFile(file) || defaultAction()

Ahora ve a escribir ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
kevin cline
fuente
1

Por supuesto, solo puede ir tan lejos en escenarios como estos, pero aquí hay un camino a seguir:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Es posible que desee filtros adicionales. Entonces haz esto:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Aunque esto también podría tener sentido:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

¿Cual es el mejor? Eso depende del problema del mundo real que estés enfrentando.
Pero lo que hay que quitar es: puedes hacer mucho con la composición y el polimorfismo.

back2dos
fuente
1

¿Qué hay de malo con lo obvio?

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

¿Me parece bastante estándar? Para ese tipo de gran procedimiento en el que tienen que suceder muchas cosas pequeñas, el fracaso de cualquiera de ellas evitaría esto último. Las excepciones lo hacen un poco más limpio si esa es una opción.

Steve Bennett
fuente
0

Me doy cuenta de que esta es una vieja pregunta, pero noté un patrón que no se ha mencionado; principalmente, estableciendo una variable para luego determinar el / los método / s que le gustaría llamar (fuera del if ... else ...).

Esto es solo como otro ángulo a tener en cuenta para facilitar el trabajo del código. También permite cuándo puede agregar otro método para llamar o cambiar el método apropiado que debe llamarse en ciertas situaciones.

En lugar de tener que reemplazar todas las menciones del método (y tal vez faltan algunos escenarios), todas se enumeran al final del bloque if ... else ... y son más fáciles de leer y modificar. Tiendo a usar esto cuando, por ejemplo, se pueden llamar varios métodos, pero dentro de los anidados si ... de lo contrario ... se puede llamar a un método en varias coincidencias.

Si establece una variable que define el estado, podría tener muchas opciones profundamente anidadas y actualizar el estado cuando algo debe realizarse (o no).

Esto podría usarse como en el ejemplo que se hace en la pregunta donde estamos verificando si 'DoSomething' ha ocurrido, y si no, realiza la acción predeterminada. O podría tener un estado para cada método al que desee llamar, establecer cuando corresponda, luego llamar al método aplicable fuera del if ... si no ...

Al final de las instrucciones anidadas if ... else ..., verifica el estado y actúa en consecuencia. Esto significa que solo necesita una sola mención de un método en lugar de todas las ubicaciones en las que se debe aplicar.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Steve Padmore
fuente
0

Para reducir el IF anidado:

1 / regreso temprano;

2 / expresión compuesta (consciente de cortocircuito)

Entonces, su ejemplo puede ser refactorizado así:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
fuente
0

Vi muchos ejemplos con "return" que también uso, pero a veces quiero evitar crear nuevas funciones y usar un bucle en su lugar:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Si desea escribir menos líneas u odia los bucles infinitos como yo, puede cambiar el tipo de bucle a "do ... while (0)" y evitar el último "break".

XzKto
fuente
0

¿Qué tal esta solución?

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Supuse que OpenFile devuelve un puntero, pero esto podría funcionar también con el tipo de valor devuelto al especificar algún valor predeterminado no retornable (códigos de error o algo así).

Por supuesto, no espero alguna acción posible a través del método SomeTest en el puntero NULL (pero nunca se sabe), por lo que esto también podría verse como una comprobación adicional del puntero NULL para la llamada SomeTest (contenido).

chedi
fuente
0

Claramente, la solución más elegante y concisa es usar una macro de preprocesador.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Lo que te permite escribir código hermoso como este:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Puede ser difícil confiar en el formateo automático si usa esta técnica con frecuencia, y algunos IDEs pueden gritarle un poco sobre lo que erróneamente supone que está mal formado. Y como dice el dicho, todo es una compensación, pero supongo que no es un mal precio a pagar para evitar los males del código repetido.

Peter Olson
fuente
Para algunas personas, y en algunos idiomas, las macros de preprocesador son códigos malignos :)
Benjol
@Benjol Dijiste que estabas abierto a malas sugerencias, ¿no? ;)
Peter Olson
sí, absolutamente, fue simplemente tu "evitar los males" :)
Benjol
44
Esto es tan horrible, solo tuve que votarlo: D
back2dos
Shirley, no hablas en serio !!!!!!
Jim In Texas
-1

Dado que hizo una consulta por curiosidad, y su pregunta no está etiquetada con un idioma específico (aunque está claro que tenía en mente los idiomas imperativos), puede valer la pena agregar que los idiomas que admiten la evaluación diferida permiten un enfoque completamente diferente. En esos idiomas, las expresiones solo se evalúan cuando es necesario, por lo que puede definir "variables" y usarlas solo cuando tenga sentido hacerlo. Por ejemplo, en un lenguaje de ficción con perezosos let/ inestructuras que se olvide de control de flujo y de escritura:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
tokland
fuente