PONG XI Platinum Edition

Question

This is my first game.

This header defines the constants:

Constants.h:

#ifndef CONSTANTS_H_INCLUDED
#define CONSTANTS_H_INCLUDED

#define WIN_WIDTH 800
#define WIN_HEIGHT 600

#define BALL_SPEED_X 4
#define BALL_SPEED_MAX_Y 4

#define BAR_FROM_BORDERS 40
#define BAR_SPEED 7

#define MAX_FPS 60



#endif // CONSTANTS_H_INCLUDED

This is the main class:

PSprite.h:

#ifndef PSPRITE_H
#define PSPRITE_H

/**
    *This class inherits the class
    *Sprite, and defines some
    *attributes to keep track of
    *a Sprite's geometry.
*/

#include <SFML/Graphics.hpp>
#include "CollisionManager.h"
#include <string>
#include <vector>

class PSprite : public sf::Sprite
{
    public:
        PSprite(std::string&, const float& x, const float& y);

        const float& getDirectionX();
        const float& getDirectionY();
        const sf::Vector2f& getLowerRight();                    //Getters
        const float& getWidth();
        const float& getHeight();

        void setPosition(float,float);
        void setDirectionX(const float&);
        void setDirectionY(const float&);                       //Setters
        void setLowerRight();

        bool strictlyHigherThan(PSprite&);
        bool strictlyLowerThan(PSprite&);
        bool strictlyLeftOf(PSprite&);                          //Relative position verification
        bool strictlyRightOf(PSprite&);


        PSprite* nextMove();                                    //Returns a ptr on the sprite after its next movement

    protected:
        sf::Texture texture;                      //The sprite's texture
        CollisionManager collision;               //The Collision manager
        float directionX;                         //The direction on X axis
        float directionY;                         //The direction on Y axis
        float width;                              //The sprite's width
        float height;                             //The Sprite's height
        sf::Vector2f lowerRight;                  //Lower right corner's coordinates

};

#endif // PSPRITE_H

PSprite.cpp:

#include "PSprite.h"
#include <string>

using namespace sf;
using namespace std;

PSprite::PSprite(std::string& filename,const float& x, const float& y)
{
    texture.loadFromFile(filename);                 //Loading the texture

    setTexture(texture);                            //Setting the sprite
    setPosition(x,y);

    width=texture.getSize().x;                      //Setting width and height
    height=texture.getSize().y;

    //setLowerRight();                                //Setting the lower right corner

    directionX=0;                                   //Setting the directions to default values
    directionY=0;
}

const float& PSprite::getDirectionX()
{
    return directionX;
}

const float& PSprite::getDirectionY()
{
    return directionY;
}

const Vector2f& PSprite::getLowerRight()
{
    return lowerRight;
}

const float& PSprite::getWidth()
{
    return width;
}

const float& PSprite::getHeight()
{
    return height;
}


void PSprite::setPosition(float x,float y)
{
    Sprite::setPosition(x,y);
    setLowerRight();                    //We set the lower right corner each time we change the position
}
void PSprite::setDirectionX(const float& dirX)
{
    directionX=dirX;
}

void PSprite::setDirectionY(const float& dirY)
{
    directionY=dirY;
}

void PSprite::setLowerRight()
{
    lowerRight.x=getPosition().x+width;
    lowerRight.y=getPosition().y+height;
}


bool PSprite::strictlyHigherThan(PSprite& spr)
{
    return lowerRight.y<spr.getPosition().y;            //If the lowest point of this is higher than spr's highest
}

bool PSprite::strictlyLowerThan(PSprite& spr)
{
    return spr.strictlyHigherThan(*this);               //If spr is strictly higher than this
}

bool PSprite::strictlyLeftOf(PSprite& spr)
{
    return lowerRight.x<spr.getPosition().x;            //If the rightest point of this is on spr's leftest left
}

bool PSprite::strictlyRightOf(PSprite& spr)
{
    return spr.strictlyLeftOf(*this);                   //If spr is strictly on the left of this
}


PSprite* PSprite::nextMove()                            //Returns a pointer on this' position after its next move
{
    PSprite *next=new PSprite(*this);

    next->setPosition(getPosition().x+directionX,getPosition().y+directionY);

    return next;
}

This class manages collisions:

CollisionManager.h:

#ifndef COLLISIONMANAGER_H
#define COLLISIONMANAGER_H


/**
  *Class that manages collisions (!)
  *between 2 PSprites or a PSprite
  *and the screen borders
*/

#include <SFML/Graphics.hpp>
#include "Constants.h"
class PSprite;

class CollisionManager
{
    public:
        CollisionManager();

        bool CollBorderV(PSprite&);             //Managing collision between a PSprite and the vertical borders of the screen
        bool CollBorderH(PSprite&);             //Managing collision between a PSprite and the horizontal borders of the screen

        bool Coll2PSprite(PSprite&,PSprite&);   //Managing the collision between two PSprites

};

#endif // COLLISIONMANAGER_H

CollisionManager.cpp:

#include "CollisionManager.h"
#include "Ball.h"
#include "PSprite.h"
#include <iostream>

using namespace sf;

CollisionManager::CollisionManager()
{

}


bool CollisionManager::Coll2PSprite(PSprite& spr1, PSprite& spr2)       //Managing the collision between two PSprites
{
    PSprite *nxt=spr1.nextMove();

    if(nxt->strictlyHigherThan(spr2) || nxt->strictlyLowerThan(spr2) || nxt->strictlyLeftOf(spr2) || nxt->strictlyRightOf(spr2))    //If they don't collide after spr1's next move
    {
        delete nxt;
        return 0;
    }

    if(spr1.strictlyHigherThan(spr2))
    {
        spr1.setPosition(spr1.getPosition().x,spr2.getPosition().y-spr1.getHeight());
        //spr1.setLowerRight();
    }

    else if(spr1.strictlyLowerThan(spr2))
    {
        spr1.setPosition(spr1.getPosition().x,spr2.getLowerRight().y);
        //spr1.setLowerRight();
    }

    if(spr1.strictlyLeftOf(spr2))
    {
        spr1.setPosition(spr2.getPosition().x-spr1.getWidth(),spr1.getPosition().y);
        //spr1.setLowerRight();
    }

    else if(spr1.strictlyRightOf(spr2))
    {
        spr1.setPosition(spr2.getLowerRight().x,spr1.getPosition().y);
        //spr1.setLowerRight();
    }

    delete nxt;
    return 1;
}

bool CollisionManager::CollBorderH(PSprite& spr)
{
    PSprite *nxt=spr.nextMove();

    if(nxt->getPosition().y>0 && nxt->getLowerRight().y<WIN_HEIGHT)       //If there is no collision
    {
        delete nxt;
        return 0;
    }

    if(nxt->getPosition().y<0)                                              //If there is a collision with the upper border
    {
        spr.setPosition(spr.getPosition().x,0);
        //spr.setLowerRight();
    }

    else if(nxt->getLowerRight().y>WIN_HEIGHT)                            //If there is a collision with the lower border
    {
        spr.setPosition(spr.getPosition().x,WIN_HEIGHT-spr.getHeight());
        //spr.setLowerRight();
    }

    delete nxt;
    return 1;
}

bool CollisionManager::CollBorderV(PSprite& spr)        //Manages collisions between a PSPrite and vertical borders
{
    PSprite *nxt=spr.nextMove();

    if(nxt->getPosition().x>0 && nxt->getLowerRight().x<WIN_WIDTH)
    {
        delete nxt;
        return 0;
    }

    if(nxt->getPosition().x<0)
    {
        spr.setPosition(0,spr.getPosition().y);
    }

    else if(nxt->getLowerRight().x>WIN_WIDTH)
    {
        spr.setPosition(WIN_WIDTH-spr.getWidth(),spr.getPosition().y);
    }

    delete nxt;
    return 1;
}

This is the ball's class:

Ball.h:

#ifndef BALL_H_INCLUDED
#define BALL_H_INCLUDED


/**
  *This class inherits from the class
  *PSprite and defines the method motion
  *that indicates how the Ball moves
*/

#include <SFML/Graphics.hpp>
#include <string>
#include "Bar.h"
#include "CollisionManager.h"
#include "PSprite.h"

class Ball:public PSprite
{
    public:
        Ball(std::string,float,float);

        void motion(std::vector<PSprite*>,bool&);
};

#endif // BALL_H_INCLUDED

Ball.cpp:

#include "Ball.h"
#include <string>
#include <iostream>
#include <vector>
#include <cstdlib>
#include "Constants.h"

using namespace sf;
using namespace std;

Ball::Ball(string filename,float x, float y):PSprite(filename,x,y)
{

}

void Ball::motion(vector<PSprite*> sprites,bool& paused)
{
    for(int i=0;i<sprites.size();i++)                           //sprites contains pointers on all the sprites on screen, in this case the 2 bars
    {
        if(collision.Coll2PSprite(*this,*sprites[i]))           //If a ball collides with a PSprite it changes directionX
        {
            directionX*=-1;
            directionY=rand()%BALL_SPEED_MAX_Y;
        }
    }

    if(collision.CollBorderH(*this))                //If a ball collides with a horizontal border it changes directionY
    {
        directionY=directionY*-1;
    }

    move(directionX,directionY);                    //Making the next movement


    if(collision.CollBorderV(*this))                //If the ball collides with a vertical border we reset the game
    {
        setPosition(WIN_WIDTH/2,WIN_HEIGHT/2);
        directionY=0;

        sprites[0]->setPosition(WIN_WIDTH-BAR_FROM_BORDERS,WIN_HEIGHT/2);
        sprites[0]->setLowerRight();

        sprites[1]->setPosition(BAR_FROM_BORDERS,WIN_HEIGHT/2);
        sprites[1]->setLowerRight();

        paused=1;
    }

    setLowerRight();
}

This is the bars' class:

Bar.h:

#ifndef BAR_H
#define BAR_H

/**
   * This class Bar inherits from the class
   * PSprite and defines the method motion
   * which indicates how the bars move.
*/

#include <string>
#include <SFML/Graphics.hpp>
#include "CollisionManager.h"
#include "PSprite.h"

class Bar:public PSprite
{
    public:
        Bar(std::string,float,float);
        void motion();
};

#endif // BAR_H

Bar.cpp:

#include "Bar.h"
#include "PSprite.h"

using namespace sf;
using namespace std;

Bar::Bar(string filename,float x,float y):PSprite(filename,x,y)
{


}

void Bar::motion()
{
    if(!collision.CollBorderH(*this))       //If a bar collides with a horizontal border it can't move in its direction
    {
        move(0,directionY);
        setLowerRight();
        directionY=0;
    }
}

main.cpp:

#include <SFML/Graphics.hpp>
#include "Ball.h"
#include "Bar.h"
#include <vector>
#include "Constants.h"

using namespace sf;
using namespace std;

int main()
{

    RenderWindow app(sf::VideoMode(WIN_WIDTH, WIN_HEIGHT), "Pong XI: Tokyo Drift");       // Create the main window

    Ball ball("Sprites/ball.png",WIN_WIDTH/2,WIN_HEIGHT/2);             //Setting the ball
    ball.setDirectionX(BALL_SPEED_X);

    Bar blu("Sprites/bluBar.png",WIN_WIDTH-BAR_FROM_BORDERS,WIN_HEIGHT/2);      //Setting the bars
    Bar red("Sprites/redBar.png",BAR_FROM_BORDERS,WIN_HEIGHT/2);

    vector<PSprite*> bars;                                          //Creating the vector and adding the two bars to it
    bars.push_back(&blu);
    bars.push_back(&red);

    bool paused=1;

    bool z=0;
    bool s=0;
    bool up=0;
    bool down=0;

    Texture background;
    background.loadFromFile("Sprites/background.png");        //Setting the background
    Sprite sp_bg;
    sp_bg.setTexture(background);

    app.setFramerateLimit(MAX_FPS);
    app.setVerticalSyncEnabled(1);

    while (app.isOpen())
    {
        Event event;        // Process events
        while (app.pollEvent(event))
        {
            switch (event.type)                  // Event's type
            {
                case Event::Closed :                // Close button on the window
                    app.close();
                break;

                case Event::KeyPressed :            // Event key pressed
                {
                    switch (event.key.code)             // The pressed key
                    {
                        case Keyboard::Escape :             // Escape
                            app.close();
                        break;

                        case Keyboard::Up:
                            if(!paused)
                            {
                                up=1;
                            }
                        break;

                        case Keyboard::Down:                //To manage simultaneous key pressing we set booleans
                            if(!paused)                     //linked to the pressed buttons and later make movements
                            {                               //based on their values
                                down=1;
                            }
                        break;

                        case Keyboard::Z:
                            if(!paused)
                            {
                                z=1;
                            }
                        break;

                        case Keyboard::S:
                            if(!paused)
                            {
                                s=1;
                            }
                        break;

                        case Keyboard::Space:
                            paused=!paused;
                        break;

                        default:
                        break;
                    }
                }
                break;

                case Event::KeyReleased:
                {
                    switch (event.key.code)             // The released key
                    {
                        case Keyboard::Up:
                            if(!paused)                 //Stopping the movement when the keys are released
                            {
                                up=0;
                            }
                        break;

                        case Keyboard::Down:
                            if(!paused)
                            {
                                down=0;
                            }
                        break;

                        case Keyboard::Z:
                            if(!paused)
                            {
                                z=0;
                            }
                        break;

                        case Keyboard::S:
                            if(!paused)
                            {
                                s=0;
                            }
                        break;

                        default:
                        break;
                    }
                }
                break;

                default :
                break;
            }
        }

        if(paused)
        {
            z=0;                    //Stopping the movements if the game is paused
            s=0;
            up=0;
            down=0;
        }

        if(z)                               //Verifying the booleans to make movements
        {
            red.setDirectionY(-BAR_SPEED);
            red.motion();
        }

        if(s)
        {
            red.setDirectionY(BAR_SPEED);
            red.motion();
        }

        if(up)
        {
            blu.setDirectionY(-BAR_SPEED);
            blu.motion();
        }

        if(down)
        {
            blu.setDirectionY(BAR_SPEED);
            blu.motion();
        }

        if(!paused)
        {
            ball.motion(bars,paused);
        }

        app.clear();        // Clear screen


        app.draw(sp_bg);
        app.draw(ball);     // Draw the sprite
        app.draw(blu);
        app.draw(red);


        app.display();      // Update the window

    }

    return EXIT_SUCCESS;
}

Answer 1

1. Prefer to make the constants proper (i.e., type-safe) constants using const or constexpr instead of a C-style macro. This increases maintainability and protects from unintended errors.

2. In PSprite (and in general), make every member function const if it is not modifying the state of the object. Case in point are your getters.

3. Use the initialization list in a constructor (see e.g., PSprite). So prefer to write along the lines of:

   PSprite::PSprite(std::string& filename, const float& x, const float& y)
      : directionX(0), directionY(0)
   {
      texture.loadFromFile(filename);                 //Loading the texture
   
      setTexture(texture);                            //Setting the sprite
      setPosition(x, y);
   
      width = texture.getSize().x;                      //Setting width and height
      height = texture.getSize().y;
   }
   

Perhaps there is room for additional members to be initialized in the initialization list as well.

Also, I find your comments too verbose. They should specify more "why" and not "what". For example, if you set two member variables to zero in a constructor, it is perfectly clear they are initialized to sensible default values without an explicit comment.

4. What is the purpose of CollisionManager as a stateless class? It seems to me that you might as well make the collision detection functionality into free (non-member) functions and get rid of the unnecessary class.

5. In the constructor of Bar, pass the string as const-ref instead of by-value. Cheap-to-copy objects (like ints and floats) are typically passed by-value.

6. Avoid using "naked pointers" to avoid leaking memory. Have a look at std::shared_ptr and others for doing this properly and safely.