• Advertisement
Sign in to follow this  

Looking for someone to review my code

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

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello!

 

I am pretty new to the game development. For my language of choice I decided to pick up C++ and used SFML as framework. As I am beginner I decided to start with a humble project, that being Pong! I've finished base game pretty quickly so I decided I would like to make it more interesting and practice a bit more on it so I added stuff like Powerups which can be hooked which will either increase size of your paddle or give you a gun that shoots bullets(Bullets don't do much atm). Now I would like for someone to go over my code and give me some tips on what I did wrong or if I could do something better. I don't really expect you to go through ALL of my code just quickly go over some parts of it. Here is my bitbucket project: https://bitbucket.org/toniron95/phingyphongy/src/d31aa1391feb8ef8435fafab85a598231d61d052/PingyPongy/src/?at=master .

 

Thank you!

Share this post


Link to post
Share on other sites
Advertisement

I had a brief look and it seems really good. It's neat, easy to follow and I can't see anything immediate that jumps out to me. You are using references a lot so you're avoiding issues with pointers but I did spot a few times pointers being passed in to methods and used without checking if they are non-null first (I think it was the power ups). Also, can anything fail on here? For example, what happens if the textures fail to load?

 

A little bit of hardening, checking variable have sensible values, pointers aren't null, any methods that can fail gives feedback for that (mainly when initialising) even if it's just returning false. Things that might seem ok and unnecessary for this project might end up causing you a lot of headache when you work on something larger. Looks great though, good job.

Share this post


Link to post
Share on other sites

I had a brief look and it seems really good. It's neat, easy to follow and I can't see anything immediate that jumps out to me. You are using references a lot so you're avoiding issues with pointers but I did spot a few times pointers being passed in to methods and used without checking if they are non-null first (I think it was the power ups). Also, can anything fail on here? For example, what happens if the textures fail to load?

 

A little bit of hardening, checking variable have sensible values, pointers aren't null, any methods that can fail gives feedback for that (mainly when initialising) even if it's just returning false. Things that might seem ok and unnecessary for this project might end up causing you a lot of headache when you work on something larger. Looks great though, good job.

Thank you for taking your time to go through my code! I was never bothered about nullchecks so far so I guess that is the part I should improve on :d I'll take some time and go over that before moving on. Thanks again!

Share this post


Link to post
Share on other sites

I just looked at Game.cpp and already caught that you: pass ints and floats by references (you should pass them by value), store something returned by value in a non const reference (getPosition() in line 48, this doesn't even compile for me), don't use const enough for function local variables (IMO), named a function local variable m_Event as if it were a member variable, put ampersand near the name instead of near the type (IMO much more common, and it's what SFML uses), handle your events completely wrong (see http://www.sfml-dev.org/documentation/2.3.2/classsf_1_1RenderWindow.php#a338e996585faf82e93069858e3b531b7 ).

Maybe you should go ask about this in SFML forum for more (unless you did and I didn't notice).

Share this post


Link to post
Share on other sites

Thank you for your input! About events tho, not so sure what is different than mine? I poll events in variable once per game loop and then I pass that variable to virtual function handleInput which every entity has. I do that so I can handle input in every entity class separately, if I need to. 

Share this post


Link to post
Share on other sites

You should handle them in a while loop because you can get more than one event per frame. You should also not touch the event member variables at all if pollEvent didn't return true and there is a chance you do that in line 17 because it always executes.

Edited by FRex

Share this post


Link to post
Share on other sites

Overall it looks like a competent effort, I would wager that this is not your first time programming.

 

I've only had time for a quick click-through but here are some things I spotted:

 

  • #includes should go in the source (.cpp) files rather than the header (.h) files by default. In the headers you should use forward-declarations instead. If a forward-declaration isn't possible/practical only then do move the #include to the header.
  • Relative #include paths for things in different modules seems (to me) to be a bad idea (Example: Ball.h references TextureManager by relative path). Use absolute paths from the base of your project tree instead. Use relative paths for referencing files within the same logical module (i.e files that might be moved around together).
  • Your Entity class is obviously a virtual base class but it doesn't have a virtual destructor!
  • Magic numbers are bad - for example the numbers 1024 and 768 in Ball.cpp. These values should be factored out to a constant. That way if they ever need to change then there is just one place to go to change them. Also it gives them a name which means other coders can know what they are.
  • Passing primitive types (like float, int, etc) by const reference - don't bother. Just pass by value.
  • Complex types (like std::string) being passed by value (see ResourceManager.h) - you should pass these by const reference.
  • public data members in classes - We've all been there but it goes against several best practice rules of thumb and I see that you have quite a few places where it happens. Better to keep them private and expose operations via the class member functions. Or it could also be an indicator that these data members don't belong in those classes in the first place.

Share this post


Link to post
Share on other sites

You define Pi in a two local scopes within Util.h. Just define it as a public static constant of the Util class or else use an externally defined Pi. It wouldn't hurt to add some precision beyond 3.14 either, since you're using a double. In fact, since this whole class is public and static, you should probably be using a namespace instead.

 

If you want a float from atan2() then use atan2f() instead. Use floats for the deltas, since you're working in a float environment. There's no reason to cast away precision here just because your mouse coords are int.
 
What is this about, anyway? Why are you using y in the x calculation and why are you doubling y in the y calculation?

        int deltaX = (int)mouseCordinates.x - (int)objPosition.x - (int)(objSize.y / 2);
        int deltaY = (int)mouseCordinates.y - (int)objPosition.y - (int)objSize.y * 2;

.
You can avoid some of this by just using the vector objects as they're intended:

sf::vector2f mouseCoordF((float)mouseCoordinates.x, (float)mouseCoordinates.y);
sf::vector2f objCenter = obj.m_Position + (obj.m_Size / 2);
sf::vector2f displacement = mouseCoordF - objCenter;
return atan2f(displacement.y, displacement.x) * (180.0f / PI); //your existing functions use 180/PI, which is incorrect

 
Whatever it is you're trying to do in Paddle::Paddle with m_Hook is not correct. The unique_ptr's that you're using will release your allocations when the constructor exits and you're going to be working with dangling pointers. It may be working for the moment, but as soon as the allocator decides to reuse that memory you're going to get bizarre bugs and crashes. You should also be using std::make_unique rather than new. However, you don't need to use pointers at all for this.

void HookCannon::attachPaddle(Entity & paddleEntity)
{
    m_Paddle = (Paddle*)&paddleEntity;
}

Why does this take an Entity instead of a Paddle? That cast is extremely dangerous and is a strong code smell. Also, this is a non-complex, one-line private function that's only called once, which means that it should not exist. Bumping up to the parent scope - the HookCannon constructor - we find:

HookCannon::HookCannon(TextureManager &textureManager, sf::RenderWindow &renderWindow, Entity &parentPaddle) : m_Angle(0), 
    m_WindowPosition(renderWindow.getPosition()),m_IsDisabled(false)
{
    m_CannonSprite.setTexture(textureManager.getResource(TEXTURE::HOOK_CANNON));
    m_Size = sf::Vector2f(m_CannonSprite.getLocalBounds().width, m_CannonSprite.getLocalBounds().height);
    m_CannonSprite.setOrigin(0, m_CannonSprite.getLocalBounds().height / 2);
    m_EntityType = EntityType::HOOK_CANNON;
    m_Paddle = (Paddle*)&parentPaddle; //replaced
}

Which leaves us with the same casting problem, but there's no reason that parentPaddle should be an Entity here. The HookCannon constructor is called by the Paddle constructor and is passed as *this, which means you're passing a Paddle into an Entity& and then casting it to Paddle by pointer. You also don't do any reseating of the pointer, and it's not nullable, so it may as well be a reference. (That'll screw with your copy/move constructors, so disable or implement them.)

 

Entity seems to be causing a bit of trouble in other places too. (https://en.wikipedia.org/wiki/Liskov_substitution_principle)

 

You may want to consider a different strategy.

 

 

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement