Sign in to follow this  
Heelp

How To Fix This Ugly Code Here?

Recommended Posts

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?

Edited by codeBoggs

Share this post


Link to post
Share on other sites

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?

Edited by codeBoggs

Share this post


Link to post
Share on other sites
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.

Edited by Lactose!

Share this post


Link to post
Share on other sites

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(); }
Edited by codeBoggs

Share this post


Link to post
Share on other sites

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

Share this post


Link to post
Share on other sites
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.

Edited by codeBoggs

Share this post


Link to post
Share on other sites
And that's basically what SDL_GetKeyboardState will do for you, except probably with its own array instead of a std::map.

(Note that I suggested SDL_GetKeyState above when I meant SDL_GetKeyboardState - it got renamed for SDL 2.)

If you ever find yourself making a ton of similar variables, consider whether they are better handled as array indices or some other unique key value instead, and switch to an array or map accordingly..

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this