3

I am writing a method, that is intended to search a nested collection recursively for a value and return the collection, which contains that value:

@SuppressWarnings("unchecked")
public static <E,
               R extends Collection<? extends E>> Optional<R> findRecursively(Collection<?> collection, E element)
                  throws ClassCastException {
   if (collection.isEmpty())
      return Optional.ofNullable(null);

   boolean nested = collection.stream()
                              .allMatch(e -> isSubclass(e.getClass(), Collection.class));
   for (Object c : collection) {
      R result;

      if (nested) {
         Optional<R> recursiveResult = findRecursively((Collection<?>) c, element);
         result = recursiveResult.orElse(null);
      } else {
         result = c.equals(element) ? (R) collection : null;
      }

      if (result != null)
         return Optional.of(result);
   }
   return Optional.ofNullable(null);
}

This works fine, but when I use that method I cannot work with the returned Optional direktly without giving it to a variable first like this:

Collection<String> collection = null;
Set<String> set = Util.findRecursively(collection, "")
                      .orElseThrow(RuntimeException::new);

In that case the compiler cannot know the return-type of the method. I don't know any good possibility to make it work.

I figured out 2 other ways, that are not very nice either:

  1. I add a parameter with type of the return-type that should be returned like:

    public static <E,
               R extends Collection<E>> Optional<R> findRecursively(Collection<?> collection,
                                                                    E element,
                                                                    Class<R> returnType) {...

    But I don't like this, because the additional parameter is (actually) unnecessary and makes the method less easily understandable.

  2. Instead of an Optional I return just the collection-object. But in that case I cannot create an Optional directly with calling it either like:

    Collection<String> collection = null;
Set<String> set = Optional.ofNullable(Util.findRecursively(collection, ""))
                          .orElseThrow(RuntimeException::new);

    Because here again the compiler doesn't know the return-type and cannot infer the type-arguments to the of() method.

Does anybody have any idea of a nice solution for this issue? Or an advise which way to go here?

David
  • 1,672
  • 2
  • 13
  • 32
  • 1
    on a side note, `Optional.empty()` is the same as `Optional.ofNullable(null)` – Ash Oct 24 '16 at 10:17
  • In which package should that option class be? I can't find it. – David Oct 24 '16 at 10:19
  • 2
    Returning an Optional of a collection is largely an abuse of Optional ([see also](http://stackoverflow.com/a/26328555/1743880) and [that as well](http://stackoverflow.com/a/23464794/1743880)). Just return an empty collection. And then throw some exception if it's empty at the call site. – Tunaki Oct 24 '16 at 10:32
  • 2
    I think your problem boils down to the unchecked cast. You are basically telling: "_I will return whatever collection implementation you request_" and then you don't check what is requested and blindly cast it. We had [similar or even worse issues](http://stackoverflow.com/questions/30517768/how-to-detect-ambiguous-method-calls-that-would-cause-a-classcastexception-in-ja) in the past because of this. The solution with the `returnType` parameter is the best one, you can even use `returnType.cast(collection)` to avoid the unchecked cast. – Didier L Oct 24 '16 at 11:07
  • Thanks for pointing that out :) What is the difference between an unchecked cast and using the cast method of the class-object? To me it looks like a throws-declaration for a ClassCastException should solve this documentation problem. – David Oct 24 '16 at 11:41
  • 3
    Well, the `cast` method of a `Class` object is *not* an unchecked cast, in other words, as the literal meaning of the term tells you, `Class.cast` will actually *check* whether the object really is an instance of the class. – Holger Oct 24 '16 at 11:56
  • 3
    Besides that, the entire method is broken. Just because a collection contains an element of type `E` does not prove that it is a `Collection`, e.g. a `Set` can contain an instance of type `Integer`, but that doesn’t make it a `Set`. Even worse, the caller can substitute any collection type, e.g. let it return a `List`, even if the input is a `Set`, without getting any warnings. Your example already demonstrates this by passing in a parameter, whose type is `Collection`, but expects to magically get a `Set` back, without any prove that the source ever was a `Set`. – Holger Oct 24 '16 at 12:02
  • 2
    And `Optional.of(…).orElseThrow(…);` makes no sense either. `of` does not accept `null` values, so the `Optional` will never be empty, instead you’ll get a `NullPointerException`. You very likely meant `ofNullable` there, though getting a new `RuntimeException` is not better than getting a `NullPointerException`… – Holger Oct 24 '16 at 12:08
  • 3
    There is probably a more general design issue of representing your data structure with generic collections instead of defining your own class – using collections behind the scenes but that would be hidden, and you wouldn't have nested collections. – Didier L Oct 24 '16 at 14:49
  • That `Class.cast` method checks whether the object really matches the requested type and throws a ClassCastException if it doesn't match. A usual cast throws a ClassCastException as well, if the object cannot be casted to the type. I really don't see the difference here. – David Oct 27 '16 at 08:29
  • I know that there is no guarantee for the returned collection to match the passed return type. If I cast the found collection to the return type and it doesn't match the type a ClassCastException will be thrown. That can be pointed out by dokumentation, so the caller can know that. So it's intended that the caller must be sure about the type of the collection, that will be found. – David Oct 27 '16 at 08:29
  • The `Optional.of(...).orElseThrow(...)` call really makes no sense, thank you. I'll fix that. And of course throwing a `RuntimeException` like that (even if I used `Optional.ofNullable`) makes no sense. This is just an Example to show the strukture of the usage. – David Oct 27 '16 at 08:32

1 Answers1

4

you can state the concrete types in the same line without declaring a var: 

Util.<Collection, String, Set<String>>findRecursively(collection, "")...
aviad
  • 1,553
  • 12
  • 15