Jump to content

  • Log In with Google      Sign In   
  • Create Account

We're offering banner ads on our site from just $5!

1. Details HERE. 2. GDNet+ Subscriptions HERE. 3. Ad upload HERE.


Pong Project near completion; looking for Critique.


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
10 replies to this topic

#1 stitchs   Crossbones+   -  Reputation: 1309

Like
0Likes
Like

Posted 02 December 2012 - 05:17 PM

Hello community,

I have completed a small Pong project, I am quite happy with it and am looking towards the community now that I am at a point to share it. At some points it got me down, as this was my first outing with SFML and there are some aspects that I definitely need to understand better (such as the ordering of event queues and key-presses), but I am happy that i persevered.

I wasn't sure where to put this, it is kind of an announcement, but I am also looking for a code critique and some feedback. Basically, I have gone back to basics and am trying to code a game as quickly as possible, with as few mistakes as possible, but they definitely exists. Currently I am aware that the Paddle and Ball classes share some methods, and this will lead me to developing an inheritance structure, this is one of the known issues and an example of what I am aware of in my code.

I am trying to find as many shortfalls and understand why some concepts shine, through the experience of not using them.

A few things I am aware of:
  • Hard-coded values,
  • Movement that jutters at times.
  • Over-diluted with methods that could be combined.
  • Unclear comments in some places.
  • A few methods that are not used or have no body.

I appreciate any feedback on my code and the application itself, and look forward to your comments. Below you will find two rar files attached, one with the exe and one with the Header/CPP files.

Regards,

Stitchs.

Attached Files


Edited by stitchs, 02 December 2012 - 05:19 PM.


Sponsor:

#2 Aart   Members   -  Reputation: 219

Like
0Likes
Like

Posted 02 December 2012 - 05:35 PM

Hey Stitchs,

The executable complains about MSVCP100.dll not being present, you might as well want to include that.
Furtermore, I took a quick look at your code, and it seems okay to me. I don't know how much experience you have in C++, but I think this program is written with a good sence of object orientation. One thing though, why do you save the score in the bat, and not in the arena?

Aart

#3 rip-off   Moderators   -  Reputation: 8689

Like
8Likes
Like

Posted 02 December 2012 - 06:53 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
  • DisplayMenu() contains duplicate code. Consider instead:
    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.

#4 superman3275   Crossbones+   -  Reputation: 2061

Like
1Likes
Like

Posted 02 December 2012 - 08:58 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
};

Now look at your code:
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?

Edited by superman3275, 02 December 2012 - 09:10 PM.

I'm a game programmer and computer science ninja ph34r.png!

Here's my 2D RPG-Ish Platformer Programmed in Python + Pygame, with a Custom Level Editor and Rendering System!

 

Here's my Custom IDE / Debugger Programmed in Pure Python and Designed from the Ground Up for Programming Education!

Want to ask about Python, Flask, wxPython, Pygame, C++, HTML5, CSS3, Javascript, jQuery, C++, Vimscript, SFML 1.6 / 2.0, or anything else? Recruiting for a game development team and need a passionate programmer? Just want to talk about programming? Email me here:

hobohm.business@gmail.com

or Personal-Message me on here smile.png!


#5 lride   Members   -  Reputation: 633

Like
0Likes
Like

Posted 02 December 2012 - 09:37 PM

Instead of using if-else chain to manage states, you could have used a nice polymorphism.
[source lang="cpp"]while(running){ currentState->update(); currentState->draw(); //change what currentState points to when necessary}[/source]

Edited by lride, 02 December 2012 - 09:47 PM.

An invisible text.

#6 stitchs   Crossbones+   -  Reputation: 1309

Like
0Likes
Like

Posted 03 December 2012 - 05:21 PM

Thank you all for your responses, I am not currently at my home computer and will make a fuller reply tomorrow, when I have had a chance to compare the notes with my code. Two things I picked up on from Superman and rip-off; the trailing '_' after variables and using 'k' in Enumerated types is something I have been following from the Google Style guide on C++. I wanted to find a way of making my code consistent. If it seems wrong, then I will look into changing that for my next project.

I only pick this out because it is the one I can answer off the top of my head.

Thanks again and I will reply in full tomorrow,

Stitchs.

#7 rip-off   Moderators   -  Reputation: 8689

Like
1Likes
Like

Posted 03 December 2012 - 06:18 PM

Two things I picked up on from Superman and rip-off; the trailing '_' after variables and using 'k' in Enumerated types is something I have been following from the Google Style guide on C++. I wanted to find a way of making my code consistent. If it seems wrong, then I will look into changing that for my next project.

These are stylistic things, so they aren't "wrong" necessarily. I use the trailing underscores myself in some projects. The "k" prefix is one I haven't seen. But if you think it makes sense for you, that is OK.

Given that C++ places the enum names in the enclosing scope, I prefer to prefix the enum names with some "tag" that groups them, e.g.
enum Direction {
	DirectionNone,
	DirectionUp,
	DirectionDown
	// ...
};

// Later
Direction direction = DirectionUp;
Some might find duplicating the name of the enumeration like this distasteful, and I can't say they are wrong.

Some people prefer to place global enums inside a namespace instead:
namespace Direction {
	enum Type {
		None,
		Up,
		Down
		// ...
	};
}

// Later
Direction::Type direction = Direction::Up;
However IMO now your declaration syntax starts getting quite funky.

There isn't a definitive answer, but it is good to think about these things and not just blindly applying them.

Edited by rip-off, 04 December 2012 - 03:31 AM.


#8 lride   Members   -  Reputation: 633

Like
0Likes
Like

Posted 03 December 2012 - 06:43 PM

Some people prefer to place global enums inside a namespace instead:
[code=auto:0]
namespace Direction {
enum Type {
None,
Up,
Down
// ...
};
}


With C++11
you can just use
[source lang="cpp"]enum class Direction: unsigned char //You can even specify underlying data type{ None, Up, Down};[/source]

Note this doesn't allow implicit type conversion
[source lang="cpp"]int direction=Direction::None;//ErrorDirection direction=Direction::None//Ok[/source]

Edited by lride, 03 December 2012 - 06:45 PM.

An invisible text.

#9 stitchs   Crossbones+   -  Reputation: 1309

Like
0Likes
Like

Posted 09 December 2012 - 04:47 PM

I know this reply comes late, I do hope that it is still a valid to post in topic.

I am currently changing the code to match the suggestions made here. I have one question: would a method named 'OnPaddleCollision' be of type Bool or type Void, I can't differentiate between the two. I would of gone with bool, but what I have written in the method does not need to return true or false for the arena to reference, which makes me want to use Void as its' type.

Or, to go against my usual thought train, would I: check for a collision with the paddle/ball in the arena and, if one occurs, call the void Ball::OnPaddleCollision(). Saying it like this seems to make slightly more sense, I guess I just need some outside input to help concrete this in my head.

Thanks and I look forward to your replies,

Regards,

Stitchs.

#10 rip-off   Moderators   -  Reputation: 8689

Like
1Likes
Like

Posted 09 December 2012 - 05:10 PM

I know this reply comes late, I do hope that it is still a valid to post in topic.

It is perfectly fine. If the thread had started to be a few months old, maybe. There is no hard rule, just common sense AFAIK. Additional leeway would probably be given in this instance as this thread is very specific to your project, as opposed to another member bumping this to ask a follow up question.

I am currently changing the code to match the suggestions made here.

Great! When you're happy with it, you can post it here again for further feedback.

would a method named 'OnPaddleCollision' be of type Bool or type Void

I don't believe there is any sensible meaning to the return value of OnPaddleCollision, thus I would use the "void" version. I'd recommend against including return types when the result is irrelevant to the caller.

would I: check for a collision with the paddle/ball in the arena and, if one occurs, call the void Ball::OnPaddleCollision()

Yes.

Edited by rip-off, 09 December 2012 - 05:17 PM.


#11 stitchs   Crossbones+   -  Reputation: 1309

Like
0Likes
Like

Posted 12 December 2012 - 02:08 PM

@rip-off:

So, as I said, I am using your notes to improve my game and I implemented Ball::OnPaddleCollision. Then it came to Paddle::OnPoint and I suddenly had a bit of a mind-blank. I was trying to figure out "what does this method need to handle apart from increasing the score by 1?"

I had a brainwave and I wanted to check with you to ensure that I have this right, in my head. I was trying to figure whether the paddle should reset itself on a point, or the Arena. It took me about 15 minutes and then I realised, "If I have a paddle handle it's own reset on a point score, what about the other paddle, and the ball position?". This is definitely something that the Arena needs to handle. To cement what I had just learned, I looked back to the Ball collision method and thought, "Well, when is it appropriate for the Object or the Arena to handle certain functions" such as the balls' direction changing. I imagined having 2 or more balls in play, and this is when it clicked. I realised that if the Arena handled all of these methods (reverse speed, increase speed...) then it will become very messy and confused, because with 2 balls or more, each of these methods need to be repeated. Inevitably, this will make it harder to trace bugs.

To summarise, am I on the correct train of thought? If I am, do I need to try to imagine scenario's like this, to try and make sense of what could happen and how easy it would be if someone using my code wanted to achieve these effects?

Regards,

Stitchs.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS