SDL/C++ Pong clone

This is my first time writing a game in C++/SDL and I decided to post the code here so you can tell me what I am doing wrong.

main.cpp

#include 
#include "Game.h"

int screenWidth = 640;
int screenHeight = 480;
const char* title = "Pong Clone";

Game game;

int main(int argc, char* args[])
{

game.init(title, SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, screenWidth, screenHeight, false);

while (game.running())
{
    game.eventHandler();
    game.render();
}
return 0;
}

Game.h

#ifndef GAME_H
#define GAME_H

#include 
#include 
#include 
#include "GameObject.h"

class Game
{
public:
bool init(const char* title, int xPos, int yPos, int width, int height,     bool flags);
void eventHandler();
void render();
void clean();
void collision();
void reset();

bool running() { return m_Running; }

private:
SDL_Window* window;
SDL_Renderer* renderer;

int screenWidth;
int screenHeight;

int xSpeed, ySpeed;

bool m_Running;

GameObject* paddle;
GameObject* ball;
};

#endif

Game.cpp

```
#include "Game.h"
#include
#include

//player speed
int playerVelocity = 0;
int playerSpeed = 15;

bool Game::init(const char* title, int xPos, int yPos, int width, int height, bool flags)
{
// screen and renderer initialization
screenWidth = width;
screenHeight = height;
window = SDL_CreateWindow(title, xPos, yPos, screenWidth, screenHeight, flags);
if (window == nullptr)
{
std::cout load("Assets/ball.png", "ball", renderer);
paddle->load("Assets/paddle.png", "paddle", renderer);

//starting speed and directions
xSpeed = rand() % 8 + 5;
ySpeed = rand() % 8 + 5;

reset();

m_Running = true;

return true;
}

void Game::reset()
{
// center screen position
ball->setPosition((screenWidth - (ball->getW() / 2)) / 2, (screenHeight - (ball->getH() / 2)) / 2);

}

void Game::eventHandler()
{
// ball speed
ball->setX(ball->getX() + xSpeed);
ball->setY(ball->getY() - ySpeed);

// game loop
SDL_Event event;
if (SDL_PollEvent(&event))
{
switch(event.type)
{
case

This is just a quick partial review. I can't comment on the SDL parts because I know very little about SDL.

main.cpp

Avoid global mutable state. All of the variables shown below could be made local.

int screenWidth = 640;
int screenHeight = 480;
const char* title = "Pong Clone";

Game game;

If for some reason, you want them in a global area, at least make them constants. Since these are in a .cpp file and not a .h, you can mark them to be static. This gives them internal linkage, which basically keeps them 'private' to the file. Of course, if you want to extern them, then do not do this.

static const int SCREEN_WIDTH = 640;
static const int SCREEN_HEIGHT = 480;
static const char* TITLE = "Pong Clone";

An alternative to static global variables, is putting them into an unnamed namespace.

namespace {
    const int SCREEN_WIDTH = 640;
    const int SCREEN_HEIGHT = 480;
    const char* TITLE = "Pong Clone";
}

If you ever need to put these values into a header file, I would place them into a class or named-namespace. If you do the former (put them in a class), it's okay to make them static. If you do the latter (put them in a named-namespace), do not make them static or else every translation unit will have its own private copy of the variable.

Game.h

Remove #include and #include . This particular header file doesn't use them at all.

Two-step initialization is old-fashioned (though admittingly still used). Your init() function could be turned into a constructor instead.

Game (const char* title, int xPos, int yPos, int width, int height, bool flags);

The const char* title should also be changed to const std::string &title, but I'll talk about that later.

I'm not seeing any reason why the objects below are pointers.

GameObject* paddle;
GameObject* ball;

Save yourself the headache of pointless memory management in this case and just make them objects.

GameObject paddle;
GameObject ball;

Game.cpp

Let's go back to my previous point of using std::string instead of char*.

bool Game::init(const char* title,//...
// ... More code
window = SDL_CreateWindow(title,//...

Since you are not checking to see if title is nullptr, it would be better to use const std::string &title instead.

If you choose to keep init() instead of creating a constructor, then I'm guessing these two if-statements should return false.

window = SDL_CreateWindow(title, xPos, yPos, screenWidth, screenHeight, flags);
if (window == nullptr)
{
    std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
    return false; // probably?
}

renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
if (renderer == nullptr)
{
    std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
    return false; // probably?
}

Checking error codes is also somewhat old fashioned. I would recommend throwing exceptions instead.

I see that you are using rand() right here.

//starting speed and directions
xSpeed = rand() % 8 + 5;
ySpeed = rand() % 8 + 5;

I do not see you using srand() anywhere though. Without srand(), your random number generator seed will always be the same and you will always get the same results.

Even better, since you are using C++11, I would recommend using functions from the new header. Since you are looking for a random number in the range [5,12], here is a rough example:

// Be sure to #include 
auto seed = std::chrono::system_clock::now ().time_since_epoch ().count () ;

// Be sure to #include  
typedef std::default_random_engine::result_type seed_type ;
std::default_random_engine generator (static_cast  (seed)) ;
std::uniform_int_distribution  distribution (5, 12) ;

xSpeed = distribution (generator) ;
ySpeed = distribution (generator) ;

You would probably want to create the std::default_random_engine` only once though, or else you'll be constantly reseeding it.

StackExchange Code Review Q#47221, answer score: 4