¿Por qué una cláusula "si" vacía sería de mal estilo? Porque es una forma no obvia de 'salir' del bloque if. Omite los else-if subsiguientes y, lo que es más importante, omite el else final, que en un bloque de múltiples condiciones termina como la acción 'predeterminada'.

Para mí hay dos problemas principales aquí:

1. El orden de la instrucción if es importante, pero no está claro por qué se elige este orden
2. La declaración else parece tener lógica, que se omite haciendo_nada en lugar de ser un do_nothing de sastre

La primera pregunta que haría al ver un código como este es ¿por qué la opción do_nothing (! seesWater(AHEAD)) no es lo primero que verificamos? ¡Qué hace ! seesWater(LEFT) lo primero que revisamos? Espero ver un comentario sobre por qué este orden fue importante, ya que los condicionales no parecen ser obviamente exclusivos (podría ver el agua en múltiples direcciones).

La segunda pregunta que haría es en qué casos esperamos terminar en la declaración else . Por el momento turn180() es el 'predeterminado', pero no me parece un comportamiento muy predeterminado para volver por donde has venido si, por algún motivo, falla la búsqueda.

Vengo de C y vivo con MISRA, pero para mí un "si" con un punto y coma es de mal estilo, independientemente de si hay un "else". La razón es que la gente normalmente no pone un punto y coma allí a menos que sea un error tipográfico. Considera esto:

 if (! seesWater(AHEAD)); { stayDry(); }

Aquí, alguien que lea el código pensará que solo permanece seco () si no ve agua adelante; debe mirar con atención para ver que en realidad se llamará incondicionalmente. Mucho mejor, si no quieres hacer nada en este caso, son llaves y un comentario:

 if (! seesWater(AHEAD)) { /* don't need to do anything here */ }

De hecho, siempre se debe colocar los corchetes alrededor del cuerpo de "if", "else", etc. A veces encuentro código como

 if (! seesWater(AHEAD)) stayDry();

entonces alguien vendrá más tarde y agregará algo más:

 if (! seesWater(AHEAD)) stayDry(); doSomethingElse();

y, por supuesto, Something Else se hace ya sea que veas agua o no, lo cual no era la intención del segundo codificador. Puede pensar que nadie cometería un error tan obvio, pero lo hacen.

No haces nada con la segunda declaración else if, deberías reescribirla así

 else if(!seesWater(AHEAD)){return }

Esto es para que el programa tenga un operador condicional para devolver una declaración booleana, espero que esto ayude -SG

¡Puedes invertir el ! seesWater(AHEAD) ve la condición ! seesWater(AHEAD) . Luego puede mover el código en su cláusula else a su cláusula if :

 if (! seesWater(LEFT)) { turn(LEFT); } else if (seesWater(AHEAD)) { if (! seesWater(RIGHT)) { turn(RIGHT); } else { turn180(); } }

Yo diría que puede decidir no cambiar la lógica de su código en absoluto. Creo que no hay razón para evitar un bloque else if o si vacío if eso lleva a que su código sea el más legible y la lógica más comprensible. A menudo puede reescribir su código para que no incluya un bloque de código vacío y aún así hacer que sea igual de comprensible. Pero si no, digo que está bien.

Sin embargo, una cosa... no me gusta el estilo de usar un ; para denotar un bloque de código vacío. Es muy fácil pasarlo por alto y normalmente no se ve, por lo que puede confundir al lector. Yo sugeriría reemplazar el ; con un conjunto vacío de llaves. También podría poner un comentario dentro de los curlies vacíos para dejar en claro que quiere decir que ese bloque de código esté vacío, es decir, // In this case, we don't want to do anything .

¿Quién dice que es "mal estilo"?

La pregunta relevante que se debe hacer es, ¿es esto más claro que las alternativas? En tu caso concreto, diría que sí. El código expresa claramente una elección entre 4 opciones, de las cuales una es "no hacer nada".

El único cambio que haría es reemplazar ese punto y coma bastante insignificante por un par de llaves vacías, posiblemente con un comentario para dejar en claro que no es un error.

 if (! seesWater(LEFT)) { turn(LEFT); } else if (! seesWater(AHEAD)) { // nothing required } else if (! seesWater(RIGHT)) { turn(RIGHT); } else { turn180(); }

Esto no respalda las 'cláusulas vacías' como un estilo generalmente aceptable; simplemente que los casos deben argumentarse en función de sus méritos, no sobre la base de alguna regla que debe ser obedecida. Se trata de desarrollar el buen gusto, y el juicio del gusto es para humanos, no para autómatas sin mente.

Bueno, realmente no sabemos el contexto de toda la pregunta. No sabemos si este es el único código en un método. No estoy seguro de por qué prueba primero la IZQUIERDA. Habría pensado que probaría AHEAD primero, ya que creo que continuaría en la misma dirección que la predeterminada si es posible. Si sigues yendo a la IZQUIERDA estarás caminando en círculo. Entonces, dada la falta de requisitos, es difícil dar una respuesta completa.

Sin embargo, tendería a estar de acuerdo en que no me gusta la declaración if vacía. Si desea seguir caminando ADELANTE, debe indicarlo de alguna manera.

Otros comentarios:

1. Preferiría tener un nombre de método positivo como noWater(...) . Luego, la declaración if se convierte en una prueba positiva en lugar de una prueba negativa.

2. ¿Por qué tiene múltiples formas del método que invoca: girar (IZQUIERDA), girar (DERECHA) y girar 180 ()? ¿Por qué turn180() es diferente? Crearía un método que aceptaría un parámetro para cualquier dirección.

Usando las sugerencias anteriores, tendría algo como:

 if (noWater(FORWARD)) turn(0); else if (noWater(LEFT)) turn(270); else if (noWater(RIGHT)) turn(90); else turn(180);

Con una estructura como esta, no tiene una declaración if vacía y el movimiento se parametriza (y es explícito) para que pueda tener diferentes valores según sea necesario.

Si esta es la lógica que desea, no hay nada de malo en su código. En términos de estilo de código, estoy de acuerdo con los otros usuarios. Algo como esto sería más claro en mi opinión:

 if (! seesWater(LEFT)) { turn(LEFT); } else if (! seesWater(AHEAD)) { //Do nothing } else if (! seesWater(RIGHT)) { turn(RIGHT); } else { turn180(); }

Pero al tener prioridad de hacer un giro a la izquierda sobre avanzar (sin hacer nada), el movimiento puede terminar en círculos:

Si desea que el movimiento "no haga nada", pero evite moverse en aguas como esta:

Es posible que desee cambiar su lógica a algo como esto:

 if (! seesWater(AHEAD)) { //Do nothing. Keep moving } else if (! seesWater(LEFT)) { turn(LEFT); } else if (! seesWater(RIGHT)) { turn(RIGHT); } else { turn180(); }

La razón por la que un verificador de estilo informa que algo tiene un estilo deficiente es que las personas que implementaron el verificador de estilo lo consideraron un estilo deficiente.

Tendría que preguntarles por qué, y su respuesta puede o no tener mérito.

Los verificadores automáticos de cualquier cosa que requiera inteligencia humana deben tratarse con precaución. A veces producen resultados útiles y otras veces no. Es muy desafortunado si un verificador de este tipo está evaluando el trabajo de un ser humano, y extremadamente desafortunado si esa evaluación tiene consecuencias.

Para este ejemplo de código en particular, no creo que la lógica sea precisa y no fluya de manera intuitiva. Parece que quieres girar si hay agua más adelante... pero ese giro a la izquierda está allí, lo que lo hace confuso y potencialmente un error. En última instancia, veo la lógica vacía como un doble negativo, 'si no hay agua adelante, no hagas nada', y los dobles negativos también están mal vistos.

 if (seesWater(AHEAD)) { if (! seesWater(LEFT)) { turn(LEFT); } else if (! seesWater(RIGHT)) { turn(RIGHT); } else { turn180(); } }

Esto tiene más sentido ya que la lógica adicional se basa en si hay agua por delante. También lo hace más fácil si desea mover las acciones a otra función si hay agua por delante.

Encuentro la lógica negativa en las declaraciones if(!condition) difíciles de leer y alucinantes, incluso más en combinación con if-else-else-else. Preferiría una lógica positiva como seesLand() en lugar de !seesWater .

Puede modificar la función turn(), de modo que turn(AHEAD) no haga nada y turn(BACK) haga el giro de 180 en lugar de necesitar una función separada para eso.

Entonces podrías reescribir esto en.

 List<Direction> directions = new ArrayList(LEFT, FORWARD, RIGHT, BACK); for (Direction d: directions) { if(seesLand(d) { turn(d); return; }

Una ventaja es que esto se generaliza aún más. Puede crear diferentes estrategias de movimiento simplemente definiendo otra matriz en la que ordene las direcciones como desee y páselas a esta función para verificar y girar en el orden elegido.

Lo que haces con algo como esto es convertirlo en un método de habla y preferiblemente dividirlo.

Opción 1:

 private turnTowardsGround(){ if(! seesWater(AHEAD){ return; } if (! seesWater(LEFT)) { turn(LEFT); return; } if (! seesWater(RIGHT)) { turn(RIGHT); return; } turn180(); }

Opcion 2:

 private determineMoveDirection(){ if(! seesWater(AHEAD){ return AHEAD; } if (! seesWater(LEFT)) { return LEFT } if (! seesWater(RIGHT)) { return RIGHT } return BACK; }

y luego use la opción 2 como:

 Direction directionToMove = determineMoveDirection(); turn(directionToMove);

¿Por qué? Porque los nombres de los métodos aclaran la intención del código. Y las salidas de retorno temprano permiten una lectura rápida donde no necesitamos leer todo el código if-else para asegurarnos de que no suceda nada más una vez que se aplica un caso. Además, en el caso de la opción 2, tiene una clara separación de preocupaciones (descubrir a dónde moverse frente a girar), lo cual es bueno para la modularidad y, en este caso, también permite un solo lugar para registrar la decisión final a dónde irá. girar, por ejemplo.

Además, preferiblemente, las comprobaciones no se negarían, pero habría un método "seesLand" o "seesAccessibleFieldType". Es psicológicamente más fácil/más rápido analizar declaraciones no negadas y deja más claro lo que realmente está buscando (es decir, una ficha de tierra, una ficha de hielo, ...).