Factory for state classes with varying numbers of reference data members

Question

Consider the following code:

#include <iostream>
#include <vector>
#include <fstream>
class Person {
 std::string name;
public:
 Person (const std::string& n) : name(n) { }
 Person() = default;
 std::string getName() const { return name; }
 void save (std::ostream& os) const { os << name << '\n'; }
 void load (std::istream& is) { while (std::getline(is, name) && name.empty()); }
};
struct InLoveState {
 Person& lovedOne;
 void save (std::ostream& os) const {
 os << "InLoveState" << '\n';
 os << lovedOne.getName() << '\n';
 }
};
struct BeatenByParentsState {
 Person &dad, &mom;
 void save (std::ostream& os) const {
 os << "BeatenByParentsState" << '\n';
 os << dad.getName() << '\n';
 os << mom.getName() << '\n';
 }
};
struct InterviewedByThreePeopleState {
 Person &president, &vicePresident, &manager;
 void save (std::ostream& os) const {
 os << "InterviewedByThreePeopleState" << '\n';
 os << president.getName() << '\n';
 os << vicePresident.getName() << '\n';
 os << manager.getName() << '\n';
 }
};
class System {
 static std::vector<Person*> everybody;
public:
 static void load (std::istream& is) {
 for (int i = 0; i < 3; ++i) {
 Person* person = new Person;
 person->load(is);
 everybody.push_back(person);
 }
 }
};
std::vector<Person*> System::everybody;
int main() {
 Person bob("Bob"), mary("Mary"), sam("Sam");
 InLoveState love{mary};
 BeatenByParentsState beaten{bob, mary};
 InterviewedByThreePeopleState interviewed{bob, mary, sam};
 
 std::ofstream outfile("Save.txt");
 bob.save(outfile);
 mary.save(outfile);
 sam.save(outfile);
 love.save(outfile);
 beaten.save(outfile);
 interviewed.save(outfile);
 
 std::ifstream infile("Save.txt");
 System::load(infile);
}

How to get System::load to load the states generically in one single Factory method when the states have varying numbers of Person& data members? One could define

const std::map<std::string, std::function<void(Person&, std::istream&)>> Factory::simultaneousStatesMap = {
 {
 "InLoveState", [](Person& person, std::istream& is) {
 std::string name;
 std::getline(is, name);
 Person lovedOne = *System::getPersonFromName(name);
 InLoveState* state = new InLoveState(lovedOne);
 state->load(is);
 state->display();
 person.addState(*state);
 }
 },
 {
 "BeatenByParentsState", [](Person& person, std::istream& is) {
 std::string name;
 std::getline(is, name);
 Person dad = *System::getPersonFromName(name);
 std::getline(is, name);
 Person mom = *System::getPersonFromName(name);
 BeatenByParentsState* state = new BeatenByParentsState(dad, mom);
 state->load(is);
 state->display();
 person.addState(*state);
 }
 },
 {
 "InterviewedByThreePeopleState", [](Person& person, std::istream& is) {
 std::string name;
 std::getline(is, name);
 Person president = *System::getPersonFromName(name);
 std::getline(is, name);
 Person vicePresident = *System::getPersonFromName(name);
 std::getline(is, name);
 Person manager = *System::getPersonFromName(name);
 InterviewedByThreePeopleState* state = new InterviewedByThreePeopleState(president, vicePresident, manager);
 state->load(is);
 state->display();
 person.addState(*state);
 }
 }
};

But this is obviously horrible and won't scale well when new state classes are defined. So I defined the following class:

template <typename T, int NumReferences>
class InstantiateWithReferences {
 std::vector<std::string> allNames;
public:
 T* operator()(Person& person, std::istream& is) {
 std::string name;
 for (int i = 0; i < NumReferences; ++i) {
 while (std::getline(is, name) && name.empty());
 allNames.push_back(name);
 }
 return [this] <std::size_t... Is>(std::index_sequence<Is...>&&) {
 return new T(person, *System::getPersonFromName(allNames[Is])...);
 }(std::make_index_sequence<NumReferences>{});
 }
};

This will be used to instantiate the states with their compile-time known number of Person& data members. Then Factory's registerStateFunction, invoked automatically by static members of the state classes, will be defined by:

static void registerStateFunction (const std::string& type) {
 simultaneousStatesMap[type] = [](Person& person, std::istream& is) {
 State* state = InstantiateWithReferences<State, State::num_references>()(person, is);
 state->load(is);
 state->display();
 person.addState(*state);
 };
}

And now all the states are instantiated generically. Here is my entire solution:

#include <iostream>
#include <vector>
#include <fstream>
#include <functional>
#include <map>
#include <utility>
class PersonState;
class Person {
 std::string name;
 std::vector<PersonState*> simultaneousStates;
public:
 Person (const std::string& n) : name(n) { }
 Person() = default;
 std::string getName() const { return name; }
 void addState (PersonState& state) { simultaneousStates.push_back(&state); }
 void save (std::ostream& os) const { os << name << '\n'; }
 void load (std::istream& is) { while (std::getline(is, name) && name.empty()); }
};
template <typename T, int NumReferences>
class InstantiateWithReferences {
 std::vector<std::string> allNames;
public:
 T* operator()(Person& person, std::istream& is) {
 std::string name;
 for (int i = 0; i < NumReferences; ++i) {
 while (std::getline(is, name) && name.empty());
 allNames.push_back(name);
 }
 return generate(person, std::make_index_sequence<NumReferences>{}); 
 }
private:
 template <std::size_t... Is> T* generate (Person&, std::index_sequence<Is...>&&); // return new T(person, *System::getPersonFromName(allNames[Is])...);
};
struct Factory {
 static std::map<std::string, std::function<void(Person&, std::istream&)>> simultaneousStatesMap;
 template <typename State>
 static void registerStateFunction (const std::string& type) {
 simultaneousStatesMap[type] = [](Person& person, std::istream& is) {
 State* state = InstantiateWithReferences<State, State::num_references>()(person, is);
 state->load(is);
 state->display();
 person.addState(*state);
 };
 }
};
std::map<std::string, std::function<void(Person&, std::istream&)>> Factory::simultaneousStatesMap;
template <typename State>
class StateRegistrar {
public:
 StateRegistrar (const std::string& type) { Factory::registerStateFunction<State>(type); }
};
class PersonState {
 Person& stateOwner;
protected:
 PersonState (Person& person) : stateOwner(person) { }
};
class InLoveState : public PersonState {
 Person& lovedOne;
 int loveScore;
 static const std::string tag;
 static const StateRegistrar<InLoveState> registrar;
public:
 static constexpr int num_references = 1;
 InLoveState (Person& stateOwner, Person& love, int score) : PersonState(stateOwner), lovedOne(love), loveScore(score) { }
 InLoveState (Person& stateOwner, Person& love) : PersonState(stateOwner), lovedOne(love) { }
 void save (std::ostream& os) const {
 os << tag << '\n';
 os << lovedOne.getName() << '\n';
 os << loveScore << '\n';
 }
 void load (std::istream& is) { is >> loveScore; }
 void display() const { std::cout << "lovedOne = " << lovedOne.getName() << ", loveScore = " << loveScore << '\n'; }
};
const std::string InLoveState::tag = "InLoveState";
const StateRegistrar<InLoveState> InLoveState::registrar(tag);
class BeatenByParentsState : public PersonState {
 Person &dad, &mom;
 int numBruises, numBeatings;
 static const std::string tag;
 static const StateRegistrar<BeatenByParentsState> registrar;
public:
 static constexpr int num_references = 2;
 BeatenByParentsState (Person& stateOwner, Person& d, Person& m) : PersonState(stateOwner), dad(d), mom(m) { }
 BeatenByParentsState (Person& stateOwner, Person& d, Person& m, int a, int b) : PersonState(stateOwner), dad(d), mom(m), numBruises(a), numBeatings(b) { }
 void save (std::ostream& os) const {
 os << tag << '\n';
 os << dad.getName() << '\n';
 os << mom.getName() << '\n';
 os << numBruises << ' ' << numBeatings << '\n';
 }
 void load (std::istream& is) {
 is >> std::skipws >> numBruises >> numBeatings;
 }
 void display() const { std::cout << "dad = " << dad.getName() << ", mom = " << mom.getName() << ", numBruises = " << numBruises << ", numBeatings = " << numBeatings << '\n'; }
};
const std::string BeatenByParentsState::tag = "BeatenByParentsState";
const StateRegistrar<BeatenByParentsState> BeatenByParentsState::registrar(tag);
class InterviewedByThreePeopleState : public PersonState {
 Person &president, &vicePresident, &manager;
 static const std::string tag;
 static const StateRegistrar<InterviewedByThreePeopleState> registrar;
public:
 static constexpr int num_references = 3;
 InterviewedByThreePeopleState (Person& stateOwner, Person& p, Person& v, Person& m) : PersonState(stateOwner), president(p), vicePresident(v), manager(m) { }
 void save (std::ostream& os) const {
 os << tag << '\n';
 os << president.getName() << '\n';
 os << vicePresident.getName() << '\n';
 os << manager.getName() << '\n';
 }
 void load (std::istream&) { }
 void display() const { std::cout << "president = " << president.getName() << ", vicePresident = " << vicePresident.getName() << ", manager = " << manager.getName() << '\n'; }
};
const std::string InterviewedByThreePeopleState::tag = "InterviewedByThreePeopleState";
const StateRegistrar<InterviewedByThreePeopleState> InterviewedByThreePeopleState::registrar(tag);
class System {
 static std::vector<Person*> everybody;
public:
 static void load (std::istream&);
 static Person* getPersonFromName (const std::string& name) {
 for (Person* person : everybody)
 if (person->getName() == name)
 return person;
 return nullptr;
 }
};
std::vector<Person*> System::everybody;
template <typename T, int NumReferences>
template <std::size_t... Is>
T* InstantiateWithReferences<T, NumReferences>::generate (Person& person, std::index_sequence<Is...>&&) {
 return new T(person, *System::getPersonFromName(allNames[Is])...);
}
void System::load (std::istream& is) {
 for (int i = 0; i < 4; ++i) { // Simplified for this example.
 Person* person = new Person;
 person->load(is);
 everybody.push_back(person);
 }
 std::string name;
// Will load the states for everybody.back() only, for brevity.
 for (int i = 0; i < 3; ++i) { // i < everybody.back()->numStates()
 while (std::getline(is, name) && name.empty());
 Factory::simultaneousStatesMap.at(name)(*everybody.back(), is);
 }
}
int main() {
 Person bob("Bob"), mary("Mary"), sam("Sam"), busyPerson("Mr. Busy");
 InLoveState love(busyPerson, mary, 10);
 BeatenByParentsState beaten(busyPerson, bob, mary, 25, 6);
 InterviewedByThreePeopleState interviewed(busyPerson, bob, mary, sam);
 busyPerson.addState(love);
 busyPerson.addState(beaten);
 busyPerson.addState(interviewed);
 
 std::ofstream outfile("Save.txt");
 bob.save(outfile);
 mary.save(outfile);
 sam.save(outfile);
 busyPerson.save(outfile);
 love.save(outfile);
 beaten.save(outfile);
 interviewed.save(outfile);
 outfile.close();
 std::ifstream infile("Save.txt");
 System::load(infile);
}

Output:

lovedOne = Mary, loveScore = 10
dad = Bob, mom = Mary, numBruises = 25, numBeatings = 6
president = Bob, vicePresident = Mary, manager = Sam

Update: What if InterviewedByThreePeopleState's president data member is of type President& instead of Person&? Then define the member type reference_types for each state class and do away with num_references, which won't be general enough anymore. For example,

class InterviewedByThreePeopleState : public PersonState {
 President &president;
 Person &vicePresident, &manager;
 static const std::string tag;
 static const StateRegistrar<InterviewedByThreePeopleState> registrar;
public:
 using reference_types = std::tuple<President, Person, Person>;
 InterviewedByThreePeopleState (Person& stateOwner, President& p, Person& v, Person& m) :
 PersonState(stateOwner), president(p), vicePresident(v), manager(m) { }
...
};

Then we instead have:

template <typename T, int NumReferences>
template <std::size_t... Is>
T* InstantiateWithReferences<T, NumReferences>::generate (Person& person, std::index_sequence<Is...>&&) {
 return new T(person,
 static_cast<std::tuple_element<Is, typename T::reference_types>::type&>
 (*System::getPersonFromName(allNames[Is]))...);
}

and State::num_references in Factory::registerStateFunction is replaced with std::tuple_size<typename State::reference_types>::value. As for state classes that do not have any extra reference data members, to avoid having to define

using reference_types = std::tuple<>;

for these state classes, which would be annoying and easy to forget, we instead define

template <typename T>
concept has_reference_types = requires(T) {
 typename T::reference_types;
};

and Factory::registerStateFunction will instead have the lines:

State* state;
if constexpr (has_num_references<State>)
 state = InstantiateWithReferences<State, State::num_references>()(person, is);
else if constexpr (has_reference_types<State>)
 state = InstantiateWithReferences<State, std::tuple_size<typename State::reference_types>::value>()(person, is);
else
 state = new State(person);

where

template <typename T>
concept has_num_references = requires(T) {
 T::num_references;
};

to allow my original solution to work if a state class has extra reference data members that are all of type Person& (to avoid static casting to Person& for no reason), i.e.

template <typename T, int NumReferences>
template <std::size_t... Is>
T* InstantiateWithReferences<T, NumReferences>::generate (Person& person, std::index_sequence<Is...>&&) {
 if constexpr (has_reference_types<T>)
 return new T(person, static_cast<std::tuple_element<Is, typename T::reference_types>::type&>(*System::getPersonFromName(allNames[Is]))...);
 else
 return new T(person, *System::getPersonFromName(allNames[Is])...);
}

All this was tested to work.

Answer 1

Firstly, I must say I appreciate your sense of humor. BeatenByParentsState!!! Lol!!!

TLDR

1. Too many global/static variables
2. Incorrect architecture causing the need to write factories that return references.

General Guidelines

1. Layout your class such that public members come before private. This makes your class easier to read
2. Try to avoid global or static variables. There is no need to have everybody as a static member variable. Statics and globals make writing unit tests harder.
3. Don't use references as a class member. This makes your class immovable which means you cannot do things like have a std::vector of your class. Use std::reference_wrapper instead.
4. Use nodiscard and noexcept where applicable.
5. delete everything you new.

Architecture

Looking at your code, I cannot tell who owns what. There are a lot of circular references. Person probably holds PersonState but PersonState has a reference to Person. This is a code smell. I am inclined to remove the simultenousStates member from the Person class and create a separate graph that models this.

Further, the word State does not really work here. Getting beaten by one's parents, loving someone, or getting interviewed is not a state per se. It is more of an action than anything else. This would work perfectly in my graph model, where nodes are Persons and edges are Actions. This in fact removes any need to create a factory that returns references.

Another observation here is that LoveState is not a PersonState as it has a different interface and hence inheriting from it breaks Liskov's Substitution Principal.

I generally like to remove member functions like load and save from all my classes and have them as free functions. This way I can change the representation to support different formats. But since you are using std::stream I won't complain much though remember std::streams are not safe and if someone uses a manipulator on the stream your formatting can get weird.

Person Class

1. Use smart pointers instead of raw pointers to signify ownership. Use std::unique_ptr<PersonState> instead of Person* in simultenousStates. From the code
2. Check the output of std::getline(is, name) seperately. Your code might get stuck if std::getline return an error.

InstantiateWithReferences Class

1. There is no need to have a member variable allNames. If someone instantiates the class and calls the call operator on it twice, you will get duplicates. Move allNames to the call operator overload.
2. If we remove the allNames member you can just replace the class with a free function if you like.
3. generate should return std::unique_ptr instead of raw pointer to signify ownership.
4. Again check return value of std::getline.
5. NumReferences must be size_t not int.

Factory class

1. Add appropriate spacing between different functions. `template is hardly visible.
2. Not sure why the factory needs to display the state, that was created. Are you using it as a log? If so then consider using a logging system.

State Registrar

1. Self-registering of factory objects is generally frowned upon as to may lead to issues with the order of initialization. You have zero-initialized the simultaneousStatesMap variable try to keep it that way else you might have a use be init bug.

InLoveState

1. It seems like you are not initializing the love score. Even if you set it in load, you must initialize the love score.

The same can be said for other states.

System Class

1. Use unique_ptr for ownership.
2. There is no need for the everybody vector to be static if u pass a reference of your System class to your factory.

Here is my code with a graph-based architecture. I am not including load and save for all the classes.

class Person {
public:
 Person(std::string name)
 : mName(std::move(name))
{}
private:
 std::string mName;
};
Person load(std::istream & is)
{
 while(true)
 {
 auto name = std::string{};
 if(!std::getline(is, name))
 throw std::runtime_error("Could get [Person] name");
 return {name};
 }
}
namespace action 
{
class Loves
{
public:
 Loves(Person & lovedOne, int loveScore)
 : mLovedOne(lovedOne)
 , mLoveScore(loveScore)
 {}
private:
 std::reference_wrapper<Person> mLovedOne;
 int mLoveScore;
};
class GotBeatenByParents
{
public:
 GotBeatenByParents(Person & dad, Person & mom)
 : mDad(dad)
 , mMom(mom)
 {}
private:
 std::reference_wrapper<Person> mMom;
 std::reference_wrapper<Person> mDad;
};
using Action = std::variant<Loves, GotBeatenByParents>;
} // namespace Actions
class System 
{
public:
 template<typename... Args>
 void addPerson(Args&&... args)
 {
 mPersons.emplace_back(std::forward<Args>(args)...);
 }
 template<typename Action, typename... Args>
 void addAction(Person& person, Args&&... args)
 {
 mActionMap.insert({&person, Action(std::forward<Args>(args)...)});
 }
private:
 std::vector<std::unique_ptr<Person>> mPersons = {};
 std::unordered_map<Person*, action::Action> mActionMap = {};
};
```

• pacmaninbw: The reason to put the public parts of a class first are only partially due to making the code easier to read, a major reason to do this is that when a team is developing code in parallel the consumers of that code will find it faster and be able to implement the calls to the public interface faster, this allows parallel development to move forward better.