Hello,

I have just started game development and for starters, I chose to practice with SFML and C++. I know a good bit of programming but not sure if my code is always efficient. So, I read this http://www.gamedev.net/page/resources/_/technical/game-programming/your-first-step-to-game-development-starts-here-r2976 and accordingly, I made my first pong game using SFML2.3.2 and C++ using some references. But as the article tells, we need to post our project for code review and then refactor it. So accordingly, I am posting my pong game here for review.

Here is a preview of the game in action:

And about the source, here is the structure:

----- pong project

---------- sources

--------------- Main.cpp

---------- headers

--------------- Game.h

--------------- Ball.h

--------------- Paddle.h

--------------- WindowObject.h

---------- resources

--------------- ball.png

--------------- bounce.ogg

--------------- paddle_red.png

--------------- paddle_blue.png

And the respective code:

**************************************** sources/Main.cpp ****************************************

#include "../headers/Game.h"

int main()
{
    Game game;
    game.start();
    return 0;
}

**************************************** headers/Game.h *****************************************

#include <SFML/Graphics.hpp>
#include <SFML/Window.hpp>
#include <SFML/System.hpp>
#include "Ball.h"
#include "Paddle.h"

class Game
{

private:

    sf::RenderWindow pingWindow;
    const float PING_WINDOW_WIDTH = 1400.f;
    const float PING_WINDOW_HEIGHT = 1400.f * 2.f / 3.f;

    const sf::Vector2f PING_WALL_SIZE = sf::Vector2f(PING_WINDOW_WIDTH, 40);
    const sf::Color PING_WALL_COLOR = sf::Color(100, 100, 100);

    sf::RectangleShape pingWallUp;
    sf::RectangleShape pingWallDown;

    Ball* pingBall;
    Paddle* pingPaddleLeft;
    Paddle* pingPaddleRight;

    const float PING_PADDLE_MARGIN = 20.f;

public:

    Game()
    {
        pingWindow.create(sf::VideoMode(PING_WINDOW_WIDTH, PING_WINDOW_HEIGHT), "Ping");

        pingWallUp.setFillColor(PING_WALL_COLOR);
        pingWallUp.setSize(PING_WALL_SIZE);
        pingWallUp.setPosition(sf::Vector2f(0, 0));

        pingWallDown.setFillColor(PING_WALL_COLOR);
        pingWallDown.setSize(PING_WALL_SIZE);
        pingWallDown.setPosition(sf::Vector2f(0, PING_WINDOW_HEIGHT - PING_WALL_SIZE.y));

        pingBall = new Ball(PING_WALL_SIZE.y);
        pingBall->setPosition(sf::Vector2f(PING_WINDOW_WIDTH/2 - pingBall->getSize().x/2, PING_WINDOW_HEIGHT/2 - pingBall->getSize().y/2));
        pingBall->setParentWindow(&pingWindow);

        pingPaddleLeft = new Paddle(Paddle::Color::Red, PING_WALL_SIZE.y);
        pingPaddleLeft->setPosition(sf::Vector2f(PING_PADDLE_MARGIN, PING_WINDOW_HEIGHT/2 - pingPaddleLeft->getSize().y/2));
        pingPaddleLeft->setParentWindow(&pingWindow);

        pingPaddleRight = new Paddle(Paddle::Color::Blue, PING_WALL_SIZE.y);
        pingPaddleRight->setPosition(sf::Vector2f(PING_WINDOW_WIDTH - pingPaddleRight->getSize().x - PING_PADDLE_MARGIN, PING_WINDOW_HEIGHT/2 - pingPaddleRight->getSize().y/2));
        pingPaddleRight->setParentWindow(&pingWindow);
    }

    void start()
    {
        while(pingWindow.isOpen())
        {
            processEvents();
            update();
            render();
        }
    }

private:

    void processEvents()
    {
        sf::Event event;
        while(pingWindow.pollEvent(event))
        {
            switch(event.type)
            {
            case sf::Event::Closed:
                pingWindow.close();
                break;
            case sf::Event::KeyPressed:
                handleKeyboard(event.key.code, true);
                break;
            case sf::Event::KeyReleased:
                handleKeyboard(event.key.code, false);
                break;
            default:
                break;
            }
        }
    }

    void update()
    {
        if(checkCollision(pingPaddleLeft->getBounds(), pingBall->getBounds()))
            pingBall->setHasCollidedPaddle(true, pingPaddleLeft->getVelocity());
        if(checkCollision(pingPaddleRight->getBounds(), pingBall->getBounds()))
            pingBall->setHasCollidedPaddle(true, pingPaddleRight->getVelocity());
        pingBall->run();
        pingPaddleLeft->run();
        pingPaddleRight->run();
    }

    void render()
    {
        pingWindow.clear(sf::Color(128, 128,128));

        pingWindow.draw(pingWallUp);
        pingWindow.draw(pingWallDown);

        pingBall->draw();
        pingPaddleLeft->draw();
        pingPaddleRight->draw();

        pingWindow.display();
    }

    void handleKeyboard(sf::Keyboard::Key key, bool isPressed)
    {
        switch(key)
        {
        case sf::Keyboard::W:
            pingPaddleLeft->setIsMovingUp(isPressed);
            break;
        case sf::Keyboard::S:
            pingPaddleLeft->setIsMovingDown(isPressed);
            break;
        case sf::Keyboard::Up:
            pingPaddleRight->setIsMovingUp(isPressed);
            break;
        case sf::Keyboard::Down:
            pingPaddleRight->setIsMovingDown(isPressed);
            break;
        default:
            break;
        }
    }

    bool checkCollision(sf::FloatRect rect1, sf::FloatRect rect2)
    {
        return rect1.intersects(rect2);
    }

};

**************************************** headers/Ball.h ****************************************

#include <SFML/Graphics.hpp>
#include <SFML/Window.hpp>
#include <SFML/System.hpp>
#include <SFML/Audio.hpp>
#include "WindowObject.h"
#include <iostream>


class Ball
: public WindowObject
{

private:

    sf::Texture obBallTexture;
    sf::Sprite obBall;
    sf::Clock lastCollision;

    sf::SoundBuffer ballBounceBuffer;
    sf::Sound ballBounce;

    float hVelocity = 0;
    float vVelocity = 0;

    //const float MAX_VELOCITY = 1.5f;
    //const float MIN_VELOCITY = 0.7f;

    float wallMargin;

    sf::Clock randomClock1;
    sf::Clock randomClock2;
    sf::Clock randomClock3;

    bool hasCollidedPaddle = false;

public:

    Ball()
    {

    }

    Ball(float newWallMargin)
    {
        obBallTexture.loadFromFile("resources/ball.png");
        obBall.setTexture(obBallTexture);

        ballBounceBuffer.loadFromFile("resources/bounce.ogg");
        ballBounce.setBuffer(ballBounceBuffer);

        hVelocity = randomVelocity(randomClock1.restart().asMilliseconds());
        vVelocity = randomVelocity(randomClock2.restart().asMilliseconds());

        std::cout << hVelocity << " " << vVelocity << std::endl;

        wallMargin = newWallMargin;
    }

    void setPosition(sf::Vector2f newPosition)
    {
        obBall.setPosition(newPosition);
    }

    void draw()
    {
        parentWindow->draw(obBall);
    }

    sf::Vector2u getSize()
    {
        return obBallTexture.getSize();
    }

    void run()
    {
        if(hasCollidedWall())
        {
            vVelocity = -vVelocity;
            hVelocity *= 1.4;
            vVelocity /= 1.4;
            ballBounce.play();
        }
        if(hasCollidedPaddle)
        {
            hVelocity = -hVelocity;
            ballBounce.play();
        }

        hasCollidedPaddle = false;
        obBall.move(sf::Vector2f(hVelocity * 3 / 2, vVelocity));
    }

    void setHasCollidedPaddle(bool newHasCollidedPaddle, float paddleYVelocity)
    {
        if(lastCollision.getElapsedTime().asSeconds() >= 1)
        {
            hasCollidedPaddle = newHasCollidedPaddle;
            vVelocity += paddleYVelocity / 5;
            hVelocity /= 1.6;
            lastCollision.restart();
        }
    }

    sf::FloatRect getBounds()
    {
        return obBall.getGlobalBounds();
    }

private:

    bool hasCollidedWall()
    {
        return (obBall.getPosition().y <= wallMargin) || (obBall.getPosition().y >= parentWindow->getSize().y - wallMargin - obBallTexture.getSize().y);
    }

    float randomVelocity(int seed)
    {
        float finalVelocity;
        srand(seed + randomClock3.restart().asMilliseconds());
        int reducedSize = (rand()%4)+5;
        finalVelocity = (float)reducedSize / 10;
        return finalVelocity;
    }

};

**************************************** headers/Paddle.h ****************************************

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>
#include "WindowObject.h"

class Paddle
: public WindowObject
{

private:

    sf::Texture obPaddleTexture;
    sf::Sprite obPaddle;

    const float SPEED = 1.f;

    bool isMovingUp = false;
    bool isMovingDown = false;

    float wallMargin;

public:

    enum Color {Red, Blue};

    Paddle()
    {

    }

    Paddle(Color paddleColor, float newWallMargin)
    {
        if(paddleColor == Color::Blue)
            obPaddleTexture.loadFromFile("resources/paddle_blue.png");
        if(paddleColor == Color::Red)
            obPaddleTexture.loadFromFile("resources/paddle_red.png");

        obPaddle.setTexture(obPaddleTexture);
        wallMargin = newWallMargin;
    }

    void setPosition(sf::Vector2f newPosition)
    {
        obPaddle.setPosition(newPosition);
    }

    void draw()
    {
        parentWindow->draw(obPaddle);
    }

    sf::Vector2u getSize()
    {
        return obPaddleTexture.getSize();
    }

    void run()
    {
        if(isMovingUp && isSpaceUp())
            obPaddle.move(sf::Vector2f(0.f, -SPEED));

        if(isMovingDown && isSpaceDown())
            obPaddle.move(sf::Vector2f(0.f, +SPEED));
    }

    void setIsMovingUp(bool newIsMovingUp)
    {
        isMovingUp = newIsMovingUp;
    }

    void setIsMovingDown(bool newIsMovingDown)
    {
        isMovingDown = newIsMovingDown;
    }

    sf::FloatRect getBounds()
    {
        return obPaddle.getGlobalBounds();
    }

    float getVelocity()
    {
        return SPEED * (isMovingDown ? 1 : -1);
    }

private:

    bool isSpaceDown()
    {
        return obPaddle.getPosition().y <= parentWindow->getSize().y - wallMargin - obPaddleTexture.getSize().y;
    }

    bool isSpaceUp()
    {
        return obPaddle.getPosition().y >= wallMargin;
    }

};

**************************************** headers/WindowObject.h ****************************************

#ifndef __WindowObject__
#define __WindowObject__

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

class WindowObject
{

protected:

    sf::RenderWindow* parentWindow;

public:

    void setParentWindow(sf::RenderWindow* newParentWindow)
    {
        parentWindow = newParentWindow;
    }

};

#endif // __WindowObject__

**************************************** resources/ball.png **************************************************

**************************************** resources/paddle_red.png ****************************************

**************************************** resources/paddle_blue.png ***************************************

**************************************** resources/bounce.ogg *********************************************

This is just the sound of a ball colliding with the wall and paddle.

*******************************************************************************************************************

Okay, so thats the complete source and I looking forward to improve the code, physics, graphics(if possible), and any additional advice you give.

I don't have a great deal to say as it is Pong and I think sometimes it's better to get the job done rather than making something that is architecturally perfect and can be expanded in amazing ways. For Pong I am a fan of having the ball itself do the work (Ball: Did I hit a paddle? in that case I should change direction) but perhaps a better method would be something of a higher level doing that job.

Is all your code in headers? You should split the code into declaration and implementation (.h/.cpp) this isn't a major issue and having it all in header is easier to paste so that I appreciate :P but splitting does have quite a few benefits particularly as your projects grow.

Ball() doesn't initialise anything (same for Paddle), I see you have initialised a few things when they are declared but not all (namely wallMargin). I don't know the SFML stuff so check default values are fine with those if you haven't already.

As far as some of your methods go I think you should avoid these types of methods:

setIsMovingUp

setHasCollidedPaddle

Or at least rename them more appropriately. Having 'Set' type methods is fine but avoid using them when they are doing behaviour type things. For example, setIsMovingUp should be more

along the lines of MoveUp if that makes sense. Tell the object to do things rather than just indirectly accessing it's inner values. You should find that it tends to make refactoring easier later on.

Is all your code in headers? You should split the code into declaration and implementation (.h/.cpp) this isn't a major issue and having it all in header is easier to paste so that I appreciate :P but splitting does have quite a few benefits particularly as your projects grow.

In particular, it is really important to put code in implementation files so that it gets parsed once instead of being parsed every time you include the header.

When your project will grow the compilation time will grow exponentially.

Only functions with few statements should be declared inline inside the headers.

Is all your code in headers? You should split the code into declaration and implementation (.h/.cpp) this isn't a major issue and having it all in header is easier to paste so that I appreciate :P but splitting does have quite a few benefits particularly as your projects grow.

In particular, it is really important to put code in implementation files so that it gets parsed once instead of being parsed every time you include the header.

When your project will grow the compilation time will grow exponentially.

Only functions with few statements should be declared inline inside the headers.

Okay, I was told about this indeed first also, while studying c++ and all. But, I thought that for such a small project its not important, but I think now that it is very important, even in the book from which I studied SFML, they were putting all the code in implementation files. I will surely start doing this as a way to improve my habit for later game development.

Ball() doesn't initialise anything (same for Paddle), I see you have initialised a few things when they are declared but not all (namely wallMargin). I don't know the SFML stuff so check default values are fine with those if you haven't already.

Yeah, that's one thing I forgot, to initialize wallMargin. I will surely do that for improving habits.

As far as some of your methods go I think you should avoid these types of methods:

setIsMovingUp

setHasCollidedPaddle

Or at least rename them more appropriately. Having 'Set' type methods is fine but avoid using them when they are doing behaviour type things. For example, setIsMovingUp should be more

along the lines of MoveUp if that makes sense. Tell the object to do things rather than just indirectly accessing it's inner values. You should find that it tends to make refactoring easier later on.

About this, I didn't exactly understand what change to do. I mean I know that the names setIsMoving Up and setHasCollidedPaddle do look awkward. But can you give me some examples or references to exactly know what change to do and what are the good practices.

Overall, What major coding or important pong game related changes can I do? Leaving aside the implementation declaration changes, what specific pong game changes can I do? Can I improve the code? Can I improve the graphics? Can I improve the math, physics code used? Thanks.