First SFML/C++ project - Need feedback

[nyenye]

This week I've taken upon myself to learn C++, and given that I like very much anything related to video games, what best to make a game to learn the language 

My first game is a PONG clone. Gameplay aspects are more or less covered. As a list of things to implement/improve could be as follows:

• Make the rebound true to angle and velocity.
• Display players scores.
• Make a menu

yet I wanted to focus on learning C++ so didn't pay much attention to details.

I would be very glad to read any and all feedback. From code styling to best and worst practices or project structure... anything is good.
A good sir from the forums already told me that I overuse passing arguments by reference when maybe it´s not due (on small objects and such).

Thanks for the awesome community!

Edit: This is the repo: https://git.albertdev.com/NexusBattle/sfml-pong/tree/develop

[Hapax]

Hi!

Just a few little things I spotted:

PongGame game = PongGame();

I'm not sure why you are creating a PongGame and then assigning it to game. It's likely this is actually going to copy this object. When an object is created, its constructor is automatically called (that's the point) so just:

PongGame game;

would create the object in its default state.

I'm not sure I understand your Player class. Shouldn't the player have a paddle?

When using a class's members, there is rarely need for the "this->" part. Members are accessible within any methods.

It doesn't technically matter which order in the class are public sections and private sections but doesn't it make more sense to provide the public interface at the top?

You can inherit from sf::Drawable, which allows you to provide a draw() function that gets called in the same way as other SFML objects. e.g. window.draw(object)

I'm not sure the logic of control works out well. For example, when both up and down are pressed, you are moving the paddle both up and down rather than not at all.

if (this->player == 1)
        this->shape.setPosition(5.0f, 130.0f);
    else
        this->shape.setPosition(615.0f, 130.0f);

Where are those positions? It's possible to work out that since this is in "reset()" and depends on the player number that these are starting positions but it could be clearer by specifying this. Constants and/or comments would help:

constexpr float playerStartingYPosition{ 130.f };
const sf::Vector2f player1StartingPosition{ 5.f, playerStartingYPosition };
const sf::Vector2f player2StartingPosition{ 615.f, playerStartingYPosition};
if (player == 1)
    shape.setPosition(player1StartingPosition);
else
    shape.setPosition(player2StartingPosition);

It seems to be more obvious the intention here. Those actual values could calculated from window or view sizes too...
Also, if this paddle was a part of the player class (as mentioned above), there would be no need to check its player number. Why does the paddle not know which paddle it is? The x position could be stored for each paddle, rather than the player number which is used to decide which x position is used.

A good sir from the forums already told me that I overuse passing arguments by reference when maybe it´s not due (on small objects and such).

Only small "const" references. That is, ones that are not changed inside the function/method. If they are not const, it's likely they are going to modified inside the function; changing them to a copy (passing by value) could not give the intended results.
The point here is that if you passed something to a function and intend it to be modified by that function, you pass it by reference. If you don't intend it to be modified, you pass it by value or const reference. An sf::Vector2f, for example, would be by value.

[nyenye]

Hi! Thanks for that huge post! It was really helpful. Coming from a Java OOP background there are quite a few quirks from basic C++ that I'm not used to (such as not calling constructor). Then the constructor is only called to create pointers with the new operand?

About the use of constexpr, I didn't quite grasp the usage of it when was doing the game, but now I've read what it's useful for.

On using this->; it was intentional. I don't quite like the underscore name styling for member class variables, so I opted for the this-> option.

Yep, now I see that it's better to put public on top. It makes sense. Will do the inerhitance par for drawable. also makes more sense.

The up and down part of the paddles was intentionar, yet an 'or' check won't hurt too much. Will do too.

Last but not least the player part is also very true. The thing is I started working with the paddle, and only when it was working, added the player, with the least number of lines, which led to this "mess".

Just to wrap it up, I noticed that SFML scales up the graphics by default when resizing the window. Do you know if mouse posituon is also scaled? And does it scale everything? Sprites and all?

Thanks a lot it really means much.

[JayhawkZombie]

It doesn't technically matter which order in the class are public sections and private sections but doesn't it make more sense to provide the public interface at the top?

Surprisingly, I've seen a lot of source code with the public interface defined at the bottom of the class.  It seems odd, but maybe there's a new habit going around?

Technically, the constructor is called whenever an object is created or copied (and some other instances).  You'll invoke a copy constructor if you do something like

myClass obj = someOtherObject;

"obj" will be a copy of "someOtherObject".  If you don't define a copy constructor, your compiler will try to generate one for you.  If you use that new object, you may not get the behavior you were expecting.
You generally want to avoid copying as much as possible.
If you just create an object, like

myClass myObject;

a default constructor will be used, if one exists.  If one does not exist, you'll get a compiler error.  The compiler will generate a default constructor for you provided you do not define any constructors yourself, or you declare it as "default" (ie "myClass() = default;" in the class declaration).

One last thing, "this" is a pointer to the calling object.  It is analogous to the one in Java.
You can use whatever naming convention you like, but your code should operate the same with and without the "this".

[nyenye]

I'm starting to feel like there is so much meat when it comes to constructors that I thought at first.

Yep, I know about the fact that 'this' is a pointer (for this reason we use the -> notation i guess). Thanks for the headsup anyways.

Then there is one quirk that I couldn't get my head around. When I created a class, and I added a constructor with arguments, omitting the argless one, the compilation failed   

Thanks again!

[JayhawkZombie]

Then there is one quirk that I couldn't get my head around. When I created a class, and I added a constructor with arguments, omitting the argless one, the compilation failed   

Ah, I feel your frustration.  If you define any constructor, your compiler will no longer generate a no-arg/default one for you unless explicitly instructed to do so.  If you want one, you'll have to define it yourself.
You can either define it completely yourself or explicitly tell the compiler to make one for you.
If you want it to make it for you, even if you have defined other constructors, use the "default" syntax,

class MyClass
{
  MyClass() = default; //Tells compiler to make default one for you
  MyClass(args...); //Some other constructor that isn't the no-arg one
  ...
};

The "default" keyword has been here since forever for switch statements, but C++11 added it as an explicit function declaration for special class member functions (see http://en.cppreference.com/w/cpp/language/member_functions#Special_member_functions). 
You can also "delete" them, if you want to completely disable to use of a specific constructor.  I "delete" some copy constructors in my classes, for example, to control how objects are copied.
You can, of course, have a class that does not have a default constructor if that is what you want to enforce.

[Hapax]

Using "this->" to access member variables as standard seems messy and pointless. The members can be accessed directly without it so it's better to avoid using the same name than trying to make sure it's the right one with the same name. It's a reason the "m_"/"m" prefixes are used: it's always clear which is which (m_value is not value). Using this->, it's not always clear (this->value is the object's member value but is value the object's member value or a local or external variable?). For this reason, I think it's important to make sure that names are different rather than trying to figure out which scope we are working with.

Just to wrap it up, I noticed that SFML scales up the graphics by default when resizing the window. Do you know if mouse posituon is also scaled? And does it scale everything? Sprites and all?

SFML draws to the window using a view. This transforms the all co-ordinates to match the window's current view and fits to the window.
To allow more (or less) window to be displayed as the window is resized, the view must be updated.
The mouse position, however, is given in pixels, not view co-ordinates.
You can, of course, convert to and fro between the two systems.

Note that the view can be confusing at first.
This extra tutorial on the SFML wiki may help.

[nyenye]

Thanks for the link on the tutorial. Really helpful. And I'll try to adapt to the C++ member variable style then. Thanks again for thr tips!