How To Better Optimize This Code?

[A_Pacifist]

Hey Guys!
   I have been using SFML for quite some time now, but I want to really focus on optimizing and using better coding practices. My question is pretty straightforward. How could I optimize the code below to look better and not be so monotonous.

                //Movement code using if statements
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
                {
                        playerSprite.move(-4, 0);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
                {
                        playerSprite.move(4, 0);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                        playerSprite.move(0, -4);
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                        playerSprite.move(0, 4);
                }

I am guessing I could use a switch statement or an enumeration, but I don't know how to implement this.

Thanks for any help!
Calvin

[Erdrick]

Use "else if" instead of "if" on the last three so it doesnt need to check unnecessary conditions

[eXpl0it3r]

Adding if else changes the behavior slightly. Now you can't move up and to the right at the same time.

The code can't be really optimized more.

[nicox11]

It's not an optimisation, but try to use defined values (variables for example) instead of random ones (ie: your "4"). Furthermore, if you suddenly want to change your 4 by something else, you need to modify 4 lines.

It'll be easier to read your code with "Player_Speed" than just "4".

[Erdrick]

Adding if else changes the behavior slightly. Now you can't move up and to the right at the same time.

The code can't be really optimized more.

thanks for mentioning this.  He mentioned switch so i thought he was accepting the change in behavior but i should have said it anyway

[Mortal]

you may try an alternative approach by using std::map or perhaps std::unordered_map or even std::vector and for-range loop, but you need to benchmark it to be sure which one is highly suitable for you.

possible implementation would be like this:

static const std::unordered_map<sf::Keyboard::Key, sf::Vector2f> keyPending{
    {sf::Keyboard::Up, sf::Vector2f{0, -4}},
    {sf::Keyboard::Down, sf::Vector2f{0, 4}},
    {sf::Keyboard::Left, sf::Vector2f{-4, 0}},
    {sf::Keyboard::Right, sf::Vector2f{4, 0}}
};

for (const auto& pair : keyPending)
{
    if (sf::Keyboard::isKeyPressed(pair.first))
        playerSprite.move(pair.second);
}

[Jesper Juhl]

IMHO trying to micro optimize something as trivial as this is pointless. It is unlikely to take up any measurable amount of CPU time and will be completly dwarfed by other code that does work like AI, Physics, rendering etc.
This code should just be written in the most clear manner possible and don't worry about performance.

[Hapax]

The code itself seems perfectly fine and would do just the job required.

How could I optimize the code below to look better

Since this seems to be more about how your code looks than how it functions, one suggestion would be to remove all of the braces since there is only one statement based of the truth of the condition in the if statements, thus:

        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))
            playerSprite.move(-4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right))
            playerSprite.move(4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
            playerSprite.move(0, -4);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
            playerSprite.move(0, 4);

Or, more compact:

        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))  playerSprite.move(-4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)) playerSprite.move(4, 0);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))    playerSprite.move(0, -4);
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))  playerSprite.move(0, 4);

[DarkRoku12]

you may try an alternative approach by using std::map or perhaps std::unordered_map or even std::vector and for-range loop, but you need to benchmark it to be sure which one is highly suitable for you.

possible implementation would be like this:

static const std::unordered_map<sf::Keyboard::Key, sf::Vector2f> keyPending{
    {sf::Keyboard::Up, sf::Vector2f{0, -4}},
    {sf::Keyboard::Down, sf::Vector2f{0, 4}},
    {sf::Keyboard::Left, sf::Vector2f{-4, 0}},
    {sf::Keyboard::Right, sf::Vector2f{4, 0}}
};

for (const auto& pair : keyPending)
{
    if (sf::Keyboard::isKeyPressed(pair.first))
        playerSprite.move(pair.second);
}

This implementation is the same ( if the compiler unroll the loop ) or worst than the first version presented if the compiler can't unroll it or inline the lookups to the map.

And @Jesper Juhl say a very good point, try to optimize these things is unnecessary, where the AI, game logic or physics, and the draw calls are the hotspot of the game.

And you can use if\else if on up/down and left/right.

When dealing with keys, is a very good point to take into account the key rollover.

[Laurent]

IMHO trying to micro optimize something as trivial as this is pointless. It is unlikely to take up any measurable amount of CPU time and will be completly dwarfed by other code that does work like AI, Physics, rendering etc.
This code should just be written in the most clear manner possible and don't worry about performance.

And this is exactly what the OP is asking:

better coding practices [...] the code below to look better [...] not be so monotonous

... so I think everyone talking about performances is slightly off-topic here

What's important is to:
- avoid using magic numbers
- avoid repeating the same piece of code

So:

/* static const */ float playerSpeed = 4.f;
sf::Vector2f direction = {0, 0};
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Left))  direction.x -= 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Right)) direction.x += 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))    direction.y -= 1;
if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))  direction.y += 1;
player.move(direction * playerSpeed /* * elapsedTime */);
 

[Elias Daler]

Another thing I can note is that it's better to use another speed for diagonal movement, because otherwise the player will go faster. If player has a normalSpeed = 1.f (which is used for horizontal and vertical movement) you need to make speed.x = speed.y = sqrt(1/2) when going diagonally, so that:
speed.x ^ 2 + speed.y ^ 2 = normalSpeed

[Hapax]

I would generally use the method proposed above by Laurent.
I, personally, would prefer ++direction.x over direction.x += 1 but it's just a matter of taste.

Another thing I can note is that it's better to use another speed for diagonal movement, because otherwise the player will go faster. If player has a normalSpeed = 1.f (which is used for horizontal and vertical movement) you need to make speed.x = speed.y = sqrt(1/2) when going diagonally, so that:
speed.x ^ 2 + speed.y ^ 2 = normalSpeed

This can also be calculated using sine and cosine but since it's diagonal, they're identical. Basically, at 45 degrees, sine and cosine are at approximately 0.707 (or just 0.7) so you can adjust the diagonal speed by multiplying both x and y movement components by 0.7.
Remember to only do this if movement is occuring in horizontal and vertical direction.

This is an example of how I once did this for an SFML game jam. It determines the direction from an enumeration passed from another function but you can see that horizontal and vertical motions are (1, 0) based where diagonal motions are (angleOnAxis, angleOnAxis) based. angleOnAxis is approximately 0.7 (as explained above)

[A_Pacifist]

Thanks for all the help!
I wasn't to concerned about the actual time it took to run the code, I was just concerned that there were a bunch of if statements. I was once told by a CS teacher that if you are copying and pasting code repeatedly, you're probably doing something wrong.
Thanks all!