How to have this const-corrected?

Question

I have a const-correctness problem which I don't seem to be able to resolve. Here is the structure of my program:

class Node
{
    private:
        int            id;
        std::set<Node*> neighbours;
    public:
        Node();
        Node(int id_p);

        void set_id(const int& id_p);
        int  get_id() const;
        void add_neighbour(Node* neighbour);
        bool is_neighbour(Node* neighbour) const;

        friend bool operator <(const Node& lhs, const Node& rhs);
};

class Graph
{
    private:
        std::set<Node> node_list;
    public:
        Graph();

        void        add_node(int id);
        const Node* get_node_by_id(int id) const;
        bool        has_node(int id) const;
        void        check_add_node(int id);
        void        add_edge(int id_1, int id_2);
        bool        has_edge(int id_1, int id_2) const;
        void        check_add_edge(int id_1, int id_2);

        (...)
};

Now the thing is, if I call the function Graph::get_node_by_id(), I want to return a pointer to a given node (type Node). But it seems impossible to do so, because the std::set implicitly converts my Node type objects to const Node objects, and I am unable fetch a non-const pointer from a const object.

However, I cannot have everything else set to const Node (which would resolve the problem), because I want to call Node::add_neighbour() from Graph::add_edge(), but whenever I do so, my compiler says that I might be violating the constness (required to have a sorted set) of the elements in the node_list set, even though I defined the less operator< to only care about the id.

Is there anything I can do to resolve this dilemma (without giving up on having a sorted set)? Thank you for your responses!

More info on the error:

If I use non-constant fields, error in Graph::get_node_by_id():

for(Node& element : this->node_list) // Error: element should be const Node&
{
    if(element->get_id() == id)
    {
        return element;
    }
}
return nullptr;

If I use constant fields, error in Graph::add_edge():

(...)
const Node* node_1 = this->get_node_by_id(id_1);
const Node* node_2 = this->get_node_by_id(id_2);
node_1->add_neighbour(node_2); // Error for disregarding constness
node_2->add_neighbour(node_1);

Answer 1

Your issue appears to be that you have two different 'value semantics' to Node.

One is that exposed by operator< which is not affected by add_neighbour. This is the one set needs, to keep things ordered, and which it enforces by making Node const.

The other is that exposed by the class API, where both set_id and add_neighbour would change the value.

To keep your sorted set, you must not allow the id of a node to change once it's in the set. But you can allow the neighbours to change.

So I'd suggest you make the neighbours set mutable, make add_neighbour private and const, and make Graph a friend of Node.

This is what mutable gives you, data members that are not part of the 'value' of a type. Note that this means you are indicating that something holding a const Node* may expect the result of is_neighbour to change between calls.

So...

class Node
{
    private:
        // Trust Graph not to mess directly with these!
        int            id;
        mutable std::set<Node*> neighbours;

        friend class Graph;
        // For Graph's exclusive use
        void add_neighbour(Node* neighbour) const;


    public:
        Node();
        Node(int id_p);

        void set_id(const int& id_p); // Callable when not in Graph's set
        int  get_id() const;
        void add_neighbour(Node* neighbour);  // Callable when not in Graph's set
        bool is_neighbour(Node* neighbour) const;

        friend bool operator <(const Node& lhs, const Node& rhs);
};

class Graph
{
    private:
        std::set<Node> node_list;
    public:
        Graph();

        void        add_node(int id);
        const Node* get_node_by_id(int id) const;
        bool        has_node(int id) const;
        void        check_add_node(int id);
        void        add_edge(int id_1, int id_2);
        bool        has_edge(int id_1, int id_2) const;
        void        check_add_edge(int id_1, int id_2);

        (...)
};

Now what you have is public, non-const mutators for Node instances that aren't in Graph's set, and an extra mutator for Graph to use to change the neighbours of the Nodes in its set.

So only Graph can do

const Node b;
b.add_neighbour(nullptr);

If you really don't trust Graph, you can replace the private const add_neighbour with an inner class, with a static add_neighbour(Node* node, Node* neighbour method, since an inner class is implicitly able to access private data of the outer class.

class NeighbourHelper {
    friend class Graph;
    static void add(const Node* node, Node* neighbour) {
        node->add_neighbour(neighbour);
    }

Now only Graph can do

const Node b;
Node::NeighbourHelper::add(&b, nullptr);

In both cases, the following works for everyone:

Node a;
a.add_neighbour(nullptr);

---

At this point, you should be suffering a code-smell... The issue is the public get_node_by_id method in Graph. You actually probably want to expose an iterator of some kind instead, rather than than the raw Node*, and make Node a private inner class of Graph.

Or even just replace the whole Node concept with std::map<int,std::set<int>>...

But it depends on your actual use case.

Answer 2

Although TBBle's analysis is correct, there's a much simpler solution: replace Graph's std::set<Node> with std::map<int,Node>.

Your current Graph::get_node_by_id() is using a linear search because set doesn't really provide the lookup you want. Making the key external allows you to remove the operator< overload and still get a faster and more natural lookup: map.find(id).

The only ugly part is that now your Node has an internal id which must match an external key. If you never use the id except to look up a node in the map, you could just remove this entirely. If you need to follow the graph edges (neighbours) and then check the ID, you can replace your set of pointers with a set of map iterators, like:

typedef std::map<int, Node> NodeMap;
typedef std::set<NodeMap::iterator> NeighbourMap;

Then your traversal has the pair<const int,Node> available.

---

NB. on reflection, changing from set to map produces almost the same distinction as TBBle's answer: you split the Node into const and mutable parts. The lookup is cleaner with this solution (you can regain logarithmic-time lookup by constructing a fake Node as a key for set::find, but it's still a little inelegant), and the object identity is slightly cleaner with the other solution.