commons-lang icon indicating copy to clipboard operation
commons-lang copied to clipboard

Lang 1689 add optional to objectutils isempty

Open hendrixjoseph opened this issue 1 year ago • 5 comments

  • Original JIRA ticket (not by me): https://issues.apache.org/jira/projects/LANG/issues/LANG-1689
  • Method isEmpty in ObjectUtils was modified to return the negation of Optional.isPresent (i.e. !optional.isPresent()) when the passed in object is an Optional.
  • The object contained within the Optional is not checked if it is also empty. This gives the caller the option to check if it's present (or not) on their own.
    • For instance, the caller might want to do something like the following: ObjectUtils.isEmpty(optional) && ObjectUtils.isNotEmpty(optional.get()).

hendrixjoseph avatar Aug 10 '22 19:08 hendrixjoseph

Codecov Report

Merging #933 (6923fb7) into master (9f71c33) will increase coverage by 0.01%. The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master     #933      +/-   ##
============================================
+ Coverage     91.65%   91.66%   +0.01%     
- Complexity     7397     7400       +3     
============================================
  Files           188      188              
  Lines         15772    15774       +2     
  Branches       2972     2974       +2     
============================================
+ Hits          14456    14460       +4     
+ Misses          725      724       -1     
+ Partials        591      590       -1     
Impacted Files Coverage Δ
...ain/java/org/apache/commons/lang3/ObjectUtils.java 94.47% <50.00%> (+0.05%) :arrow_up:
...he/commons/lang3/concurrent/AtomicInitializer.java 100.00% <0.00%> (+25.00%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Aug 10 '22 20:08 codecov-commenter

Hm, at first I thought this would be OK but now I'm not sure and would like community feedback:

  • There are APIs in the JRE called isEmpty() on various classes that usually return true when size() == 0; for example Collection, String, and Map.
  • There isn't an Optional API called isEmpty, instead, there is a different concept represented by isPresent(); which returns true when the wrapped value is non-null.

So it feels like this PR conflates the two concepts.

garydgregory avatar Aug 12 '22 21:08 garydgregory

I was thinking about this, and if we wanted to look at the contents of the Optional rather than the optional itself, there are three ways I've though of doing it - two recursive and two non-recursive:

Recursive with Functional Methods:

if (object instanceof Optional<?>) {
    return ((Optional<?>) object).map(ObjectUtils::isEmpty).orElse(false);
}

Recursive with Non-Functional Methods:

if (object instanceof Optional<?>) {
    return ((Optional<?>) object).isPresent() && isEmpty(((Optional<?>) object).get());
}

Non-recursive

public static boolean isEmpty(Object object) {
    if (object instanceof Optional<?>) {
        object = ((Optional<?>) object).orElse(null);
    }
    if (object == null) {
        return true;
    }
    if (object instanceof CharSequence) {
        return ((CharSequence) object).length() == 0;
    }
    if (isArray(object)) {
        return Array.getLength(object) == 0;
    }
    if (object instanceof Collection<?>) {
        return ((Collection<?>) object).isEmpty();
    }
    if (object instanceof Map<?, ?>) {
        return ((Map<?, ?>) object).isEmpty();
    }
    return false;
}

I like the non-recursive method best, however, it does change the Object object parameter from final to non-final.


Also, there is an isEmpty() method on optionals that was added in Java 11: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Optional.html#isEmpty()

hendrixjoseph avatar Aug 15 '22 19:08 hendrixjoseph

I create another PR (#934) that uses the approach mentioned in my previous comment (using the non-recursive way) instead of looking at the optional itself.

I'm unsure which is the "better" or "preferred" way and I agree more community input is needed.

hendrixjoseph avatar Aug 16 '22 02:08 hendrixjoseph

Hi All:

The addition of Optional#isEmpty() in Java 11 seems to be an argument in favor of supporting Optional in our isEmpty() method, I think we need to consider if it makes sense, as opposed to just being doable. IOW, is a wrapper of a null value "empty"? Is a wrapper of an empty String empty? Is a wrapper of an empty Collection empty?

Using recursion seems to be a recipe for confusion: Why recurse into one kind of object and not another? IOW, why recuse into Optional and not into Collections and Maps?

What do others think?

I do not think we need to consider an implementation vs. another until we can give answers and provide a consistent picture.

garydgregory avatar Aug 16 '22 10:08 garydgregory