I have some code that looks like this:

// Player.hpp
class Player
{
public: enum class State { RUN, WALK, STAND };
};

The enum class State is used by the Player class, as well as by many other external components. This way every other component that needs the Player::State definition has to include the Player.hpp file. I'm not 100% sure about this solution.

Another option would be to put the state in a separate "types.hpp" file, like so:

// types.hpp
enum class PlayerState { RUN, WALK, STAND };

What are in your opinion the pros and the cons of these designs?

up vote 8 down vote accepted

The primary reasons to put a type into a class/struct is for template metaprogramming, or if the type is private to the class. Otherwise, just put it next to the class:

// Player.hpp
...

enum class PlayerState { RUN, WALK, STAND };

class Player {
public:
  ...
private:
   PlayerState m_state;
};

In C++, it is common that you will have some helper declarations for a class. So it is not reasonable to enforce a one declaration per header policy.

Putting this declaration in a separate file can be sensible if the PlayerState is used separately from the Player. But given these names, that's unlikely to be the case.

Separate headers are especially helpful when the header needs to include additional headers for some declarations. Because C++ (currently) has no module systems, the contents of those headers also become available to whoever includes the header you are writing – an impact we would like to minimize. An alternative solution to these header dependencies is to abstract over details of your class definitions, e.g. using polymorphism (the header declares only an interface whereas the implementation classes are internal to a .cpp file), or using the pImpl idiom.

  • Did you mean to remove the enums public access modifier? – candied_orange May 20 at 13:48
  • @CandiedOrange In C++, visibility only affects class/struct members. Declarations in namespaces (or the top level) are always public. To have private declarations in a header, by convention they are put into a nested namespace detail or similar. – amon May 20 at 13:51

This looks like a case where you want namespaces. They're the natural way in C++ to group related names.

Namespaces, unlike classes, can be reopened. That means you can have multiple headers all adding to the same namespace. Just look at all the headers that contribute to namespace std

Sometimes a good practice is to have a "Defs.h" file which will have the emuns and constants that are used by several classes, which those classes will include.

  • 1
    Disagree completely. Definitions should go where there are used. PlayerState should go in Players.hpp because it is only used by the Player class. If you have definitions that are used thru-out your app, they should go in AppDefs.hpp. – shawnhcorey May 20 at 12:35
  • @shawnhcorey, that's what I've meant by "used by several classes". your last sentence is what I wrote, you "disagree completely" (drama) with something I didn't even wrote. – Gomunkul May 20 at 23:09
  • The states of a state machine will only be used by a single class and its derived classes. To place the identifiers in a file not associated with the class is to break encapsulation. – shawnhcorey May 21 at 8:03

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.