Is my programming logic correct here?

Question

const char IsPressed = 1; // 1
const char WasHeldDown = 2; // 10
const char IsFirstPress = 4; // 100

char* keystates[256];

Class::CalculateKeyStates()
{
    for(int i = 0; i < 256; ++i)
    {
        if(this->IsDown(i))
        {
            keystates[i] |= IsPressed; // turn on
            if(keystates[i] & WasHeldDown)
            {
                //keystates[i] |= IsFirstPress;
                keystates[i] &= ~IsFirstPress; // turn off
            }
            else
            {
                keystates[i] |= WasHeldDown + IsFirstPress; // Turn on
            }
        }
        else
        {
            keystates[i] = 0; // Turn ALL off
        }
    }
}

This function would be a member function of a class, Class. The other member function, IsDown, will return a true if the key in question is down and false if not.

Can you see any way of further improving this function?

Thanks

EDIT:

I will expand a bit as to what is done why. This is a modification of an bit of code that works through an array keyStates (which was a struct of three bools) setting IsPressed to false for all of the keys. then again setting Ispressed to the value of this->IsDown and then a third time looping through checking if the key had been held, if it has then its no longer the first press so set that to false. if it was not held down, then set first press to true and was held to true as well, so next time it is flagged as having been held.

EDIT2:

Added some comments to code and corrected one line

Answer 1

You are always setting IsFirstPress if the key is down, which might not be what you want.

Answer 2

Personally, I would define the key-states as disjoint states and write a simple state-machine, thus:

enum keystate
{
    inactive,
    firstPress,
    active
};

keystate keystates[256];

Class::CalculateKeyStates()
{
    for (int i = 0; i < 256; ++i)
    {
        keystate &k = keystates[i];

        switch (k)
        {
        inactive:
            k = (isDown(i)) ? firstPress : inactive;
            break;
        firstPress:
            k = (isDown(i)) ? active : inactive;
            break;
        active:
            k = (isDown(i)) ? active : inactive;
            break;
        }
    }
}

This is easier to extend, and easier to read if it gets any more complex.

Answer 3

I'm not sure what you want to achieve with IsFirstPress, as the keystate cannot remember any previous presses anyways. If you want to mark with this bit, that it's the first time you recognized the key being down, then your logic is wrong in the corresponding if statement.

keystates[i] & WasHeldDown evaluates to true if you already set the bit WasHeldDown earlier for this keystate. In that case, what you may want to do is actually remove the IsFirstPress bit by xor-ing it: keystates[i] ^= IsFirstPress