Pong game in C++ SFML

Question

This is my first game, I need to know how I could improve this code:

main.cpp

#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>
#include <iostream>
#include <sstream>
#include <math.h>
#include "Menu.h"


int main()
{


const int screenX = 600;
const int screenY = 400;
sf::Vector2f paddleSize(10, 60);
const int ballRad = 5;
float ballSpeed = 400.f;
float ballAngle = 75.f;
sf::Vector2f velocity;


sf::RenderWindow window(sf::VideoMode(screenX,screenY), "Pong", sf::Style::Titlebar | sf::Style::Close);
Menu menu(window.getSize().x, window.getSize().y);

window.setVerticalSyncEnabled(true);

sf::RectangleShape paddle1;
paddle1.setSize(paddleSize);
paddle1.setFillColor(sf::Color::White);
paddle1.setPosition(paddleSize.x,(screenY/2)-paddleSize.y);
paddle1.setOrigin(sf::Vector2f(paddle1.getSize().x / 2, paddle1.getSize().y / 2));

sf::RectangleShape paddle2;
paddle2.setSize(paddleSize);
paddle2.setFillColor(sf::Color::White);
paddle2.setPosition(screenX-(paddleSize.x*2), (screenY/2)-paddleSize.y);

sf::RectangleShape topBorder;
topBorder.setSize(sf::Vector2f(screenX,paddleSize.x));
topBorder.setPosition(0, 0);

sf::RectangleShape botBorder;
botBorder.setSize(sf::Vector2f(screenX, paddleSize.x));
botBorder.setPosition(0, screenY-paddleSize.x);

sf::Font font;

if (!font.loadFromFile("digit.ttf"))
    return EXIT_FAILURE;

//bool Sound;
sf::SoundBuffer bufferPaddle;
if (!bufferPaddle.loadFromFile("res/SoundFX/paddle.wav")) {}
sf::Sound soundPaddle;
soundPaddle.setBuffer(bufferPaddle);

sf::SoundBuffer bufferScore;
if (!bufferScore.loadFromFile("res/SoundFX/score.wav")) {}
sf::Sound soundScore;
soundScore.setBuffer(bufferScore);

sf::SoundBuffer bufferWall;
if (!bufferWall.loadFromFile("res/SoundFX/wall.wav")) {}
sf::Sound soundWall;
soundWall.setBuffer(bufferWall);

int intScore1 = 0;
sf::Text textScore1;
std::string strScore1 = std::to_string(intScore1);
textScore1.setString(strScore1);
textScore1.setFont(font);
textScore1.setCharacterSize(paddleSize.y*1.5f);
textScore1.setPosition(sf::Vector2f(screenX/5.f, 0.f));

int intScore2 = 0;
sf::Text textScore2;
std::string strScore2 = std::to_string(intScore2);
textScore2.setString(strScore2);
textScore2.setFont(font);
textScore2.setCharacterSize(paddleSize.y*1.5f);
textScore2.setPosition(sf::Vector2f(365.f, 0.f));

//add sf::VertexArray, dashed line splitting map into 2

sf::Vertex line[] =
{
    sf::Vertex(sf::Vector2f(screenX/2+1,0)),
    sf::Vertex(sf::Vector2f(screenX/2+1,screenY))
};

sf::CircleShape ball(ballRad);
ball.setPointCount(10);
ball.setFillColor(sf::Color::White);
ball.setPosition(sf::Vector2f(screenX/2.f, screenY/2.f));

//run prog as long as the window is open
sf::Clock clock;
bool Playing = false;
while (window.isOpen())
{
    if (Playing) 
    {   
        float deltaTime = clock.restart().asSeconds();
        float factor = deltaTime * ballSpeed;
        velocity.x = std::cos(ballAngle)*factor;
        velocity.y = std::sin(ballAngle)*factor;

        ball.move(velocity.x, velocity.y);

        if ((ball.getGlobalBounds().intersects(paddle2.getGlobalBounds())) || ball.getGlobalBounds().intersects(paddle1.getGlobalBounds()))
        {
            ball.move(-ball.move);
            velocity.x = -(velocity.x);
            ballSpeed = -ballSpeed;
            ballAngle = -ballAngle;
            soundPaddle.play();
        }


        if (ball.getGlobalBounds().intersects(topBorder.getGlobalBounds()) || ball.getGlobalBounds().intersects(botBorder.getGlobalBounds()))
        {
            velocity.x = -velocity.x;
            ballAngle = -ballAngle;
            soundWall.play();
        }

        if ((sf::Keyboard::isKeyPressed(sf::Keyboard::W)) && !(paddle1.getGlobalBounds().intersects(topBorder.getGlobalBounds()))) //only move up if not touching top border
            paddle1.move(sf::Vector2f(0, -(paddleSize.x / 2)));
        if ((sf::Keyboard::isKeyPressed(sf::Keyboard::S)) && !(paddle1.getGlobalBounds().intersects(botBorder.getGlobalBounds())))
            paddle1.move(sf::Vector2f(0, paddleSize.x / 2));

        if ((sf::Keyboard::isKeyPressed(sf::Keyboard::Up)) && !(paddle2.getGlobalBounds().intersects(topBorder.getGlobalBounds())))
            paddle2.move(sf::Vector2f(0, -(paddleSize.x / 2)));
        if ((sf::Keyboard::isKeyPressed(sf::Keyboard::Down)) && !(paddle2.getGlobalBounds().intersects(botBorder.getGlobalBounds())))
            paddle2.move(sf::Vector2f(0, paddleSize.x / 2));

        if (ball.getPosition().x > paddle2.getPosition().x + 2*ballRad)
        {
            intScore1++;
            strScore1 = std::to_string(intScore1);
            textScore1.setString(strScore1);
            ball.setPosition(sf::Vector2f(screenX / 2, screenY / 2));
            soundScore.play();
        }

        if (ball.getPosition().x < paddle1.getPosition().x - 2*ballRad)
        {
            intScore2++;
            strScore2 = std::to_string(intScore2);
            textScore2.setString(strScore2);
            ball.setPosition(sf::Vector2f(screenX/2, screenY/2));
            soundScore.play();
        }


        window.clear(sf::Color::Black);
        window.draw(paddle1);
        window.draw(paddle2);
        window.draw(ball);
        window.draw(topBorder);
        window.draw(botBorder);
        window.draw(line, 2, sf::Lines);
        window.draw(textScore1);
        window.draw(textScore2);
        window.display();
    }

    if (!Playing)
    {
        window.clear(sf::Color::Black);
        menu.draw(window);
        window.display();
    }



    //check all the window's events that were triggered since the last iteration of the loop
    sf::Event event;
    while (window.pollEvent(event))
    {

        //"close requested" event: we close the window

        switch (event.type)
        {
        case sf::Event::Closed:
            window.close();

        case sf::Event::KeyPressed:
            switch (event.key.code)
        case sf::Keyboard::Escape:
            Playing = false;
            break;

        case sf::Event::KeyReleased:  //menu controls
            switch (event.key.code)
            {
            case sf::Keyboard::Up:  
            case sf::Keyboard::W:
                menu.MoveUp();
                break;
            case sf::Keyboard::Down:
            case sf::Keyboard::S:
                menu.MoveDown();
                break;

            case sf::Keyboard::Return:
                switch (menu.GetPressedItem())
                {
                case 0:
                    Playing = true;
                    break;
                case 1:
                    //open sound menu
                    //Sound = !Sound; add to sound menu
                    break;
                case 2:
                    window.close();
                    break;
                }
                break;
            }
            break;
        }

    }

}
return EXIT_SUCCESS;
}

Menu.h

#pragma once
#include <SFML/Graphics.hpp>

#define MAX_NUMBER_ITEMS 3

class Menu
{
public:
Menu(float width, float height);
~Menu();

void draw(sf::RenderWindow &window);
void MoveUp();
void MoveDown();
int GetPressedItem() { return selectecItemIndex; }

private:
int selectecItemIndex;
sf::Font font;
sf::Text menu[MAX_NUMBER_ITEMS];
};

Menu.cpp

#include "Menu.h"



Menu::Menu(float width, float height)
{
if (!font.loadFromFile("pong.ttf"))
{  }


menu[0].setFont(font);
menu[0].setFillColor(sf::Color(128,128,128));
menu[0].setString("Play");
menu[0].setPosition(sf::Vector2f(width / 2,height /(MAX_NUMBER_ITEMS+1)*1));

menu[1].setFont(font);
menu[1].setFillColor(sf::Color::White);
menu[1].setString("Options");
menu[1].setPosition(sf::Vector2f(width / 2,height /(MAX_NUMBER_ITEMS+1)*2));

menu[2].setFont(font);
menu[2].setFillColor(sf::Color::White);
menu[2].setString("Exit");
menu[2].setPosition(sf::Vector2f(width / 2,height/(MAX_NUMBER_ITEMS+1)*3));

selectecItemIndex = 0;
}


Menu::~Menu()
{
}

void Menu::draw(sf::RenderWindow &window)
{
for (int i = 0; i < MAX_NUMBER_ITEMS; i++)
{
    window.draw(menu[i]);
}
}

void Menu::MoveUp()
{
if (selectecItemIndex - 1 >= 0)
{
    menu[selectecItemIndex].setFillColor(sf::Color::White);
    selectecItemIndex--;
    menu[selectecItemIndex].setFillColor(sf::Color(128,128,128));
}
}

void Menu::MoveDown()
{
if (selectecItemIndex + 1 < MAX_NUMBER_ITEMS)
{
    menu[selectecItemIndex].setFillColor(sf::Color::White);
    selectecItemIndex++;
    menu[selectecItemIndex].setFillColor(sf::Color(128, 128, 128));
}
}

Note I'm not quite sure of all the technical details of the menu class as it came from a slightly advanced tutorial, I'll need to go into classes in greater detail.

Answer 1

I just want to say that your code is very readable and understandable!

Things you could do better:

Indentation

The function and class definitions are not indented when they should be. I know it means that most of the code would be indented, but it adds to the readability, so please do it.

How this helps you:
It lets you separate functions more easily and lets you be able to tell wether a chunk of code is inside or outside a function.

Classes (MOST IMPORTANT)

You have too many game objects that are hard-coded in. Make a class for each: (in separate files of course)

• Paddle
• Wall
• Ball

How this helps you:
Classes add code reusability. Say you wanted to reuse your current code to make a four-player pong game (yes that exists). At the moment, you’d need to hard-code every paddle one by one. With classes, you could just add two paddles and that’s about it.

Vector

C++ programmers tend to prefer using std::vectors over c style arrays.
In the Menu class, use std::vector<sf::Text> menu; instead of sf::text menu[MAX_NUMBER_ITEMS]; so you can push menu items to it instead of putting the index of each one. Make its constructor use an std::vector<sf::Text> so you can customize it within main. Put the settings of the menu items (Font, FillColor, and Position) in a for loop and then set the first item’s special color.

How this helps you:
Say you wanted to make another menu that is used like your first menu. You could copy & paste the original menu to create a new SubMenu1 class, but that’s a big no-no in programming. Instead, you could make menu and subMenus[0] both instances of the Menu class.

Empty conditional blocks

In your code I’ve seen many statement like:

if (!font.loadFromFile("pong.ttf"))
{  }

They serve no purpose. Just write:

font.loadFromFile("pong.ttf");

(Unless you’re going to do something like throw an error or exit.)

How this helps you:
It literally just removes bloat.

Things I congratulate you for getting right:

Constants

There aren’t too many magic numbers (unnamed constants), which really helps people who want to jump in the middle of your code and still be able to understand what it does.

How this helps you:
If you abandon a project for a while (say a month), you forget what some pieces of code do. Using variable names to identify constants helps your mind understand what’s going on.

Adaptive fps

You game runs at whatever fps the computer is ready to run at.

How this helps you:
People with slow computers will be able to play the game at a normal speed, just at a lower fps.

Answer 2

Adding to what @BenjaminPhillipe has already mentioned:

1. You shouldn't include math.h, but should include cmath instead. math.h is a C header while cmath is a C++ header (with ultimately about the same content). I am somewhat astonished that your current code compiles, as you use std::cos and std::sin while the math.h is only guaranteed to put its definitions into the global namespace (cmath guarantees them in the std:: namespace).

2. As a good practice, you should order your includes alphabetically and group them into sets belonging to each other. Preferably, include the header the current file implements first (i.e A.h in A.cpp), then all headers from the project you're working on, then all headers from other projects, libraries etc. and finally the headers from the standard library. This will facilitate checks of include-correctness.

3. Speaking of headers, you're actually missing an include in main.cpp, the include for EXIT_SUCCESS which is cstdlib. However, in my opinion writing return 0; or omitting the return at the end of main are totally fine, so you could do without the macro and the include.

4. Don't use defines for constants, just declare your constants as variables. Defines are all kinds of bad, so it's better to use them as little as possible.

5. You don't need to declare and define a constructor if it doesn't do anything special.

6. Adding directly to what @BenjaminPhilippe said in his answer, you do not only need more classes, you also need to structure your code more. Currently, your main function is doing way too much. You should introduce more functions and classes to improve your code structure, make testing and debugging easier and increase reusability.

7. Disclaimer: I don't actually know anything at all about SFML. If SFML doesn't provide any callback capabilities, ignore this point.

   Polling for key events is usually not sustainable. Even in your little case, key event handling is quite a mess (including nested switch-cases). You should instead register callbacks for each key event that change some state variables somewhere (often, these variables are globals, which isn't exactly great either, but still better than drowning in a switch-case mess) and use those state variables for determining what to do.

8. Why is Playing in main capitalized? All other variables seem to start with a lowercase letter, so you should be consistent and stick with playing.

---

Conclusion

Overall, your code is not bad. The thing that bothers me the most is lack of structure, which is the core issue you should attack. I didn't spot anything that would require immediate intervention, such as UB (except for your includes, maybe), which is a great sign that your code is pretty good.