Tree parent and child nodes

Question

I have this method that returns true if attribute is already used and false otherwise:

public boolean alreadyUsed(Node node, int attribute) 
{
    if (node.parent == null) 
    {
        return false;
    }

    if (node.children != null) 
    {
        if (node.splitAttribute == attribute )
        {
            return true;
        }
    }
    return alreadyUsed(node.parent, attribute);
}

Can this be written in a more understandable way?

Answer 1

Well, following conventions helps making your code more understandable. One convention is, for methods that return a boolean, using a predicate as a name, or as said in this answer, using a question.

So to begin with, I'd name the method isAlreadyUsed.

A second improvement by following conventions would be using accessor methods:

node.getParent() instead of node.parent. But that depends on the scope of Node.

Item 14 in Joshua Bloch's Effective Java:

In public classes, use accessor methods, not public fields

Answer 2

Your second if statements don't need to be nested, as there's only two possible exits (return true or fall through and recurse), so I'd combine them into one:

if (node.children != null && node.splitAttribute == attribute ) {
    return true;
}

To me this looks more like a function that takes in a Node object as a parameter, rather than being a method on a Node object, which I think it possibly could/should be:

public class Node {

    private Node parent;
    private Object children;
    private int splitAttribute;

    //...
    //Existing state and behaviour of Node class here
    //...

    public boolean alreadyHasAttribute(int attribute) {
        if (this.parent == null) {
            return false;
        }

        if (this.children != null && this.splitAttribute == attribute) {
            return true;
        }

        return this.parent.alreadyHasAttribute(attribute);
    }
}

Then, whenever you have an instance of a Node, you can replace all your existing calls to alreadyUsed(node, attribute) with node.alreadyHasAttribute(attribute), like in the following (entirely made up) example:

public class Foo {
    public static void main(String[] args) {
        Node node = getANodeSomehow();

        //if (alreadyUsed(node, attribute)) { /*This old approach isn't used anymore*/
        if (node.alreadyHasAttribute(attribute)) {
          //Do whatever you previously did
          //...
        }
    }

    //...
}

Answer 3

In respect to the other postings i made a design review. Therefore i will not repeat what the others said but relay on them.

From what the method does

As far as i understand your code it searches for a Node or all of its parent-Nodes until a node has his attribute in a specific value.

For such a deep-going-operation i recommend a different class to differ a complex work beside the responsibilitys of a simple java-bean / container.

public static final class ParentingHelper {
  private ParentingHelper(){} // Tools should have only static-methods 
                              // and are not allowed to be instaniated 
  public static boolean inheritsAttributeValue(Node n, int attr) {
    // hey, this is oldscool for tools, lets translate this into 
    // logical speak: Has null the attribute 5? Nope (brainless and exploitable)! 
    if (n == null) {
       return false;
    }

    // early return does not mean kill the performance.
    if (n.splitAttribute == attr) {
       return true;
    }

    return inheritsAttributeValue(n.getParent(), attr);
  }
}

Why no IAE or NPE? 1. In every way the exception would be a subsequent-error that does never point you to the real problem alone. The Stacktrace therefore is useless. 2. Also i decide to have a different result from a method like inheritsNotAttributeValue if the results do not differ. 3. Also i think this is a view-scope of a MVC, so lets be honest.

BTW: Note that it also see no responsibility for looking for loops in the node-parenting.