¿Cómo evitar la cadena if / else if al clasificar un título en 8 direcciones?

111

Tengo el siguiente código:

if (this->_car.getAbsoluteAngle() <= 30 || this->_car.getAbsoluteAngle() >= 330)
  this->_car.edir = Car::EDirection::RIGHT;
else if (this->_car.getAbsoluteAngle() > 30 && this->_car.getAbsoluteAngle() <= 60)
  this->_car.edir = Car::EDirection::UP_RIGHT;
else if (this->_car.getAbsoluteAngle() > 60 && this->_car.getAbsoluteAngle() <= 120)
  this->_car.edir = Car::EDirection::UP;
else if (this->_car.getAbsoluteAngle() > 120 && this->_car.getAbsoluteAngle() <= 150)
  this->_car.edir = Car::EDirection::UP_LEFT;
else if (this->_car.getAbsoluteAngle() > 150 && this->_car.getAbsoluteAngle() <= 210)
  this->_car.edir = Car::EDirection::LEFT;
else if (this->_car.getAbsoluteAngle() > 210 && this->_car.getAbsoluteAngle() <= 240)
  this->_car.edir = Car::EDirection::DOWN_LEFT;
else if (this->_car.getAbsoluteAngle() > 240 && this->_car.getAbsoluteAngle() <= 300)
  this->_car.edir = Car::EDirection::DOWN;
else if (this->_car.getAbsoluteAngle() > 300 && this->_car.getAbsoluteAngle() <= 330)
  this->_car.edir = Car::EDirection::DOWN_RIGHT;

Quiero evitar la ifcadena s; es realmente feo. ¿Existe otra forma, posiblemente más limpia, de escribir esto?

c++ if-statement Oraekia
fuente
77
@Oraekia Se vería mucho menos feo, menos para escribir y mejor para leer si se selecciona this->_car.getAbsoluteAngle()una vez antes de toda la cascada.
πάντα ῥεῖ
26
Toda esa desreferenciación explícita de this( this->) no es necesaria y realmente no hace nada bueno para la legibilidad ..
Jesper Juhl
2
@Neil Par como clave, enumeración como valor, lambda de búsqueda personalizada.
πάντα ῥεῖ
56
El código sería mucho menos feo sin todas esas >pruebas; no son necesarios, ya que cada uno de ellos ya ha sido probado (en la dirección opuesta) en la ifdeclaración anterior .
Pete Becker
10
@PeteBecker Esa es una de mis cosas que me molestan sobre un código como este. Demasiados programadores no entienden else if.
Barmar

Respuestas:

176 
#include <iostream>

enum Direction { UP, UP_RIGHT, RIGHT, DOWN_RIGHT, DOWN, DOWN_LEFT, LEFT, UP_LEFT };

Direction GetDirectionForAngle(int angle)
{
    const Direction slices[] = { RIGHT, UP_RIGHT, UP, UP, UP_LEFT, LEFT, LEFT, DOWN_LEFT, DOWN, DOWN, DOWN_RIGHT, RIGHT };
    return slices[(((angle % 360) + 360) % 360) / 30];
}

int main()
{
    // This is just a test case that covers all the possible directions
    for (int i = 15; i < 360; i += 30)
        std::cout << GetDirectionForAngle(i) << ' ';

    return 0;
}

Así es como lo haría yo. (Según mi comentario anterior).

Borgleader
fuente
92
Literalmente pensé que vi el Código Konami en lugar de la enumeración allí por un segundo.
Zano
21
@CodesInChaos: C99 y C ++ tienen el mismo requisito que C #: que si q = a/by r = a%bluego q * b + rdeben ser iguales a. Por lo tanto, es legal en C99 que un resto sea negativo. BorgLeader, puede solucionar el problema con (((angle % 360) + 360) % 360) / 30.
Eric Lippert
7
@ericlippert, usted y su conocimiento de matemáticas computacionales continúan impresionando.
gregsdennis
33
Esto es muy inteligente, pero es completamente ilegible y no es más probable que se pueda mantener, por lo que no estoy de acuerdo en que sea una buena solución a la "fealdad" percibida del original. Hay un elemento de gusto personal, aquí, supongo, pero encuentro que las versiones de ramificación limpiadas por x4u y motoDrizzt son muy preferibles.
IMSoP
4
@cyanbeam el bucle for en main es solo una "demostración", GetDirectionForAnglees lo que propongo como reemplazo de la cascada if / else, ambos son O (1) ...
Borgleader
71

Puede usar map::lower_boundy almacenar el límite superior de cada ángulo en un mapa.

Ejemplo de trabajo a continuación:

#include <cassert>
#include <map>

enum Direction
{
    RIGHT,
    UP_RIGHT,
    UP,
    UP_LEFT,
    LEFT,
    DOWN_LEFT,
    DOWN,
    DOWN_RIGHT
};

using AngleDirMap = std::map<int, Direction>;

AngleDirMap map = {
    { 30, RIGHT },
    { 60, UP_RIGHT },
    { 120, UP },
    { 150, UP_LEFT },
    { 210, LEFT },
    { 240, DOWN_LEFT },
    { 300, DOWN },
    { 330, DOWN_RIGHT },
    { 360, RIGHT }
};

Direction direction(int angle)
{
    assert(angle >= 0 && angle <= 360);

    auto it = map.lower_bound(angle);
    return it->second;
}

int main()
{
    Direction d;

    d = direction(45);
    assert(d == UP_RIGHT);

    d = direction(30);
    assert(d == RIGHT);

    d = direction(360);
    assert(d == RIGHT);

    return 0;
}
Steve Lorimer
fuente
No se requiere división. ¡Bueno!
O. Jones
17
@ O.Jones: La división por una constante de tiempo de compilación es bastante barata, solo una multiplicación y algunos cambios. Yo iría con una de las table[angle%360/30]respuestas porque es barata y no tiene ramas. Mucho más barato que un bucle de búsqueda de árbol, si se compila en un conjunto similar al de la fuente. ( std::unordered_mapgeneralmente es una tabla hash, pero std::mapgeneralmente es un árbol binario rojo-negro. La respuesta aceptada se usa efectivamente angle%360 / 30como una función hash perfecta para ángulos (después de replicar un par de entradas, y la respuesta de Bijay incluso lo evita con un desplazamiento)).
Peter Cordes
2
Podrías usarlo lower_bounden una matriz ordenada. Eso sería mucho más eficiente que un map.
Wilx
La búsqueda de mapas de @PeterCordes es fácil de escribir y de mantener. Si los rangos cambian, la actualización del código hash puede introducir errores y si los rangos se vuelven no uniformes, simplemente puede desmoronarse. A menos que ese código sea crítico para el rendimiento, no me molestaría.
OhJeez
@OhJeez: Ya no son uniformes, lo que se maneja teniendo el mismo valor en varios depósitos. Simplemente use un divisor más pequeño para obtener más cubos, a menos que eso signifique usar un divisor muy pequeño y tener demasiados cubos. Además, si el rendimiento no importa, entonces una cadena if / else tampoco es mala, si se simplifica factorizando this->_car.getAbsoluteAngle()con un tmp var y eliminando la comparación redundante de cada una de las if()cláusulas del OP (verificando algo que ya hubiera coincidido el anterior if ()). O use la sugerencia de matriz ordenada de @ wilx.
Peter Cordes
58

Cree una matriz, cada elemento del cual está asociado con un bloque de 30 grados:

Car::EDirection dirlist[] = { 
    Car::EDirection::RIGHT, 
    Car::EDirection::UP_RIGHT, 
    Car::EDirection::UP, 
    Car::EDirection::UP, 
    Car::EDirection::UP_LEFT, 
    Car::EDirection::LEFT, 
    Car::EDirection::LEFT, 
    Car::EDirection::DOWN_LEFT,
    Car::EDirection::DOWN, 
    Car::EDirection::DOWN, 
    Car::EDirection::DOWN_RIGHT, 
    Car::EDirection::RIGHT
};

Luego puede indexar la matriz con el ángulo / 30:

this->_car.edir = dirlist[(this->_car.getAbsoluteAngle() % 360) / 30];

No se requieren comparaciones ni ramificaciones.

Sin embargo, el resultado es ligeramente diferente al original. Los valores en los bordes, es decir, 30, 60, 120, etc. se colocan en la siguiente categoría. Por ejemplo, en el código original los valores válidos para UP_RIGHTson 31 a 60. El código anterior asigna 30 a 59 a UP_RIGHT.

Podemos evitar esto restando 1 del ángulo:

this->_car.edir = dirlist[((this->_car.getAbsoluteAngle() - 1) % 360) / 30];

Esto ahora nos da RIGHT30, UP_RIGHT60, etc.

En el caso de 0, la expresión se convierte en (-1 % 360) / 30. Esto es válido porque -1 % 360 == -1y -1 / 30 == 0, por lo que todavía obtenemos un índice de 0.

La sección 5.6 del estándar C ++ confirma este comportamiento:

4 El /operador binario produce el cociente y el %operador binario produce el resto de la división de la primera expresión por la segunda. Si el segundo operando de /o %es cero, el comportamiento no está definido. Para operandos integrales, el /operador produce el cociente algebraico con cualquier parte fraccionaria descartada. si el cociente a/bes representable en el tipo de resultado, (a/b)*b + a%bes igual a a.

EDITAR:

Se plantearon muchas preguntas con respecto a la legibilidad y mantenibilidad de una construcción como esta. La respuesta dada por motoDrizzt es un buen ejemplo de cómo simplificar la construcción original que es más fácil de mantener y no es tan "fea".

Ampliando su respuesta, aquí hay otro ejemplo que utiliza el operador ternario. Dado que cada caso en la publicación original se asigna a la misma variable, el uso de este operador puede ayudar a aumentar aún más la legibilidad.

int angle = ((this->_car.getAbsoluteAngle() % 360) + 360) % 360;

this->_car.edir = (angle <= 30)  ?  Car::EDirection::RIGHT :
                  (angle <= 60)  ?  Car::EDirection::UP_RIGHT :
                  (angle <= 120) ?  Car::EDirection::UP :
                  (angle <= 150) ?  Car::EDirection::UP_LEFT :
                  (angle <= 210) ?  Car::EDirection::LEFT : 
                  (angle <= 240) ?  Car::EDirection::DOWN_LEFT :
                  (angle <= 300) ?  Car::EDirection::DOWN:  
                  (angle <= 330) ?  Car::EDirection::DOWN_RIGHT :
                                    Car::EDirection::RIGHT;
dbush
fuente
49

Ese código no es feo, es simple, práctico, legible y fácil de entender. Se aislará con su propio método, por lo que nadie tendrá que lidiar con él en la vida cotidiana. Y en caso de que alguien tenga que verificarlo, tal vez porque está depurando su aplicación por un problema en otro lugar, es tan fácil que le tomará dos segundos entender el código y lo que hace.

Si estuviera haciendo tal depuración, estaría feliz de no tener que pasar cinco minutos tratando de entender qué hace su función. En este sentido, todas las demás funciones fallan por completo, ya que cambian una rutina simple, olvidada y libre de errores, en un lío complicado que las personas al depurar se verán obligadas a analizar y probar en profundidad. Como gerente de proyecto, me molestaría mucho que un desarrollador tomara una tarea simple y, en lugar de implementarla de una manera simple e inofensiva, pierda el tiempo para implementarla de una manera demasiado complicada. Solo piense en todo el tiempo que perdió pensando en ello, luego viniendo a SO preguntando, y todo por el simple hecho de empeorar el mantenimiento y la legibilidad de la cosa.

Dicho esto, hay un error común en su código que lo hace bastante menos legible, y un par de mejoras que puede hacer con bastante facilidad:

int angle = this->_car.getAbsoluteAngle();

if (angle <= 30 || angle >= 330)
  return Car::EDirection::RIGHT;
else if (angle <= 60)
  return Car::EDirection::UP_RIGHT;
else if (angle <= 120)
  return Car::EDirection::UP;
else if (angle <= 150)
  return Car::EDirection::UP_LEFT;
else if (angle <= 210)
  return Car::EDirection::LEFT;
else if (angle <= 240)
  return Car::EDirection::DOWN_LEFT;
else if (angle <= 300)
  return Car::EDirection::DOWN;
else if (angle <= 330)
  return Car::EDirection::DOWN_RIGHT;

Pon esto en un método, asigna el valor devuelto al objeto, contrae el método y olvídalo por el resto de la eternidad.

PD: hay otro error por encima del umbral de 330, pero no sé cómo quiere tratarlo, así que no lo solucioné en absoluto.

Actualización posterior

Según el comentario, incluso puede deshacerse del else si es que lo hace:

int angle = this->_car.getAbsoluteAngle();

if (angle <= 30 || angle >= 330)
  return Car::EDirection::RIGHT;

if (angle <= 60)
  return Car::EDirection::UP_RIGHT;

if (angle <= 120)
  return Car::EDirection::UP;

if (angle <= 150)
  return Car::EDirection::UP_LEFT;

if (angle <= 210)
  return Car::EDirection::LEFT;

if (angle <= 240)
  return Car::EDirection::DOWN_LEFT;

if (angle <= 300)
  return Car::EDirection::DOWN;

if (angle <= 330)
  return Car::EDirection::DOWN_RIGHT;

No lo hice porque siento que en cierto punto se convierte en una cuestión de preferencias propias, y el alcance de mi respuesta fue (y es) para dar una perspectiva diferente a su preocupación por la "fealdad del código". De todos modos, como dije, alguien lo señaló en los comentarios y creo que tiene sentido mostrarlo.

motoDrizzt
fuente
1
¿Qué diablos se suponía que iba a lograr?
abstracción lo es todo.
8
si quieres seguir esta ruta, al menos debes deshacerte de lo innecesario else if, ifes suficiente.
2017
10
@ Ðаn estoy completamente en desacuerdo sobre el else if. Encuentro útil poder echar un vistazo a un bloque de código y ver que es un árbol de decisiones, en lugar de un grupo de declaraciones no relacionadas. Sí, elseo breakno son necesarios para el compilador después de a return, pero son útiles para el humano que está mirando el código.
IMSoP
@ Ðаn Nunca he visto un idioma en el que se requiera un anidamiento adicional allí. O hay una palabra clave elseif/ separada elsif, o técnicamente está usando un bloque de una declaración que comienza if, como aquí. Ejemplo rápido de lo que creo que estás pensando y en lo que estoy pensando: gist.github.com/IMSoP/90bc1e9e2c56d8314413d7347e76532a
IMSoP
7
@ Ðаn Sí, estoy de acuerdo en que sería horrible. Pero no es lo elseque te obliga a hacer eso, es una guía de estilo pobre que no reconoceelse if como una declaración distinta. Siempre usaría llaves, pero nunca escribiría un código como ese, como mostré en mi esencia.
IMSoP
39

En pseudocódigo:

angle = (angle + 30) %360; // Offset by 30.

Por lo tanto, tenemos 0-60, 60-90, 90-150, ... como las categorías. En cada cuadrante con 90 grados, una parte tiene 60, una parte tiene 30. Entonces, ahora:

i = angle / 90; // Figure out the quadrant. Could be 0, 1, 2, 3 

j = (angle - 90 * i) >= 60? 1: 0; // In the quardrant is it perfect (eg: RIGHT) or imperfect (eg: UP_RIGHT)?

index = i * 2 + j;

Use el índice en una matriz que contenga las enumeraciones en el orden apropiado.

Bijay Gurung
fuente
7
Esto es bueno, probablemente la mejor respuesta aquí. Lo más probable es que si el interrogador original observara su uso de la enumeración más tarde, encontraría que tiene un caso en el que simplemente se convierte de nuevo en un número. Eliminar la enumeración por completo y simplemente seguir con un entero de dirección probablemente tenga sentido en otros lugares de su código y esta respuesta lo lleva directamente allí.
Bill K
18 
switch (this->_car.getAbsoluteAngle() / 30) // integer division
{
    case 0:
    case 11: this->_car.edir = Car::EDirection::RIGHT; break;
    case 1: this->_car.edir = Car::EDirection::UP_RIGHT; break;
    ...
    case 10: this->_car.edir = Car::EDirection::DOWN_RIGHT; break;
}
Caleth
fuente
ángulo = esto -> _ car.getAbsoluteAngle (); sector = (ángulo% 360) / 30; El resultado son 12 sectores. Luego indexe en una matriz, o use switch / case como arriba, que el compilador se transforma en tabla de salto de todos modos.
ChuckCottrill
1
Cambiar no es realmente mejor que las cadenas if / else.
Bill K
5
@BillK: Podría ser, si el compilador lo convierte en una búsqueda de tabla para usted. Eso es más probable que con una cadena if / else. Pero dado que es fácil y no requiere ningún truco específico de la arquitectura, probablemente sea mejor escribir la búsqueda de tabla en el código fuente.
Peter Cordes
En general, el rendimiento no debería ser una preocupación, es la legibilidad y el mantenimiento, cada cambio y la cadena if / else generalmente significa un montón de código desordenado de copiar y pegar que debe actualizarse en varios lugares cada vez que agrega un nuevo elemento. Es mejor evitar ambos y tratar de enviar tablas, cálculos o simplemente cargar datos de un archivo y tratarlos como datos si puede.
Bill K
Es probable que PeterCordes, el compilador, genere un código idéntico para la LUT que para el conmutador. @BillK puede extraer el interruptor en una función 0..12 -> Car :: EDirection, que tendría una repetición equivalente a una LUT
Caleth
16

Ignorando el primero, ifque es un caso un poco especial, los restantes siguen exactamente el mismo patrón: un mínimo, un máximo y una dirección; pseudocódigo:

if (angle > min && angle <= max)
  _car.edir = direction;

Hacer este C ++ real podría verse así:

enum class EDirection {  NONE,
   RIGHT, UP_RIGHT, UP, UP_LEFT, LEFT, DOWN_LEFT, DOWN, DOWN_RIGHT };

struct AngleRange
{
    int min, max;
    EDirection direction;
};

Ahora, en lugar de escribir un montón de ifs, simplemente recorra sus diversas posibilidades:

EDirection direction_from_angle(int angle, const std::vector<AngleRange>& angleRanges)
{
    for (auto&& angleRange : angleRanges)
    {
        if ((angle > angleRange.min) && (angle <= angleRange.max))
            return angleRange.direction;
    }

    return EDirection::NONE;
}

( throwIng una excepción más que returnING NONEes otra opción).

Que luego llamarías:

_car.edir = direction_from_angle(_car.getAbsoluteAngle(), {
    {30, 60, EDirection::UP_RIGHT},
    {60, 120, EDirection::UP},
    // ... etc.
});

Esta técnica se conoce como programación basada en datos . Además de deshacerse de un montón de ifs, le permitiría agregar fácilmente más direcciones (por ejemplo, NNW) o reducir el número (izquierda, derecha, arriba, abajo) sin tener que volver a trabajar en otro código.

(Manejar su primer caso especial se deja como "un ejercicio para el lector" :-))

Un
fuente
1
Técnicamente, podría eliminar min dado que todos los rangos de ángulos coinciden, lo que reduciría la condición a if(angle <= angleRange.max)+1 para usar características de C ++ 11 como enum class.
Pharap
12

Aunque las variantes propuestas basadas en una tabla de búsqueda para angle / 30son probablemente preferibles, aquí hay una alternativa que usa una búsqueda binaria codificada para minimizar el número de comparaciones.

static Car::EDirection directionFromAngle( int angle )
{
    if( angle <= 210 )
    {
        if( angle > 120 )
            return angle > 150 ? Car::EDirection::LEFT
                               : Car::EDirection::UP_LEFT;
        if( angle > 30 )
            return angle > 60 ? Car::EDirection::UP
                              : Car::EDirection::UP_RIGHT;
    }
    else // > 210
    {
        if( angle <= 300 )
            return angle > 240 ? Car::EDirection::DOWN
                               : Car::EDirection::DOWN_LEFT;
        if( angle <= 330 )
            return Car::EDirection::DOWN_RIGHT;
    }
    return Car::EDirection::RIGHT; // <= 30 || > 330
}
x4u
fuente
2

Si realmente desea evitar la duplicación, puede expresarlo como una fórmula matemática.

En primer lugar, suponga que estamos usando la enumeración de @ Geek

Enum EDirection { RIGHT =0, UP_RIGHT, UP, UP_LEFT, LEFT, DOWN_LEFT,DOWN, DOWN_RIGHT}

Ahora podemos calcular la enumeración usando matemáticas enteras (sin la necesidad de matrices).

EDirection angle2dir(int angle) {
    int d = ( ((angle%360)+360)%360-1)/30;
    d-=d/3; //some directions cover a 60 degree arc
    d%=8;
    //printf ("assert(angle2dir(%3d)==%-10s);\n",angle,dir2str[d]);
    return (EDirection) d;
}

Como señala @motoDrizzt, el código conciso no es necesariamente un código legible. Tiene la pequeña ventaja de que expresarlo como matemáticas hace explícito que algunas direcciones cubren un arco más amplio. Si desea ir en esta dirección, puede agregar algunas afirmaciones para ayudar a comprender el código.

assert(angle2dir(  0)==RIGHT     ); assert(angle2dir( 30)==RIGHT     );
assert(angle2dir( 31)==UP_RIGHT  ); assert(angle2dir( 60)==UP_RIGHT  );
assert(angle2dir( 61)==UP        ); assert(angle2dir(120)==UP        );
assert(angle2dir(121)==UP_LEFT   ); assert(angle2dir(150)==UP_LEFT   );
assert(angle2dir(151)==LEFT      ); assert(angle2dir(210)==LEFT      );
assert(angle2dir(211)==DOWN_LEFT ); assert(angle2dir(240)==DOWN_LEFT );
assert(angle2dir(241)==DOWN      ); assert(angle2dir(300)==DOWN      );
assert(angle2dir(301)==DOWN_RIGHT); assert(angle2dir(330)==DOWN_RIGHT);
assert(angle2dir(331)==RIGHT     ); assert(angle2dir(360)==RIGHT     );

Habiendo agregado las afirmaciones, ha agregado duplicación, pero la duplicación en las afirmaciones no es tan mala. Si tiene una afirmación inconsistente, lo sabrá muy pronto. Las afirmaciones se pueden compilar fuera de la versión de lanzamiento para no inflar el ejecutable que distribuye. Sin embargo, este enfoque probablemente sea más aplicable si desea optimizar el código en lugar de hacerlo menos feo.

gmatht
fuente
1

Llego tarde a la fiesta, pero podríamos usar indicadores de enumeración y comprobaciones de rango para hacer algo ordenado.

enum EDirection {
    RIGHT =  0x01,
    LEFT  =  0x02,
    UP    =  0x04,
    DOWN  =  0x08,
    DOWN_RIGHT = DOWN | RIGHT,
    DOWN_LEFT = DOWN | LEFT,
    UP_RIGHT = UP | RIGHT,
    UP_LEFT = UP | LEFT,

    // just so we be clear, these won't have much use though
    IMPOSSIBLE_H = RIGHT | LEFT, 
    IMPOSSIBLE_V = UP | DOWN
};

la comprobación (pseudocódigo), asumiendo que el ángulo es absoluto (entre 0 y 360):

int up    = (angle >   30 && angle <  150) * EDirection.UP;
int down  = (angle >  210 && angle <  330) * EDirection.DOWN;
int right = (angle <=  60 || angle >= 330) * EDirection.Right;
int left  = (angle >= 120 && angle <= 240) * EDirection.LEFT;

EDirection direction = (Direction)(up | down | right | left);

switch(direction){
    case RIGHT:
         // do right
         break;
    case UP_RIGHT:
         // be honest
         break;
    case UP:
         // whats up
         break;
    case UP_LEFT:
         // do you even left
         break;
    case LEFT:
         // 5 done 3 to go
         break;
    case DOWN_LEFT:
         // your're driving me to a corner here
         break;
    case DOWN:
         // :(
         break;
    case DOWN_RIGHT:
         // completion
         break;

    // hey, we mustn't let these slide
    case IMPOSSIBLE_H:
    case IMPOSSIBLE_V:
        // treat your impossible case here!
        break;
}
Leonardo Pina
fuente