Simplify many if checks with Optional

Question

if (reg[0] != null && reg[0].trim().length() > 0) {
   orderData.setCity(reg[0]);
}
if (reg[1] != null && reg[1].trim().length() > 0) {
   orderData.setCountry(reg[1]);
}
if (reg[2] != null && reg[2].trim().length() > 0) {
   orderData.setObjectType(reg[2]);
}
if (reg[3] != null && reg[3].trim().length() > 0) {
   orderData.setChannel(reg[3]);
}

Can I simplify this with Optional, or other Java 8 features?

create a method that accepts a `String` and does that check for you, that would improve the readability a lot more. — Naman, May 07 '20 at 19:39
`Optional.ofNullable(reg[0]).filter(r -> r.trim().length() > 0).ifPresent(orderData::setCity);` - Though I wouldn't say it's much cleaner. — Vince, May 07 '20 at 19:41
Seems like a duplicate of https://stackoverflow.com/q/32567208 — Dawood ibn Kareem, May 07 '20 at 19:49

score 4 · Accepted Answer · answered May 07 '20 at 19:44

If you were to really look for an implementation involving Optional here, the method I had suggested in comments could be implemented to return one

private Optional<String> validInput(String input) {
    return Optional.ofNullable(input)
                   .filter(in -> in.trim().length() > 0);
}

then this can be used as

validInput(reg[0]).ifPresent(city -> orderData.setCity(city));
validInput(reg[1]).ifPresent(country -> orderData.setCountry(country));
... and the likes

Hint: `city -> orderData.setCity(city)` can be shortened to `orderData::setCity` — Peter Walser, May 07 '20 at 19:48

score 3 · Answer 2 · answered May 07 '20 at 19:47

3

Optional would be the wrong tool here.

Leverage StringUtils.isNotBlank instead.

if (StringUtils.isNotBlank(reg[0])) {
   orderData.setCity(reg[0]);
}

You still have to write each value out since they map to different fields, but this will clean it up and allow you to use a more conventional way to check for a blank string.

answered May 07 '20 at 19:47

Makoto

104,088
27
192
230

1

Naman's answer is a good example of how `Optional` can improve readability: `validInput(reg[0]).ifPresent(orderData::setCity)`. However, your answer better points out the actual issue he's having: he's wasn't abstracting out the validation into a re-usable form. Focusing on `Optional` is XY, but I wouldn't say it's a wrong tool for his situation, it's just not required to fix his problem, and kind of distracts from the actual issue he was having. Your answer makes it very clear that `Optional` isn't a requirement for solving this. – Vince May 07 '20 at 19:57

score 2 · Answer 3 · answered May 07 '20 at 19:47

Optional is probably not going to help you here.

The first thing to think of, is sentinel values. Here you are clearly indicating that you think that, semantically, reg[0] being null is equivalent to reg[0] being the empty string (or even a string with only spaces in it). That is supposed to be extraordinary. null is not a standin for 'nothing' or 'empty'. null is supposed to be a standin for 'There is no value'. The best way to do this, is to find the place that generates reg[0], and ensure that the proper semantic meaning (an empty string) is set up here. Then this code can simply be: if (!reg[0].isEmpty()) orderData.setCity(reg[0]); - so much cleaner.

That is, of course, not always possible. For example, if reg is coming down the pipe from a library or other code that simply isn't under your control, or it is an object created by, say, a JSON demarshaller.

In that case I generally would advise creating an explicit step to convert an object that is 'not clean' (has nulls even when semantically that's supposed to be an empty string) into a clean one.

If that is not feasible or not possible, well, working with an 'unclean' object is never going to be particularly pretty, style wise. A helper method would help a lot, here:

normalizeThenSetIfNonBlank(reg[0], orderData::setCity);
normalizeThenSetIfNonBlank(reg[1], orderData::setCountry);

private void normalizeThenSetIfNonBlank(String in, Consumer<String> target) {
    if (in == null) return;
    in = in.trim();
    if (in.isEmpty()) return;
    target.accept(in);
}

This changes the semantics. OP's code processes untrimmed data; trimming was only for validation. Your code processes trimmed data. Although it may not matter, it's something to keep in mind. To keep the semantics the same, introduce a new local variable to reference the trimmed value, and validate on that instead of re-assigning the parameter variable. — Vince, May 07 '20 at 20:01

score 0 · Answer 4 · answered May 07 '20 at 20:06

Optional.ofNullable(reg[0]).filter(val -> val.trim().length() > 0).ifPresent(orderData::setCity);
Optional.ofNullable(reg[1]).filter(val -> val.trim().length() > 0).ifPresent(orderData::setCountry);
Optional.ofNullable(reg[2]).filter(val -> val.trim().length() > 0).ifPresent(orderData::setObjectType);
Optional.ofNullable(reg[3]).filter(val -> val.trim().length() > 0).ifPresent(orderData::setChannel);

Simplify many if checks with Optional

4 Answers4