How To Fix This Ugly Code Here?

Started by
10 comments, last by Randomvoid 7 years, 8 months ago

Guys, I already made my very primitive chat server and it's working perfectly. The problem is the code, which is quite ugly.

For example, I don't like the way I declare the booleans for the keys.


    bool aKey, bKey, cKey, dKey, eKey,
    fKey, gKey, hKey, iKey, jKey,
    kKey, lKey, mKey, nKey, oKey,
    pKey, qKey, rKey, sKey, tKey,
    uKey, vKey, wKey, xKey, yKey,
    zKey, spaceKey, enterKey, escapeKey, f1Key;

I don't like this too. But I guess there's no other way here.


            if( event.key.keysym.sym == SDLK_a )
            {
                aKey = true;
            }
            if( event.key.keysym.sym == SDLK_b )
            {
                bKey = true;
            }
            if( event.key.keysym.sym == SDLK_c )
            {
                cKey = true;
            }
            if( event.key.keysym.sym == SDLK_d )
            {
                dKey = true;
            }
            if( event.key.keysym.sym == SDLK_e )
            {
                eKey = true;
            }
            if( event.key.keysym.sym == SDLK_f )
            {
                fKey = true;
            }
            if( event.key.keysym.sym == SDLK_g )
            {
                gKey = true;
            }
            if( event.key.keysym.sym == SDLK_h )
            {
                hKey = true;
            }
            if( event.key.keysym.sym == SDLK_i )
            {
                iKey = true;
            }
            if( event.key.keysym.sym == SDLK_j )
            {
                jKey = true;
            }
            //And so on.

And the biggest problem is this:


void App::resetAllKeys()
{
    aKey = bKey = cKey = dKey = eKey =
    fKey = gKey = hKey = iKey = jKey =
    kKey = lKey = mKey = nKey = oKey =
    pKey = qKey = rKey = sKey = tKey =
    uKey = vKey = wKey = xKey = yKey =
    zKey = spaceKey = enterKey = escapeKey = f1Key = false;
}

Every time I add a new key as a boolean, I need to remember to add it to this function, too, because if I forget to reset it, there are really strange bugs happening. I'm sure there is a way to beautify some of this. Any ideas?

Advertisement
std::map<int, bool> keyState;

bool isPressed(int key)
{
   auto it = keyState.find(key);
   if (it != keyState.end())
      return *it;
   else
      return false;
}

void setKeyState(int key, bool pressed)
{
   keyState[key] = pressed;
}

void clearState()
{
   keyState.clear();
}
Not the ideal solution but I'm in no mood to check the range of key values SDL can deliver and just about everything is better than the mess you currently have.

Seems ideal to me. Thanks. ^_^

EDIT: I'm wondering which map is better: <int, bool> or <string, bool>. I'm really scared of using string in a map because from my experience, it is really heavy, but that was kind of a different situation, that's why I can't decide which is better, what do you think?

I'm wondering which map is better: or <int, bool> or <string, bool>

"Better" depends on what you need, there is no universal answer.

The SDLK things are constants from an enum, using ints here is appropriate. In this case, you can think of an enum entry as being essentially just a nickname for an integer value.

Hello to all my stalkers.

Lactose, I forgot I could use the enums, true.

Guys, I will post the modified code. It is a lot better, the long initializing of the bool vars is removed, and the long reset() function is now just 1 line: keyState.clear().

The SDL keys are kind of chaotic, so the letters in the alphabet are not on a +1 distance from each other, I can't really use a loop. Nevermind. Here is the code.


map<int, bool> keyState; //Initialization

....

            if( event.key.keysym.sym == SDLK_a )
            {
                keyState[ SDLK_a ] = true;
            }
            if( event.key.keysym.sym == SDLK_b )
            {
                keyState[ SDLK_b ] = true;
            }
            if( event.key.keysym.sym == SDLK_c )
            {
                keyState[ SDLK_c ] = true;
            }
            if( event.key.keysym.sym == SDLK_d )
            {
                keyState[ SDLK_d ] = true;
            }
            if( event.key.keysym.sym == SDLK_e )
            {
                keyState[ SDLK_e ] = true;
            }
            if( event.key.keysym.sym == SDLK_f )
            {
                keyState[ SDLK_f ] = true;
            }
            if( event.key.keysym.sym == SDLK_g )
            {
                keyState[ SDLK_g ] = true;
            }
            if( event.key.keysym.sym == SDLK_h )
            {
                keyState[ SDLK_h ] = true;
            }
            //And so on

...


    //Usage
    if( keyState[ SDLK_a ] == true )
    {
        str += "a";
    }
    if( keyState[ SDLK_b ] == true )
    {
        str += "b";
    }
    if( keyState[ SDLK_c ] == true )
    {
        str += "c";
    }
    if( keyState[ SDLK_d ] == true )
    {
        str += "d";
    }
    if( keyState[ SDLK_e ] == true )
    {
        str += "e";
    }
    if( keyState[ SDLK_f ] == true )
    {
        str += "f";
    }
    if( keyState[ SDLK_g ] == true )
    {
        str += "g";
    }
    if( keyState[ SDLK_h ] == true )
    {
        str += "h";
    }
...


void App::resetAllKeys() { keyState.clear(); }

I do such things using bit fields that make it way more simple and you could define an enum for each index in the field


class Input
{
    unsigned int keys;

    public:

    inline Input() : keys(0)
    { }

    inline bool GetKey(int index)
    {
        return (keys >> index) & 1u;
    }

    inline void SetKey(int index, bool value)
    {
       if(value) keys |= 1u << index;
       else keys &= ~(1u << index);
    }

    inline void Clear()
    {
        keys = 0;
    }
};

When using more then 32 bools you need to go for the next higher integer unsigned long long int

Instead of all the ifs, you can do the following (using BitMaster's suggestion).


setKeyState(event.key.keysym.sym, true);

Hello to all my stalkers.

Is there a reason why you're duplicating the functionality of SDL_GetKeyboardState? (https://wiki.libsdl.org/SDL_GetKeyboardState)

(EDIT: Removed link to old functionality, replaced it with the new one.)

Is there a reason why you're duplicating the functionality of SDL_GetKeyState? (http://sdl.beuc.net/sdl.wiki/SDL_GetKeyState)

because i'm stupid? :huh:

Lesson for next time: Before coding, study the lib functionality.

Instead of all the ifs, you can do the following (using BitMaster's suggestion). setKeyState(event.key.keysym.sym, true);

Lactose, only now I realize why you wrote this.

Instead of my 30 'if' statements, I just need to write this one line and it works perfectly and it somehow does the job of all the other 30 'if' statements combined. This SDL event polling was always magic to me.

This topic is closed to new replies.

Advertisement