First project (pong) code review

127

Author

August 03, 2014 11:28 AM

Hi guys. I recently made a Pong game in order to practice my programming skills and was wondering if any of you would like to do a quick code review for me. This is my first attempt to make a game so there are certainly quite a lot of things I'll need to improve on and any amount of advice would be appreciated. I'm using C++ with SFML 2.1.

Code:

main.cpp

[spoiler]


//Pong

#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>
#include <iostream>
#include <vector>
#include <string>
#include "Player.h"
#include "Barrier.h"
#include "Ball.h"

//function and template declarations
template <class T, class U>
bool colision(T a, U b);

void bounce(Ball& ball, Player& player);
void bounce(Ball& ball);

//Global constants
const sf::VideoMode mode = sf::VideoMode::getDesktopMode();
const sf::Vector2i screenDimensions(mode.width, mode.height);
const int PLAYER_WIDTH = screenDimensions.x * 0.02;
const int PLAYER_HEIGHT = screenDimensions.y * 0.18;
const int BARRIER_WIDTH = screenDimensions.x;
const int BARRIER_HEIGHT = screenDimensions.y * 0.01;
const int BALL_WIDTH = screenDimensions.x * 0.02;
const int BALL_HEIGHT = BALL_WIDTH;
const int BALL_X_SPEED = 1000;
const int BALL_Y_SPEED = 300;
const int LARGE_FONT = screenDimensions.x * 0.08;
const int SMALL_FONT = screenDimensions.x * 0.02;

int main()
{
	//setting up window
	sf::RenderWindow window(mode, "Pong", sf::Style::Fullscreen);

	//creating instances
	Player player1(1, PLAYER_WIDTH, PLAYER_HEIGHT, screenDimensions.x * 0.95, (screenDimensions.y - PLAYER_HEIGHT) * 0.5, 600, true);
	Player player2(2, PLAYER_WIDTH, PLAYER_HEIGHT, screenDimensions.x * 0.05, (screenDimensions.y - PLAYER_HEIGHT) * 0.5, 600, false);
	Barrier barrierTop(BARRIER_WIDTH, BARRIER_HEIGHT, 0, 0);
	Barrier barrierBottom(BARRIER_WIDTH, BARRIER_HEIGHT, 0, screenDimensions.y - BARRIER_HEIGHT);
	Ball ball(BALL_X_SPEED, BALL_Y_SPEED, BALL_WIDTH, BALL_HEIGHT);

	if(player2.isServing())
	{
		ball.setMovement(-BALL_X_SPEED, BALL_Y_SPEED);
	}

	//Generating net
	sf::Vector2f blockDimentions(screenDimensions.x * 0.003, screenDimensions.y * 0.02);
	std::vector<sf::VertexArray> net;

	for(int i = 0; i < screenDimensions.y / blockDimentions.y; ++i)
	{
		sf::VertexArray netBlock(sf::Quads, 4);
		netBlock[0].position = sf::Vector2f((screenDimensions.x - blockDimentions.x) * 0.5, blockDimentions.y * i);
		netBlock[1].position = sf::Vector2f((screenDimensions.x + blockDimentions.x) * 0.5, blockDimentions.y * i);
		netBlock[2].position = sf::Vector2f((screenDimensions.x + blockDimentions.x) * 0.5, blockDimentions.y * (i + 1));
		netBlock[3].position = sf::Vector2f((screenDimensions.x - blockDimentions.x) * 0.5, blockDimentions.y * (i + 1));

		if(i % 2 == 0)
		{
			for(int j = 0; j < 4; ++j)
			{
				netBlock[j].color = sf::Color::White;
			}
		}
		else
		{
			for(int j = 0; j < 4; ++j)
			{
				netBlock[j].color = sf::Color::Black;
			}
		}

		net.push_back(netBlock);
	}

	//setting up scoreboard
	sf::Font font;

	if(!font.loadFromFile("cour.ttf"))
	{
		std::cout << "Could not load font\n";
	}

	sf::Text player1Score(std::to_string(player1.getScore()), font, LARGE_FONT);
	sf::Text player2Score(std::to_string(player2.getScore()), font, LARGE_FONT);

	player2Score.setOrigin(player2Score.getGlobalBounds().width, 0);

	player1Score.setPosition(screenDimensions.x * 0.6, screenDimensions.y * 0.025);
	player2Score.setPosition(screenDimensions.x * 0.4, screenDimensions.y * 0.025);

	//Loading sound
	sf::SoundBuffer colisionBuffer;
	if(!colisionBuffer.loadFromFile("boop.wav"))
	{
		std::cout << "Could not load sound file\n";
	}

	sf::Sound paddleColisionSound;
	paddleColisionSound.setBuffer(colisionBuffer);
	paddleColisionSound.setPitch(0.5);
	paddleColisionSound.setVolume(75);

	sf::Sound barrierColisionSound;
	barrierColisionSound.setBuffer(colisionBuffer);

	sf::SoundBuffer scoreBuffer;
	if(!scoreBuffer.loadFromFile("score.wav"))
	{
		std::cout << "Could not load sound file\n";
	}

	sf::Sound scoreSound;
	scoreSound.setBuffer(scoreBuffer);

	//Starting game
	sf::Text gameStart("Press enter to start", font, LARGE_FONT);
	while(!sf::Keyboard::isKeyPressed(sf::Keyboard::Return))
	{
		window.draw(gameStart);
		window.display();
	}

	//Setting timers
	sf::Clock frameTimer; //Times how long it takes for one frame to pass
	sf::Clock ballTimer;  //Keeps the ball stationary until a certain time is reached

	ball.setPosition((screenDimensions.x - BALL_WIDTH) * 0.5, (screenDimensions.y - BALL_HEIGHT) * 0.5);

	//Game loop
	while(window.isOpen() && player1.getScore() != 10 && player2.getScore() != 10)
	{
		//Event loop
		sf::Event event;
		while(window.pollEvent(event))
		{
			switch(event.type)
			{
			case sf::Event::Closed:
				window.close();
				break;
			case sf::Event::KeyPressed:
				if(event.key.code == sf::Keyboard::Escape)
				{
					window.close();
				}
				break;
			}
		}

		float time = frameTimer.restart().asSeconds();

		//Player1 movement
		if(sf::Keyboard::isKeyPressed(sf::Keyboard::K) || sf::Keyboard::isKeyPressed(sf::Keyboard::M))
		{
			player1.move(time);
			player1.update();
			if(colision(&player1, &barrierTop))
			{
				player1.setPosition(player1.getImage().getPosition().x, barrierTop.getImage().getPosition().y + BARRIER_HEIGHT);
			}
			else if(colision(&player1, &barrierBottom))
			{
				player1.setPosition(player1.getImage().getPosition().x, barrierBottom.getImage().getPosition().y - PLAYER_HEIGHT);
			}
		}

		//Player2 movement
		if(sf::Keyboard::isKeyPressed(sf::Keyboard::A) || sf::Keyboard::isKeyPressed(sf::Keyboard::Z))
		{
			player2.move(time);
			player2.update();
			if(colision(&player2, &barrierTop))
			{
				player2.setPosition(player2.getImage().getPosition().x, barrierTop.getImage().getPosition().y + barrierTop.getImage().getSize().y);
			}
			else if(colision(&player2, &barrierBottom))
			{
				player2.setPosition(player2.getImage().getPosition().x, barrierBottom.getImage().getPosition().y - player2.getImage().getSize().y);
			}
		}

		//Ball movement
		ball.move(time);
		ball.update();

		//Ball -> Barrier colision
		if(colision(&ball, &barrierBottom) || colision(&ball, &barrierTop))
		{
			barrierColisionSound.play();
			if(colision(&ball, &barrierBottom))
			{
				ball.setPosition(ball.getPosition().x, barrierBottom.getImage().getPosition().y - BALL_HEIGHT);
			}
			else if(colision(&ball, &barrierTop))
			{
				ball.setPosition(ball.getPosition().x, BARRIER_HEIGHT);
			}
			bounce(ball);
		}

		//Ball -> Player1 colision
		if(colision(&player1, &ball) || colision(&player2, &ball))
		{
			paddleColisionSound.play();

			if(colision(&player1, &ball))
			{
				ball.setPosition(player1.getPosition().x - BALL_WIDTH, ball.getPosition().y);
				bounce(ball, player1);
			}
			else if(colision(&player2, &ball))
			{
				ball.setPosition(player2.getPosition().x + PLAYER_WIDTH, ball.getPosition().y);
				bounce(ball, player2);
			}
		}

		//Ball exits screen bounds
		if(ball.getPosition().x < -BALL_WIDTH || ball.getPosition().x > screenDimensions.x)
		{
			if(ball.getPosition().x < -BALL_WIDTH)
			{	
				player1.incrementScore();
				player1Score.setString(std::to_string(player1.getScore()));
				player1.setServe(true);
				player2.setServe(false);
			}
			else if(ball.getPosition().x > screenDimensions.x)
			{
				player2.incrementScore();
				player2Score.setString(std::to_string(player2.getScore()));
				player1.setServe(false);
				player2.setServe(true);
			}

			scoreSound.play();
			ball.setPosition((screenDimensions.x - BALL_WIDTH) * 0.5, (screenDimensions.y - BALL_HEIGHT) * 0.5);
			ball.setMovement(0,0);
			ball.getImage().setFillColor(sf::Color(255,255,255,0));
			ballTimer.restart();
		}

		//Reset ball
		if(ballTimer.getElapsedTime().asSeconds() > 3 && ball.getMovement().x == 0)
		{
			ball.getImage().setFillColor(sf::Color(255,255,255,255));
			if(player1.isServing())
			{
				ball.setMovement(BALL_X_SPEED, BALL_Y_SPEED);
			}
			else if(player2.isServing())
			{
				ball.setMovement(-BALL_X_SPEED, BALL_Y_SPEED);
			}
		}

		//Drawing to screen
		for(int i = 0; i < net.size(); ++i)
		{
			window.draw(net[i]);
		}

		window.draw(player2Score);
		window.draw(player1Score);
		window.draw(barrierTop.getImage());
		window.draw(barrierBottom.getImage());
		window.draw(player1.getImage());
		window.draw(player2.getImage());
		window.draw(ball.getImage());
		window.display();
		window.clear();
	}

	//Displaying winner
	if(player1.getScore() == 10)
		{
			player1Score.setString("WINNER");
		}
		else
		{
			player2Score.setString("WINNER");
			player2Score.setOrigin(player2Score.getGlobalBounds().width, 0);
			player2Score.setPosition(screenDimensions.x * 0.4, screenDimensions.y * 0.025);
		}

	sf::Text exit("Press Escape to exit", font, SMALL_FONT);
	exit.setOrigin(exit.getGlobalBounds().width * 0.5, exit.getGlobalBounds().height * 0.5);
	exit.setPosition(screenDimensions.x * 0.5, screenDimensions.y * 0.5);

	while(window.isOpen() && !sf::Keyboard::isKeyPressed(sf::Keyboard::Escape))
	{
		for(int i = 0; i < net.size(); ++i)
		{
			window.draw(net[i]);
		}

		window.draw(exit);
		window.draw(player2Score);
		window.draw(player1Score);
		window.draw(barrierTop.getImage());
		window.draw(barrierBottom.getImage());
		window.draw(player1.getImage());
		window.draw(player2.getImage());
		window.draw(ball.getImage());
		window.display();
		window.clear();
	}

	return 0;
}

//function and template defenitions
template <class T, class U>
bool colision(T a, U b)
{
	std::vector<int> aSides = a->getSides();
	int aRight = aSides[0];
	int aLeft = aSides[1];
	int aTop = aSides[2];
	int aBottom = aSides[3];

	std::vector<int> bSides = b->getSides();
	int bRight = bSides[0];
	int bLeft = bSides[1];
	int bTop = bSides[2];
	int bBottom = bSides[3];

	if((aTop < bBottom && aBottom > bTop) && (aRight > bLeft && aLeft < bRight))
	{
		return true;
	}
	return false;
}

void bounce(Ball& ball, Player& player)
{
	int playerCenter = player.getPosition().y + (PLAYER_HEIGHT * 0.5);
	int ballCenter = ball.getPosition().y + (BALL_HEIGHT * 0.5);
	int difference = ballCenter - playerCenter;

	float movementX = ball.getMovement().x;
	float movementY;

	if(player.getId() == 1)
	{
		movementY = (difference / (PLAYER_HEIGHT * 0.5 + BALL_HEIGHT)) * movementX;
	}
	else if(player.getId() == 2)
	{
		movementY = -(difference / (PLAYER_HEIGHT * 0.5 + BALL_HEIGHT)) * movementX;
	}
		
	movementX = -movementX;

	ball.setMovement(movementX, movementY);
}

void bounce(Ball& ball)
{
	ball.setMovement(ball.getMovement().x, ball.getMovement().y * -1);
}

[/spoiler]

Player.h

[spoiler]


#ifndef PADDLE_H
#define PADDLE_H

#include "Barrier.h"

class Player
{
public:
	Player(int id, int width, int height, int x, int y, int moveSpeed, bool serving);
	void move(float time);
	void resetPosition();
	void setPosition(int x, int y);
	sf::Vector2i getPosition();
	void incrementScore();
	void update();
	int getScore();
	sf::RectangleShape& getImage();
	std::vector<int> getSides();
	int getId();
	bool isServing();
	void setServe(bool serve);

protected:
	sf::RectangleShape image;
	int id;
	int moveSpeed;
	int score;
	int right;
	int left;
	int top;
	int bottom;
	bool serving;
};

#endif

[/spoiler]

Player.cpp

[spoiler]


#include <SFML/Graphics.hpp>
#include "Barrier.h"
#include "Player.h"
#include <vector>
#include <iostream>

Player::Player(int id, int width, int height, int x, int y, int moveSpeed, bool serving):
	id(id),
	score(0),
	image(sf::RectangleShape(sf::Vector2f(width,height))),
	moveSpeed(moveSpeed),
	serving(serving),
	right(x + width),
	left(x),
	top(y),
	bottom(y + height)
{
	setPosition(x,y);
}

void Player::move(float time)
{
	if(this->getId() == 1)
	{
		if(sf::Keyboard::isKeyPressed(sf::Keyboard::K))
		{
			image.move(0, -moveSpeed * time);
		}
		else if(sf::Keyboard::isKeyPressed(sf::Keyboard::M))
		{
			image.move(0, moveSpeed * time);
		}
	}
	if(this->getId() == 2)
	{
		if(sf::Keyboard::isKeyPressed(sf::Keyboard::A))
		{
			image.move(0, -moveSpeed * time);
		}
		else if(sf::Keyboard::isKeyPressed(sf::Keyboard::Z))
		{
			image.move(0, moveSpeed * time);
		}
	}
}

void Player::incrementScore()
{
	++score;
}

void Player::update()
{
	right = image.getPosition().x + image.getSize().x;
	left = image.getPosition().x;
	top = image.getPosition().y;
	bottom = image.getPosition().y + image.getSize().y;
}

int Player::getScore()
{
	return score;
}

sf::RectangleShape& Player::getImage()
{
	return image;
}

void Player::setPosition(int x, int y)
{
	image.setPosition(x,y);
}

sf::Vector2i Player::getPosition()
{
	return sf::Vector2i(image.getPosition().x, image.getPosition().y);
}

std::vector<int> Player::getSides()
{
	std::vector<int> sides;
	sides.push_back(right);
	sides.push_back(left);
	sides.push_back(top);
	sides.push_back(bottom);
	return sides;
}

int Player::getId()
{
	return id;
}

bool Player::isServing()
{
	return serving;
}

void Player::setServe(bool serve)
{
	this->serving = serve;
}

[/spoiler]

Barrier.h

[spoiler]


#ifndef BARRIER_H
#define BARRIER_H

class Barrier
{
public:
	Barrier(int width, int height, int x, int y);
	void setPosition(int x, int y);
	std::vector<int> getSides();
	const sf::RectangleShape& getImage();

private:
	sf::RectangleShape image;
	int right;
	int left;
	int top;
	int bottom;
};

#endif

[/spoiler]

Barrier.cpp

[spoiler]


#include <SFML/Graphics.hpp>
#include "Barrier.h"
#include <vector>
#include <iostream>

Barrier::Barrier(int width, int height, int x, int y):
	image(sf::RectangleShape(sf::Vector2f(width,height))),
	right(x + width),
	left(x),
	top(y),
	bottom(y + height)
{
	this->setPosition(x,y);
}

void Barrier::setPosition(int x, int y)
{
	image.setPosition(x,y);
}

std::vector<int> Barrier::getSides()
{
	std::vector<int> sides;
	sides.push_back(right);
	sides.push_back(left);
	sides.push_back(top);
	sides.push_back(bottom);
	return sides;
}

const sf::RectangleShape& Barrier::getImage()
{
	return image;
}

[/spoiler]

Ball.h

[spoiler]


#ifndef BALL_H
#define BALL_H

class Ball
{
public:
	Ball(float horizontalSpeed, float angle, int width, int height);
	sf::Vector2f& getMovement();
	sf::RectangleShape& getImage();
	sf::Vector2i getPosition();
	void setMovement(float x, float y);
	void setPosition(int x, int y);
	void move(float time);
	void update();
	std::vector<int> getSides();

private:
	sf::Vector2f movement;
	sf::RectangleShape image;
	int right;
	int left;
	int top;
	int bottom;
};

#endif

[/spoiler]

Ball.cpp

[spoiler]


#include "SFML/Graphics.hpp"
#include "Ball.h"
#include <iostream>

Ball::Ball(float horizontalSpeed, float verticalSpeed, int width, int height):
	movement(sf::Vector2f(horizontalSpeed, verticalSpeed)),
	image(sf::RectangleShape(sf::Vector2f(width, height)))
{
	update();
}
	
sf::Vector2f& Ball::getMovement()
{
	return movement;
}

void Ball::setMovement(float x, float y)
{
	movement.x = x;
	movement.y = y;
}
		
sf::RectangleShape& Ball::getImage()
{
	return image;
}

void Ball::setPosition(int x, int y)
{
	image.setPosition(x,y);
}

sf::Vector2i Ball::getPosition()
{
	return sf::Vector2i(this->getImage().getPosition());
}

void Ball::move(float time)
{
	image.move(time * movement.x, time * movement.y);
}

void Ball::update()
{
	right = this->getPosition().x + image.getSize().x;
	left = this->getPosition().x;
	top = this->getPosition().y;
	bottom = this->getPosition().y + image.getSize().y;
}

std::vector<int> Ball::getSides()
{
	std::vector<int> sides;
	sides.push_back(right);
	sides.push_back(left);
	sides.push_back(top);
	sides.push_back(bottom);
	return sides;
}

[/spoiler]

beanybob95

127

Author

August 04, 2014 03:33 PM

Let me just upload a screenshot aswell.

[attachment=22996:screenshot.png]

Ashaman73

13,718

August 05, 2014 05:50 AM

Not really diving into your code, but it looks really clean and the screenshot looks good to. So, from a quick review there's not much to add (you use OOP, constants, clean code format, no use of ugly code like goto). Just try to add more comments to your code, otherwise is looks really good, almost perfect

The lack of responses is a good sign (people tend to point out errors and missconceptions more often than good work), keep up the good work and try to create a little bit more complex project now.

Ashaman

Gnoblins: Website - Facebook - Twitter - Youtube - Steam Greenlit - IndieDB - Gamedev Log

rave3d

691

August 05, 2014 08:04 AM

From a quick overview your code seems to have a clear structure, the positive things are already mentioned by Ashaman73 so i will not repeat them
;)

I have some hints for you which could improve the readabilty of your code, and will maybe help you if you are working on a more complex project

Add comments to your code, describe in a short header what each class is inteded for, do the same with the functions, describe also the parameters of the functions an the class members
your main function is really long, split it up in some functions (at least init and shutdown)

You can also create a class for the playing field, which holds the dimensions, barries aso.

Also it is maybe a good idea to create a Main class for the game which holds all the neccesary states of the game, and where the game loop is implemented

beanybob95

127

Author

August 05, 2014 08:29 AM

Thanks for the brilliant feedback and encouragement guys. I'll be sure to add what you guys suggested.

Madhed

4,095

August 05, 2014 11:06 AM

I noticed one thing while looking through your header files:

There are no includes and/or forward declarations in there. Apparently they work only because you include all needed stuff in the main.cpp before including your own header files.

While this works now, you will have to do the same if you want to use them in another project. IMO these headers should be self contained and not rely on the structure of the source file in which they are included.

beanybob95

127

Author

August 05, 2014 02:23 PM

Cheers Madhed. In fact, after reading up on creating self contained header files, I realized that one of the earlier bugs in my program was caused by me including other files in the wrong order. So it's great that you mentioned this.

Strewya

2,802

August 06, 2014 12:38 PM

I looked through your code a bit deeper than i expected.

The stuff below was written as i was reading the code, and mostly are suggestions. The code in general looks pretty good, so there are no big remarks

Player.h + .cpp
- For your header files, it's (IMO) better to use #pragma once in favour of #ifndef guards. Most compilers today support it (even if it's not standard), so it saves you from having to think of a unique name for the include guard.
- In your Player class you're returning an std::vector<int> from getSides(). You might benefit from making a class Rectangle { int left,right,top,bottom; } of your own (or use it from SFML if it has one) since returning a vector seems like overkill for just getting 4 integers out.
- Some compilers might warn you (if you set the warning level high enough) about initializing member variables in the constructor initializer list in a different order from which they are listed in the class declaration. I also find it looks nicer to have the initialization and declaration in the same order, but that might just be me
- In your update() method, you can first assign left, and then use it in the calculation for right to avoid calling the getPosition() two times. Same goes for top and bottom.
- Most of your getter functions can be made const, since you're not changing the internals of the object.

Barrier.h + .cpp
- Same thing for the #pragma once and returning std::vector<int> and making the getters() const

Ball.h + .cpp
- getters() const and returning std::vector<int>

main.cpp
- Making the player width and height dependent on the screenDimensions will look really different on a wide screen resolution and a 4:3 resolution. I personally find making the dimensions a certain predefined size a nicer solution.
- When generating the net, your for loop will generate more segments than you need (because half of them are not visible). You can change it so the code that creates the vertices for the visible segments is inside the if(i%2==0) block. You can further modify it so you don't have the if(i%2==0) but instead have the loop step as i+=2.
- I'm not familiar with sfml, but what would happen if your font failed to load (and you don't return)? Would the game continue with some preset default font defined by sfml? Would the font simply not show? Or would it crash the game?
- Same as the previous for the sounds failing to load, but here i'm assuming they just don't play?
- In your loop, in your "move the paddles" phase, you check the keys for player 1 and player 2, then call the player1.move() method which checks the keys again. You should remove one of these checks, since they're redundant.
- When checking the collision for the ball, you should cache the result of the colision() call, so you don't have to call the function two times for each barrier and each player.

devstropo.blogspot.com - Random stuff about my gamedev hobby

beanybob95

127

Author

August 08, 2014 05:37 PM

Thanks Strewya, especially for taking the time to go into this much detail!

vvv2

285

August 11, 2014 08:18 AM

Thanks, especially for taking the time to go into this much detail!

Hi,

previously, here have done a good overview of your code. I would like to draw attention to the game's playability. By making a new game, it is more important than clean code.

Ggood success.

First project (pong) code review

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

First project (pong) code review

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines