Code Review

This is a code review of Potato Pirates made by group 18. Most of the design changes I’m going to suggest that could have been made are things that could be good to keep in mind for future projects, but some of these might just add unnecessary workload to implement right now for their project, as it’s relatively near the end. Some of these changes would mean a bit more code that would be put in managers in the beginning, but would mean less code for each entity that is added to the game. Overall it should make the code as a whole shorter, more relevant to everything else at any specific place in the code, and easier to modify. As this assignment we got was to focus only on the player class, all of the suggestion I give will somehow concern that class.
It’s unnecessary to define the position and position related function like setPos(), getPos(), getX(), and getY() in the player class, same for other game entities/game objects. There are a few other options I think they would be good do here; one way is to make a game entity/game object class, add a position variable in it and position related functions in it, and make their player class and other entities inherit from it. An other way is to make a class for holding a position and position related functions, and each of their entities stores an instance of that class in a variable in the entity. A class containing a position, rotation, and scale, and its related methods like e.g. move() and rotate() is often called transform. Keeping these in a separate class like transform means they won’t have to redefine them in each entity they create. A third way would be a mix of the first and the second option; by having each entity inherit of a game object and having a transform variable in the game object class. I will now give an example of the first and second option.
Example:

class Player : Tranform
{
void update()
{
setPosition(50, 30);
}
}

Or:

class Player
{
Tranform transform;
void update()
{
transform.setPosition(50, 30);
}
}
And the Transform class for both cases:
class Transform
{
Vector2 position;
void setPosition(float x, float y)
{
position.x = x;
position.y = y;
}
}

99% of Everything related to an player ability should be in its own class and everything about a pick up should be in its own class. That way, if they want to find anything related to that ability, they’ll know where to find it. It will also create less dependencies, if they want to change one thing about it then they won’t have to change it in several separate places in the code. For example instead of having the “//Handles abilities //Firework” part in the player class, they could do:

if ((system_.inputManager->isKeyPressed(sf::Keyboard::Num1) || system_.inputManager→isKeyPressed(sf::Keyboard::Numpad1))
{
fireworks->activate();
}

Put it all in that activate function. Anything else related to firework like e.g. fireworkCooldown_ could be put in fireworks. An instance seems to be created of the AbilityFirework class for each firework in the game, but to make it work with everything related to fireworks you could either make fireworks→activate(), and possibly other things a static function in the AbilityFirework class, or they could alternatively make a new firework manager class, and put fireworkCooldown_ as static in there. The fireworks->activate() would create a new instance of firework like done currently in the Player class. There shouldn’t need to be any other reference to anything firework related in the player class. This way everything related to firework is in the firework class. Potato cloud be should dealt with in a similar manner.
It’s unnecessary to keep variables for the sprites in the player class. They could instead just tell the sprite manager/texture manager the path of the texture, and then if it’s not loaded, the sprite manager/texture manager would load that texture and store the sprite in a std::map or other container, otherwise if it is loaded it would find the matching sprite by the in the std::map by its path.
Everything related to the tutorial could be in a tutorial class or similar, instead of having the tutorial stuff in the player class. The code under “//Handle escape = quite to mission select.” is not relevant to the player class and should not be put there. It could be good to seperate the noise-enemy-detection-circle into its own class.
If their hitbox is to be the same size as its sprite, then the size needn’t be specified and can instead be gotten from the texture inside the sprite manager using the SFML function sprite->getGlobalBounds(). The position of the sprite and hitbox doesn’t need to be defined in the player class either, if they use a transform class as specified above, then they could do this in a generic way by having all the game entities/game objects kept in a list, iterating through all of them in the sprite manager, getting the transform of each game entity/game object, and rendering the sprite according to the position, size, and rotation that is specified in the transform of that game entity/game object. This would happen in the update of the sprite manager, and the same could be done in the collision manager.
The ‘System system’ variable that is passed in the player class constructor, as well in a lot of other constructors, is unnecessary. For the first; it’s passed to the constructor by value, which means that the whole class instance is copied to the to the constructor. This means it would use up more memory, and be slightly slower, though the difference in speed in this case is likely negligible. Only relative small stuff should preferably be passed by value, you should otherwise pass by reference or pass a pointers. For the second; Why pass it to the constructors at all, it only contains pointers that points to the same things anyway in each instance, just make it a global variable instead. For the Third; Actually; don’t make it a global variable, as there’s really no reason for it to be an instance; you could instead set its member variables and functions as static, and access it with ‘::’. This could just be my preference, but if a class exists during the whole game, and there always only is one of it, then I think it always should be static. For the fourth; the variables in system could also just as well be static, so the whole system class is unnecessary. Maybe I ventured a bit too much away from the player class..
Just to inform, the ‘settings.txt’ file doesn’t seem to be source controlled/version controlled. Maybe the ‘.gitignore’ file is set to ignore ‘.txt’ files?
There’s a lot that could improved, but I think it’s not necessarily bad code, but it could definitely be improved a lot. What matters in the end is what the code produces though, and if the game is good and they’re satisfied with the end product, then the code should be good enough.