Best practice: Adding child node to a parent in child constructor or not?

Question

I'm working with some code that adds a child node to it's parent in the child's constructor. The code looks something like this:

Class:

class Node1 {
  public Node1(Node1 parent, String name) {
     if(parent != null) {
       parent.add(this);
     }
     this.parent = parent;
  }

  private void add(Node1 child) {
    children.add(child);
  }
}

Usage:

Node1 parent = new Node1(null, "parent");
Node1 child1 = new Node1(parent, "child1");
Node1 child2 = new Node1(parent, "child2");

By implementing it this way the user of the Class Node1 doesn't have to explicitly add the child node (less code) to it's parent and you have guaranteed a child node has a parent.

I personally wouldn't have written it like this, but more like the following:

class Node2 {
  public Node2(String name) {
  }

  public void add(Node2 child) {
    children.add(child);
    child.setParent(this);
  }
}

Node2 parent = new Node2("parent");
Node2 child1 = new Node2("child1");
parent.add(child1);
Node2 child2 = new Node2("child2");
parent.add(child2);

So my question is, is this a good idea to implement it as shown in Class Node1 or are there any objections to do it that way? Or is there no argument of why one is better than the other?

Answer 1

I think both implementations are ok.

The question is more how you will use this class: will you create a list of nodes first and then start adding their relationships or will you do it in an orderly fashion, like in your example?

The second solution offers more flexibility, also if you ever want to use Node1 as a superclass, you're not bound to the constructor signature.

Answer 2

I personally don't like the first example because you are adding a Node which is not "ready" yet (as the constructor didn't finish executing). In most cases it will work fine, but in extreme cases you can have bugs that are quite hard to find.

Answer 3

I would use second case as then I will be able to add children later but not only in the constructor.

Answer 4

If you derive Node from the TreeNode class then you automatically get the 2nd implementation of yours. You can add your "custom" treenode to the regular Treeview class.

Answer 5

I don't like the first case - the parent object is magically modified just by being passed in - you have to read code or docs to realise this is happening. Just passing in the parent only implies that the child knows the parent - not that the child is added to the parent's internal list as well.

Second example is better. You could also return the child from the 'add' operation to allow operation chaining like:

Node2 child = parent.add(new Node2());

Answer 6

I see no reason why not to combine the two approaches for convenience:

class Node1 {
  public Node1(String name, Node1 parent = null) {
    this.name = name;
    // etc., do initialization

    if(parent != null)
      parent.add(this);
  }

  private void add(Node1 child) {
    child.setParent(this);
    children.add(child);
  }
}

Note that setParent() should either throw an exception when the child already has its parent set, or properly remove itself from the former parent's child list.

Answer 7

Firstly the bad in your first sample is: if(parent != null) { this check is needed because you should have a chance to create root of tree, but two-state parameter looks ugly.

Also first your sample not good because doing implicit actions (adding child to passed parent).

Answer 8

I'd prefer implementation #1 only when it is absolutely essential to have a parent node with a child node. In this case, it is quite possible for the client code to omit the call to Node1.add(Node1 child) leading to errors later.

I'd prefer implementation #2 otherwise because it is clearer to have a usage like

Node1 parent = new Node1();
Node1 child = new Node1();
parent.add(child);