4
\$\begingroup\$

Please review my solution for the following problem. I am interested in:

  • Maintainability
  • Usability
  • Logical correctness
  • Efficiency

Problem: It has 1+ objects each in separated thread and between the objects has an 1:N relations in the emitter side and N:1 relations in the receiver side. The connection has to be removable from both owner object.

Solution: C++ Shell

#include <exception>
#include <iostream>
#include <unordered_map>
#include <memory>
#include <mutex>
#include <string>
#include <vector>

class ConnectionPool;
typedef std::shared_ptr<ConnectionPool> ConnectionPoolPtr;

struct OwnerPools
{
    ConnectionPoolPtr emitterPool_;
    ConnectionPoolPtr receiverPool_;
};

class Connection;
typedef std::shared_ptr<Connection> ConnectionPtr;

void connecting(const ConnectionPtr& connection, const OwnerPools& pools);

typedef std::string ConnectionID;

class Connection
{
public:
    Connection(ConnectionID id);

    ConnectionID id() const;

    void disconnect() const;

protected:
    friend void connecting(const ConnectionPtr& connection, const OwnerPools& pools);

    std::weak_ptr<ConnectionPool> emitterPool_;
    std::weak_ptr<ConnectionPool> receiverPool_;

private:
    ConnectionID id_;

    mutable std::mutex mutex_disconnect_;

    // Connection specific part
    // ...
};

class ConnectionPool
{
public:
    ~ConnectionPool();

    std::vector<ConnectionID> keys() const;

    ConnectionPtr connection(const ConnectionID& id) const;

protected:
    friend void connecting(const ConnectionPtr& connection, const OwnerPools& pools);
    friend void Connection::disconnect() const;

    void add(const ConnectionPtr& connection);
    void remove(const ConnectionID& id);

private:
    mutable std::mutex mutex_connections_;
    std::unordered_map<ConnectionID, ConnectionPtr> connections_;
};

// Connection
Connection::Connection(ConnectionID id)
    : id_(id)
{}

ConnectionID Connection::id() const
{
    return id_;
}

void Connection::disconnect() const
{
    std::lock_guard<std::mutex> lock(mutex_disconnect_);

    // Do connection specific disconnect
    // ...

    // Remove from connections pool
    auto emitterPool = emitterPool_.lock();
    if(emitterPool)
        emitterPool->remove(id_);

    auto receiverPool = receiverPool_.lock();
    if(receiverPool)
        receiverPool->remove(id_);
}

// ConnectionPool
ConnectionPool::~ConnectionPool()
{
    while(!connections_.empty())
    {
        ConnectionPtr connection;

        {
            std::lock_guard<std::mutex> lock(mutex_connections_);

            auto it = connections_.cbegin();
            if(connections_.cend() != it)
            {
                connection = it->second;
                it = connections_.erase(it);
            }
        }

        if(connection)
            connection->disconnect();
    }
}

std::vector<ConnectionID> ConnectionPool::keys() const
{
    std::lock_guard<std::mutex> lock(mutex_connections_);

    std::vector<ConnectionID> keys;

    for(const auto& it: connections_)
        keys.push_back(it.first);

    return keys;
}

ConnectionPtr ConnectionPool::connection(const ConnectionID& id) const
{
    std::lock_guard<std::mutex> lock(mutex_connections_);

    const auto& it = connections_.find(id);
    if(connections_.cend() == it)
        throw std::runtime_error("connections is not found");

    return it->second;
}

void ConnectionPool::add(const ConnectionPtr& connection)
{
    std::lock_guard<std::mutex> lock(mutex_connections_);

    connections_[connection->id()] = connection;
}

void ConnectionPool::remove(const ConnectionID& id)
{
    std::lock_guard<std::mutex> lock(mutex_connections_);

    auto it = connections_.find(id);
    if(connections_.cend() != it)
        connections_.erase(it);
}

// Helper function
void connecting(const ConnectionPtr& connection, const OwnerPools& pools)
{
    connection->emitterPool_ = pools.emitterPool_;
    pools.emitterPool_->add(connection);

    connection->receiverPool_ = pools.receiverPool_;
    pools.receiverPool_->add(connection);
}

int main()
{
    auto emitterPool = std::make_shared<ConnectionPool>();
    auto receiverPool = std::make_shared<ConnectionPool>();

    connecting(
        std::make_shared<Connection>("id_0")
        , {emitterPool, receiverPool}
    );
    connecting(
        std::make_shared<Connection>("id_1")
        , {emitterPool, receiverPool}
    );

    std::cout << "id_0 connection before delete: ";
    std::cout << emitterPool->connection("id_0")->id() << '\n';

    emitterPool->connection("id_0")->disconnect();

    try
    {
        std::cout << "id_0 connection after delete in emitter pool: ";
        std::cout << emitterPool->connection("id_0")->id() << '\n';
    }
    catch(const std::runtime_error& e)
    {
        std::cout << e.what() << '\n';
    }

    try
    {
        std::cout << "id_0 connection after delete in receiver pool: ";
        std::cout << receiverPool->connection("id_0")->id() << '\n';
    }
    catch(const std::runtime_error& e)
    {
        std::cout << e.what() << '\n';
    }

    std::cout << std::flush;
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It would be great if you could put testing code here too. Links might go down, so SE tries to internalize knowledge. \$\endgroup\$ – Incomputable Mar 31 '18 at 12:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.