Is there a good way to do this C++ single-player class?

Question

I am making a little single-player game and to help with scoring I have created a Player class. Can you review my implementation?

I know that for a multiplayer game this isn't good. For multiplayer I would need to call functions and have all players in one instance.

File player.h

namespace SDLGamecore {
namespace player {
 class Player
 {
 private:
 long cash;
 int workers;
 std::string name;
 public:
 Player(std::string playerName)
 {
 name = playerName;
 cash = 0;
 workers = 0;
 }
 ~Player(){}
 long getCurrentCash() const;
 int getWorkers() const;
 std::string getPlayerName() const;
 };
}}

File player.cpp

namespace SDLGamecore {
namespace player {
 long Player::getCurrentCash() const
 {
 return cash;
 }
 int Player::getWorkers() const
 {
 return workers;
 }
 std::string Player::getPlayerName() const
 {
 return name;
 }
}}

File main.cpp

Player player("Test");
std::string pName = player.getPlayerName();
std::cout << pName << std::endl;

The code works as expected. Thank you for any feedback and assistance in improving my single player class.

Answer 1

There's not a lot of substance here to review, but anyway, here are some things that may help you improve your code.

Use include guards

There should be an include guard in the .h file. That is, start the file with:

#ifndef PLAYER_H
#define PLAYER_H
// file contents go here
#endif // PLAYER_H

Make sure you have all required #includes

The code uses std:string within player.h but doesn't #include <string>. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation.

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got, so you can simply omit both the declaraton and implementation from your code.

Prefer modern initializers for constructors

The constructor could use the more modern initializer style rather than the old style you're currently using. Instead of this:

Player(std::string playerName) 
{
 name = playerName;
 cash = 0;
 workers = 0;
}

You could use this:

Player(std::string playerName) :
 cash{0},
 workers{0},
 name{playerName}
{ }

Write member initializers in declaration order

When you start writing constructors in the modern style mentioned above, you'll want to make sure that you write the initializers in the same order as they are declared. Because members are always initialized in declaration order and cash is declared before name in this class. To avoid misleading another programmer, you should make the order match in all constructors.

Don't write Java in C++

While it may be fashionable in Java to write canonical getters and setters of the form you've got, it's definitely not C++ style. Consider these two lines from main:

std::string pName = player.getPlayerName();
std::cout << pName << std::endl;

We already know the object is a player, so saying player.getPlayerName() seems redundant. I'd prefer to just write player.name() like this:

std::cout << player.name() << std::endl;

Rethink namespaces

Is it really necessary to have a player namespace encapsulating a Player class? Specifying your own namespace is a good idea, but nesting them deeply is probably not.

Consider using inheritance

It's likely that your eventual game is going to have more than just Player class objects and it's also likely that those other objects will also have names. This suggests that it might make sense to have a base class that includes common things such as name and then derive more specialized objects from it.

Provide the entire code to simplify reviews

Finally, it's not really about the code but about the review process. A minimal real main.cpp would look something like this:

#include <iostream>
#include <string>
#include "player.h"
int main() {
 using namespace SDLGamecore::player;
 Player player("Test");
 std::cout << player.name() << std::endl;
}

It's usually better to post code that will actually compile without modification rather than snippets of code that require the reviewer to imagine the missing bits. Including everything needed will help you get better and more useful reviews.

• leviathanbadger: I may be wrong, but I believe that the default constructor that the compiler generates will NOT initialize the int/long to 0. You need to explicitly define the constructor to have this behavior.
• Edward: I wrote only about the default destructor.
• leviathanbadger: Oops, you're right. My bad!
• Ped7g: I had hard time to imagine what to answer to OP, yet I agreed with every line from you, surprised by the length of it, nice review. I have just one tiny addendum: constructor Player(std::string playerName) -> I would rather define it as Player(const std::string & playerName) to pass the initial value by reference.

Answer 2

No Updates?

The main thing that stands out about your current code is that none of your class members can be set after the class has been constructed. This probably makes sense for the name, however for cash and workers are probably going to need to change during the course of the game.

You'll need to consider if you want to go down the simple property style:

void setCurrentCash(long newCashAmount)

Or a more logic centric approach:

long spendCash(long amountToSpend) // returns new cash amount

Which approach works is dependant on how the rest of your application is being put together.

getPlayerName

I'd also consider renaming getPlayerName to getName. You're calling it on the player, so Player is implied.

• Daniel Jour: "no updates" can work perfectly well when writing rather functional style code, though it would make more sense to KISS then and make it a plain data object struct Player {long cash; int workers; string name;}; Though without knowing more about the rest of the code it's hard to tell whether this would make any sense.
• forsvarir: @DanielJour Whilst immutable objects can make perfect sense, in this particular instance since cash and workers can't be set from the constructor they will always be at their default value of 0. It's possible that the OP intended to create an immutable object and omitted them from the constructor by accident, however it seems more likely (at least to me) that they haven't thought that far ahead which suggests that updating the object is the more likely outcome.

Answer 3

First it seems your cash and workers are not used. You set them to 0 in the constructor but you can never change them after as they are private and have no setters.

If you only use your class as a place to stick variables together, why not putting those in a data structure (public members and no functions) instead ? After all, why would you bother making your variables private and write getters when you could simply make them public (and const if you don't want them to be set after initialization).

struct Player {
 long cash;
 int workers;
 std::string name;
}

If you need some of those variables to be read-only, you can simply make them const. For example :

struct Player {
 long cash;
 int workers;
 const std::string name;
}

Your main can then become :

Player player = {"name"};
std::cout << player.name << std::endl;