Transfer existing class structure to smart pointers

Question

I know this question is quite long, but I was not sure how to explain my problem in short. The question itself is the design of the class hierarchy and, especially, how to transfer the existing hierarchy based on pointers to one using smart pointers. If anyone can come up with some way to simplify my explanations and thus make this question more general, please let me know. Thus, this may be useful for more SO readers.

I am developing a C ++ application for processing a system that allows me to read some sensors. The system consists of remotes, from which I collect measurements. This application should work with two different subsystems:

• Aggregated system: this type of system contains several components from which I collect measurements. All communications pass through an aggregated system, which, if necessary, redirects data to a specific component (global commands sent to the aggregated system itself should not be transmitted to individual components).

• Autonomous system: in this case there is only one system, and all communication (including global teams) is sent to this system.

Next you can see the class diagram I came across:

A separate system inherits from both ConnMgr and MeasurementDevice . On the other hand, an aggregated system shares its functionality between AggrSystem and Component .

Basically, as a user, I want to have a MeasurementDevice object and transparently send data to the corresponding endpoint, whether it be an aggregated system or an autonomous one.

CURRENT IMPLEMENTATION

This is my current implementation. Firstly, two basic abstract classes:

 class MeasurementDevice { public: virtual ~MeasurementDevice() {} virtual void send_data(const std::vector<char>& data) = 0; }; class ConnMgr { public: ConnMgr(const std::string& addr) : addr_(addr) {} virtual ~ConnMgr() {} virtual void connect() = 0; virtual void disconnect() = 0; protected: std::string addr_; }; 

These are the classes for the aggregated system:

 class Component : public MeasurementDevice { public: Component(AggrSystem& as, int slot) : aggr_sys_(as), slot_(slot) {} void send_data(const std::vector<char>& data) { aggr_sys_.send_data(slot_, data); } private: AggrSystem& aggr_sys_; int slot_; }; class AggrSystem : public ConnMgr { public: AggrSystem(const std::string& addr) : ConnMgr(addr) {} ~AggrSystem() { for (auto& entry : components_) delete entry.second; } // overridden virtual functions omitted (not using smart pointers) MeasurementDevice* get_measurement_device(int slot) { if (!is_slot_used(slot)) throw std::runtime_error("Empty slot"); return components_.find(slot)->second; } private: std::map<int, Component*> components_; bool is_slot_used(int slot) const { return components_.find(slot) != components_.end(); } void add_component(int slot) { if (is_slot_used(slot)) throw std::runtime_error("Slot already used"); components_.insert(std::make_pair(slot, new Component(*this, slot))); } }; 

This is the code for a standalone system:

 class StandAloneSystem : public ConnMgr, public MeasurementDevice { public: StandAloneSystem(const std::string& addr) : ConnMgr(addr) {} // overridden virtual functions omitted (not using smart pointers) MeasurementDevice* get_measurement_device() { return this; } }; 

These are factory-like functions responsible for creating ConnMgr and MeasurementDevice objects:

 typedef std::map<std::string, boost::any> Config; ConnMgr* create_conn_mgr(const Config& cfg) { const std::string& type = boost::any_cast<std::string>(cfg.find("type")->second); const std::string& addr = boost::any_cast<std::string>(cfg.find("addr")->second); ConnMgr* ep; if (type == "aggregated") ep = new AggrSystem(addr); else if (type == "standalone") ep = new StandAloneSystem(addr); else throw std::runtime_error("Unknown type"); return ep; } MeasurementDevice* get_measurement_device(ConnMgr* ep, const Config& cfg) { const std::string& type = boost::any_cast<std::string>(cfg.find("type")->second); if (type == "aggregated") { int slot = boost::any_cast<int>(cfg.find("slot")->second); AggrSystem* aggr_sys = dynamic_cast<AggrSystem*>(ep); return aggr_sys->get_measurement_device(slot); } else if (type == "standalone") return dynamic_cast<StandAloneSystem*>(ep); else throw std::runtime_error("Unknown type"); } 

And finally, here is main() , showing a very simple usage example:

 #define USE_AGGR int main() { Config config = { { "addr", boost::any(std::string("192.168.1.10")) }, #ifdef USE_AGGR { "type", boost::any(std::string("aggregated")) }, { "slot", boost::any(1) }, #else { "type", boost::any(std::string("standalone")) }, #endif }; ConnMgr* ep = create_conn_mgr(config); ep->connect(); MeasurementDevice* dev = get_measurement_device(ep, config); std::vector<char> data; // in real life data should contain something dev->send_data(data); ep->disconnect(); delete ep; return 0; } 

SUGGESTED CHANGES

First of all, I am wondering if there is a way to avoid dynamic_cast in get_measurement_device . Since AggrSystem::get_measurement_device(int slot) and StandAloneSystem::get_measurement_device() have different signatures, it is impossible to create a common virtual method in the base class. I was thinking of adding a generic method that takes a map containing parameters (like a slot). In this case, I would not need to perform dynamic casting. Is this second approach preferable in terms of cleaner design?

To transfer the class hierarchy to smart pointers, I used unique_ptr . First, I changed the map components in AggrSystem to:

 std::map<int, std::unique_ptr<Component> > components_; 

Adding a new Component now looks like this:

 void AggrSystem::add_component(int slot) { if (is_slot_used(slot)) throw std::runtime_error("Slot already used"); components_.insert(std::make_pair(slot, std::unique_ptr<Component>(new Component(*this, slot)))); } 

To return Component I decided to return a raw pointer, because the lifetime of a Component object is determined by the lifetime of the AggrSystem object:

 MeasurementDevice* AggrSystem::get_measurement_device(int slot) { if (!is_slot_used(slot)) throw std::runtime_error("Empty slot"); return components_.find(slot)->second.get(); } 

Does the source pointer return the correct solution? However, if I use shared_ptr , then I have problems with the implementation for an autonomous system:

 MeasurementDevice* StandAloneSystem::get_measurement_device() { return this; } 

In this case, I cannot return shared_ptr using this . I think I could create one additional level of indirection and have something like StandAloneConnMgr and StandAloneMeasurementDevice , where the first class will contain shared_ptr for an instance of the second.

So, overall, I wanted to ask if this is good when using smart pointers. Would it be preferable to use map from shared_ptr and return a shared_ptr , or is the current approach based on using unique_ptr for ownership and a raw pointer for access better?

PS: create_conn_mgr and main also change so that instead of using a raw pointer ( ConnMgr* ) I now use unique_ptr<ConnMgr> . I did not add code, since the question was already quite long.

Answer 1

First of all, I am wondering if there is a way to avoid dynamic_cast in get_measurement_device.

I would try to unify get_measurement_device signatures so you can make this a virtual function in the base class.

So, overall, I wanted to ask if this is good at using smart pointers.

I think you did a good job. You basically converted your "sole owner" news and pretty easily changed to unique_ptr . This is just the first (and possibly last) step.

I also think that you made the right decision to return the source pointers from get_measurement_device , because in your source code the clients of this function did not own this pointer. Working with raw pointers when you are not going to share or transfer ownership is a good model that most programmers will recognize.

So, you correctly translated your existing design to use smart pointers without changing the semantics of your design.

From here, if you want to explore the possibility of changing your design to one related to joint ownership, this is the right step. My preference is to give preference to unique property designs until the use case or circumstances require joint ownership.

Unique ownership is not only more efficient, but easier to reason about. This ease of reasoning usually results in fewer random cyclic patches for memory ownership (cyclic memory). Encoders that simply click shared_ptr each time they see a pointer are more likely to receive memory ownership cycles.

Considering that the use of cyclic memory is also possible using only unique_ptr . And if that happens, you need weak_ptr to break the loop, and weak_ptr only works with shared_ptr . Thus, the introduction of a ownership cycle is another good reason to switch to shared_ptr .