# 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<T> getInfix() {
List<T> result = new ArrayList<>();
getInfixInternal(root, result);
return result;
}

private void getInfixInternal(Node node, List<T> infixValues) {
if (node != null) {
getInfixInternal(node.getLeft(), infixValues);
getInfixInternal(node.getRight(), infixValues);
}
}


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

public class Node <T extends Comparable>  {
private T value;
private Node leftChild;
private Node rightChild;
private Node parent;
}

• Recursion is itself a code smell – Brad Thomas Jul 17 '16 at 16:19
• @BradThomas that statement is possibly the most offensive and stupid statement about programming that I heard in months. Congratulations. Recursion is a standard approach for method / function behaviour in all languages – Vogel612 Jul 17 '16 at 16:48
• "Standard" means nothing. Bad practice is also "standard" across the industry, in all languages. I have never come across a meaningful use of recursion (outside of academic interest) that could not be more readably and maintainably coded without the use of recursion. – Brad Thomas Jul 17 '16 at 16:50
• @Brad Thomas care to elaborate? I have read similar in the power of ten rules but cant really follow the Argumentation – AdHominem Jul 17 '16 at 17:17
• Recursion is used for the sake of purported brevity, elegance but has a heavy cost in terms of readability and maintainability. I'm not saying it's never the most appropriate solution, only that I believe it is widely over-used for most line-of-business applications. In contrast to academic study questions, these applications are often in service over many years and require considerable ongoing maintenance. IMO a simpler flat enumeration over the tree would often suffice and be easier to analyze and maintain in most cases. Being widely over-used makes it a code smell i.e. likely inappropriate – Brad Thomas Jul 17 '16 at 17:27

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<T> getInfix() {
List<T> result = new ArrayList<>();
getInfixInternal(root, result);
return result;
}

private void getInfixInternal(Node<T> node, List<T> infixValues) {
if (node != null) {
getInfixInternal(node.getLeft(), infixValues);
getInfixInternal(node.getRight(), infixValues);
}
}


and

public class Node<T extends Comparable<? super T>>


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<T extends Comparable<? super T>> {

// ...

public List<T> getInfix() {
List<T> result = new ArrayList<>();
if (leftChild != null) {
}
if (rightChild != null) {
}
return result;
}

}


With such a method, you can then have:

private List<T> getInfix() {
return root == null ? Collections.emptyList() : root.getInfix();
}


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

public List<T> getInfix() {
List<T> result = new ArrayList<>();