I have the following code:
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;
I want to avoid the if
s chain; it's really ugly. Is there a another, possibly cleaner, way of writing this?
this->_car.getAbsoluteAngle()
once before the whole cascade.
this
(this->
) is not needed and doesn't really do anything good to the readability..
>
tests; they aren't needed, since each of them has already been tested (in the opposite direction) in the previous if
statement.
else if
.
#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).
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;
}
table[angle%360/30]
answers because it's cheap and branchless. Far cheaper than a tree-search loop, if this compiles to asm that's similar to the source. (std::unordered_map
is usually a hash table, but std::map
is typically a red-black binary tree. The accepted answer effectively uses angle%360 / 30
as a perfect hash function for angles (after replicating a couple entries, and Bijay's answer even avoids that with an offset)).
lower_bound
on a sorted array. That would be a lot more efficient than a map
.
this->_car.getAbsoluteAngle()
with a tmp var, and removing the redundant compare from each of the OP's if()
clauses (checking for something that would have already matched the preceding if()). Or use @ wilx's sorted-array suggestion.
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;
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.
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.
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;
}
Ignoring your first if
which is a bit of a special case, the remaining ones all follow the exact same pattern: a min, max and direction; pseudo-code:
if (angle > min && angle <= max)
_car.edir = direction;
Making this real C++ might look like:
enum class EDirection { NONE,
RIGHT, UP_RIGHT, UP, UP_LEFT, LEFT, DOWN_LEFT, DOWN, DOWN_RIGHT };
struct AngleRange
{
int min, max;
EDirection direction;
};
Now, rather than writing a bunch of if
s, just loop over your various possibilies:
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;
}
(throw
ing an exception rather than return
ing NONE
is another option).
Which you would then call:
_car.edir = direction_from_angle(_car.getAbsoluteAngle(), {
{30, 60, EDirection::UP_RIGHT},
{60, 120, EDirection::UP},
// ... etc.
});
This technique is known as data-driven programming. Besides getting rid of a bunch of if
s, it would allow you to use easily add more directions (e.g., NNW) or reduce the number (left, right, up, down) without re-working other code.
(Handling your first special case is left as "an exercise for the reader." :-) )
if(angle <= angleRange.max)
but +1 for using C++11 features like enum class
.
Although the proposed variants based on a lookup table for angle / 30
are probably preferable, here is an alternative that uses a hard coded binary search to minimize the number of comparisons.
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
}
If you really want to avoid duplication you can express this as a mathematical formula.
First of all, assume that we are using @Geek's Enum
Enum EDirection { RIGHT =0, UP_RIGHT, UP, UP_LEFT, LEFT, DOWN_LEFT,DOWN, DOWN_RIGHT}
Now we can compute the enum using integer mathematics (with out the need for arrays).
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;
}
As @motoDrizzt points out, concise code isn't necessarily readable code. It does have the small advantage that expressing it as maths makes it explicit that some directions cover a wider arc. If you want to go this direction you can add some asserts to help understand the code.
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 );
Having added the asserts you have added duplication, but duplication in asserts isn't so bad. If you have an inconsistent assert you will find out soon enough. Asserts can be compiled out of release version so as not to bloat the executable you distribute. Nevertheless, this approach is probably most applicable if you want to optimize the code rather than just make it less ugly.
I'm Late to the party, but We could use enum flags and range checks to do something neat.
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
};
the checking(pseudo-code), assuming angle is absolue (between 0 and 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;
}
Success story sharing
q = a/b
andr = a%b
thenq * b + r
must equala
. Thus it is legal in C99 for a remainder to be negative. BorgLeader, you can fix the problem with(((angle % 360) + 360) % 360) / 30
.GetDirectionForAngle
is what I'm proposing as a replacement for the if/else cascade, they're both O(1)...