# Code Review: Breakout

This topic is 1716 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Hello Gamedev.net,

I recently finished making a breakout clone with C++ and sfml and would love to get some feedback on my rather long code. For my future projects I would like to possibly write better quality code than what I have here so would appreciate it if any of the pros would take the time to review it. I'd like to know the areas where I can improve and also anything that I am doing totally the wrong way.

I would also like some feedback on the game play, you can download the executable from here https://www.dropbox.com/s/t0vkmw6qk13r552/Eent%20Tord%20setup.rar

Here is the code:

Eent Tord.cpp

// Eent Tord.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"

int main(int argc, _TCHAR* argv[])
{
srand((unsigned int)time(0));	// Seed the random number generator

Game game;	// Initialize the game object and create a window

game.run();

return 0;
}

stdafx.h

// stdafx.h : include file for standard system include files,
// or project specific include files that are used frequently, but
// are changed infrequently
//

#pragma once

#include "targetver.h"

#include <stdio.h>
#include <tchar.h>

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

#include <memory>
#include <iostream>
#include <cstdlib>
#include <ctime>
#include <cmath>
#include <vector>
#include <fstream>
#include <sstream>

#include "Random.h"
#include "Game_Object.h"
#include "Brick.h"
#include "Ball.h"
#include "Image_Manager.h"
#include "Input_Manager.h"
#include "Level_Manager.h"
#include "Game.h"

#define SCREENWIDTH 832
#define SCREENHEIGHT 600


Game.h

#pragma once

class Game
{
public:
Game(void);
~Game(void);

void run();
void setTextures();
void update();
void drawAll();
bool isExiting();
void loseLife();
int  livesLeft();
bool isLevelWon();

enum GAMESTATE	 { uninitialized, title, about, running, paused, gamelost, gamewon, exiting };
sf::RenderWindow m_Window;
sf::Event		 m_Event;

std::vector<std::unique_ptr<Brick>>		m_Bricks;

private:
sf::VideoMode							m_VMode;
GAMESTATE								m_GameState;
Image_Manager							m_imgMgr;
Input_Manager							m_inpMgr;
Level_Manager							m_lvlMgr;
sf::Clock								m_Clock;
Ball									m_Ball;
sf::Sprite								m_Background;
sf::Sprite								m_Quit;
sf::Sprite								m_NewGame;
sf::Sprite								m_GameOver;
sf::Sprite								m_GameWon;
sf::Sprite								m_Paused;
sf::Text								m_LivesText;
sf::Font								m_Font;
};


Game.cpp

#include "stdafx.h"

Game::Game(void)
{
// Initialize the game object.
m_VMode.width		 = SCREENWIDTH;
m_VMode.height		 = SCREENHEIGHT;
m_VMode.bitsPerPixel = 32;

m_Window.create(m_VMode, "Eent Tord (Breakout/Arkanoid clone)", sf::Style::Titlebar);	// Actually creates the window.
m_GameState = uninitialized;
m_LivesText.setFont(m_Font);
m_LivesText.setCharacterSize(16);

}

Game::~Game(void)
{
}

void Game::setTextures()
{
static GAMESTATE background = uninitialized;
switch (m_GameState)
{
case title:
if (background != title)
{
m_Background.setTexture(m_imgMgr.getTitle());
m_Quit.setTexture(m_imgMgr.getQuit());
m_NewGame.setTexture(m_imgMgr.getNewGame());
m_GameOver.setTexture(m_imgMgr.getLost());
m_GameWon.setTexture(m_imgMgr.getWon());
m_Paused.setTexture(m_imgMgr.getPaused());
m_Ball.getSprite().setTexture(m_imgMgr.getBall());
}
background = title;
break;
{
m_AboutText.setString("Left and right keys for movement, escape to pause, and space to launch the ball! Avoid hitting the red bricks!");
}
break;
case running:
if (background != running && background != paused)
{
m_Ball.initialize();
m_Clock.restart();
if (!m_Bricks.empty())
{
m_Bricks.clear();
m_lvlMgr.setupLevel(m_Bricks, m_imgMgr);
}
else
m_lvlMgr.setupLevel(m_Bricks, m_imgMgr);
}
background = running;
break;
default:
break;
}
}

void Game::update()
{
switch (m_GameState)
{
case title:
m_Quit.setPosition(675.0f, 540.0f);
m_NewGame.setPosition(670.0f, 460.0f);
break;
break;
case running:
{
std::stringstream printLives;
printLives << "Lives left = ";
m_LivesText.setString(printLives.str());
}
break;
case gamelost:
m_GameOver.setPosition(350.0f, 250.0f);
break;
case gamewon:
m_GameWon.setPosition(350.0f, 250.0f);
break;
case paused:
m_Paused.setPosition(350.0f, 250.0f);
default:
break;
}
}

void Game::drawAll()
{
switch (m_GameState)
{
case title:
m_Window.draw(m_Background);
m_Window.draw(m_Quit);
m_Window.draw(m_NewGame);
break;
{
}
break;
case running:
{
m_Window.draw(m_LivesText);
m_Window.draw(m_Ball.getSprite());
for (auto iter = m_Bricks.begin(); iter != m_Bricks.end(); ++iter)
{
if ((*iter)->isAlive())
{
m_Window.draw((*iter)->getSprite());
}
}
}
break;
case gamelost:
{
m_Window.draw(m_LivesText);
m_Window.draw(m_Ball.getSprite());
for (auto iter = m_Bricks.begin(); iter != m_Bricks.end(); ++iter)
{
if ((*iter)->isAlive())
{
m_Window.draw((*iter)->getSprite());
}
}
m_Window.draw(m_GameOver);
}
break;
case gamewon:
{
m_Window.draw(m_LivesText);
m_Window.draw(m_Ball.getSprite());
for (auto iter = m_Bricks.begin(); iter != m_Bricks.end(); ++iter)
{
if ((*iter)->isAlive())
{
m_Window.draw((*iter)->getSprite());
}
}
m_Window.draw(m_GameWon);
}
break;
case paused:
{
m_Window.draw(m_LivesText);
m_Window.draw(m_Ball.getSprite());
for (auto iter = m_Bricks.begin(); iter != m_Bricks.end(); ++iter)
{
if ((*iter)->isAlive())
{
m_Window.draw((*iter)->getSprite());
}
}
m_Window.draw(m_Paused);
}
break;
default:
break;
}
}

bool Game::isLevelWon()
{
bool isWon = false;
int aliveBricks = 0;
for (auto iter = m_Bricks.begin(); iter != m_Bricks.end(); iter++)
{
if ((*iter)->getType() == "blue" && (*iter)->isAlive() == true)
{
aliveBricks++;
}
}
if(aliveBricks > 0)
return false;
else
return true;
}

void Game::run()
{
//m_Window.setFramerateLimit(60);
m_GameState = title;

while (!isExiting())	// This is the main game loop responsible for handling events, updating game logic and drawing assets.
{
m_Window.pollEvent(m_Event);

static Input_Manager::titleResult tresult;
static Input_Manager::gameResult  gresult;
static Input_Manager::gamelostResult lresult;
static Input_Manager::gamewonResult wresult;
static Input_Manager::pauseResult presult;

switch (m_GameState)
{
case title:
tresult = m_inpMgr.titleEvents(m_Event);	// Accesses the titleEvents function in input manager class and returns a result.
if (tresult == Input_Manager::exit)			// Based on the result the game state changes appropriately.
m_GameState = exiting;
else if (tresult == Input_Manager::newGame)
m_GameState = running;
else
break;
break;
aresult = m_inpMgr.aboutEvents(m_Event);	// Accesses the aboutEvents function in input manager class and returns a result.
if (aresult == Input_Manager::back)			// Based on the result the game state changes appropriately.
m_GameState = title;
else
break;
break;
case running:
gresult = m_inpMgr.gameEvents(m_Event, m_Paddle, m_Ball);		// Accesses the gameEvents function in input manager class and returns a result.
if (gresult == Input_Manager::prev)			// Based on the result the game state changes appropriately.
m_GameState = title;
else if (gresult == Input_Manager::paused)
m_GameState = paused;
else if (gresult == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == false);

else if (gresult == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == true)
m_GameState = gamewon;

else if (gresult == Input_Manager::idle && m_Paddle.livesLeft() < 0)
m_GameState = gamelost;

else
break;
break;
case gamelost:
lresult = m_inpMgr.lostEvents(m_Event);

m_GameState = title;
break;
case gamewon:
wresult = m_inpMgr.wonEvents(m_Event);
if (wresult == Input_Manager::idling);

m_GameState = title;
break;
case paused:
presult = m_inpMgr.pauseEvents(m_Event);
if (presult == Input_Manager::doNothing);

m_GameState = title;
else if (presult == Input_Manager::resume)
m_GameState = running;
break;
default:
break;
}

setTextures();		// Sets all the textures according to game states.

update();			// Updates the position of all the items on screen

m_Clock.restart();	// Reset the clock

m_Window.clear();	// Clear the window to black color

drawAll();			// Draws all the sprite members to the renderTarget.

m_Window.display();	// Finally display the window.

sf::sleep(sf::milliseconds(2));
}

m_Window.close();
}

bool Game::isExiting()
{
if (m_GameState == exiting)
return true;
else
return false;
}


Random.h

#pragma once

int Random(int a, int b);


Random.cpp

#include "stdafx.h"
#include "Random.h"

// Returns a random number in [low, high].
int Random(int low, int high)
{
return low + rand() % ((high + 1) - low);
}


Game_Object.h

#pragma once
class Game_Object
{
public:
Game_Object(void);
~Game_Object(void);

virtual void initialize();
void setTexture(sf::Texture &texture);
sf::Sprite& getSprite();
sf::Vector2f getCenter();

protected:
sf::Sprite			m_Sprite;
};



Game_Object.cpp

#include "StdAfx.h"
#include "Game_Object.h"

Game_Object::Game_Object(void)
{
}

Game_Object::~Game_Object(void)
{
}

void Game_Object::setTexture(sf::Texture &texture)
{
m_Sprite.setTexture(texture);
}

sf::Sprite& Game_Object::getSprite()
{
return m_Sprite;
}

void Game_Object::initialize()
{
}

sf::Vector2f Game_Object::getCenter()
{
return sf::Vector2f(m_Sprite.getGlobalBounds().left + m_Sprite.getGlobalBounds().width / 2, m_Sprite.getGlobalBounds().top + m_Sprite.getGlobalBounds().height / 2);
}


#pragma once

{
public:

void initialize();
void update(sf::Clock& clock);
void resetLives();
void loseLife();
int livesLeft();

float m_Speed;

private:
int	m_Lives;

};


#include "StdAfx.h"

{

}

{
}

{
m_Sprite.setPosition((SCREENWIDTH / 2) - (m_Sprite.getLocalBounds().width / 2), PADDLEYPOS);
m_Speed = 0.0f;
}

{
m_Lives = 5;
}

{
m_Lives -= 1;
/*if ( m_Lives < 0 )
m_Lives = 0;*/
}

{
return m_Lives;
}

{
m_Sprite.move(m_Speed * clock.getElapsedTime().asSeconds(), 0.0f);

if (m_Sprite.getPosition().x < 0.0f)			// If the sprite goes past the bounds of the window
m_Sprite.setPosition(sf::Vector2f(0.0f, PADDLEYPOS));	// the position is reset to keep it inside the window

else if (m_Sprite.getPosition().x + m_Sprite.getLocalBounds().width > (float)SCREENWIDTH)
}


Brick.h

#pragma once

#include "game_object.h"

class Brick : public Game_Object
{

public:
Brick(void);
Brick(sf::Texture& texture, std::string type);
~Brick(void);

void initialize();
void setPos(sf::Vector2f pos);
bool isAlive();
void kill();
std::string getType();
private:
bool alive;
std::string	m_Type;
};



Brick.cpp

#include "StdAfx.h"
#include "Brick.h"

Brick::Brick(void) : alive(true), m_Type("")
{
}

Brick::Brick(sf::Texture& texture, std::string type)
{
m_Sprite.setTexture(texture);
m_Type = type;
alive = true;
}

Brick::~Brick(void)
{
}

void Brick::initialize()
{
}

std::string Brick::getType()
{
return m_Type;
}

void Brick::setPos(sf::Vector2f pos)
{
m_Sprite.setPosition(pos);
}

bool Brick::isAlive()
{
if (alive)
return true;
else
return false;
}

void Brick::kill()
{
alive = false;
}


Ball.h

#pragma once

class Ball : public Game_Object
{
public:
Ball(void);
~Ball(void);

void initialize();
void normalizeDirectionVec();
void updatePosition(sf::Clock& clock);
bool lineIntersects(sf::Vector2f p1, sf::Vector2f p2, sf::Vector2f p3, sf::Vector2f p4);

bool			isBallMoving;
sf::Vector2f	m_Position;
sf::Vector2f	m_Direction;
private:
float			m_Speed;
sf::Vector2f	m_LastPosition;
};



Ball.cpp

#include "StdAfx.h"
#include "Ball.h"

{
}

Ball::~Ball(void)
{
}

void Ball::initialize()
{
if(isBallMoving)
isBallMoving = false;

m_Direction.x = 0.0f;
m_Direction.y = 0.0f;

int x = Random(-1, 1);		// Generates a random number between -1 and 1 so that the direction of the ball is randomly chosen
m_Direction.x = static_cast<float>(x);	// between three possible directions.
m_Direction.y = 1.0f;

normalizeDirectionVec();

m_Position.x = (SCREENWIDTH / 2) - (m_Sprite.getLocalBounds().width / 2);
m_Sprite.setPosition(m_Position);
}

void Ball::normalizeDirectionVec()
{
float len = sqrtf(m_Direction.x * m_Direction.x + m_Direction.y * m_Direction.y); // Get the length of the vector

m_Direction.x /= len;
m_Direction.y /= len;
}

{
if (isBallMoving)
{
updatePosition(clock);

if (m_Sprite.getPosition().x + m_Sprite.getLocalBounds().width > static_cast<float>(SCREENWIDTH))
{
m_Sprite.setPosition(static_cast<float>(SCREENWIDTH) - m_Sprite.getLocalBounds().width, m_Sprite.getPosition().y);
m_Direction.x = m_Direction.x * (-1);
}
else if (m_Sprite.getPosition().x  < 0.0f)
{
m_Sprite.setPosition(0.0f , m_Sprite.getPosition().y);
m_Direction.x = m_Direction.x * (-1);
}
else if (m_Sprite.getPosition().y < 0.0f)
{
m_Sprite.setPosition(m_Sprite.getPosition().x, 0.0f);
m_Direction.y *= (-1);
}
else if (m_Sprite.getPosition().y + m_Sprite.getLocalBounds().height > static_cast<float>(SCREENHEIGHT))
{
initialize();
}
else
}
}

void Ball::updatePosition(sf::Clock& clock)
{
m_Sprite.move(m_Direction.x * m_Speed * clock.getElapsedTime().asSeconds(), m_Direction.y * m_Speed * clock.getElapsedTime().asSeconds());
}

bool Ball::lineIntersects(sf::Vector2f p1, sf::Vector2f p2, sf::Vector2f p3, sf::Vector2f p4)
{
float ua = (p4.x - p3.x) * (p1.y - p3.y) - (p4.y - p3.y) * (p1.x - p3.x);
float ub = (p2.x - p1.x) * (p1.y - p3.y) - (p2.y - p1.y) * (p1.x - p3.x);
float de = (p4.y - p3.y) * (p2.x - p1.x) - (p4.x - p3.x) * (p2.y - p1.y);
bool intersects = false;

if (fabsf(de) >= 0.00001f)
{
ua /= de;
ub /= de;

if (ua >= 0.0f && ua <= 1.0f && ub >= 0.0f && ub <= 1.0f)
intersects = true;
}
return intersects;
}

{
if (m_Sprite.getGlobalBounds().top > static_cast<float>(SCREENHEIGHT) / 2.0f)	// Check the position of the ball and only check for collisions with the appropriate object
{
if (m_Direction.y > 0.0f && paddle.getSprite().getGlobalBounds().intersects(this->getSprite().getGlobalBounds()))
{
m_Direction.y *= (-1);

m_Direction.x = (getCenter().x - paddle.getCenter().x) / (paddle.getSprite().getGlobalBounds().width / 2); // Changes the direction
// of the ball depending upon where the ball hits the paddle.

normalizeDirectionVec();
}
}
else
{
sf::Vector2f centerTop(this->getSprite().getGlobalBounds().width / 2.0f, 0.0f);
sf::Vector2f centerLeft(0.0f, this->getSprite().getGlobalBounds().height / 2.0f);
sf::Vector2f centerRight(this->getSprite().getGlobalBounds().width, this->getSprite().getGlobalBounds().height / 2.0f);
sf::Vector2f centerBottom(this->getSprite().getGlobalBounds().width / 2.0f, this->getSprite().getGlobalBounds().height);
sf::Vector2f newDirection(m_Direction.x, m_Direction.y);
bool hit = false;
bool hitRed = false;

for (auto iter = bricks.begin(); iter != bricks.end(); ++iter)
{
if (!hit)
{
if ((*iter)->getSprite().getGlobalBounds().intersects(this->getSprite().getGlobalBounds()))
{
if ((*iter)->isAlive())
{
bool changed = false;
sf::Vector2f topLeft((*iter)->getSprite().getGlobalBounds().left, (*iter)->getSprite().getGlobalBounds().top);
sf::Vector2f topRight((*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width, (*iter)->getSprite().getGlobalBounds().top);
sf::Vector2f bottomLeft((*iter)->getSprite().getGlobalBounds().left, (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height);
sf::Vector2f bottomRight((*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width, (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height);

if (m_Direction.x > 0.0f && lineIntersects(m_LastPosition + centerRight, this->getSprite().getPosition() + centerRight, topLeft, bottomLeft))
{
newDirection.x *= -1.0f;
std::cout << "left" << std::endl;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
}
else if (m_Direction.x < 0.0f && lineIntersects(m_LastPosition + centerLeft, this->getSprite().getPosition() + centerLeft, topRight, bottomRight))
{
newDirection.x *= -1.0f;
std::cout << "right" << std::endl;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
}

if (m_Direction.y > 0.0f && lineIntersects(m_LastPosition + centerBottom, this->getSprite().getPosition() + centerBottom, topLeft, topRight))
{
newDirection.y *= -1.0f;
std::cout << "top" << std::endl;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
}
else if (m_Direction.y < 0.0f && lineIntersects(m_LastPosition + centerTop, this->getSprite().getPosition() + centerTop, bottomLeft, bottomRight))
{
newDirection.y *= -1.0f;
std::cout << "bottom" << std::endl;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
}

// Hit a corner
if (!changed)
{
if (m_Direction.x > 0.0f && m_Direction.y < 0.0f)
{
if (this->getSprite().getPosition().x < (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y > (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.x *= -1.0f;
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else if (this->getSprite().getPosition().x > (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y > (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else
{
newDirection.x *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
}
else if (m_Direction.x < 0.0f && m_Direction.y < 0.0f)
{
if (this->getSprite().getPosition().x > (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y > (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.x *= -1.0f;
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else if (this->getSprite().getPosition().x < (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y > (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else
{
newDirection.x *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
hitRed = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
}
else if (m_Direction.x > 0.0f && m_Direction.y > 0.0f)
{
if (this->getSprite().getPosition().x < (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y < (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.x *= -1.0f;
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else if (this->getSprite().getPosition().x > (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y < (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else
{
newDirection.x *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
}
else
{
if (this->getSprite().getPosition().x > (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y < (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.x *= -1.0f;
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else if (this->getSprite().getPosition().x < (*iter)->getSprite().getGlobalBounds().left + (*iter)->getSprite().getGlobalBounds().width / 2.0f && this->getSprite().getPosition().y < (*iter)->getSprite().getGlobalBounds().top + (*iter)->getSprite().getGlobalBounds().height / 2.0f)
{
newDirection.y *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
else
{
newDirection.x *= -1.0f;
if ((*iter)->getType() == "blue")
{
(*iter)->kill();
changed = true;
hit = true;
break;
}
else if ((*iter)->getType() == "red")
{
initialize();
hitRed = true;
break;
}
std::cout << "corner" << std::endl;
}
}
}
}
}
}

}
if(!hitRed)
m_Direction = newDirection;
m_LastPosition = this->getSprite().getPosition();
}

}


Image_Manager.h

#pragma once

class Image_Manager
{
public:
Image_Manager(void);
~Image_Manager(void);

sf::Texture& getTitle();
sf::Texture& getQuit();
sf::Texture& getNewGame();
sf::Texture& getBall();
sf::Texture& getBlue();
sf::Texture& getRed();
sf::Texture& getLost();
sf::Texture& getWon();
sf::Texture& getPaused();

private:
sf::Texture		m_Title;
sf::Image		m_Image_Quit;
sf::Texture		m_Texture_Quit;
sf::Image		m_Image_NewGame;
sf::Texture		m_Texture_NewGame;
sf::Texture		m_GameLost;
sf::Texture		m_GameWon;
sf::Image		m_Image_Ball;
sf::Texture		m_Texture_Ball;
sf::Texture		m_Texture_Blue;
sf::Texture		m_Texture_Red;
};


Image_Manager.cpp

#include "stdafx.h"

Image_Manager::Image_Manager(void)
{
}

Image_Manager::~Image_Manager(void)
{
}

{

}

sf::Texture& Image_Manager::getTitle()
{
return m_Title;
}

sf::Texture& Image_Manager::getQuit()
{
return m_Texture_Quit;
}

{
}

sf::Texture& Image_Manager::getNewGame()
{
return m_Texture_NewGame;
}

sf::Texture& Image_Manager::getLost()
{
return m_GameLost;
}

sf::Texture& Image_Manager::getWon()
{
return m_GameWon;
}

sf::Texture& Image_Manager::getPaused()
{
}

{
}

sf::Texture& Image_Manager::getBall()
{
return m_Texture_Ball;
}

sf::Texture& Image_Manager::getBlue()
{
return m_Texture_Blue;
}

sf::Texture& Image_Manager::getRed()
{
return m_Texture_Red;
}


Input_Manager.h

#pragma once

class Input_Manager
{
public:
Input_Manager(void);
~Input_Manager(void);

enum titleResult { newGame, about, exit, nothing };
enum aboutResult { noAction, back };
enum gameResult { idle, prev, paused };
enum gamewonResult { menu, idling };
enum pauseResult { theMainMenu, resume, doNothing };

titleResult titleEvents(sf::Event &Event);
gamelostResult lostEvents(sf::Event &Event);
gamewonResult wonEvents(sf::Event &Event);
pauseResult pauseEvents(sf::Event &Event);
};


Input_Manager.cpp

#include "StdAfx.h"

Input_Manager::Input_Manager(void)
{
}

Input_Manager::~Input_Manager(void)
{
}

Input_Manager::titleResult Input_Manager::titleEvents(sf::Event &Event)
{
switch(Event.type)
{
case sf::Event::Closed:
return exit;
break;
case sf::Event::KeyPressed:
switch(Event.key.code)
{
case sf::Keyboard::Escape:
return exit;
break;
default:
return nothing;
break;
}
case sf::Event::MouseButtonPressed:
if (Event.mouseButton.button == sf::Mouse::Left)		// If mouse click is inside the Quit button
{														// The application will exit.
if(Event.mouseButton.x > 675.0f
&& Event.mouseButton.x < 675.0f + 54.0f
&& Event.mouseButton.y > 540.0f
&& Event.mouseButton.y < 540.0f + 16.0f)
return exit;
else if (Event.mouseButton.x > 675.0f
&& Event.mouseButton.x < 675.0f + 64.0f			// If mouse click is inside the about button
&& Event.mouseButton.y > 500.0f					// The application will display the about screen.
&& Event.mouseButton.y < 500.0f + 22.0f)
else if (Event.mouseButton.x > 670.0f				// If mouse click is inside the new game button
&& Event.mouseButton.x < 675.0f + 108.0f		// The application will display the game screen.
&& Event.mouseButton.y > 460.0f
&& Event.mouseButton.y < 460.0f + 32.0f)
return newGame;
else
return nothing;
}
else
return nothing;
break;
default:
return nothing;
break;
}
return nothing;
}

{
switch(Event.type)
{
case sf::Event::Closed:
return back;
break;
case sf::Event::KeyPressed:
switch(Event.key.code)
{
case sf::Keyboard::Escape:
return back;
break;
default:
return noAction;
break;
}
case sf::Event::MouseButtonPressed:
if (Event.mouseButton.button == sf::Mouse::Left)
{
if(Event.mouseButton.x > 0.0f						// Clicking the mouse button anywhere inside the
&& Event.mouseButton.x < (float)SCREENWIDTH					// window will take it back to the title screen.
&& Event.mouseButton.y > 0.0f
&& Event.mouseButton.y < (float)SCREENHEIGHT)
return back;
else
return noAction;
}
default:
return noAction;
break;
}
return noAction;
}

{
switch(Event.type)
{
case sf::Event::Closed:
return prev;
break;
case sf::Event::KeyPressed:
switch(Event.key.code)
{
case sf::Keyboard::Escape:
return paused;
break;
case sf::Keyboard::Left:
if (ball.isBallMoving)
{
}
return idle;
break;
case sf::Keyboard::Right:
if (ball.isBallMoving)
{
}
return idle;
break;
case sf::Keyboard::Space:
if(!ball.isBallMoving)
ball.isBallMoving = true;
return idle;
break;
default:
return idle;
break;
}
case sf::Event::KeyReleased:
switch(Event.key.code)
{
case sf::Keyboard::Left:
return idle;
break;
case sf::Keyboard::Right:
return idle;
break;
default:
return idle;
break;
}
default:
return idle;
break;
}
return idle;
}

Input_Manager::gamelostResult Input_Manager::lostEvents(sf::Event &Event)
{
switch (Event.type)
{
case sf::Event::MouseButtonPressed:
if (Event.mouseButton.button == sf::Mouse::Left)
{
if (Event.mouseButton.x	>= 380.0f &&
Event.mouseButton.x <= 460.0f &&
Event.mouseButton.y	>= 385.0f &&
Event.mouseButton.y	<= 400.0f)
}
break;
default:
break;
}
}

Input_Manager::gamewonResult Input_Manager::wonEvents(sf::Event &Event)
{
switch (Event.type)
{
case sf::Event::MouseButtonPressed:
if (Event.mouseButton.button == sf::Mouse::Left)
{
if (Event.mouseButton.x	>= 380.0f &&
Event.mouseButton.x <= 460.0f &&
Event.mouseButton.y	>= 375.0f &&
Event.mouseButton.y	<= 385.0f)
}
break;
default:
break;
}
return idling;
}

Input_Manager::pauseResult Input_Manager::pauseEvents(sf::Event &Event)
{
switch (Event.type)
{
case sf::Event::MouseButtonPressed:
if (Event.mouseButton.button == sf::Mouse::Left)
{
if (Event.mouseButton.x	>= 385.0f &&
Event.mouseButton.x <= 445.0f &&
Event.mouseButton.y	>= 260.0f &&
Event.mouseButton.y	<= 280.0f)
return resume;
else if (Event.mouseButton.x >= 370.0f &&
Event.mouseButton.x <= 440.0f &&
Event.mouseButton.y >= 375.0f &&
Event.mouseButton.y <= 390.0f)
}
break;
case sf::Event::KeyPressed:
switch (Event.key.code)
{
case sf::Keyboard::Escape:
return resume;
}
default:
break;
}
return doNothing;
}


Level_Manager.h

#pragma once

class Level_Manager
{
public:
Level_Manager(void);
~Level_Manager(void);

void setLevel(int level);
void setupLevel(std::vector<std::unique_ptr<Brick>>& bricks, Image_Manager& m_imgMgr);
int getCurrentLevel();
int getNumberOfLevels();

private:
int				m_Current_Level;
std::vector<int>	m_brickType;
};



Level_Manger.cpp

#include "StdAfx.h"
#include "Level_Manager.h"

Level_Manager::Level_Manager(void) : m_Current_Level(1)
{

}

Level_Manager::~Level_Manager(void)
{
}

void Level_Manager::setLevel(int level)
{
m_Current_Level = level;
}

void Level_Manager::setupLevel(std::vector<std::unique_ptr<Brick>>& bricks, Image_Manager& m_imgMgr)
{
m_brickType.clear();
const int MAXBRICKS = 65;
float xPos = 0.0f, yPos = 50.0f;
const float HOFFSET = 64.0f;
const float VOFFSET = 20.0f;
const int BRICKROW = 13;
int count = 0;
std::ifstream inFile("levels/Walls.txt");
if (inFile)
{
int numLevel = 0, level = 0, type = 0;
std::string garbage = "";
inFile >> garbage >> numLevel;
inFile >> garbage >> level;
for ( int i = 0; i < MAXBRICKS; ++i )
{
inFile >> type;
m_brickType.push_back(type);
}

for (auto iter = m_brickType.begin(); iter != m_brickType.end(); ++iter)
{
std::string type = "";
if ( *iter == 1 )
{
type = "blue";
bricks.push_back(std::unique_ptr<Brick>(new Brick(m_imgMgr.getBlue(), type)));

}
else if ( *iter == 2)
{
type = "red";
bricks.push_back(std::unique_ptr<Brick>(new Brick(m_imgMgr.getRed(), type)));
}
}

for (auto iter = bricks.begin(); iter != bricks.end(); iter++)
{
(*iter)->setPos(sf::Vector2f(xPos, yPos));
xPos += HOFFSET;
count++;
if ( count % BRICKROW == 0 )
{
xPos = 0.0f;
yPos += VOFFSET;
}
}

}
else
std::cout << "Could not open file!" << std::endl;
}

int Level_Manager::getCurrentLevel()
{
return m_Current_Level;
}

int Level_Manager::getNumberOfLevels()
{
return 1;						// To be updated later!!!!
}

{
m_brickType.push_back(num);
}


Edited by frazchaudhry

##### Share on other sites

One, personal appeal is that i don't like to install games from untrusted sources.

Id much rather just download a zip or smt, scan, unzip, play.

I wish you luck, when someone reviews your game!

It is nice to read(on the eyes), and mostly its nicely structured.

A good syntax you have there, makes it so easy to skim true the code and end up where the searcher wants to be.

From first .h and .cpp file i already knew how you structure the code.

Also i would like a project file much more then a whole post of files...

There is a reason why writing code is happening in IDE instead of a regular text editor.

##### Share on other sites

I feel Image_Manager has too many variables. You could use unordered_map<string, sf::Texture> then acquire textures from it. This way you'll have only single variable and won't need to hardcode 200 textures.

Your switch() for game states is massive as well, see http://lspiroengine.com/?p=351 for decent solution.

##### Share on other sites

One, personal appeal is that i don't like to install games from untrusted sources.

Id much rather just download a zip or smt, scan, unzip, play.

I wish you luck, when someone reviews your game!

It is nice to read(on the eyes), and mostly its nicely structured.

A good syntax you have there, makes it so easy to skim true the code and end up where the searcher wants to be.

From first .h and .cpp file i already knew how you structure the code.

Also i would like a project file much more then a whole post of files...

There is a reason why writing code is happening in IDE instead of a regular text editor.

Thanks for the feedback. I have now replaced the executable with a .rar file. If I post a project file then people who do not have visual c++ express will probably not be able to view the code or they'll have to go through the process of installing it, and I do not want to force people to do that. But you're right if this was a bigger game than breakout then this really is a problem. For next time I'll probably put the code up on github and link to it so people can easily see it without any hassle. I appreciate you taking the time to view the code though. Thanks

Edited by frazchaudhry

##### Share on other sites

I feel Image_Manager has too many variables. You could use unordered_map<string, sf::Texture> then acquire textures from it. This way you'll have only single variable and won't need to hardcode 200 textures.

Your switch() for game states is massive as well, see http://lspiroengine.com/?p=351 for decent solution.

As I was coding the game this image manager class was really bugging me. As I thought that when I work on a bigger game which has hundreds of textures this solution won't really work. But I decided to go with it for this game as it was not that big of an issue for such a small game as breakout. I now realize that using your solution would have been a much smarter way of handling this even for this type of game.

I'll look into the solution that you have provided for game states as well. Hopefully I'll be able to put out a higher quality code for my next game. I really appreciate the time you took to look at the code. Thanks a lot  .

##### Share on other sites

1: Thanks for the feedback. I have now replaced the executable with a .rar file.

If I post a project file then people who do not have visual c++ express will probably not be able to view the code or they'll have to go through the process of installing it

Good response:

Also what i meant is that i don't want "Installer" of a application, i keep my computer clean of random trash.

Regarding the installed game:

I installed the game. I run it, i see application window open for like 100miliseconds and then it closes without error...

Did you test the game you upload does it work?

Regarding the code:

If i couldn't open your project file. I would just open all the .cpp and .h/.hpp files and i would work with that in my editor instead of using the internet browser...

"I like to meat with fork, do you? i can eat it with spoon like you served me, but it would just bug me..."

##### Share on other sites

I installed the game. I run it, i see application window open for like 100miliseconds and then it closes without error...
Did you test the game you upload does it work?

well that's a bummer. I actually did test it. It runs perfectly fine on my machine. Tested it on a friend's machine works fine on that as well. What OS are you running? I am using Windows 7 and my friend has Win8.

##### Share on other sites

ok so I installed it on another machine that was running win8, and I had the exact same problem that you mentioned. The window would pop up and instantly close. I right clicked on the icon and clicked "Run as administrator" and now it runs perfectly fine. I have no idea why it's like this though. Maybe someone who has any idea can shed some light on this.

There is another problem that I have noticed running this game on different computers. This game was created on my laptop that I use for work, its a core I3 2.3 GHz, with 4gb ram and an integrated graphics card. This laptop has windows 7 and a ton of programs running on it with several running in the background. When I run the game on this the framerate can get quite choppy, going to down to as low as 4 or 5 FPS, which make the game rather unplayable on occasion. Usually it runs ok with a few slowdowns that are bearable. I have another laptop that is a core I5 2.6 GHZ with 4gb ram and an Nvidia GT 525M graphics card, it is running windows 8 and has hardly any programs installed on it. The game runs absolutely fine on this, no visible slowdowns whatsoever and is smooth as silk.

Can somebody explain to me why its like this. I would imagine a game like breakout should run very smoothly on any modern computer. I'm guessing its probably the way I am handling my timestep??? or probably something wrong with the way I am handling the movement of the paddle and ball in their respective update functions. I would appreciate it if somebody could have a look at that and let me know what I am doing wrong. I am currently already working on my next project which is going to be a pacman clone, and I would like to have problems like these sorted out before I finish that one. Thanks.

##### Share on other sites

ok so I installed it on another machine that was running win8, and I had the exact same problem that you mentioned. The window would pop up and instantly close.

A exception has bean thrown and not handled properly is probably what is happening.

##### Share on other sites

A exception has bean thrown and not handled properly is probably what is happening.

If that was the case then it would not have run at all. I think it has to do with how the user's rights have been set up. It requires elevated rights and privileges to run and as such runs only when you run the program as administrator. I've tried this on several machines now some of which don't have this problem at all and those that do the game runs when I run it as administrator.

##### Share on other sites

@rip-off: Wow. I was hoping for somebody to dissect my code like that. I really appreciate the time you took to look at my game. Thanks a lot.

By placing the projects own header files in stdafx.h, you are causing the entire game to be recompiled when any header changes. This might be manageable in smaller projects, but it will not scale as your project grows. As the automatically generated comments in stdafx.h state, the file should generally be used for external headers (e.g. SFML). There are "internal" headers that it may make sense to include here, such as commonly included utility headers (random.h might be a good candidate).

I'll remember that for my next C++ project. Now that I think about it with what I have done, I have defeated the whole purpose of using pre-compiled headers. I feel like an idiot

You should not use preprocessor defines for compile time constants. Use the language feature

I have made this change, but can you tell me the downside of using preprocessor defines and what are the benefits of using compile time constants, as for this program I don't apparently see the benefit of using constants. To me the only difference I see is the program would have to allocate memory for the constants, and no memory would be allocated of the "define".

Your game class has a a number helper functions, for example setTextures(). These should be "private", not "public". Likewise, try to design your program so you do not have public fields. The bricks and window members should be private. The event member can probably be a local variable to your event handling code.

The function setTextures() and the member m_Window could have been made private, but I'm passing the brick object to another class which would then need to call some of its functions, if I set it to private the class to which the brick was passed to will not be able to call its functions and that is why I have made this public. I see how the event member should have been part of the event handling class. I'll be sure to adjust this accordingly for my next game. Thanks

You seem to have a lot of static variables. I don't think you really need any of them. It should be simple to convert them to members of the Game class. There may be better solutions.

Yes I could have made them members of the game class, but I didn't want my game class to be littered with a hundred members. So, instead I decided to make these variables local to the functions that needed them and made them static so they persist even when the function ends. These variables are not needed anywhere except the functions of they are a part of. In a much bigger project what would be a disadvantage or danger of using these static variables?

Boolean functions such as Game::isExiting can be written to return the expression, rather than write a full if/else statement:

Thanks I've made this change. This was really basic I should have known that m_GameState == exiting; will return either a true or false depending on the value in m_GameState.

If statements ending in a semicolon is highly unusual and normally sets alarm bells ringing for most programmers. Consider at least moving the semicolon to the next line and adding a comment saying "intentionally blank" or something. Better still is to avoid writing if conditions that require no logic.

Noted. But the reason I have it there was that it was possible for the lresult variable to get a value of "nada". So I explicitly included it there and told the compiler to not do anything if that happens.

Some parts of your Game class are complicated by trying to handle each case in one large if/else block. Moving sub-conditions into sub-blocks makes it much easier to read and understand:

I see, yeah that if/else block that you highlighted is pretty ugly. I've made the change like you have suggested looks a lot more readable now.

If you decouple the brick drawing from the brick type, you could probably have a vector of values rather than smart pointers.

There is a reason why I did that. My brick object has a property called "alive". I only want to draw bricks that are marked alive = true. Whenever a ball hits a brick that is breakable the bricks value is changed to alive = false. So during my for loop I have to check each brick's status. Now if you have a better solution to this problem please let me know how I can decouple the brick drawing from the brick type.

Your Game_Object class has a virtual member function, it's destructor should also be virtual. Alternatively, consider removing the virtual initialise function, instead require all game objects to be correctly initialised during construction. The base class should require the texture be passed through it's constructor, it should not be necessary to then allow clients to change the texture afterwards.

What benefit would I have for making the destructor virtual? There is nothing that I am allocating on the heap for any of the derived classes of the Game_Object class. The paddle and ball are allocated on the stack so destruction of those should be done when the stack gets destroyed. Yes I am allocating the bricks on the heap, BUT i am using smart pointers so that should take care of the destruction automatically for me right? There is no member variable in any of the derived classes that gets allocated to the heap, only the object itself (in the brick's case) gets allocated to the heap. Am I missing something here? The initialize function is different for the derived classes, it basically initializes the position back to the center of the screen for the ball and the paddle. This function gets called each time the player misses the ball or hits the red brick which also counts as a life lost. I need to call this function several times in the game, if I moved the contents of this function the the constructor then I wouldn't be able to initialize the position of the paddle and the ball each time the player misses the ball.

I'll remove the setTexture() function from the Game_Object class and actually set the texture during construction.

Sometimes code can be easier to read if you pull a common expression into a local variable. Compare:

// Original
sf::Vector2f Game_Object::getCenter(){
return sf::Vector2f(m_Sprite.getGlobalBounds().left + m_Sprite.getGlobalBounds().width / 2, m_Sprite.getGlobalBounds().top + m_Sprite.getGlobalBounds().height / 2);
}

// Local variable
sf::Vector2f Game_Object::getCenter(){
sf::FloatRect bounds = m_Sprite.getGlobalBounds();
return sf::Vector2f(bounds.left + bounds.width / 2, bounds.top + bounds.height / 2);
}


Wow, that looks really clean and is easily readable, I'll be sure to keep this in mind. I have done similar sort of this ugly stuff in several places, and it makes it hard to understand when I go back and look at it.

Your coding convention is inconsistently applied. For example, Brick::alive and Ball::isBallMoving should start with "m_" to match the other members.

Thanks for pointing that out, I have made the change.

The logic in Ball::handleCollisions is very complex and appears to contain much duplicate code. Consider splitting this into multiple functions, one of which determines if any collision occurred, another which calculates which way to bounce, and another one which handles the red/blue brick distinctions.

This function is probably the ugliest part of my game. I had a lot of problems with how I should go about handling the collision with the bricks, and my limited math knowledge only made it worse. The majority of the if else blocks are determining the various directions of the ball hitting the brick and reacting differently to where the ball hits the brick. I just couldn't come up with a better solution due to my limited math knowledge. You're right it could have been easier on the eyes if I broke it down into different functions but this was one of the last things I had to do to finish my game and I just hacked together this code, as I was losing motivation on this project fast (I wanted to move on to another project), but I forced myself to finish this game.

I spent at least three or four days on just this function alone. I am considering using Box2D to handle the physics and collision for my next projects, or I'll have to take the time and learn the math. Coming into gamedev I had no idea the amount of stuff that goes into making even the smallest of games, there is so much more that I still need to learn that it sometimes feels too overwhelming, which will probably make me go the easier route and use Box2D .

Your input manager is tightly coupled to the specifics of the game states. In one instance, it requires detailed knowledge of where the buttons are and their dimensions. This is exacerbated by the fact that magic numbers are used here, rather than named constants.

I'll probably make a button class for the UI stuff in later projects, where each button knows its position and will be able to tell the game when it was clicked instead of using magic numbers. What would be a better way to handle the input?

Your level loading code computes the positions for each brick as it loads individual bricks. This could probably be moved into a separate loop that is run after the individual bricks are instantiated. In fact, your current code contains a bug because the "count" variable in initialised at the start of the function, but it gets incremented many times as the two loops run. "yPos", and to a lesser extend "xPos",

The loop is separate. It appears to be inside the previous loop because the indentation is messed up. The count variable is also fine. It is used in only the second loop, the first loop ends before the second one starts. I have a const variable called BRICKROW = 13, which means that I can only have 13 bricks in one row, so after the 13th brick has been set in a row the yPos should be incremented with the yOffset so new row could be made and the xPos is initialized back to 0.0f. I do this by using that count variable that I have declared before the loop starts, when each brick is set, I increment count by 1, when count reaches 13, then the if block (if (count % BRICKROW == 0)) kicks in and the xpos and ypos are adjusted for the new row. The next time the new row will be made when count reaches 26 because the remainder will then be 0 again when dividing 26 by 13, after that the new row will be made when the count reaches 39 so on and so forth. It should not cause a bug.

I could have done this a little differently and reset the count back to 0 when the new row was made but then I would have had to change the if block to ( if (count == BRICKROW)). Both of these solutions do the same thing. Now unless I am totally missing something please let me know what it is I need to be doing.

Your level loading code does not handle errors very well. Ideally, your code should be able to report to the user if the file is missing, unreadable or corrupt. At the moment, your code prints a log message in the first case, but it appears that the program would still try to run without any bricks. If the file itself is corrupt, then you'll get very strange results.

You're right, I am aware of this. I should be throwing an exception here and then handling it accordingly, but as I have stated above I was in a hurry to just finish this game, and this part I wrote at the end so I was too lazy to do all that. I promise this won't happen in my future projects I swear .

There is no need to declare and define empty destructors, unless you wish to mark them as "virtual", or perhaps to show that the class obeys the rule of three but for some reason the destructor ends up being empty (in that case I would also consider adding a comment to this effect). Speaking of the rule of three, in larger projects consider marking classes as noncopyable where they cannot or should not have value semantics.

I wasn't the one that put them there. Visual C++ did that for me automatically, when it created the class for me. I had no intention of putting any code inside the destructors, but I decided to leave them there as I thought they aren't doing anything so no harm. I'll remove them if they get put in there automatically for all my future projects if I don't intend to use them. I had briefly read about the rule of three when I was learning C++ but I had totally forgot what it was, so thanks for reminding me, I'll keep the rule in mind next time.

For my next project I have decided to use C# and XNA, that should remove most of the C++ related problems like including headers etc. I'm already enjoying C# a lot and XNA makes a lot of the stuff real easy. Thanks for reviewing my code, I really appreciate it.

Edited by frazchaudhry

##### Share on other sites

Your Game_Object class has a virtual member function, it's destructor should also be virtual. Alternatively, consider removing the virtual initialise function, instead require all game objects to be correctly initialised during construction. The base class should require the texture be passed through it's constructor, it should not be necessary to then allow clients to change the texture afterwards.

What benefit would I have for making the destructor virtual? There is nothing that I am allocating on the heap for any of the derived classes of the Game_Object class. The paddle and ball are allocated on the stack so destruction of those should be done when the stack gets destroyed. Yes I am allocating the bricks on the heap, BUT i am using smart pointers so that should take care of the destruction automatically for me right? There is no member variable in any of the derived classes that gets allocated to the heap, only the object itself (in the brick's case) gets allocated to the heap. Am I missing something here? The initialize function is different for the derived classes, it basically initializes the position back to the center of the screen for the ball and the paddle. This function gets called each time the player misses the ball or hits the red brick which also counts as a life lost. I need to call this function several times in the game, if I moved the contents of this function the the constructor then I wouldn't be able to initialize the position of the paddle and the ball each time the player misses the ball.
I'll remove the setTexture() function from the Game_Object class and actually set the texture during construction.

ok so I did a little research on my own and checked my game of how the objects were being destroyed and it is fine. However, you were 100% correct, I should be using a virtual destructor in this case nonetheless. My ignorance in this could have easily resulted in a memory leak or the objects not being destroyed properly. I was lucky I was not upcasting my brick class pointer to the game object class anywhere. I was doing this:

bricks.push_back(std::unique_ptr<Brick>(new Brick(m_imgMgr.getBlue(), type)));


If I had done something like this:

std::unique_ptr<Game_Object> pGame_Object = std::unique_ptr<Game_Object>(new Brick());


then I would have been in trouble. Because the unique pointer will not properly destroy both objects. This following code WILL properly destroy both objects

std::shared_ptr<Game_Object> pGame_Object = std::shared_ptr<Game_Object>(new Brick());


It seems the shared pointer will destroy the objects properly even without the virtual destructor. If I want to use unique pointers like in my case and I have for example a base class called enemy and several derived classes, and I have a container of enemy class unique pointers and I have filled it up by upcasting then I NEED vitual destructors, or I would need to use shared pointers instead. I could have run into this problem also if I had a factory class responsible for instantiating these classes.

For my own understanding I did a test and I'm sharing it here so other people like me can benefit too. Consider the following snippet of code:

#include <iostream>
#include <memory>

class Tiger
{
public:
Tiger() {}
virtual void talk() { std::cout << "I am just a tiger!" << std::endl; }
~Tiger() { std::cout << "Tiger is destroyed!" << std::endl; }
};

class BengalTiger : public Tiger
{
public:
BengalTiger() {}
void talk() { std::cout << "I am a Bengal Tiger!" << std::endl; }
~BengalTiger() { std::cout << "Bengal tiger is destroyed" << std::endl; }
};

int main()
{
std::unique_ptr<Tiger> pT = std::unique_ptr<Tiger>(new BengalTiger());

pT->talk();

return 0;
}


Im upcasting the Bengal Tiger unique pointer to a Tiger unique pointer, and don't have a vitual destructor. The output is:

I am a Bengal Tiger!
Tiger is destroyed!


The object was not destroyed properly "Bengal tiger is destroyed" didn't show up.

Changing the unique pointer to a shared pointer in main():

std::shared_ptr<Tiger> pT = std::shared_ptr<Tiger>(new BengalTiger());


I still don't have a virtual destructor, but object will be destroyed properly and the output should now be:

I am Bengal Tiger!
Bengal tiger is destroyed
Tiger is destroyed!


Now if I change the shared pointer back to a unique pointer and make destructor virtual I'll get the same output as the one I got by using shared pointers and the object will be destroyed properly.

Thanks @rip-off for pointing this out, it seems one needs to be careful in C++ even when dealing with smart pointers.

##### Share on other sites

@rip-off: Wow. I was hoping for somebody to dissect my code like that. I really appreciate the time you took to look at my game. Thanks a lot.

You are most welcome.

I have made this change, but can you tell me the downside of using preprocessor defines and what are the benefits of using compile time constants, as for this program I don't apparently see the benefit of using constants.

The preprocessor does not understand C++. It only has a rather basic concept of token replacement. For example, a preprocessor define inside a namespace will not be restricted to replacing symbols inside that namespace.

To me the only difference I see is the program would have to allocate memory for the constants, and no memory would be allocated of the "define".

C++ does not map directly to machine code. There is no particular reason why a modern compiler, in Release mode, would allocate memory for a const value unless you do something such as take it's address.

The function setTextures() and the member m_Window could have been made private, but I'm passing the brick object to another class which would then need to call some of its functions, if I set it to private the class to which the brick was passed to will not be able to call its functions and that is why I have made this public.

That isn't quite how private works. A class may pass it's private members to any other function or class. If it does so by reference, then the class or function may write directly to the "private" value. Private does not prohibit a class designer from making the decision to share it's member with code it "trusts". It could also publish a member function pointer to a private function.

What private is intended for is to ensure that members and functions the class designer does want to keep "secret" cannot legally be accessed by other portions of the code. One reason for this is that most classes establish invariants.

There are strong invariants such as the standard containers, where if some code decided to change on of the members it could break the class (and usually the program!). A slightly "weaker" form of invariant might be covering the "business logic" or "game mechanics". That is, you might want to enforce a rule such as the player can only have so many items in their inventory, or that to obtain a new item from a shopkeeper the player must lose the corresponding number of coins from their pouch. If these kinds of code can be hidden behind a class interface that does not allow the logic to be misused, then the chances of a bug in the game logic is greatly reduced - and when one does occur it is most likely in that classes implementation.

Yes I could have made them members of the game class, but I didn't want my game class to be littered with a hundred members. So, instead I decided to make these variables local to the functions that needed them and made them static so they persist even when the function ends. These variables are not needed anywhere except the functions of they are a part of.

I believe the static variables in Game::setTextures() and Game::run() could be made non-static local variables instead.

For example:


switch (m_GameState)
{
case title:
{
Input_Manager::titleResult result = m_inpMgr.titleEvents(m_Event);
if (result == Input_Manager::exit)
m_GameState = exiting;
else if (result == Input_Manager::newGame)
m_GameState = running;
}
break;
{
if (result == Input_Manager::back)
m_GameState = title;
}
break;
case running:
{
Input_Manager::gameResult result = m_inpMgr.gameEvents(m_Event, m_Paddle, m_Ball);
if (result == Input_Manager::prev)
m_GameState = title;
else if (result == Input_Manager::paused)
m_GameState = paused;
else if (result == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == false)
/* nothing to do... */;
else if (result == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == true)
m_GameState = gamewon;

else if (result == Input_Manager::idle && m_Paddle.livesLeft() < 0)
m_GameState = gamelost;
}
break;
case gamelost:
{
Input_Manager::gamelostResult result = m_inpMgr.lostEvents(m_Event);
/* nothing to do... */;
m_GameState = title;
}
break;
case gamewon:
{
Input_Manager::gamewonResult result = m_inpMgr.wonEvents(m_Event);
if (result == Input_Manager::idling)
/* nothing to do... */;
m_GameState = title;
}
break;
case paused:
{
Input_Manager::pauseResult result = m_inpMgr.pauseEvents(m_Event);
if (result == Input_Manager::doNothing)
/* nothing to do... */;
m_GameState = title;
else if (result == Input_Manager::resume)
m_GameState = running;
}
break;
default:
break;
}



Adding a block inside each case, I've created a separate scope for each result variable of the appropriate type. At this point, it becomes quite clear that each case can be written as a helper function which can change the current gamestate:


void Game::handleTitleEvents() {
Input_Manager::titleResult result = m_inpMgr.titleEvents(m_Event);
if (result == Input_Manager::exit)
m_GameState = exiting;
else if (result == Input_Manager::newGame)
m_GameState = running;
}

if (result == Input_Manager::back)
m_GameState = title;
}

viod Game::handleRunningEvents() {
Input_Manager::gameResult result = m_inpMgr.gameEvents(m_Event, m_Paddle, m_Ball);
if (result == Input_Manager::prev)
m_GameState = title;
else if (result == Input_Manager::paused)
m_GameState = paused;
else if (result == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == false)
/* nothing to do... */;
else if (result == Input_Manager::idle && m_Paddle.livesLeft() > 0 && isLevelWon() == true)
m_GameState = gamewon;
else if (result == Input_Manager::idle && m_Paddle.livesLeft() < 0)
m_GameState = gamelost;
}

void Game::handleGameLostEvents() {
Input_Manager::gamelostResult result = m_inpMgr.lostEvents(m_Event);
/* nothing to do... */;
m_GameState = title;
}

void Game::handleGameWonEvents() {
Input_Manager::gamewonResult result = m_inpMgr.wonEvents(m_Event);
if (result == Input_Manager::idling)
/* nothing to do... */;
m_GameState = title;
}

void Game::handlePausedEvents() {
Input_Manager::pauseResult result = m_inpMgr.pauseEvents(m_Event);
if (result == Input_Manager::doNothing)
/* nothing to do... */;
m_GameState = title;
else if (result == Input_Manager::resume)
m_GameState = running;
}



switch (m_GameState)
{
case title: handleTitleEvents(); break;
case running: handleRunningEvents() break;
case gamelost: handleGameLostEvents(); break;
case gamewon: handleGameWonEvents(); break;
case paused: handlePausedEvents(); break;
default:
assert(false && "Unknown gamestate");
break;
}



In a much bigger project what would be a disadvantage or danger of using these static variables?

If you ever start using multiple threads then the static variables become a hazard, as they are effectively globals shared by any threads which happen to call that function. Aside from that, static variable suffer many of the downsides of global variables. It is harder to understand and reason about a function which makes use of hidden, global state. The function becomes hard to unit test in isolation because you cannot control the value of the static variable explicitly during each test.

Noted. But the reason I have it there was that it was possible for the lresult variable to get a value of "nada". So I explicitly included it there and told the compiler to not do anything if that happens.

It is unnecessary to instruct the compiler to do nothing in this case. The compiler will only do what you tell it to do. Now, there is a reasonable argument to be made that this is for the benefit of the code maintainers, you're showing that you are aware of this case but do not need to take any special steps. I would use a comment to make this crystal clear, as the first instinct of most programmers reading if(condition); is that the author has omitted some code from the branch body.

There are a couple of ways of making the intent clearer, for example:


case gamelost:
lresult = m_inpMgr.lostEvents(m_Event);
m_GameState = title;
} else {
// lresult == Input_Manager::nada, nothing to do...
}
break;



You can see I used such a mechanism earlier in this post.

There is a reason why I did that. My brick object has a property called "alive". I only want to draw bricks that are marked alive = true. Whenever a ball hits a brick that is breakable the bricks value is changed to alive = false. So during my for loop I have to check each brick's status. Now if you have a better solution to this problem please let me know how I can decouple the brick drawing from the brick type.

One way is to have two classes. One is just the Brick data, and another knows how to draw bricks:

class Brick {
public:
bool isAlive() const;
BrickType getType() const;
// ...
};

class BrickRenderer {
public:
BrickRenderer(TextureHandle red, TextureHandle blue);

// Note: Bricks are value objects!
void render(const std::vector<Brick> &bricks, Window &window);
private:
Sprite sprite;
Texture red;
Texture blue;
};

void BrickRenderer::render(const std::vector<Brick> &bricks) {
for(auto i = bricks.cbegin() ; i != bricks.cend() ; ++i) {
Brick &brick = bricks;
if(brick.isAlive()) {
switch(brick.getType()) {
case BrickType_Red:
sprite.setTexture(red);
break;
case BrickType_Blue:
sprite.setTexture(blue);
break;
default:
assert(false && "No such brick type!");
break;
}
sprite.setPosition(brick.getPosition());
window.draw(sprite);
}
}
}


It might be possible to render a texture directly to the screen at a given co-ordinate, but I haven't used SFML so the above is psuedo-code.

Of course, the other convenience functionality that Sprite provides might make it worthwhile to keep the per-Brick sprite. It isn't a huge deal here, but the basic idea can have a lot of value when the number of objects gets very high (e.g. particle systems).

This function is probably the ugliest part of my game. I had a lot of problems with how I should go about handling the collision with the bricks, and my limited math knowledge only made it worse. The majority of the if else blocks are determining the various directions of the ball hitting the brick and reacting differently to where the ball hits the brick. I just couldn't come up with a better solution due to my limited math knowledge. You're right it could have been easier on the eyes if I broke it down into different functions but this was one of the last things I had to do to finish my game and I just hacked together this code, as I was losing motivation on this project fast (I wanted to move on to another project), but I forced myself to finish this game.

I spent at least three or four days on just this function alone. I am considering using Box2D to handle the physics and collision for my next projects, or I'll have to take the time and learn the math. Coming into gamedev I had no idea the amount of stuff that goes into making even the smallest of games, there is so much more that I still need to learn that it sometimes feels too overwhelming, which will probably make me go the easier route and use Box2D .

That is fair enough.

What would be a better way to handle the input?

There are a number of solutions. It is nice to decouple input detection from the input "response". A simple structure which has served me well for small games, create a separate class for each game state with a basic interface for interacting with a given state:

class State {
virtual ~State() {}

virtual void render(/* ... */) = 0;
virtual void update(/* ... */) = 0;
virtual void handleEvent(const Event &event) = 0;
};


With such a structure, your top level object will have a pointer to the active state, and can poll for events, passing them to the active state. You probably no longer need an "Input Manager" with such an approach.

The loop is separate. It appears to be inside the previous loop because the indentation is messed up. ...

My bad! I was confused that it worked at all, but I didn't think about it too much, I figured that your data made it appear to work.

ok so I did a little research on my own and checked my game of how the objects were being destroyed and it is fine. However, you were 100% correct, I should be using a virtual destructor in this case nonetheless.

Yes, it is just a common C++ rule to say that any class which has a virtual function must also have a virtual destructor. It is a safety net that catches any cases where an object is destroyed through a pointer to a parent type.

##### Share on other sites

@rip-off: well those are some very good ideas, but you have opened my eyes to the fact that I really need to get educated on good software practices. I've decided to hold off on my next project and concentrate on a ton of reading for a few weeks. Apart from learning more about state machines and handling input I've got this book called Code Complete A practical handbook of software construction, which I think I should finally give a read. Learning some syntax and doing some tutorials on some graphics API are just not gonna cut it. Hopefully the reading will educate me on good software engineering practices which I will then try to incorporate into my future projects. Thanks for the feedback.

##### Share on other sites

@rip-off: well those are some very good ideas, but you have opened my eyes to the fact that I really need to get educated on good software practices. I've decided to hold off on my next project and concentrate on a ton of reading for a few weeks. Apart from learning more about state machines and handling input I've got this book called Code Complete A practical handbook of software construction, which I think I should finally give a read. Learning some syntax and doing some tutorials on some graphics API are just not gonna cut it. Hopefully the reading will educate me on good software engineering practices which I will then try to incorporate into my future projects. Thanks for the feedback.

While this is always a good idea I just wanted to say the best way to get better at code design is to just keep coding. This is because their is no one way to do things. Though it will be a good idea to read about but try to implement it also. Trust me, no matter how much you read your first couple projects will not be the best code design. Professionals still modify some things from game to game and engine to engine.

So read and try to write down something's and see if you can draw it out. Then code. At this point on your first couple projects you should be thinking more about writing clean code than the structure of the program. Of you do this before you know it you'll be doing some good code design without even knowing it.

##### Share on other sites

I concur, keep writing programs. You will only know when to apply which software practises when you build up a working knowledge of how larger and larger programs are written.

An important thing is not to get too bogged down in producing "perfect" software, to the point where your ability to complete projects is degraded. It is something that I, and I would guess many, hobby developers struggle with. There is a delicate balance between being a productive programmer (in terms of the project end goals, not mere lines of code!) while simultaneously keeping the code manageable. It is only by experiencing the problems caused by being unproductive due to spending too much time making your code "perfect". and the problems caused by being unable to be productive due to unaddressed issues in the code, that you will learn when and how to strike this balance.

After several years working as a programmer, and many more as a hobbyist, I am still learning.