Writing a recursive method in java

Question

I am trying to write a recursive method that searches through a list of IData objects and returns a specific implementation. The list contains objects that implements the interface IData.

There are two implementations of this interface:

1) DataImpl

2) DataContainerImpl

The DataContainerImpl has a:

 List<IData> children;

so it can hold nested DataContainerImpl elements or just plain DataImpl's. Here is what I do:

  public static DataContainerImpl findDataContainerWithName(Collection<IData> elements, String name) {
    for (IData  element : elements) {
      if (element instanceof DataContainerImpl) {
        DataContainerImpl container = (DataContainerImpl) element;
        if (container.getName().equals(name)) {
          return container ;
        }

       container = findDataContainerWithName(container.getChildren(), name);
       if (container != null) {
         return container ;
       }
      }
    }
    return null;
  }

OK, so you are really searching a tree. The StackOverFlow exception indicates that you are searching quite a deep tree, so the stack can't hold all that recursion. Why are you using EcoreUtil rather than the built-in Java Collection methods? Are you actually getting a ConcurrentModificationException with the code above, when the data is small enough to avoid a StackOverflow Exception. — DNA
– DNA, Commented May 8, 2011 at 21:43

Kal · Accepted Answer · 2011-05-08 22:03:59Z

5

Not exactly sure what you are doing here, but calling "findContainerByName()" recursively without a "return" in front of it wont do you any good.

The recursive call will return , but then the call will drop to the bottom and return null.

For example, if your list contained just 1 DataContainerImpl whose name did not match the name you called the method with, but it contains inside it a list with again just 1 DataImpl, you will still get null back. Is this what you want?

edited May 8, 2011 at 22:03

answered May 8, 2011 at 21:52

Kal

24.9k8 gold badges67 silver badges66 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Neil · Accepted Answer · 2011-05-08 21:43:55Z

4

Stick to your original loop to handle the list, and only recurse when you find a DataContainerImpl.

  public static DataContainerImpl findDataContainerWithName(Collection<IData> elements, String name) {
    for (IData element : elements) {
      if (element instanceof DataContainerImpl) {
        DataContainerImpl container = (DataContainerImpl) element;
        if (container.getName().equals(name)) {
          return container;
        }
        container = findDataContainerWithName(container.getChildren(), name);
        if (container) {
          return container;
        }
      }
    }
    return null;
  }

answered May 8, 2011 at 21:43

Neil

55.5k8 gold badges65 silver badges75 bronze badges

Comments

Péter Török · Accepted Answer · 2011-05-08 21:52:45Z

1

I think the root cause of your problem is that you keep recursively searching through the same elements in the list. I.e. here

    // Call with remaining list...
    Collection<IData> copyAll = EcoreUtil.copyAll(elements);
    copyAll.remove(count);
    findDataContainerWithName(copyAll, name);

you call with the original list minus the current element, not only with the elements after the current element. So the recursive calls will inspect the same elements over and over again, needlessly. Especially so as you are looping over the list each time, which would be needless with a pure recursive approach.

So if you absolutely want to make this search recursive, you should only inspect the first element, then remove it from the list and call recursively with the remaining list. Or if you prefer looping over the list, you don't need any recursion in the common case (when the element is not a DataContainerImpl). That would be the Java way of doing things btw, as Java - unlike functional languages - has limited support for recursion.

answered May 8, 2011 at 21:52

Péter Török

117k31 gold badges277 silver badges332 bronze badges

2 Comments

u123 Over a year ago

But how do you call with the remaining list? Manually create a copy of the existing list or is it better to use a more raw type like an IData[] and then iterate with a integer index - specifying start stop intervals?

Péter Török Over a year ago

@tul, either way could be technically working, but both is a bit awkward (and the first is wasting a lot of memory). This is why using recursion to solve this problem is not really the Java way.

DNA · Accepted Answer · 2011-05-09 19:39:02Z

1

Apart from the problems already mentioned in answers, you are making a recursive call for each element in the list, passing in all the other elements.

So for a list of length N, you will recurse N-deep (needlessly), popping one element off the list each level of recursion. But all of these elements can be handled at the same level.

You are artificially increasing the recursion depth, hence your StackOverflowError problem

edited May 9, 2011 at 19:39

answered May 8, 2011 at 22:02

DNA

42.7k12 gold badges114 silver badges153 bronze badges

Collectives™ on Stack Overflow

Writing a recursive method in java

4 Answers 4

Comments

Comments

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related