How to avoid if / else if chain when classifying a heading into 8 directions?

Solution 1:

#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;
}

This is how I would do it. (As per my previous comment).

Solution 2:

You can use map::lower_bound and store the upper-bound of each angle in a map.

Working example below:

#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;
}

Solution 3:

Create an array, each element of which is associated with a block of 30 degrees:

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
};

Then you can index the array with the angle / 30:

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

No comparisons or branching required.

The result however is slightly off from the original. Values on the borders, i.e. 30, 60, 120, etc. are placed in the next category. For example, in the original code the valid values for UP_RIGHT are 31 to 60. The above code assigns 30 to 59 to UP_RIGHT.

We can get around this by subtracting 1 from the angle:

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

This now gives us RIGHT for 30, UP_RIGHT for 60, etc.

In the case of 0, the expression becomes (-1 % 360) / 30. This is valid because -1 % 360 == -1 and -1 / 30 == 0, so we still get an index of 0.

Section 5.6 of the C++ standard confirms this behavior:

4 The binary / operator yields the quotient, and the binary % operator yields the remainder from the division of the first expression by the second. If the second operand of / or % is zero the behavior is undefined. For integral operands the / operator yields the algebraic quotient with any fractional part discarded. if the quotient a/b is representable in the type of the result, (a/b)*b + a%b is equal to a.

EDIT:

There were many questions raised regarding the readability and maintainability of a construct like this. The answer given by motoDrizzt is a good example of simplifying the original construct that is more maintainable and isn't quite as "ugly".

Expanding on his answer, here's another example making use of the ternary operator. Since each case in the original post is assigning to the same variable, using this operator can help increase readability further.

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;

Solution 4:

That code is not ugly, it's simple, practical, readable and easy to understand. It will be isolated in it's own method, so nobody will have to deal with it in everyday life. And just in case someone has to check it -maybe because he is debugging your application for a problem somewhere else- it's so easy it will take him two seconds to understand the code and what it does.

If I was doing such a debug I'd be happy to not have to spend five minutes trying to understand what your function does. In this regards, all other functions fail completely, as they change a simple, forget-about-it, bugs free routine, in a complicated mess that people when debugging will be forced to deeply analyse and test. As a project manager myself I'd strongly get upset by a developer taking a simple task and instead of implementing it into a simple, harmless way, wastes time to implement it into an over complicate way. Just think all the time you wasted thinking about it, then coming to SO asking, and all for just the sake of worsening maintenance and readability of the thing.

That said, there is a common mistake in your code that make it quite less readable, and a couple improvements you can do quite easily:

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;

Put this into a method, assign the returned value to the object, collapse the method, and forget about it for the rest of eternity.

P.S. there is another bug over the 330 threshold, but I don't know how you want to treat it, so I didn't fix it at all.

Later update

As per comment, you can even get rid of the else if at all:

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;

I didn't do it 'cause I feel that a certain point it becomes just a matter of own preferences, and the scope of my answer was (and is) to give a different perspective to your concern about "ugliness of code". Anyway, as I said, someone pointed it out in the comments and I think it makes sense to show it.

Solution 5:

In pseudocode:

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

So, we have 0-60, 60-90, 90-150,... as the categories. In each quadrant with 90 degrees, one part has 60, one part has 30. So, now:

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 the index in an array containing the enums in the appropriate order.