1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ What do you think is not understandable about this code? It can be consolidated into a single boolean expression, but there isn't much there to begin with. \$\endgroup\$ – unholysampler Jan 5 '15 at 14:22
4
\$\begingroup\$

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

\$\endgroup\$
  • 2
    \$\begingroup\$ Well, if it's inside Node class it's perfectly OK to use .parent instead of getParent \$\endgroup\$ – Marco Acierno Jan 5 '15 at 16:26
  • \$\begingroup\$ I never get the "obsession" about declaring getters and setters by rule. Why would the public modifier exist then? You'd say well "maybe the implementation can change and something else can be returned and you would have to change every call to the attribute for the new thing while if you just used the method that's the only thing you'd have to change". But come on... a node parent is a node parent, an attribute, a property, let's give public some love. \$\endgroup\$ – dabadaba Jan 5 '15 at 16:41
  • \$\begingroup\$ public exists for other reasons. What if the parent is not a field anymore? But it's a calculated field now? How should I handle this situation now? \$\endgroup\$ – Marco Acierno Jan 5 '15 at 20:30
3
\$\begingroup\$

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
          //...
        }
    }

    //...
}
\$\endgroup\$
  • \$\begingroup\$ however parent is defined in another class that is why I am doing node.parent \$\endgroup\$ – user3599420 Jan 5 '15 at 15:03
  • \$\begingroup\$ @user3599420 I was suggesting adding this as a method onto your Node class, which I presume has visibility of its parent (or else I don't understand your existing code). To me that keeps all the core behaviour relating to a Node together in a single source file. \$\endgroup\$ – Edd Jan 5 '15 at 15:06
  • \$\begingroup\$ can you show me how? I did not understand by adding this as a method onto Node class \$\endgroup\$ – user3599420 Jan 5 '15 at 15:08
  • \$\begingroup\$ i see but why should I put it in that class? that class just takes care of setting parent and children \$\endgroup\$ – user3599420 Jan 5 '15 at 15:21
  • \$\begingroup\$ @user3599420 It's the idea of encapsulation and increasing cohesion - You generally want the class to group the required data and behaviour to do with a given thing together \$\endgroup\$ – Edd Jan 5 '15 at 15:35
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.