Let's say I want to build a Car which has components like Motor, Tire, etc. which are all derived from a "component" base class. Each component has its own states (i.e. motor has RPM, tire has pressure, and so on) as a subclass. A Car class stores all component classes.
Now I want the Car class to have a "save car states" function which returns an object that loops over a vector of all components and holds all states of all components to restore them later. Each component should individually be responsible of storing and restoring its states to the car state object.
The code below is a minimal example of what I came up with. However it has (at least) two ugly parts in the code, which I think should be avoided:
First, I need to downcast a Base class of states, when I want to set the car components back to a previously saved state. Is this a valid case to downcast? I read that this is usually a sign of bad design. Thus, how can I improve this? Did I completely misuse the concept of polymorphism?
Second, I need to get the raw pointer of an unique_ptr to pass the state back to the component and read it there. Seems pretty ugly to me as well. Thanks for your comments!
This is my code:
#include <iostream>
#include <vector>
class StateClass{
};
class component{
public:
virtual std::unique_ptr<StateClass> saveStates() = 0;
virtual void loadStates(StateClass* states) = 0;
};
class Motor:public component
{
public:
Motor(){
mstates.rpm = 6000;
mstates.motorstates::oilLevel = 1.0;
}
struct motorstates: public StateClass{
double rpm;
double oilLevel;
void setStates(){
}
};
std::unique_ptr<StateClass> saveStates(){
std::unique_ptr<StateClass> tmp(new motorstates(mstates));
return tmp;
};
void loadStates(StateClass* states){
motorstates* savedState = static_cast<motorstates*>(states); // <=== should this be avoided??
mstates.rpm = savedState->rpm;
mstates.oilLevel = savedState->oilLevel;
};
// private:
void someMethod1();
void someMethod2();
motorstates mstates;
};
class Car{
public:
Car(){
listOfComponents.push_back(&amotor);
}
std::vector<component*> listOfComponents;
// private:
Motor amotor;
std::vector<std::unique_ptr<StateClass>> saveState(){
std::vector<std::unique_ptr<StateClass> > states;
for(auto comp : listOfComponents){
states.push_back(comp->saveStates());
}
return states;
}
void loadState(std::vector<std::unique_ptr<StateClass>>& savedStates){
int cntstates = 0;
for(auto comp: listOfComponents){
comp->loadStates(savedStates.at(cntstates++).get()); // <=== this seems pretty ugly
};
}
};
int main()
{
Car acar;
std::cout << "Car rpm: " << acar.amotor.mstates.rpm << std::endl;
std::cout << "Saving states..." << std::endl;
std::vector<std::unique_ptr<StateClass>> savedState = acar.saveState();
std::cout << "Changing car rpm..." << std::endl;
acar.amotor.mstates.rpm = 5000;
std::cout << "Current car rpm: " << acar.amotor.mstates.rpm << std::endl;
StateClass* tmpState = savedState.at(0).get();
Motor::motorstates* tmpMotorstates = static_cast<Motor::motorstates*>(tmpState);
std::cout << "Saved rpm: " << tmpMotorstates->rpm << std::endl;
std::cout << "Loading state... " << std::endl;
acar.loadState(savedState);
std::cout << "Current car rpm: " << acar.amotor.mstates.rpm << std::endl;
}
Additional remarks (see comments):
These ugly things are symptoms of a design issue caused by the mutual dependency between the Component structure and the State structure.
In other words, you have defined a polymorphic State, but most of the time you use it expecting a specific state subclass.
In addition, the configuration of the car is in fact also a state: if you would change anything in its configuration, you'd no longer be able to restore anything (even if you just added an irrelevant spare-part).
If you keep this design, you should in any case make the code more robust. What happens, for example, if by accident you give a State pointer which does not meet the expectations ?
motorstates* savedState = static_cast<motorstates*>(states);
This would be UB ! Fortunately your state is already a polymorphic type. So you could use dynamic cast instead:
motorstates* savedState = dynamic_cast<motorstates*>(states);
if (savedState==nullptr) { // this is true if the state was not of correct class
//ouch ! At least you'd know
}
(By the way, as a rule of thumb that can prevent problems: if you've got a virtual member function, better give your class a virtual destructor.)
A good option for a save/restore capability is to use a memento design pattern. The idea is that the the obect saves or restores its state from a memento, which content and structure is unknown from the "caretaker" (i.e. the code responsible of keeping the memnto and invoke the safekeeping).
Consequence: the internals of a CarMemento would be known to the Car only. This leans that you'll not be able to restore a partial state (e.g. engine state only). The mementos would not necessarily be polymorphic objects : the save state/restore state would anyway use specific Memento types for the different Components.
It seems that you have the components as private members and not as a dynamic item. If this is confirmed this would be the safest way to do.
Another approach would be adopt the composite design pattern for your component and combine it with the memento.
I'd then implement the state-saving using the principle of serialisation (even if it's into a memory object), and saveguard not only the states but the full composite structure. I'd then use a factory to deserialize a saved state.
However, if you prefer to save only the states of the object, you could combine composite with memento:
In both case however, you'd have to use dynamic cast to ensure that the types of the saved state match with the type of the state to be restored.