Invertir una declaración IF

39

Así que he estado programando durante algunos años y recientemente he comenzado a usar ReSharper más. Una cosa que ReSharper siempre me sugiere es "invertir la instrucción 'if' para reducir el anidamiento".

Digamos que tengo este código:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

Y ReSharper me sugerirá que haga esto:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Parece que ReSharper SIEMPRE sugerirá que invierta los IF, sin importar cuánto anidamiento esté ocurriendo. Este problema es que me gusta anidar al menos en ALGUNAS situaciones. Para mí es más fácil de leer y descubrir qué está pasando en ciertos lugares. Este no es siempre el caso, pero a veces me siento más cómodo anidando.

Mi pregunta es: además de la preferencia personal o la legibilidad, ¿hay alguna razón para reducir la anidación? ¿Hay alguna diferencia de rendimiento o alguna otra cosa que no conozca?

Barry Franklin
fuente
1
Posible duplicado de mantener bajo el nivel de sangría
mosquito
24
La clave aquí es "Sugiere Resharper". Eche un vistazo a la sugerencia, si hace que el código sea más fácil de leer y sea consistente, úselo. Hace lo mismo con for / for each loops.
Jon Raynor
Por lo general, degrado esas sugerencias de resharper, no siempre las sigo, por lo que ya no aparecen en la barra lateral.
CodesInChaos
1
ReSharper eliminó lo negativo, que generalmente es algo bueno. Sin embargo, no sugirió un condicional ternario que podría encajar bien aquí, si el bloque es realmente tan simple como su ejemplo.
dcorking
2
¿ReSharper tampoco sugiere mover el condicional a la consulta? foreach (someObject in someObjectList.Where (object => object! = null)) {...}
Roman Reiner el

Respuestas:

63

Depende. En su ejemplo, tener la ifdeclaración no invertida no es un problema, porque el cuerpo de la ifes muy corto. Sin embargo, generalmente desea invertir las declaraciones cuando crecen los cuerpos.

Solo somos humanos y mantener múltiples cosas a la vez en nuestras cabezas es difícil.

Cuando el cuerpo de una declaración es grande, invertir la declaración no solo reduce el anidamiento, sino que también le dice al desarrollador que pueden olvidarse de todo lo que está arriba de esa declaración y centrarse solo en el resto. Es muy probable que use las declaraciones invertidas para deshacerse de los valores incorrectos lo antes posible. Una vez que sepa que están fuera del juego, lo único que queda son valores que realmente pueden procesarse.

Andy
fuente
64
Me encanta ver la marea de opinión de los desarrolladores yendo de esta manera. Hay tanta anidación en el código simplemente porque hay una ilusión popular extraordinaria que break, continuey múltiples return, no están estructuradas.
JimmyJames
66
el problema es la interrupción, etc., sigue siendo un flujo de control que debe tener en mente 'esto no puede ser x o habría salido del bucle en la parte superior' podría no necesitar más reflexión si es una comprobación nula que arrojaría una excepción de todos modos. Pero no es tan bueno cuando tiene más matices 'solo ejecuta la mitad de este código para este estado los jueves'
Ewan
2
@Ewan: ¿Cuál es la diferencia entre 'esto no puede ser x o habría salido del bucle en la parte superior' y 'esto no puede ser x o no habría entrado en este bloque'?
Collin
nada es la complejidad y el significado de la x que es importante
Ewan
44
@Ewan: Creo que break/ continue/ returntengo una ventaja real sobre un elsebloque. Con salidas anticipadas, hay 2 bloques : el bloque de salida y el bloque de permanecer allí; con elsehay 3 bloques : if, else, y lo que viene después (que se usa sea cual sea la rama tomada). Prefiero tener menos bloques.
Matthieu M.
33

No evites los negativos. Los humanos apestan al analizarlos incluso cuando la CPU los ama.

Sin embargo, respeta los modismos. Los humanos somos criaturas de hábitos, por lo que preferimos caminos bien usados ​​a caminos directos llenos de malezas.

foo != nulltristemente se ha convertido en un idioma. Ya casi no me doy cuenta de las partes separadas. Miro eso y solo pienso, "oh, es seguro salir fooahora".

Si quieres anular eso (oh, algún día, por favor) necesitas un caso realmente fuerte. Necesitas algo que permita que mis ojos se deslicen sin esfuerzo y lo entiendan en un instante.

Sin embargo, lo que nos diste fue esto:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

¡Lo cual no es estructuralmente igual!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

Eso no está haciendo lo que mi cerebro perezoso esperaba que hiciera. Pensé que podría seguir agregando código independiente pero no puedo. Esto me hace pensar en cosas en las que no quiero pensar ahora. Rompiste mi idioma pero no me diste uno mejor. Todo porque querías protegerme de la única negativa que ha quemado su pequeño ser desagradable en mi alma.

Lo siento, tal vez ReSharper tenga razón al sugerir esto el 90% del tiempo, pero en las comprobaciones nulas, creo que ha encontrado un caso de esquina donde es mejor ignorarlo. Las herramientas simplemente no son buenas para enseñarle qué es el código legible. Pregúntale a un humano. Haz una revisión por pares. Pero no sigas ciegamente la herramienta.

naranja confitada
fuente
1
Bueno, si tienes más código en el bucle, cómo funciona depende de cómo se junta todo. No es un código independiente. El código refactorizado en la pregunta era correcto para el ejemplo, pero puede no ser correcto cuando no está aislado: ¿era ese el punto al que estaba yendo? (+1 para el no no evitar negativos)
Baldrickk
@Baldrickk if (foo != null){ foo.something() }me permite seguir agregando código sin importarme si fooera nulo. Esto no lo hace. Incluso si no necesito hacer eso ahora, no me siento motivado para arruinar el futuro diciendo "bueno, pueden refactorizarlo si lo necesitan". Feh El software solo es suave si es fácil de cambiar.
candied_orange
44
"seguir agregando código sin importarle" O bien el código debe estar relacionado (de lo contrario, probablemente debería estar separado) o debe tenerse cuidado para comprender la funcionalidad de la función / bloque que está modificando, ya que agregar cualquier código tiene el potencial de cambiar el comportamiento más allá de lo previsto. Para casos simples como este, no creo que importe de ninguna manera. Para casos más complejos, esperaría entender que el código tiene prioridad, por lo que elegiría el que considere más claro, que podría ser cualquiera de los estilos.
Baldrickk
relacionados y entrelazados no son lo mismo.
candied_orange
1
No evite los negativos: D
VitalyB
20

Es el viejo argumento sobre si un descanso es peor que anidar.

La gente parece aceptar la devolución temprana de las verificaciones de parámetros, pero está mal visto en la mayor parte de un método.

Personalmente evito continuar, romper, ir a etc., incluso si eso significa más anidamiento. Pero en la mayoría de los casos puedes evitar ambos.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Obviamente, anidar hace que las cosas sean difíciles de seguir cuando tienes muchas capas

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

Lo peor es cuando tienes ambos

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}
Ewan
fuente
11
Me encanta esta línea: foreach (subObject in someObjectList.where(i=>i != null))no sé por qué nunca he pensado en algo así.
Barry Franklin
@BarryFranklin ReSharper debería tener un subrayado punteado verde en el foreach con "Convertir a expresión LINQ" como la sugerencia que haría eso por usted. Dependiendo de la operación, también podría hacer otros métodos linq como Sum o Max.
Kevin Fee
@BarryFranklin Probablemente sea una sugerencia que desee establecer en un nivel bajo y solo usar discrecionalmente. Si bien funciona bien para casos triviales como este ejemplo, encuentro en un código más complejo la forma en que selecciona los condicionales más complejos en partes que son linqificables y el cuerpo restante tiende a crear cosas que son mucho más difíciles de entender que el original. Por lo general, al menos intentaré la sugerencia porque cuando puede convertir un ciclo completo en Linq, los resultados suelen ser razonables; pero la mitad y la mitad de los resultados tienden a ponerse feo rápido IMO.
Dan Neely
Irónicamente, la edición por resharper tiene exactamente el mismo nivel de anidamiento, porque también tiene un if seguido de una línea.
Peter
je, sin llaves! que aparentemente está permitido: /
Ewan
6

El problema con esto continuees que si agrega más líneas al código, la lógica se complica y es probable que contenga errores. p.ej

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

¿Quieres llamar doSomeOtherFunction()para todos someObject, o solo si no es nulo? Con el código original tienes la opción y es más claro. Con continueuno podría olvidar "oh, sí, allá atrás nos saltamos nulos" y ni siquiera tiene la opción de llamarlo para todos algunos Objetos, a menos que ponga esa llamada al comienzo del método, lo que puede no ser posible o correcto por otras razones

Sí, muchos anidamientos son feos y posiblemente confusos, pero en este caso usaría el estilo tradicional.

Pregunta adicional: ¿Por qué hay nulos en esa lista para empezar? Puede ser mejor nunca agregar un valor nulo en primer lugar, en cuyo caso la lógica de procesamiento es mucho más simple.

user949300
fuente
12
No debería haber suficiente código dentro del bucle para que no pueda ver la comprobación nula en la parte superior sin desplazarse.
Bryan Boettcher
@Bryan Boettcher estuvo de acuerdo, pero no todo el código está tan bien escrito. E incluso varias líneas de código complejo pueden hacer que uno se olvide (o se equivoque) sobre la continuación. En la práctica, estoy bien con returns causa que forman una "ruptura", pero la OMI breaky continueconducen a la confusión y, en la mayoría de los casos, lo mejor es evitarlo.
user949300
44
@ user949300 Después de diez años de desarrollo, nunca he encontrado un descanso o continúo causando confusión. Los he involucrado en la confusión, pero nunca han sido la causa. Por lo general, las malas decisiones sobre el desarrollador fueron la causa, y habrían causado confusión sin importar qué arma usaran.
corsiKa
1
@corsiKa El error SSL de Apple es en realidad un error Goto, pero fácilmente podría haber sido un error de interrupción o continuación. Sin embargo, el código es horrible ...
user949300
3
@BryanBoettcher, el problema me parece que los mismos desarrolladores que piensan que continuar o que los retornos múltiples están bien, también piensan que enterrarlos en grandes cuerpos de métodos también está bien. Por lo tanto, cada experiencia que tengo con continuar / devoluciones múltiples ha estado en un enorme lío de código.
Andy
3

La idea es el regreso temprano. Temprano returnen un ciclo se vuelve tempranocontinue .

Muchas buenas respuestas a esta pregunta: ¿Debo regresar temprano de una función o usar una declaración if?

Tenga en cuenta que, si bien la pregunta se cierra en función de la opinión, más de 430 votos están a favor de la devolución anticipada, ~ 50 votos son "depende" y 8 están en contra de la devolución anticipada. Entonces, hay un consenso excepcionalmente fuerte (o eso, o soy malo contando, lo cual es plausible).

Personalmente, si el cuerpo del bucle estaría sujeto a una continuación temprana y el tiempo suficiente para que tenga importancia (3-4 líneas), tiendo a extraer el cuerpo del bucle en una función donde utilizo el retorno temprano en lugar de continuar temprano.

Peter
fuente
2

Esta técnica es útil para certificar condiciones previas cuando la mayor parte de su método ocurre cuando se cumplen esas condiciones.

Con frecuencia invertimos ifsen mi trabajo y empleamos un fantasma más. Solía ​​ser bastante frecuente que mientras revisaba un RP pudiera señalar cómo mi colega podía eliminar tres niveles de anidamiento. Ocurre con menos frecuencia ya que implementamos nuestros ifs.

Aquí hay un ejemplo de juguete de algo que se puede implementar

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Un código como este, donde hay UN caso interesante (donde el resto son simplemente retornos o pequeños bloques de enunciados) son comunes. Es trivial reescribir lo anterior como

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Aquí, nuestro código significativo, las 10 líneas no incluidas, no tiene una sangría triple en la función. Si tiene el primer estilo, está tentado a refactorizar el bloque largo en un método o tolerar la sangría. Aquí es donde entran en juego los ahorros. En un lugar, esto es un ahorro trivial, pero a través de muchos métodos en muchas clases, ahorras la necesidad de generar una función adicional para hacer que el código sea más limpio y no tienes tantos niveles horribles de varios niveles muesca .


En respuesta al ejemplo de código de OP, estoy de acuerdo en que es un colapso excesivo. Especialmente porque en 1TB, el estilo que uso, la inversión hace que el código aumente de tamaño.

Lan
fuente
0

Las sugerencias de los resharpers a menudo son útiles, pero siempre deben evaluarse críticamente. Busca algunos patrones de código predefinidos y sugiere transformaciones, pero no puede tener en cuenta todos los factores que hacen que el código sea más o menos legible para los humanos.

En igualdad de condiciones, es preferible menos anidamiento, por lo que ReSharper sugerirá una transformación que reduzca el anidamiento. Pero todas las demás cosas no son iguales: en este caso, la transformación requiere la introducción de a continue, lo que significa que el código resultante es realmente más difícil de seguir.

En algunos casos, intercambiando un nivel de anidamiento por un continue poder ser una mejora, pero esto depende de la semántica del código en cuestión, que está más allá de la comprensión de las reglas mecánicas de ReSharpers.

En su caso, la sugerencia de ReSharper empeoraría el código. Así que ignóralo. O mejor: no use la transformación sugerida, pero piense un poco en cómo se podría mejorar el código. Tenga en cuenta que puede estar asignando la misma variable varias veces, pero solo la última asignación tiene efecto, ya que la anterior simplemente se sobrescribirá. Esto hace ver como un insecto. La sugerencia de ReSharpers no soluciona el error, pero hace que sea un poco más obvio que está ocurriendo una lógica dudosa.

JacquesB
fuente
0

Creo que el punto más importante aquí es que el propósito del ciclo dicta la mejor manera de organizar el flujo dentro del ciclo. Si está filtrando algunos elementos antes de recorrer el resto, entonces continuetiene sentido salir temprano. Sin embargo, si está haciendo diferentes tipos de procesamiento dentro de un bucle, podría tener más sentido hacerlo ifrealmente obvio, como:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Tenga en cuenta que en Java Streams u otros modismos funcionales, puede separar el filtrado del bucle, utilizando:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Por cierto, esta es una de las cosas que me encantan de Perl invertido if ( unless) aplicado a declaraciones individuales, lo que permite el siguiente tipo de código, que encuentro muy fácil de leer:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}
Gaurav
fuente