Code smell when recursively returning a List So this is part of my AVL tree implementation, namely the infix representation of the tree. Since I wanted to use a recursive method to stay close to the original algorithm and want to return a list, I have to store the values in a parameter.

The smelly part is in getInfix when I reference that parameter to return the list. I am not even sure what's the name of this code smell but it feels wrong. I am literally only calling getInfixInternal for its side effect! But I can't simply return a list either...

How can I make this better?

private List&lt;T&gt; getInfix() {
    List&lt;T&gt; result = new ArrayList&lt;&gt;();
    getInfixInternal(root, result);
    return result;
}

private void getInfixInternal(Node node, List&lt;T&gt; infixValues) {
    if (node != null) {
        getInfixInternal(node.getLeft(), infixValues);
        infixValues.add((T) node.getValue());
        getInfixInternal(node.getRight(), infixValues);
    }
}


My Node class (excluding the getters and setters for reasons of space):

public class Node &lt;T extends Comparable&gt;  {
    private T value;
    private Node leftChild;
    private Node rightChild;
    private Node parent;
}
I wouldn't say there is a code smell with what you have now. However, it could be made more OOP-friendly.

An actual problem is that you're using raw types for Node, and that is never a good idea. So, first of all, consider having this instead:

private List&lt;T&gt; getInfix() {
    List&lt;T&gt; result = new ArrayList&lt;&gt;();
    getInfixInternal(root, result);
    return result;
}

private void getInfixInternal(Node&lt;T&gt; node, List&lt;T&gt; infixValues) {
    if (node != null) {
        getInfixInternal(node.getLeft(), infixValues);
        infixValues.add(node.getValue());
        getInfixInternal(node.getRight(), infixValues);
    }
}


and

public class Node&lt;T extends Comparable&lt;? super T&gt;&gt;


Then, to avoid this getInfixInternal, you can reason with what the method getInfix means. The first thing to realize is that it works on a given Node. In fact, it is really a property of the node itself: every node can return a list of values resulting of a infix traversal of the subtree starting at this node. Therefore, instead of having an internal method taking a Node as parameter, make it a method of Node:

public class Node&lt;T extends Comparable&lt;? super T&gt;&gt; {

    // ...

    public List&lt;T&gt; getInfix() {
        List&lt;T&gt; result = new ArrayList&lt;&gt;();
        if (leftChild != null) {
            result.addAll(leftChild.getInfix());
        }
        result.add(value);
        if (rightChild != null) {
            result.addAll(rightChild.getInfix());
        }
        return result;
    }

}


With such a method, you can then have:

private List&lt;T&gt; getInfix() {
    return root == null ? Collections.emptyList() : root.getInfix();
}


With Java 8 Optional, you could potentially write that more concisely:

public List&lt;T&gt; getInfix() {
    List&lt;T&gt; result = new ArrayList&lt;&gt;();
    Optional.ofNullable(leftChild).map(Node::getInfix).ifPresent(result::addAll);
    result.add(value);
    Optional.ofNullable(rightChild).map(Node::getInfix).ifPresent(result::addAll);
    return result;
}


Final note: you could also consider returning unmodifiable lists.