• Advertisement

Yet Another Pong

Recommended Posts

Hi, everyone!

I "finished" building my first game. Obviously Pong.
It's written in C++ on Visual Studio with SFML.

Pong.cpp

What do you think? What should I consider doing to improve the code?

Thank you very much.

 

EDIT: added some screenshot and .zip file of the playable game

 

Pong.zip

Screenshot (1).png

Screenshot (4).png

Edited by reders

Share this post


Link to post
Share on other sites
Advertisement

classes are used to associate variables and functions with a common name. I notice you have a lot of variables that can indeed be grouped under a common name, this would be object-orientation. An obvious association to me are your actual game objects: ball, paddle. Right now you have graphics objects and do work on them all over the place, you could instead put these graphic objects into a class and add methods to do the related work. Another observation is that you have nameless constants all over, you could actually make these constants or defines to give them a name. 

Share this post


Link to post
Share on other sites

Thank you very much. I'll work on this. One more question. What's the best way to divide the code in different files like headers and such? Is there a standard?

Share this post


Link to post
Share on other sites

Pong is an extremely simple game, and it is easy to over-engineer.

I'd ignore the comments about classes for Pong.  The code you've got there does not need them.

The only variable really are the position and velocity of the ball, the position of the paddles, and the scores.  You can add more fluff, but that's all the game requires.  That does not lend itself to game objects with classes and interfaces and interdependencies.  I've put together a pong game for the Challenge this month and it was about 90 lines before I started adding the fluff. Pong does not need to be complex since typically the complex stuff of graphics and sound are handled by libraries.

That is partially why Pong it an excellent beginner exercise.  Larger projects certainly need it, but this doesn't.

 

For a code review...

Lines 15-47, you don't need to make all of those static.  Global variables are static by definition.  

I thought it was interesting that you made some global constants, but you also scattered numbers around the code.  You should probably put all your constants up there, but more on that below.

Overall you place multiple statements on a single line, that is not a typical pattern.  For example, lines 69-75 contain 32 statements.  Why did you do that?  Typical C++ code has one statement per line, even one declaration per line in most cases.  Trying to cram 32 statements into 6 lines makes the code difficult to read.

If each is a block of functionality, consider adding a comment to each block, or pulling it out to a function.

From your code example, perhaps instead of three lines, try this:

//Old code, this seems difficult for me to read
	ball.setRadius(10);	ball.setOrigin(10, 10);	ball.setFillColor(ball_color);	ball.setPosition(640, 360);
	paddle1.setFillColor(paddle1_color); paddle1.setOrigin(8, 60); paddle1.setPosition(35, 360);
	paddle2.setFillColor(paddle2_color); paddle2.setOrigin(8, 60); paddle2.setPosition(1245, 360);

// Suggested version

    // Initialize ball and paddles
	ball.setRadius(10);     //TODO: Replace all these numbers with named constants
	ball.setOrigin(10, 10);	
	ball.setFillColor(ball_color);	
	ball.setPosition(640, 360);

	paddle1.setFillColor(paddle1_color); 
	paddle1.setOrigin(8, 60); 
	paddle1.setPosition(35, 360);

	paddle2.setFillColor(paddle2_color); 
	paddle2.setOrigin(8, 60); 
	paddle2.setPosition(1245, 360);

In that snippit of code you've got a lot of magic numbers.  These are difficult to maintain.  It is usually easier to provide named constants that are all found in one convenient location. Then you can change one constant and it is reflected everywhere in the code.  As written you may decide you want to change the paddle origin value from 60 to 48, and so you'll be searching the code for everywhere that has a 60 in it. Then when you find a 60, you'll need to make sure it is the right 60 and not used for something else. And even then, maybe for some reason you didn't use 60 but were using another value to represent something, or maybe you already added to values together but now you want to modify one of them.  Lots of reasons, always use named constants. They should be added up near your other constants used for colors up around line 20.

 

As for dividing code between files, typically the approach is to block them together by functionality, which usually means by feature or by class. Sometimes a third file will be used when using something like the pImpl paradigm, where the publicly used interface doesn't need to contain all the inner details which can help reduce build times on large projects.

For organization, it depends. Often (but not always) directory hierarchies follow namespace hierarchies, and often (but not always) large subsystems and external subsystems go in their own hierarchies. The exact details vary from project to project depending on what works for the team.

 

 

Overall it looks good for this skill level, and looks like it does the job of being a good pong clone.

Share this post


Link to post
Share on other sites
1 hour ago, frob said:

Pong is an extremely simple game, and it is easy to over-engineer.

I'd ignore the comments about classes for Pong.  The code you've got there does not need them

I disagree, how is adding some classes over-engineering? In this particular case he did not add classes because he does not understand how to use them. There's a lot of code in there that could be a lot more readable and correct with objects, why favor less for the sake of "engineering less"? If there is one thing this fellow needs to learn about, judging from the code posted, is that he needs to engineer more - a lot more - be it with classes or not. 

Share this post


Link to post
Share on other sites

Thank you very much for your help ;)
I reworked the variables moving most of the "magic numbers" in to const as both of you suggested. 
I also moved the multiple statements per line.

As for the classes I started working on that but as predictable I had trouble doing it now since I would need to redo most of the entire thing.
So now I'm working on a different version for the contest (if I manage to finish it in time) and I'm working with objects from the start.

Here's the upgraded .cpp

 

Pong.cpp

Share this post


Link to post
Share on other sites
23 hours ago, h8CplusplusGuru said:

I disagree, how is adding some classes over-engineering? In this particular case he did not add classes because he does not understand how to use them. There's a lot of code in there that could be a lot more readable and correct with objects, why favor less for the sake of "engineering less"? If there is one thing this fellow needs to learn about, judging from the code posted, is that he needs to engineer more - a lot more - be it with classes or not. 

The whole reason OOP and classes are used is for reusability of code. if he doesn't reuse anything, he shouldn't use classes.  Namespaces are what you use when you want to group similar functions.

Share this post


Link to post
Share on other sites
2 hours ago, Vityou said:

The whole reason OOP and classes are used is for reusability of code. if he doesn't reuse anything, he shouldn't use classes.  Namespaces are what you use when you want to group similar functions.

wholly incorrect on that.

Share this post


Link to post
Share on other sites
38 minutes ago, h8CplusplusGuru said:

wholly incorrect on that.

Not sure what you mean.  From the Wikipedia article on OOP: "Languages that support object-oriented programming typically use inheritance for code reuse and extensibility in the form of either classes or prototypes."  Also, from Microsoft's website on C++: "Namespaces are used to organize code into logical groups and to prevent name collisions that can occur especially when your code base includes multiple libraries"

Edited by Vityou
added quote about c++ namespaces

Share this post


Link to post
Share on other sites

* Moderator hat on*

This is a gentle reminder that we are in the For Beginners forum, discussing a project being implemented by a beginner asking for help with is code.

Discussion on the poorly-defined meanings of object oriented-ness are not appropriate.  Discussions in For Beginners should be focused on the actual problem the beginner is having.

* Moderator hat off*

Now, on to the updated code.

7 hours ago, reders said:

Here's the upgraded .cpp ... What should I consider doing to improve the code?

I think that code is much more readable, even though it takes up more lines.  Hopefully you agree, as it is your code and you're the one who gets to live with it.

If I use the code-collapse feature of my IDE, the code is broken up into easily understood blocks:  main(), Initialize(), Move_ball(), AI_Movement(), Player2_Movement(), and Reset_all().  I can look at that and understand your game.

Expanding any one of those gives me logic that I can easily understand. For example Player2_Movement() opens to keypressed up and keypressed down.  Each of those actions is easily understood when I expand them.

Those are good, especially in beginner code.

If I were looking to improve it -- even though what you've got in there looks like a runnable game -- I would next try to simplify by merging duplicate functionality. 

Moving up and moving down seem like the same operation, the only difference is that one is positive and the other negative, you can use a clamp function to limit movement both at the top and the bottom.  That's 7 lines removed.   The AI has different logic controlling if they move the paddle up or down, but the actual logic of moving of a paddle is the same as you're only modifying the y value. Extract the duplicate paddle-moving logic to a new function that accepts both a reference to a paddle and the direction it moves, and you remove another 10 or so lines of duplication.

Moving the ball also has a lot of duplicate math that depends on ballsp_x is positive or negative. That duplicate math can probably also be merged.

I'd also look at more consistent naming conventions.  Sometimes they start with uppercase or lowercase, sometimes they have underscores, sometimes they have abbreviations. There are automated ways to rename all variable instances in most IDEs, so it is usually an easy thing to clean up.

Games are shipped but never really done. There are always improvements developers want to see and bugs that could be fixed.  Players don't normally dig through the source code, and if they did often they'd encounter nightmare fuel.  If your code does the things you want to do and fits whatever performance criteria you've been given, you can call it good enough, even when it could be better.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now


  • Advertisement
  • Advertisement
  • Popular Tags

  • Advertisement
  • Popular Now

  • Similar Content

    • By francoisdiy
      So I wrote a programming language called C-Lesh to program games for my game maker Platformisis. It is a scripting language which tiles into the JavaScript game engine via a memory mapper using memory mapped I/O. Currently, I am porting the language as a standalone interpreter to be able to run on the PC and possibly other devices excluding the phone. The interpreter is being written in C++ so for those of you who are C++ fans you can see the different components implemented. Some background of the language and how to program in C-Lesh can be found here:

      http://www.codeloader.net/readme.html
      As I program this thing I will post code from different components and explain.
    • By isu diss
      I'm trying to duplicate vertices using std::map to be used in a vertex buffer. I don't get the correct index buffer(myInds) or vertex buffer(myVerts). I can get the index array from FBX but it differs from what I get in the following std::map code. Any help is much appreciated.
      struct FBXVTX { XMFLOAT3 Position; XMFLOAT2 TextureCoord; XMFLOAT3 Normal; }; std::map< FBXVTX, int > myVertsMap; std::vector<FBXVTX> myVerts; std::vector<int> myInds; HRESULT FBXLoader::Open(HWND hWnd, char* Filename, bool UsePositionOnly) { HRESULT hr = S_OK; if (FBXM) { FBXIOS = FbxIOSettings::Create(FBXM, IOSROOT); FBXM->SetIOSettings(FBXIOS); FBXI = FbxImporter::Create(FBXM, ""); if (!(FBXI->Initialize(Filename, -1, FBXIOS))) { hr = E_FAIL; MessageBox(hWnd, (wchar_t*)FBXI->GetStatus().GetErrorString(), TEXT("ALM"), MB_OK); } FBXS = FbxScene::Create(FBXM, "REALMS"); if (!FBXS) { hr = E_FAIL; MessageBox(hWnd, TEXT("Failed to create the scene"), TEXT("ALM"), MB_OK); } if (!(FBXI->Import(FBXS))) { hr = E_FAIL; MessageBox(hWnd, TEXT("Failed to import fbx file content into the scene"), TEXT("ALM"), MB_OK); } FbxAxisSystem OurAxisSystem = FbxAxisSystem::DirectX; FbxAxisSystem SceneAxisSystem = FBXS->GetGlobalSettings().GetAxisSystem(); if(SceneAxisSystem != OurAxisSystem) { FbxAxisSystem::DirectX.ConvertScene(FBXS); } FbxSystemUnit SceneSystemUnit = FBXS->GetGlobalSettings().GetSystemUnit(); if( SceneSystemUnit.GetScaleFactor() != 1.0 ) { FbxSystemUnit::cm.ConvertScene( FBXS ); } if (FBXI) FBXI->Destroy(); FbxNode* MainNode = FBXS->GetRootNode(); int NumKids = MainNode->GetChildCount(); FbxNode* ChildNode = NULL; for (int i=0; i<NumKids; i++) { ChildNode = MainNode->GetChild(i); FbxNodeAttribute* NodeAttribute = ChildNode->GetNodeAttribute(); if (NodeAttribute->GetAttributeType() == FbxNodeAttribute::eMesh) { FbxMesh* Mesh = ChildNode->GetMesh(); if (UsePositionOnly) { NumVertices = Mesh->GetControlPointsCount();//number of vertices MyV = new XMFLOAT3[NumVertices]; for (DWORD j = 0; j < NumVertices; j++) { FbxVector4 Vertex = Mesh->GetControlPointAt(j);//Gets the control point at the specified index. MyV[j] = XMFLOAT3((float)Vertex.mData[0], (float)Vertex.mData[1], (float)Vertex.mData[2]); } NumIndices = Mesh->GetPolygonVertexCount();//number of indices MyI = (DWORD*)Mesh->GetPolygonVertices();//index array } else { FbxLayerElementArrayTemplate<FbxVector2>* uvVertices = NULL; Mesh->GetTextureUV(&uvVertices); int idx = 0; for (int i = 0; i < Mesh->GetPolygonCount(); i++)//polygon(=mostly triangle) count { for (int j = 0; j < Mesh->GetPolygonSize(i); j++)//retrieves number of vertices in a polygon { FBXVTX myVert; int p_index = 3*i+j; int t_index = Mesh->GetTextureUVIndex(i, j); FbxVector4 Vertex = Mesh->GetControlPointAt(p_index);//Gets the control point at the specified index. myVert.Position = XMFLOAT3((float)Vertex.mData[0], (float)Vertex.mData[1], (float)Vertex.mData[2]); FbxVector4 Normal; Mesh->GetPolygonVertexNormal(i, j, Normal); myVert.Normal = XMFLOAT3((float)Normal.mData[0], (float)Normal.mData[1], (float)Normal.mData[2]); FbxVector2 uv = uvVertices->GetAt(t_index); myVert.TextureCoord = XMFLOAT2((float)uv.mData[0], (float)uv.mData[1]); if ( myVertsMap.find( myVert ) != myVertsMap.end() ) myInds.push_back( myVertsMap[ myVert ]); else { myVertsMap.insert( std::pair<FBXVTX, int> (myVert, idx ) ); myVerts.push_back(myVert); myInds.push_back(idx); idx++; } } } } } } } else { hr = E_FAIL; MessageBox(hWnd, TEXT("Failed to create the FBX Manager"), TEXT("ALM"), MB_OK); } return hr; } bool operator < ( const FBXVTX &lValue, const FBXVTX &rValue) { if (lValue.Position.x != rValue.Position.x) return(lValue.Position.x < rValue.Position.x); if (lValue.Position.y != rValue.Position.y) return(lValue.Position.y < rValue.Position.y); if (lValue.Position.z != rValue.Position.z) return(lValue.Position.z < rValue.Position.z); if (lValue.TextureCoord.x != rValue.TextureCoord.x) return(lValue.TextureCoord.x < rValue.TextureCoord.x); if (lValue.TextureCoord.y != rValue.TextureCoord.y) return(lValue.TextureCoord.y < rValue.TextureCoord.y); if (lValue.Normal.x != rValue.Normal.x) return(lValue.Normal.x < rValue.Normal.x); if (lValue.Normal.y != rValue.Normal.y) return(lValue.Normal.y < rValue.Normal.y); return(lValue.Normal.z < rValue.Normal.z); }  
    • By Karol Plewa
      Hi, 
       
      I am working on a project where I'm trying to use Forward Plus Rendering on point lights. I have a simple reflective scene with many point lights moving around it. I am using effects file (.fx) to keep my shaders in one place. I am having a problem with Compute Shader code. I cannot get it to work properly and calculate the tiles and lighting properly. 
       
      Is there anyone that is wishing to help me set up my compute shader?
      Thank you in advance for any replies and interest!
    • By fishyperil
      I'm looking for some references that could help me learn how to program some really basic 2D enemy behaviours.
      I wasn't sure whether to post this here or in the AI section but I think it might be more suitable to be posted here since it has more to do with basic maths than any AI related algorithms.
      Could anyone help recommend some resources (books, posts, videos) that could help me understand how to properly implement the basics of enemy movement in 2d games ? So far I've only managed to get them to chase the player character and to stop moving on collision, but the movement is pretty unrealistic and once the collision occurs the enemies all "pile up" on the player character. I'm doing this in C++ so no guides that explain how to script this using an engine api please.
    • By LifeArtist
      Good Evening,
      I want to make a 2D game which involves displaying some debug information. Especially for collision, enemy sights and so on ...
      First of I was thinking about all those shapes which I need will need for debugging purposes: circles, rectangles, lines, polygons.
      I am really stucked right now because of the fundamental question:
      Where do I store my vertices positions for each line (object)? Currently I am not using a model matrix because I am using orthographic projection and set the final position within the VBO. That means that if I add a new line I would have to expand the "points" array and re-upload (recall glBufferData) it every time. The other method would be to use a model matrix and a fixed vbo for a line but it would be also messy to exactly create a line from (0,0) to (100,20) calculating the rotation and scale to make it fit.
      If I proceed with option 1 "updating the array each frame" I was thinking of having 4 draw calls every frame for the lines vao, polygons vao and so on. 
      In addition to that I am planning to use some sort of ECS based architecture. So the other question would be:
      Should I treat those debug objects as entities/components?
      For me it would make sense to treat them as entities but that's creates a new issue with the previous array approach because it would have for example a transform and render component. A special render component for debug objects (no texture etc) ... For me the transform component is also just a matrix but how would I then define a line?
      Treating them as components would'nt be a good idea in my eyes because then I would always need an entity. Well entity is just an id !? So maybe its a component?
      Regards,
      LifeArtist
  • Advertisement