¿Es una rama If que no hace nada un olor a código o una buena práctica?

Question

He respondido a los hilos aquí (o al menos commented) con respuestas que contienen un código como este, pero me pregunto si es buena o mala forma escribir una serie de ramas if con una (o más) ramas haciendo nada en ellos, generalmente para eliminar la comprobación de null en cada rama.

Un ejemplo (código C#):

if (str == null) { /* Do nothing */ } 
else if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 

en lugar de:

if (str != null && str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str != null && str.Length > 1) 
{ 
    // ... 
} 

Y, por supuesto, esto es sólo un ejemplo, ya que tienden a utilizar estos con grandes y más complejos clases Y en la mayoría de estos casos, un valor null indicaría no hacer nada.

Para mí, esto reduce la complejidad de mi código y tiene sentido cuando lo veo. Entonces, ¿esta forma es buena o mala (un código huele, incluso)?

Answer 1

de hecho, es bueno para evitar lo siguiente, porque innecesariamente vuelve a verificar una de las condiciones (el hecho de que el compilador optimizar esta distancia no viene al caso - potencialmente marcas más trabajo para las personas que intentan leer su código):

if (str != null && str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str != null && str.Length > 1) 
{ 
    // ... 
} 

Pero también es bastante extraño que hacer lo que usted sugiere, a continuación:

if (str == null) { /* Do nothing */ } 
else if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 

que digo esto es extraño porque ofusca su intención y desafía las expectativas del lector.Si busca una condición, la gente espera que haga algo si está satisfecho, pero no lo está. Esto se debe a que su intención no es realmente proceso de la condición nula, sino más bien para evitar un puntero nulo cuando se echa las dos condiciones que usted está realmente interesa. En efecto, en lugar de tener dos estados conceptuales para manejar, con una provisión de cordura (entrada no nula), en cambio dice que tiene tres estados conceptuales para manejar. El hecho de que, computacionalmente, podría decirse que hay tres de esos estados está al lado del punto, es menos claro.

El enfoque caso habitual en este tipo de situación es como Oren Una sugirió - comprobar la hipótesis nula, y después comprobar las otras condiciones dentro del bloque resultado:

if (str != null) 
{ 
if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 
} 

Esto es poco más que una cuestión de estilo de mejora de la legibilidad, a diferencia de un problema de olor a código.

EDITAR: Sin embargo, si ya está listo con la condición de no hacer nada, yo hago muy parecida a la que incluyó un comentario "no hacer nada". De lo contrario, la gente podría pensar que simplemente se olvidó de completar el código.

Answer 2

prefiero hacerlo así-

if (str != null) 
{ 
if (str == "[NULL]") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 
} 

Creo que siempre se puede "reformular" un if con un cuerpo vacío en él es la negación de un cuerpo, y que se ve mejor y tiene más sentido.

Answer 3

Su primer ejemplo es perfectamente legible para mí: no huele en absoluto.

Answer 4

En este caso particular, voy a volver pronto y que hace que el código más fácil de leer

if (string.IsNullOrEmpty(str)) { return; } 

me gusta poner una declaración explícita de retorno.

Answer 5

Todo depende del contexto. Si poner una declaración if vacía hace que el código sea más legible, entonces vaya por eso.

Answer 6

Es legible, si es bueno o malo depende de lo que está tratando de lograr - generalmente las declaraciones anónimas de tipo "va por siempre" tipo if son malas. No se olvide de los métodos de cuerda estática horneados en el marco: string.IsNullOrEmpty() y string.IsNullOrWhiteSpace().

Su línea if (str == null) { /* Do nothing */ } es inusual, pero tiene un punto positivo: está dejando que otros desarrolladores sepan de antemano que no está haciendo nada deliberadamente para ese caso, con su larga estructura if/else sus intenciones podrían no ser claras si cambiado a

if (str != null) 
{ 
    /* carry on with the rest of the tests */ 
} 

Answer 7

que normalmente poner un return o algo así en la primera if:

void Foo() 
{ 
    if (str == null) { return; } 
    if (str == "SomeSpecialValue") 
    { 
     // ... 
    } 
    else if (str.Length > 1) 
    { 
     // ... 
    } 
} 

Si no puede hacer esto, ya que la función hace algo más después de la if/else, diría es hora de refactorizar, y dividir la parte if/else en una función separada, desde la cual puede regresar temprano.

Answer 8

Sí es un olor a código.

Una indicación es que usted pensó en hacer esta pregunta.

Otra indicación es que el código parece incompleto, como si algo debería pertenecer allí. Puede ser legible seguro, pero se siente apagado.

Al leer ese código, un extraño debe detenerse por un segundo y usar la capacidad intelectual para determinar si el código es válido/completo/correcto/como previsto/adjetivo.

user359996 ha puesto el dedo en la cabeza:

que digo esto es extraño porque ofusca su intención y desafía las expectativas del lector.