Is Optional<List> bad practice?

user_5127598 0
Viewed 906

I keep seeing some users saying that using a collection with an optional is not recommended / is bad practice. I couldn't find much information on why it would be bad practice, so here I am.

In case it matters, in my particular case I have a JpaRepository with 2 methods. One Optional<List> getValues() and a second List getDefaultValues(), as such:

public interface someRepository extends JpaRepository<x, y> {

    Optional<List> getValues();

    List getDefaultValues();
}

The idea is that the values might not be present so I want to force the method caller to do .orElse(getDefaultValues()) so there is no chance getting returned an empty List. I also figured it would be tidier than just doing something like this:

List list = getValues();

if (list.isEmpty()) {
   list = getDefaultValues();
}

Any feedback is appreciated.

Edit: I realise my example is not the most well suited to the question, as pointed out in the answers below. However, I would still like to understand why Optional<List> is considered bad practice by some.

This question does not really answer my question.

java
2 Answers

I think the point is moot. There are two possible cases:

1. The caller needs to be aware that default values have been returned
In this case, the caller will not be able to use the orElse()/orElseGet() construct, and will have to check with isPresent(). This is no better than checking whether the list is empty.

2. The caller does not need to be aware that default values have been returned
In which case you might as well hide the implementation details behind a single List getValues() method that returns the default values in case no values were found.

As to the general applicability of using Optional<List>, I think the Brian Goetz quote from this answer says it best:

Our intention was to provide a limited mechanism for library method return types where there needed to be a clear way to represent "no result", and using null for such was overwhelmingly likely to cause errors.

When it comes to lists (and collections in general), there is already a clear way to represent "no result", and that is an empty collection.

user_35589600

Lets first consider this example:

public class <X> Example {

    public List<X> getValues() {
        if (...) {
            return null;
        } else {
            return /* a non-empty list */
        }
    }

    public List<X> getValuesAgain() {
        if (...) {
            return Collections.emptyList();
        } else {
            return /* a non-empty list */
        }
    }
}

Which of those ways of returning no values is better?

I would argue that the better way is (nearly always) the way that getValuesAgain does it; i.e. by returning an empty list. Consider the following.

With the first version, I need to test for a null before using the result:

  List<String> l = example.getValues();
  if (l != null) {
      for (s String: l) {
          /* do something */
      }
  }

With the second version, I can use the result directly:

  List<String> l = example.getValuesAgain();
  for (s String: l) {
      /* do something */
  }

In short, returning null makes life more complicated for the caller compared with returning an empty list. In theory, you may have saved the creation of an empty list object ... except that the javadoc for emptyList() states that it doesn't need to return a distinct list object each time. (And it typically won't!)

Now consider what Optional<List<String>> means: either a List<String> or no value. But as we have already seen, returning an empty list is simpler for the caller that returning a null. And the same logic applies here too.

So if getValues returns an Optional<List<X>>, we would need to use it like this:

  Optional<List<String>> l = example.getValues();
  if (l.isPresent()) {
      for (s String: l.get()) {
          /* do something */
      }
  }

Compare with the original getValuesAgain:

  List<String> l = example.getValuesAgain();
  for (s String: l) {
      /* do something */
  }

In short, using Optional to return a list or null is not an improvement.

The only scenario where I think you should contemplate using Optional<List<?>> as a return type is if the API needs to make a semantic distinction between an empty list versus no list.

The same applies for other collection types, and also for arrays, strings and other examples.

Another way of thinking about this is that the designers' intention for Optional is to provide a way to indicate there is "no result"; see Brian Goetz's answer to this question. But we don't need to indicate there is "no result" here since we can return an empty collection.

user_1399850
Related