Porting an existing class structure to smart pointers

Question

I know this question is rather long, but I was not sure how to explain my problem in a shorter way. The question itself is about class hierarchy design and, especially, how to port an existing hierarchy based on pointers to one using smart pointers. If anyone can come up with some way to simplify my explanation and, thus, make this question more generic, please let me know. In that way, it might be useful for more SO readers.

I am designing a C++ application for handling a system that allows me to read some sensors. The system is composed of remotes machines from where I collect the measurements. This application must actually work with two different subsystems:

1. Aggregated system: this type of system contains several components from where I collect measurements. All the communication goes through the aggregated system which will redirect the data to the specific component if needed (global commands sent to the aggregated system itself do not need to be transferred to individual components).

2. Standalone system: in this case there is just a single system and all the communication (including global commands) is sent to that system.

Next you can see the class diagram I came up with:

The standalone system inherits both from ConnMgr and MeasurementDevice. On the other hand, an aggregated system splits its functionality between AggrSystem and Component.

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

CURRENT IMPLEMENTATION

This is my current implementation. First, the two base 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 an 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 it is main(), showing a very simple usage case:

#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;
}

PROPOSED CHANGES

First of all, I wonder whether there is a way to avoid the dynamic_cast in get_measurement_device. Since AggrSystem::get_measurement_device(int slot) and StandAloneSystem::get_measurement_device() have different signatures, it is not possible to create a common virtual method in the base class. I was thinking to add a common method accepting a map containing the options (e.g., the slot). In that case, I would not need to do the dynamic casting. Is this second approach preferable in terms of a cleaner design?

In order to port the class hierarchy to smart pointers I used unique_ptr. First I changed the map of components in AggrSystem to:

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

The addition of a new Component now looks like:

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))));
}

For returning a Component I decided to return a raw pointer since the lifetime of a Component object is defined by the lifetime of an 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();
}

Is returning a raw pointer a correct decision? If I use a shared_ptr, however, then I run into problems with the implementation for the standalone system:

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

In this case I cannot return a shared_ptr using this. I guess I could create one extra level of indirection and have something like StandAloneConnMgr and StandAloneMeasurementDevice, where the first class would hold a shared_ptr to an instance of the second.

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

P.S: create_conn_mgr and main are changed as well so that instead of using a raw pointer (ConnMgr*) now I use unique_ptr<ConnMgr>. I did not add the code since the question was already long enough.

Answer 1

First of all, I wonder whether there is a way to avoid the dynamic_cast in get_measurement_device.

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

So, overall, I wanted to ask whether this a good approach when using smart pointers.

I think you've done a good job. You've basically converted your "single ownership" news and deletes to unique_ptr in a fairly mechanical fashion. This is exactly the right first (and perhaps last) step.

I also think you made the right decision in returning raw pointers from get_measurement_device because in your original code the clients of this function did not take ownership of this pointer. Dealing with raw pointers when you do not intend to share or transfer ownership is a good pattern that most programmers will recognize.

In summary, you've correctly translated your existing design to use smart pointers without changing the semantics of your design.

From here if you want to study the possibility of changing your design to one involving shared ownership, that is a perfectly valid next step. My own preference is to prefer unique ownership designs until a use case or circumstance demands shared ownership.

Unique ownership is not only more efficient, it is also easier to reason about. That ease in reasoning typically leads to fewer accidental cyclic memory ownership patters (cyclic memory ownership == leaked memory). Coders who just slap down shared_ptr every time they see a pointer are far more likely to end up with memory ownership cycles.

That being said, cyclic memory ownership is also possible using only unique_ptr. And if it happens, you need weak_ptr to break the cycle, and weak_ptr only works with shared_ptr. So the introduction of an ownership cycle is another good reason to migrate to shared_ptr.