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.
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".
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.