Writing an overloaded assignment operator

Question

Consider the class definition below, I have a question regarding what the overloaded assignment operator should look like.

class Node{
public:
    void setName(string name) { _name = name;}
    void setParent(Node * n) {_parent = n;}
private:
    string _name;
    Node* _parent;
}


main()
{
  vector <Node*> nodes;
  Node * n1 = new Node();
  n1->setName("a");
  n1->setParent(NULL);

  Node *n2 = new Node();
  n2->setName("a");
  n2->setParent(n1);

  Node *n3;
  *n3 = *n2; 
}

My question is, how should the overloaded assignment operator for class Node look like.

Node & operator = (const &n)
{
  if(this == &n)
   return *this;
  _name = that._name;
  _parent = new Node(); // Should this be done?
  _parent(that.parent); //Should this done?
}

Have you read the [FAQ for operator overloading](http://stackoverflow.com/questions/4421706)? — Daniel Frey, Oct 17 '13 at 17:03
That's really hard to tell what should be done for `_parent`. There's no common rule, this would completely depend on your use case and semantics you want to achieve by implementing assignment operators (and copy constructors). Since you have an extra method to set the `_parent` member, I'd say initializing it to `nullptr` would be the best choice. — πάντα ῥεῖ, Oct 17 '13 at 17:05
Sorry was caught up. However, I was thinking that, in case it is just a reference to memory location it should be OK to just copy the address location. When the destructor is called for the object, it will make the address NULL, however the memory location will not be deleted, because the destructor does not delete the memory. — mickeyj, Oct 22 '13 at 21:17

πάντα ῥεῖ · Answer 1 · 2013-10-17T19:26:10.810

This is not really an answer, but another question about your design approach in general:

Apparently you're trying to design a tree node, right?
Why are you storing the parent class in the node and not vice versa (i.e. holding a std::vector of the child node instances or pointers). This would simplify semantics for assignment (copying) a lot. You'll need methods to add/remove/clear child nodes in turn of course, setting the _parent member should be done internally within add/remove implementation then. Tree leaf implementations of nodes would just provide empty methods for this functionality. Also I would add another level of indirection then and declare Node as a pure abstract class and provide s.th. like ContainerNodeBase and LeafNodeBase as 1st level of implementation.

Also I would avoid using raw pointers in (at least) production code. You should use std::shared_ptr or alike.

Let me give an example what I think your Node design should look like:

1st Level provide abstract interfaces:

 struct INode
 {
     virtual const std::string & name() const = 0;
     virtual void name(const std::string& value) = 0;
     virtual INode* parent() const = 0;
     virtual ~INode() {}
 };

 struct INodeCotainer
 {
     virtual void add(std::shared_ptr<INode> child) = 0;
     virtual void remove(const INode* child) = 0;
     virtual void clear() = 0;
     virtual const std::vector<std::shared_ptr<INode>>& childs() const = 0; 
     virtual ~INodeCotainer() {}
 };

2nd Level provide base implementation:

 class NodeBase
 : public INode
 {
 public:
     virtual const std::string & name() const { return name_; }
     virtual void name(const std::string& value) { name_ = value; }
     virtual INode* parent() const { return parent_; }

 protected: // protect this class from direct instantiation
     NodeBase() {}
     NodeBase(const NodeBase& rhs) { parent_ = nullptr; name_ = rhs.name; }
     NodeBase& operator=(const NodeBase& rhs) 
     { 
        if(&rhs != this)
        {
            parent_ = nullptr; name_ = rhs.name;
        }
        return *this;
     }

     INode* parent_;

 private:
     std::string name_;
 };

3rd Level provide concrete implementation(s)

 class ContainerNode
 : public NodeBase
 , public INodeContainer
 {
 public:
     ContainerNode() : NodeBase {}
     ContainerNode(const ContainerNode& rhs) : NodeBase(rhs) 
     {
         std::copy(childs_,rhs.childs_);
     }
     ContainerNode& operator=(const ContainerNode& rhs)
     {
         NodeBase::operator=(rhs);
         childs_.clear();
         std::copy(childs_,rhs.childs_);
     }

     virtual void add(std::shared_ptr<INode> child) 
     { 
         childs_.push_back(child);
         childs_.back()->parent_ = this;
     }         
     virtual void remove(const INode* child)
     {
         // Find the child reference in the childs_ vector remove it 
         // and set it's parent_ member to nullptr
     }
     virtual void clear() { childs_.clear(); }
     virtual const std::vector<std::shared_ptr<INode>>& childs() const
         { return childs_; }

 private:
     std::vector<std::shared_ptr<INode>> childs_;
 }

 class LeafNode
 : public NodeBase
 {
 public:
     LeafNode() : NodeBase {}
     LeafNode(const LeafNode& rhs) : NodeBase(rhs) {}
     LeafNode& operator=(const LeafNode& rhs)
     {
         NodeBase::operator=(rhs);
     }
 }

Actually it is not a linked list that I am implementing. It is just an example that I came up with to show a case where only a reference to an object is being stored in a class and there is not memory allocated being done in the class. The memory allocation is being done outside the class — mickeyj, Oct 22 '13 at 21:15
OK, my example also doesn't refer to a linked list but a tree implementation. Can't tell if this gives any advice for your actual use case. You might try to keep your examples more 'neutral' and nail your questions down to the point! — πάντα ῥεῖ, Oct 22 '13 at 21:23

Writing an overloaded assignment operator

1 Answers1