13

Here is my code:

bool Character::keyPress(char c)
{
    switch(c)
    {
        case up_key:
            move(0, -1);
            break;

        case down_key:
            move(0, 1);
            break;

        case left_key:
            move(-1, 0);
            break;

        case right_key:
            move(1,0);
            break;

        default:
            return false;
    }

    return true;
}

And the compiler complains:

error C2051: case expression not constant
error C2051: case expression not constant
error C2051: case expression not constant
error C2051: case expression not constant

In my header file I have:

protected:
    char up_key;
    char down_key;
    char right_key;
    char left_key;

I am using Visual C++ 2008.

Improve this question
2
  • 2
    switch case expressions must be compile time constants. change those to static const char up_key = 1; and such, and problem solved. Commented Jan 19, 2012 at 4:06
  • 1
    Because the Standard says so. It's a remnant of the old days, where switch were introduced as a "nicer" presentation that was transformed into an array look-up automagically (and thus required constants). Nowadays it makes less sense, but the syntax has not been changed so... Commented Jan 19, 2012 at 7:49

7 Answers 7

Reset to default
17

As the error message states, the case expressions must be constant. The compiler builds this as a very fast look-up table at compile time and it can't do that if there is a possibility that the values could change as the program runs.

If you do need them to be variable, not constant, your best bet is to use if/else statements instead.

Improve this answer
Sign up to request clarification or add additional context in comments.

is it possible to somehow "see" the output of compiler in this case, and maybe compare it with if/else?
7

Replace this long clumsy code,

switch(c)
{
    case up_key:
        move(0, -1);
        break;

    case down_key:
        move(0, 1);
        break;

    case left_key:
        move(-1, 0);
        break;

    case right_key:
        move(1,0);
        break;

    default:
        return false;
}

with something like this:

move( (c==right_key) - (c==left_key) , (c==down_key) - (c==up_key) );

You can litterly replace that 17 lines long of code with that much more neat single line of code.

Improve this answer

What makes you think that that code is neat?
It is a neat line - maybe you should just add a small explanatory comment to the code to assist the hard-of-reading types ;)
doesn't it increase time complexity? 4 comparisons, 4 type casts and 2 arthmatic ops are to be made with ur single line!
@neckTwi I don't think those extra operations matter, considering that a keypress doesn't happen very often. (Then it is also possible that the compiler might optimize that line to something that is equivalent to the switch–case statement above, although I don't know how much you can trust that it actually does)
@EralpB It is neat because it consists of less code which in my opinion 1) makes reading the code go faster, and 2) makes writing and maintaining the code less error-prone since you need to make changes to fewer places.
1

You can't because the language doesn't work that way. For example, what would happen if up_key, down_key, right_key, and left_key were all equal?

Improve this answer

,i see what you mean. But if i use if/else and they are all equal??
Actually, you can have the same value here more than once, though you'll always end up in the first one in the list. With if/else, too, you'll get to the first one and never to the later ones.
Even if they were all constants, which is the crucial aspect here, the values in the cases must all be distinct. But the distinctness is not the problem - it is the lack of constness.
You can't have the same constant twice? I believe GCC allows it, though now you make me unsure.
1

Because the switch statement can take only constants, you know when reading the code that the things you're comparing against are all constants. On the other hand, you would use if statements (or some other structure) to compare against variables:

if (c == up_key) {
    move(0, -1);
} else if (c == down_key) {
    move(0, 1);
} else ...

This provides a distinct difference in structure which can greatly aid those who come after you in reading your code. Imagine if you had to look up every case label to see whether it was a variable or not?

Improve this answer

Comments

1

I believe it's because the compiler generates a jump table, with the values hardcoded in, although I may be wrong. The way the tables are generated just doesn't allow for it.

Improve this answer

Comments

1

Since other answers have covered why you are getting an error, here is a way to move in one of the four directions in response to a key press: use lookup tables instead of the conditionals/switches.

Setup portion:

std::map<char,pair<int,int> > moves;
moves[up_key] = make_pair(0, -1);
moves[down_key] = make_pair(0, 1);
moves[left_key] = make_pair(-1, 0);
moves[right_key] = make_pair(1, 0);

The function:

bool Character::keyPress(char c) {
    if (moves.count(c)) {
        pair<int,int> dir = moves[c];
        move(dir.first, dir.second);
        return true;
    } else {
        return false;
    }
}
Improve this answer

Comments

0 
//here is the full functional code snippet which can be compiled and run with most of C++  
//compiler/link ...console app was demoed but you can apply the code/logic to win32 app...
//if you have any problem, send me email to [email protected]

#include <iostream.h>
#include <map>
#include <conio.h>

class CkbdHanler{
  private:
    map<char,pair<int,int> > moves;
  protected:
    char up_key;
    char down_key;
    char right_key;
    char left_key;
  public:

CkbdHanler(char a,char b,char c,char d):up_key(a),
                                        down_key(b),
                                       right_key(c),
                                       left_key(d)
{
    moves[up_key] = make_pair(0, -1);
    moves[down_key] = make_pair(0, 1);
    moves[left_key] = make_pair(-1, 0);
    moves[right_key] = make_pair(1, 0);
 }

bool keyPress(char c){
    if (moves.count(c)) {
            pair<int,int> dir = moves[c];
            move(dir.first, dir.second);
            return true;
   } else return false;

}
void move(int i,int j){
   cout<<"(i,j)=("<<i<<","<<j<<")"<<endl;
  }
};

int main(int argc, char* argv[])
{
  CkbdHanler CmyKbdH('u','d','l','r');

  cout << "Hello C++... here is a demo of Map to replace switch-case" << endl;
  CmyKbdH.keyPress('d');
  cout << endl << "Press any key to continue...";

  getch();
  return 0;
}
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.