El desarrollador insiste si las declaraciones no deberían tener condiciones negadas, y siempre deberían tener un bloque else

Tengo un conocido, un desarrollador más experimentado que yo. Estábamos hablando de prácticas de programación y me sorprendió su enfoque sobre las declaraciones 'si'. Insiste en algunas prácticas con respecto a las declaraciones if que me parecen bastante extrañas.

En primer lugar , una declaración if debe ir seguida de una declaración else, ya sea que haya algo que poner en ella o no. Lo que lleva a que el código se vea así:

if(condition) 
{
    doStuff();
    return whatever;
}
else
{
}

En segundo lugar , es mejor probar los valores verdaderos en lugar de los falsos. Eso significa que es mejor probar una variable 'doorClosed' en lugar de una variable '! DoorOpened'

Su argumento es que aclara lo que está haciendo el código.

Lo que me confunde bastante, ya que una combinación de esas dos reglas puede llevarlo a escribir este tipo de código si quiere hacer algo cuando no se cumple la condición.

if(condition)
{
}
else
{
    doStuff();
    return whatever;
}

Mi opinión acerca de esto es que de hecho es muy feo y / o que la mejora de la calidad, si la hay, es insignificante. Pero como junior, soy propenso a dudar de mi instinto.

Entonces mis preguntas son: ¿Es una práctica buena / mala / "no importa"? ¿Es una práctica común?

elseBloque explícito

La primera regla solo contamina el código y lo hace ni más legible ni menos propenso a errores. El objetivo de su colega, supongo, es ser explícito, mostrando que el desarrollador era plenamente consciente de que la condición puede evaluar false. Si bien es bueno ser explícito, dicha explicidad no debería tener un costo de tres líneas adicionales de código .

Ni siquiera menciono el hecho de que una ifdeclaración no es necesariamente seguida por una elseo nada: también podría ser seguida por una o más elif.

La presencia de la returndeclaración empeora las cosas. Incluso si realmente tuviera código para ejecutar dentro del elsebloque, sería más legible hacerlo así:

if (something)
{
    doStuff();
    return whatever;
}

doOtherThings();
return somethingElse;

Esto hace que el código tome dos líneas menos y desinfecta el elsebloque. Las cláusulas de guardia tienen que ver con eso.

Sin embargo, tenga en cuenta que la técnica de su colega podría resolver parcialmente un patrón muy desagradable de bloques condicionales apilados sin espacios:

if (something)
{
}
if (other)
{
}
else
{
}

En el código anterior, la falta de un salto de línea sano después del primer ifbloque hace que sea muy fácil malinterpretar el código. Sin embargo, aunque la regla de su colega dificultaría la lectura incorrecta del código, una solución más sencilla sería simplemente agregar una nueva línea.

Prueba para true, no parafalse

La segunda regla puede tener algún sentido, pero no en su forma actual.

No es falso que la prueba para una puerta cerrada sea más intuitiva que la prueba para una puerta no abierta . Las negaciones, y especialmente las negaciones anidadas, generalmente son difíciles de entender:

if (!this.IsMaster || (!this.Ready && !this.CanWrite))

Para resolver eso, en lugar de agregar bloques vacíos, cree propiedades adicionales, cuando sea relevante, o variables locales .

La condición anterior podría hacerse legible con bastante facilidad:

if (this.IsSlave || (this.Synchronizing && this.ReadOnly))

Con respecto a la primera regla, este es un ejemplo de escritura inútil. No solo toma más tiempo escribir, también causará una gran confusión a cualquiera que lea el código. Si el código no es necesario, no lo escriba. Esto incluso se extendería a no tener un poblado elseen su caso ya que el código regresa del ifbloque:

if(condition) 
{
    doStuff();
    return whatever;
}

doSomethingElse(); // no else needed
return somethingelse;

Con respecto al segundo punto, es bueno evitar nombres booleanos que contengan un negativo:

bool doorNotOpen = false; // a double negative is hard to reason
bool doorClosed = false; // this is much clearer

Sin embargo, extender esto para no volver a probar un negativo, como señala, conduce a una escritura más inútil. Lo siguiente es mucho más claro que tener un ifbloque vacío :

if(!condition)
{
    doStuff();
    return whatever;
}

1. Un argumento a favor de las elsedeclaraciones vacías .

A menudo uso (y defiendo) algo similar a esa primera construcción, un vacío más. Les indica a los lectores del código (herramientas de análisis tanto humanas como automatizadas) que el programador ha pensado un poco en la situación. Las elsedeclaraciones faltantes que deberían haber estado presentes han matado a personas, chocado vehículos y costado millones de dólares. MISRA-C, por ejemplo, exige al menos un comentario que diga que el final que falta es intencional en una if (condition_1) {do_this;} else if (condition_2) {do_that;} ... else if (condition_n) {do_something_else;}secuencia. Otros estándares de alta confiabilidad van aún más allá: con algunas excepciones, se prohíben las declaraciones que faltan.

Una excepción es un comentario simple, algo similar a /* Else not required */. Esto señala la misma intención que las tres líneas vacías más. Otra excepción donde ese espacio vacío no es necesario es donde es obvio para los lectores del código y para las herramientas de análisis automatizadas que ese espacio vacío es superfluo. Por ejemplo, de if (condition) { do_stuff; return; }manera similar, no se necesita un espacio vacío en el caso de throw somethingo goto some_label¹ en lugar de return.

2. Un argumento para preferir if (condition)sobre if (!condition).

Este es un elemento de factores humanos. La lógica booleana compleja hace tropezar a muchas personas. Incluso un programador experimentado tendrá que pensar if (!(complex || (boolean && condition))) do_that; else do_this;. Como mínimo, reescribe esto como if (complex || (boolean && condition)) do_this; else do_that;.

3. Esto no significa que uno deba preferir thendeclaraciones vacías .

La segunda sección dice "preferir" en lugar de "debes". Es una guía más que una regla. La razón para que esa directriz prefiera ifcondiciones positivas es que el código debe ser claro y obvio. Una cláusula vacía (por ejemplo, if (condition) ; else do_something;) viola esto. Es una programación ofuscada, que hace que incluso los programadores más experimentados realicen una copia de seguridad y vuelvan a leer la ifcondición en su forma negada. Por lo tanto, escríbalo en la forma negada en primer lugar y omita la declaración else (o tenga un else vacío o comente a tal efecto si se le ordena hacerlo).

¹ Escribí que luego cláusulas que terminan con return, throwo gotono requieren un vacío más. Es obvio que la cláusula else no es necesaria. ¿Pero que pasa goto? Por otro lado, las reglas de programación críticas para la seguridad a veces no permiten el retorno temprano, y casi siempre no permiten lanzar excepciones. Sin embargo, permiten gotoen forma restringida (por ejemplo, goto cleanup1;). Este uso restringido de gotoes la práctica preferida en algunos lugares. El kernel de Linux, por ejemplo, está repleto de tales gotodeclaraciones.

Utilizo una rama vacía (y a veces una rama vacía) en casos muy raros: cuando es obvio que tanto la parte si como la otra deben manejarse de alguna manera, pero por alguna razón no trivial, el caso puede ser manejado por haciendo nada. Y por lo tanto, cualquiera que lea el código con la acción else necesaria sospechará inmediatamente que falta algo y perderá su tiempo.

if (condition) {
    // No action needed because ...
} else {
    do_else_action()
}

if (condition) {
    do_if_action()
} else {
    // No action needed because ...
}

Pero no:

if (condition) {
    do_if_action()
} else {
    // I was told that an if always should have an else ...
}

En igualdad de condiciones, prefiera la brevedad.

Lo que no escribes, nadie tiene que leer y entender.

Si bien ser explícito puede ser útil, ese es el caso si resulta obvio, sin verbosidad indebida, que lo que escribiste es realmente lo que querías escribir.

Por lo tanto, evite las ramas vacías, no solo son inútiles sino también poco comunes y, por lo tanto, generan confusión.

Además, evite escribir una rama más si sale directamente de la rama si.

Una aplicación útil de lo explícito sería poner un comentario cada vez que caiga en casos de cambio // FALLTHRU, y usar un comentario o un bloque vacío donde necesite una declaración vacía for(a;b;c) /**/;.

No existe una regla estricta sobre condiciones positivas o negativas para una declaración IF, que yo sepa. Personalmente, prefiero codificar un caso positivo en lugar de uno negativo, cuando corresponda. Sin embargo, ciertamente no haré esto si me lleva a hacer un bloque IF vacío, seguido de un ELSE lleno de lógica. Si surgiera tal situación, tomaría como 3 segundos refactorizarla nuevamente para probar un caso positivo de todos modos.

Sin embargo, lo que realmente no me gusta de sus ejemplos es el espacio vertical completamente innecesario que ocupa el ELSE en blanco. Simplemente no hay razón alguna para hacer esto. No agrega nada a la lógica, no ayuda a documentar lo que está haciendo el código y no aumenta la legibilidad en absoluto. De hecho, yo diría que el espacio vertical agregado posiblemente podría disminuir la legibilidad.

elseBloque explícito

No estoy de acuerdo con esto como una declaración general que cubre todas las ifdeclaraciones, pero a veces elsees bueno agregar un bloque por hábito.

Una ifdeclaración, en mi opinión, en realidad cubre dos funciones distintas.

Si se supone que debemos hacer algo, hazlo aquí.

Cosas como esta obviamente no necesitan una elseparte.

    if (customer.hasCataracts()) {
        appointmentSuggestions.add(new CataractAppointment(customer));
    }
    if (customer.isDiabetic()) {
        customer.assignNurse(DiabeticNurses.pickBestFor(customer));
    }

y en algunos casos insistir en agregar un elseerror podría inducir a error

    if (k > n) {
        return BigInteger.ZERO;
    }
    if (k <= 0 || k == n) {
        return BigInteger.ONE;
    }

no es lo mismo que

    if (k > n) {
        return BigInteger.ZERO;
    } else {
        if (k <= 0 || k == n) {
            return BigInteger.ONE;
        }
    }

a pesar de que es funcionalmente igual. Escribir el primero ifcon un vacío elsepuede llevarlo al segundo resultado que es innecesariamente feo.

Si buscamos un estado específico, a menudo es una buena idea agregar un espacio vacío elsesolo para recordarle que cubra esa eventualidad

        // Count wins/losses.
        if (doors[firstChoice] == Prize.Car) {
            // We would have won without switching!
            winWhenNotSwitched += 1;
        } else {
            // We win if we switched to the car!
            if (doors[secondChoice] == Prize.Car) {
                // We picked right!
                winWhenSwitched += 1;
            } else {
                // Bad choice.
                lost += 1;
            }
        }

Recuerde que estas reglas solo se aplican cuando escribe un código nuevo . En mi humilde opinión las elsecláusulas vacías deben eliminarse antes de registrarse.

Prueba para true, no parafalse

Nuevamente, este es un buen consejo a nivel general, pero en muchos casos esto hace que el código sea innecesariamente complejo y menos legible.

Aunque el código como

    if(!customer.canBuyAlcohol()) {
        // ...
    }

es discordante para el lector, pero lo hace

    if(customer.canBuyAlcohol()) {
        // Do nothing.
    } else {
        // ...
    }

es al menos tan malo, si no peor.

Codifiqué en BCPL hace muchos años y en ese idioma hay una IFcláusula y una UNLESScláusula para que pueda codificar de manera mucho más legible como:

    unless(customer.canBuyAlcohol()) {
        // ...
    }

que es significativamente mejor, pero aún no es perfecto.

Mi proceso personal

En general, cuando escribo un código nuevo , a menudo agrego un elsebloque vacío a una ifdeclaración solo para recordarme que aún no he cubierto esa eventualidad. Esto me ayuda a evitar la DFStrampa y asegura que cuando reviso el código, noto que hay más por hacer. Sin embargo, generalmente agrego un TODOcomentario para realizar un seguimiento.

  if (returnVal == JFileChooser.APPROVE_OPTION) {
    handleFileChosen();
  } else {
    // TODO: Handle case where they pressed Cancel.
  }

Encuentro que generalmente uso elseraramente en mi código, ya que a menudo puede indicar un olor a código.

Para el primer punto, he usado un lenguaje que obligó a las declaraciones IF a usarse de esta manera (en Opal, el lenguaje detrás de un raspador de pantalla de mainframe para poner una interfaz gráfica de usuario en los sistemas de mainframe), y con solo una línea para el IF y el otro. ¡No fue una experiencia agradable!

Esperaría que cualquier compilador optimice tales cláusulas ELSE adicionales. Pero para el código en vivo no está agregando nada (en desarrollo puede ser un marcador útil para más código).

Una vez que uso algo como estas cláusulas adicionales es cuando uso el procesamiento de tipo CASE / WHEN. Siempre agrego una cláusula predeterminada, incluso si está vacía. Este es un hábito a largo plazo de los lenguajes que generará un error si no se usa dicha cláusula, y obliga a pensar si las cosas realmente deberían pasar por alto.

Hace mucho tiempo, la práctica de mainframe (por ejemplo, PL / 1 y COBOL) se aceptaba que las comprobaciones negativas eran menos eficientes. Esto podría explicar el segundo punto, aunque en estos días hay ahorros de eficiencia masivamente más importantes que se ignoran como micro optimizaciones.

La lógica negativa tiende a ser menos legible, aunque no tanto en una declaración IF tan simple.

Yo diría que la mayoría de las respuestas dicen que los elsebloques vacíos son prácticamente siempre un desperdicio dañino de tinta electrónica. No agregue estos a menos que tenga una muy buena razón para hacerlo, en cuyo caso el bloque vacío no debe estar vacío en absoluto, debe contener un comentario que explique por qué está allí.

Sin embargo, el problema de evitar los negativos merece más atención: por lo general, cada vez que necesita usar un valor booleano, necesita algunos bloques de código para operar cuando está configurado, y algunos otros bloques para operar cuando no está configurado. Como tal, si aplica una regla de no negativos, impone if() {} else {...}declaraciones que tienen (¡con un ifbloque vacío !) O crea un segundo booleano para cada booleano que contiene su valor negado. Ambas opciones son malas, ya que confunden a sus lectores.

Una política útil es esta: nunca use una forma negada dentro del nombre de un booleano, y exprese la negación como una sola !. Una declaración como if(!doorLocked)es perfectamente clara, una declaración como if(!doorUnlocked)nudos cerebros. El último tipo de expresión es lo que debe evitar a toda costa, no la presencia de una sola !en una if()condición.

Yo diría que definitivamente es una mala práctica. Agregar declaraciones else agregará un montón de líneas sin sentido a su código que no hacen nada y lo hacen aún más difícil de leer.

Hay otro patrón que aún no se ha mencionado sobre el manejo del segundo caso que tiene un bloque if vacío. Una forma de lidiar con esto sería regresar en el bloque if. Me gusta esto:

void foo()
{
    if (condition) return;

    doStuff();
    ...
}

Este es un patrón común para verificar condiciones de error. El caso afirmativo todavía se usa, y su interior ya no está vacío, lo que deja a los futuros programadores preguntándose si un no operativo fue intencional o no. También puede mejorar la legibilidad ya que es posible que deba realizar una nueva función (dividiendo funciones grandes en funciones individuales más pequeñas).

Hay un punto al considerar el argumento "siempre tiene una cláusula else" que no he visto en ninguna otra respuesta: puede tener sentido en un estilo de programación funcional. Algo así como.

En un estilo de programación funcional, manejas expresiones en lugar de declaraciones. Por lo tanto, cada bloque de código tiene un valor de retorno, incluida una if-then-elseexpresión. Sin embargo, eso impediría un elsebloque vacío . Déjame darte un ejemplo:

var even = if (n % 2 == 0) {
  return "even";
} else {
  return "odd";
}

Ahora, en lenguajes con sintaxis inspirada en estilo C o estilo C (como Java, C # y JavaScript, solo por nombrar algunos), esto parece extraño. Sin embargo, parece mucho más familiar cuando se escribe como tal:

var even = (n % 2 == 0) ? "even" : "odd";

Dejar la elserama vacía aquí provocaría que un valor sea indefinido; en la mayoría de los casos, no es lo que queremos que sea un caso válido al programar la funcionalidad. Lo mismo con dejarlo fuera por completo. Sin embargo, cuando está programando iterativamente, establezco muy pocas razones para tener siempre un elsebloque.

... una declaración if debe ir seguida de una declaración else, ya sea que haya algo que poner en ella o no.

No estoy de acuerdo, if (a) { } else { b(); }debería reescribirse como if (!a) { b(); }y if (a) { b(); } else { }debería reescribirse como if (a) { b(); }.

Sin embargo, vale la pena señalar que rara vez tengo una rama vacía. Esto se debe a que normalmente me registro que fui a la rama vacía. De esta manera puedo desarrollar mensajes puramente fuera de registro. Raramente uso un depurador. En entornos de producción no se obtiene un depurador; es bueno poder solucionar problemas de producción con las mismas herramientas que usas para desarrollar.

En segundo lugar, es mejor probar los valores verdaderos en lugar de los falsos. Eso significa que es mejor probar una variable 'doorClosed' en lugar de una variable '! DoorOpened'

Tengo sentimientos encontrados sobre esto. Una desventaja de tener doorClosedy doorOpenedes que potencialmente duplica la cantidad de palabras / términos que debe tener en cuenta. Otra desventaja es con el tiempo el significado doorClosedy doorOpenedpuede cambiar (otros desarrolladores vienen después de usted) y puede terminar con dos propiedades que ya no son negaciones precisas entre sí. En lugar de evitar las negaciones, valoro adaptar el lenguaje del código (nombres de clase, nombres de variables, etc.) al vocabulario de los usuarios comerciales y a los requisitos que se me dan. No quisiera inventar un término completamente nuevo para evitar!si ese término solo tiene un significado para un desarrollador que los usuarios comerciales no entenderán. Quiero que los desarrolladores hablen el mismo idioma que los usuarios y las personas que escriben los requisitos. Ahora, si los nuevos términos simplifican el modelo, entonces eso es importante, pero eso debe manejarse antes de que se finalicen los requisitos, no después. El desarrollo debe manejar este problema antes de que comiencen a codificar.

Soy propenso a dudar de mi instinto.

Es bueno seguir preguntándote.

¿Es una práctica buena / mala / "no importa"?

Hasta cierto punto, mucho de esto no importa. Revise su código y adáptese a su equipo. Eso suele ser lo correcto. Si todos en su equipo quieren codificar de cierta manera, probablemente sea mejor hacerlo de esa manera (cambiar a una persona, usted mismo, requiere menos puntos de historia que cambiar a un grupo de personas).

¿Es una práctica común?

No recuerdo haber visto nunca una rama vacía. Sin embargo, veo personas agregando propiedades para evitar negaciones.

Solo voy a abordar la segunda parte de la pregunta y esto proviene de mi punto de vista de trabajar en sistemas industriales y dispositivos de manipulación en el mundo real.

Sin embargo, esto es de alguna manera un comentario extendido en lugar de una respuesta.

Cuando se dice

En segundo lugar, es mejor probar los valores verdaderos en lugar de los falsos. Eso significa que es mejor probar una variable 'doorClosed' en lugar de una variable '! DoorOpened'

Su argumento es que aclara lo que está haciendo el código.

Desde mi punto de vista, aquí se asume que el estado de la puerta es un estado binario cuando en realidad es al menos un estado ternario:

1. La puerta está abierta.
2. La puerta no está completamente abierta ni completamente cerrada.
3. La puerta está cerrada.

Por lo tanto, dependiendo de dónde se encuentre un solo sensor digital binario, solo puede suponer uno de los dos conceptos:

1. Si el sensor está en el lado abierto de la puerta: la puerta está abierta o no abierta
2. Si el sensor está en el lado cerrado de la puerta: la puerta está cerrada o no cerrada.

Y no puede intercambiar estos conceptos mediante el uso de una negación binaria. De este modo doorClosed, y !doorOpenedno es probable sinónimos, y cualquier intento de fingir que son sinónimos es pensamiento erróneo que asume un mayor conocimiento del estado del sistema que en realidad existe.

Al volver a su pregunta, apoyaría la verbalización que coincida con el origen de la información que representa la variable. Por lo tanto, si la información se deriva del lado cerrado de la puerta, entonces continúe con doorClosedetc. Esto puede implicar el uso de cualquiera doorClosedo !doorClosedsegún sea necesario en varias declaraciones según sea necesario, lo que puede generar confusión con el uso de la negación. Pero lo que no hace es propagar implícitamente supuestos sobre el estado del sistema.

Con fines de discusión para el trabajo que hago, la cantidad de información que tengo disponible sobre el estado del sistema depende de la funcionalidad requerida por el sistema en general.

A veces solo necesito saber si la puerta está cerrada o no. En cuyo caso, un solo sensor binario será suficiente. Pero en otras ocasiones necesito saber que la puerta está abierta, cerrada o en transición. En tales casos, habría dos sensores binarios (en cada extremo del movimiento de la puerta, con comprobación de errores para que la puerta se abra y se cierre simultáneamente) o habrá un sensor analógico que mida qué tan "abierta" estaba la puerta.

Un ejemplo de lo primero sería la puerta de un horno microondas. Habilita el funcionamiento del horno en función de que la puerta esté cerrada o no cerrada. No te importa lo abierta que sea la puerta.

Un ejemplo del segundo sería un actuador accionado por un motor simple. Inhibe conducir el motor hacia adelante cuando el actuador está completamente apagado. E inhibe la conducción del motor en reversa cuando el actuador está completamente adentro.

Básicamente, el número y tipo de sensores se reduce a ejecutar un análisis de costos de los sensores contra un análisis de requisitos de lo que se necesita para lograr la funcionalidad requerida.

Las reglas de estilo concretas siempre son malas. Las pautas son buenas, pero al final a menudo hay casos en los que puede aclarar las cosas haciendo algo que no sigue las pautas.

Dicho esto, hay mucho odio por el espacio vacío "desperdicia líneas", "mecanografía extra", que son simplemente malas. Podría argumentar para mover los corchetes a la misma línea si realmente necesita el espacio vertical, pero si eso es un problema, no debería poner su {en una línea separada de todos modos.

Como se mencionó en otra parte, tener el bloque else es muy útil para mostrar que explícitamente no quieres que pase nada en el otro caso. Después de hacer una gran cantidad de programación funcional (donde más se requiere), he aprendido que siempre debes considerar lo contrario al escribir el if, aunque como @OldCurmudgeon menciona que realmente hay dos casos de uso diferentes. Uno debería tener otro, uno no debería. Lamentablemente, no es algo que siempre se puede ver de un vistazo, y mucho menos con un linter, por lo tanto, el dogmático 'siempre pone un bloque else'.

En cuanto al 'no negativo', nuevamente, las reglas absolutas son malas. Tener un if vacío puede ser extraño, especialmente si es del tipo de si eso no necesita otra cosa, ¡así que escribe todo eso en lugar de un! o un == falso es malo. Dicho esto, hay muchos casos en que lo negativo tiene sentido. Un ejemplo común sería almacenar en caché un valor:

static var cached = null

func getCached() {
    if !cached {
        cached = (some calculation, etc)
    }

    return cached
}

Si la lógica real (mental / inglés) implica un negativo, también lo debería hacer la declaración if.