• Create Account

### #Actualsuperman3275

Posted 02 December 2012 - 09:10 PM

One problem I see is that, probably because of hard coded values, what's happening is that no matter what angle the ball hits the player controlled paddle from, the ball always goes towards the bottom of the screen. This can be easily fixed by changing some values however, so it's not that big of a deal .

Also, before you continue reading, realize I also finished Pong with SFML in C++ only about a month ago.

Another problem in your code is over-commenting. Holy crap, you're probably getting half of what you should be getting done finished because you're commenting obvious things. And if you're having to write comments for obvious things, it's bad code. An example would be this code in your Paddle.cpp:
void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < 800)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > 0)
{
position_.y -= speed_ * elapsedTime;
}
}
}


This is hard to understand, even though you have that comment.

It would have been better to have some const int's like so:
enum Screen
{
Left = 0,
Right = 800,
Top = 0,
Bottom = 600
};


void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < Screen.Right)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > Screen.Left)
{
position_.y -= speed_ * elapsedTime;
}
}
}


Not to mention all the other variables with other names. If you corrected those, then it'd be way, way, way easier to understand. Also, what's with the _ after the name of everything?

### #2superman3275

Posted 02 December 2012 - 09:09 PM

Just a list of things I see that you might want to think about:

• Dynamically allocating non-polymorphic members
• C++ rule of three violations. Not using pointers would solve most of these.
• The lowercase "k" prefixes on the GameState enumerated value is poor. Why not have the prefix "GameState" or just "State"?
• class Game has public helper functions - helper functions should be private
• Game:::ConvertFtoS and Game::CalculateFPS should be free fuctions. I would put them in an anonymous namespace in Game.cpp, and move ConvertFtoS into a utility file should the need arise to call it elsewhere.
• ConvertFtoS is poorly named. A conventional name might be ToString() or AsString(). However, as it also truncates the value, it could be named FloorToString. Note that this awkward function name indicates that the function is doing too much.
• The "Result" variable in ConvertFtoS is unnecessary. You can return "convert.str()".
• You should use C++ style casts (static_cast, dynamic_cast, reinterpret_cast, etc) where possible. These are better because they communicate intent, and also stand out more.
• I would call srand() in main(). It is minor, but it means you could add deterministic unit tests for the game class
• If you aren't "using" a class in a header file (e.g. the declarations are pointers or references), consider using forward declarations.
• ConvertItoS is also a free function, and again a candidate for a separate file. Also note that you can remove redundancy by implementing ConvertFtoS in terms of ConvertItoS:
std::string ConvertItoS(int value) {
// ...
}

std::string ConvertFtoS(int value) {
return ConvertItoS(static_cast<int>(value));
}

• You could change your design to allow the paddles share the same image. In larger games, not re-using resources will become problematic.
• Arena::DetectCollisions seems more complex than just calling Intersects() directly
• Instead of increasing the speed, reversing the speed, calculating the angle and setting the ball's Y speed, why not internalise all this logic into the ball? I'd just have a function such as Ball::OnPaddleCollision(/* ... */)
• You are duplicating the logic for the collision for the left and right paddle. They could easily get out of sync. This could be a helper function HandleCollision(const Paddle &, Ball &) - though internalising the logic like I mentioned above would mostly alleviate this.
• Ball::ReverseSpeed(int) is confusing. 1 is the Y axis, 2 is X? What if someone calls ReverseSpeed(-1), or ReverseSpeed(4200)? Consider a named enumeration, or again internalise the logic Ball::OnWallCollision(/* ... */)
• There is duplication in resetting the Arena between StartNewGame and when a point is scored
• Don't write foo.setScore(foo.getScore() + 1) - write a foo.OnPoint() or something.
• GenerateRandomDirection() should be a free function. It should probably return -1 or 1, as the "direction" in a given axis. The caller can multiply this by the magnitude they desire.
• xSpeed and ySpeed should probably be bundled into a sf::Vector2f
• Don't write comments that merely repeat the code.
xSpeed_ = GenerateRandomDirection();			// X-speed at 150
ySpeed_ = GenerateRandomDirection() * 1.5f;		// make the Y-speed 300
if(xSpeed_ > 0)
{
speedMultiplier_ = 0.2f;	// Increment X-speed at rates of 30.
}
else
{
speedMultiplier_ = -0.2f;
}

In this case, the comments are out of date
• It is harder and more bug prone to try and establish an invariant between the sign of the X speed and the speed multiplier's sign. Instead, calculate the correct sign when the speed multiplier is used - if this is even necessary?
• Empty non-virtual destructors are unnecessary, the compiler will generate this for you.
• Your choice of error handling is questionable. I'm not sure anyone wants to play Pong if they cannot see the paddle and ball. One option would be to fall back to primitive rendering, another is to report the error to the user. Or both.
• Establish and follow a strong naming convention. You've done fairly well at this, but Ball::getArea does not match your other function names.
• I'm not familiar with SFML, but your use of isFileLoaded_ is inconsistent. For example, Ball::getArea unconditionally accesses the Sprite's size.
• I'd prefer to test if(abs(x) > MAX), rather than if(x > MAX || x < -MAX).
• You seem to have an unnecessary cast from Direction to Direction
• The paddle class appears to disagree about the arena height. In one place, it is 800, in another, it is 600. Use a named constant. Better still, the Paddle class should not handle the bounds. The arena should inform the paddle if it has left the bounds.
• Even when you do get constants to match, you should still use named constants. I don't understand why Paddle::SetPositionY has a 10 pixel "radius" that it appears to respond to. I think it is meant to be hysteresis, but better naming would make this clear.
• The paddle should not have to be aware of whether an AI or Human is controlling it.
• Try to give better names rather than just "fooIn". In particular, Paddle::SetPositionY's first argument should be something like "targetPositionY".
• It is poor practise to place "using" directives in header files. Any code that #includes you file cannot "undo" this.
• numOptions_ seems to duplicate optionsList_.size()
• prevButtonState_ could be a local variable
sf::String option(optionsList_.at(i));
option.SetSize(40);

if(selectedOption_ == i)
{
// show the selected option in a different colour.
option.SetPosition((float)position_.x, (float)position_.y + (i * 40) -15);
option.SetColor(sf::Color(255, 255, 0));
}
else
{
// everything else drawn in non-selected colour.
sf::String option(optionsList_.at(i));
option.SetPosition((float)position_.x, (float)position_.y + (i * 40));
}

targetWindow.Draw(option);


It was just a quick skim really, this was just what jumped out at me and I might have made some mistakes.

You don't seem to make use of "const" anywhere, which is something you might want to look into for larger projects. Retrofitting it can be annoying though.

Man, that's not a "few" things ! One problem I see is that, probably because of hard coded values, what's happening is that no matter what angle the ball hits the player controlled paddle from, the ball always goes towards the bottom of the screen. This can be easily fixed by changing some values however, so it's not that big of a deal.

Also, before you continue reading, realize I also finished Pong with SFML in C++ only about a month ago.

Another problem in your code is over-commenting. Holy crap, you're probably getting half of what you should be getting done finished because you're commenting obvious things. And if you're having to write comments for obvious things, it's bad code. An example would be this code in your Paddle.cpp:
void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < 800)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > 0)
{
position_.y -= speed_ * elapsedTime;
}
}
}


This is hard to understand, even though you have that comment.

It would have been better to have some const int's like so:
enum Screen
{
Left = 0,
Right = 800,
Top = 0,
Bottom = 600
};


void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < Screen.Right)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > Screen.Left)
{
position_.y -= speed_ * elapsedTime;
}
}
}


Not to mention all the other variables with other names. If you corrected those, then it'd be way, way, way easier to understand. Also, what's with the _ after the name of everything?

### #1superman3275

Posted 02 December 2012 - 08:58 PM

Just a list of things I see that you might want to think about:

• Dynamically allocating non-polymorphic members
• C++ rule of three violations. Not using pointers would solve most of these.
• The lowercase "k" prefixes on the GameState enumerated value is poor. Why not have the prefix "GameState" or just "State"?
• class Game has public helper functions - helper functions should be private
• Game:::ConvertFtoS and Game::CalculateFPS should be free fuctions. I would put them in an anonymous namespace in Game.cpp, and move ConvertFtoS into a utility file should the need arise to call it elsewhere.
• ConvertFtoS is poorly named. A conventional name might be ToString() or AsString(). However, as it also truncates the value, it could be named FloorToString. Note that this awkward function name indicates that the function is doing too much.
• The "Result" variable in ConvertFtoS is unnecessary. You can return "convert.str()".
• You should use C++ style casts (static_cast, dynamic_cast, reinterpret_cast, etc) where possible. These are better because they communicate intent, and also stand out more.
• I would call srand() in main(). It is minor, but it means you could add deterministic unit tests for the game class
• If you aren't "using" a class in a header file (e.g. the declarations are pointers or references), consider using forward declarations.
• ConvertItoS is also a free function, and again a candidate for a separate file. Also note that you can remove redundancy by implementing ConvertFtoS in terms of ConvertItoS:
std::string ConvertItoS(int value) {
// ...
}

std::string ConvertFtoS(int value) {
return ConvertItoS(static_cast<int>(value));
}

• You could change your design to allow the paddles share the same image. In larger games, not re-using resources will become problematic.
• Arena::DetectCollisions seems more complex than just calling Intersects() directly
• Instead of increasing the speed, reversing the speed, calculating the angle and setting the ball's Y speed, why not internalise all this logic into the ball? I'd just have a function such as Ball::OnPaddleCollision(/* ... */)
• You are duplicating the logic for the collision for the left and right paddle. They could easily get out of sync. This could be a helper function HandleCollision(const Paddle &, Ball &) - though internalising the logic like I mentioned above would mostly alleviate this.
• Ball::ReverseSpeed(int) is confusing. 1 is the Y axis, 2 is X? What if someone calls ReverseSpeed(-1), or ReverseSpeed(4200)? Consider a named enumeration, or again internalise the logic Ball::OnWallCollision(/* ... */)
• There is duplication in resetting the Arena between StartNewGame and when a point is scored
• Don't write foo.setScore(foo.getScore() + 1) - write a foo.OnPoint() or something.
• GenerateRandomDirection() should be a free function. It should probably return -1 or 1, as the "direction" in a given axis. The caller can multiply this by the magnitude they desire.
• xSpeed and ySpeed should probably be bundled into a sf::Vector2f
• Don't write comments that merely repeat the code.
xSpeed_ = GenerateRandomDirection();			// X-speed at 150
ySpeed_ = GenerateRandomDirection() * 1.5f;		// make the Y-speed 300
if(xSpeed_ > 0)
{
speedMultiplier_ = 0.2f;	// Increment X-speed at rates of 30.
}
else
{
speedMultiplier_ = -0.2f;
}

In this case, the comments are out of date
• It is harder and more bug prone to try and establish an invariant between the sign of the X speed and the speed multiplier's sign. Instead, calculate the correct sign when the speed multiplier is used - if this is even necessary?
• Empty non-virtual destructors are unnecessary, the compiler will generate this for you.
• Your choice of error handling is questionable. I'm not sure anyone wants to play Pong if they cannot see the paddle and ball. One option would be to fall back to primitive rendering, another is to report the error to the user. Or both.
• Establish and follow a strong naming convention. You've done fairly well at this, but Ball::getArea does not match your other function names.
• I'm not familiar with SFML, but your use of isFileLoaded_ is inconsistent. For example, Ball::getArea unconditionally accesses the Sprite's size.
• I'd prefer to test if(abs(x) > MAX), rather than if(x > MAX || x < -MAX).
• You seem to have an unnecessary cast from Direction to Direction
• The paddle class appears to disagree about the arena height. In one place, it is 800, in another, it is 600. Use a named constant. Better still, the Paddle class should not handle the bounds. The arena should inform the paddle if it has left the bounds.
• Even when you do get constants to match, you should still use named constants. I don't understand why Paddle::SetPositionY has a 10 pixel "radius" that it appears to respond to. I think it is meant to be hysteresis, but better naming would make this clear.
• The paddle should not have to be aware of whether an AI or Human is controlling it.
• Try to give better names rather than just "fooIn". In particular, Paddle::SetPositionY's first argument should be something like "targetPositionY".
• It is poor practise to place "using" directives in header files. Any code that #includes you file cannot "undo" this.
• numOptions_ seems to duplicate optionsList_.size()
• prevButtonState_ could be a local variable
sf::String option(optionsList_.at(i));
option.SetSize(40);

if(selectedOption_ == i)
{
// show the selected option in a different colour.
option.SetPosition((float)position_.x, (float)position_.y + (i * 40) -15);
option.SetColor(sf::Color(255, 255, 0));
}
else
{
// everything else drawn in non-selected colour.
sf::String option(optionsList_.at(i));
option.SetPosition((float)position_.x, (float)position_.y + (i * 40));
}

targetWindow.Draw(option);


It was just a quick skim really, this was just what jumped out at me and I might have made some mistakes.

You don't seem to make use of "const" anywhere, which is something you might want to look into for larger projects. Retrofitting it can be annoying though.

Man, that's not a "few" things ! One problem I see is that, probably because of hard coded values, what's happening is that no matter what angle the ball hits the player controlled paddle from, the ball always goes towards the bottom of the screen. This can be easily fixed by changing some values however, so it's not that big of a deal.

PARTNERS