3

Yesterday I had to write an ugly piece of code, in order to perform many null-checks on the fields of an object, in order to avoid NPE from ternary operator construct.

The problematic piece of code:

ResourceThresholds rt = getThresholdsFromConfig();
Thresholds defaultPerContainer = getDefaultThresholds();
    
return new Thresholds((!rt.getCpu().equals("")) ? Long.parseLong(rt.getCpu()) : defaultPerContainer.getCpu(),
           (!rt.getMemory().equals("")) ? Long.parseLong(rt.getMemory())  : defaultPerContainer.getMemory(),/*omitted for brevity*/);

I get a NPE on defaultPerContainer.getCpu(), because the field cpu = null. And this is fine, Java works the way it works. Why I didn't just defaulted the field Long cpu = 0L; ? Because I need the null value as an indicator that we do not set any value.

The final functional variant of this particular piece of code ended up being:

        Long cpuVal;
        if (!rt.getCpu().equals("")) {
            cpuVal = Long.parseLong(rt.getCpu());
        } else {
            cpuVal = defaultPerContainer.getCpu();
        }
        Long memory;
        if (!rt.getMemory().equals("")) {
            memory = Long.parseLong(rt.getMemory());
        } else {
           memory = defaultPerContainer.getMemory();
        }
        //... many similar if-elses that give me the desired value;
        //which is really ugly, and I believe I am not the only one hitting this.
        return new Thresholds(cpuVal, memory..);

This code works as I needed to work but it is ugly!

Q1: Can someone hint me on whether I can find a way of using Optional<T> to resolve the NPE in the first variant with the ternary operator? Because this snippet works: !rt.getCpu().equals("")) ? Long.parseLong(rt.getCpu()) : null i.e. if I explicitly put null as a value, I get null when the condition is met.

In general, is there any elegant Java 8+ way to deal with this?

Q2: how do you optimize the glorious if-else construct for null-checking?

gai-jin
  • 653
  • 2
  • 10
  • 24
  • @DawoodibnKareem yes – gai-jin Jan 21 '21 at 06:41
  • @DawoodibnKareem java.lang.Long – gai-jin Jan 21 '21 at 06:48
  • OK I think I know what's happening. Do you still get the problem if you change the part between the `?` and the `:` to `new Long(rt.getCpu())` at all? If that fixes it, it's because the ternary operation causes both the "true" and the "false" part to be interpreted as `long` instead of `Long`. – Dawood ibn Kareem Jan 21 '21 at 06:57
  • I am still getting the NPE, the logically correct 3rd operand would be ((Long) defaultPerContainer.getCpu()). In general, I think that you cannot cast null? ( defaultPerContainer.getCpu() returns null! ) – gai-jin Jan 21 '21 at 07:02

2 Answers2

2

The problem is that in the ternary expression A ? B : C, if B and C are both compatible numeric types, but one is the boxed object and the other is a primitive, most people would think the result is boxed, by auto-boxing the primitive.

That is not the case. The ternary operator instead unboxes the object, so they are both primitives, and the result is then a primitive.

Which means that the following are the same:

long B = ...;
Long C = ...;

Long R = ... ? B : C;

Long R = (Long) (... ? B : (long) C);

The result is that if C is null, you get NPE.

One way to fix it is to force auto-boxing of B:

Long R = ... ? (Long) B : C;

With that change, a null C value will simply set R = null.

In the case in the question, B is Long.parseLong(rt.getCpu()), so instead of adding a cast to force auto-boxing, use long.valueOf(String s) instead.

Also, unrelated, use isEmpty() instead of equals(""), and there's no need for parentheses around A.

Change the code to:

return new Thresholds(!rt.getCpu().isEmpty() ? Long.valueOf(rt.getCpu()) : defaultPerContainer.getCpu(),
                      !rt.getMemory().isEmpty() ? Long.valueOf(rt.getMemory())  : defaultPerContainer.getMemory(),
                      /*omitted for brevity*/);
Andreas
  • 154,647
  • 11
  • 152
  • 247
0
  1. There's no null checks in the code snippet.
  2. It's better to implement a simple utility method and use it when setting cpu and memory, and use Joda condition "".equals(val) to prevent NPE if val is null
  3. To avoid unboxing, use Long.valueOf instead of Long.parseLong which returns primitive long:
public static Long getValue(String val, Long defaultValue) {
    return "".equals(val) ? defaultValue : Long.valueOf(val);
}

Long cpuVal = getValue(rt.getCpu(), defaultPerContainer.getCpu());
Long memory = getValue(rt.getMemory(), defaultPerContainer.getMemory());

It is also possible to provide an overloading utility method using supplier of arguments and then pass the method references to it:

public static Long getValue(Supplier<String> str, Supplier<Long> defVal) {
    return getValue(str.get(), defVal.get()); // calling implementation above 
}

Long cpuVal = getValue(rt::getCpu, defaultPerContainer::getCpu);
Long memory = getValue(rt::getMemory, defaultPerContainer::getMemory);
Nowhere Man
  • 19,170
  • 9
  • 17
  • 42
  • **`NullPointerException`** when `defaultValue = null`. --- Moving the code to a helper method doesn't do anything if you don't fix the actual problem. See [my answer](https://stackoverflow.com/a/65822543/5221149) for actual problem. – Andreas Jan 21 '21 at 07:08
  • Agree, prevented NPE by avoiding unboxing. Hopefully, `defaultPerContainer` in the question is not null. – Nowhere Man Jan 21 '21 at 07:27
  • *Curious:* What's the point on the `Supplier` version? – Andreas Jan 21 '21 at 07:27
  • If `defaultPerContainer` is null, then the fix in the question using `if` statements wouldn't work, so that is not the case. – Andreas Jan 21 '21 at 07:28
  • @Andreas, just an example that a function can be used instead of the value – Nowhere Man Jan 21 '21 at 07:30