如何避免如果/否如果链时分类一个标题成8个方向？

小开

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

小开

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.

小开

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;

小开

最佳答案

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

小开

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

(throwing an exception rather than returning 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 ifs, 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." :-) )

小开

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
}

小开

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.

小开

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