How to optimize the if-else statement due to Optional Object's properties also being optional?

Question

I have an Optional Object named Form. This has 3 other optional properties to it. Reason being, if this form object is not present, I have to generate my own form data. If the form is present, users can still choose to leave their properties empty, requiring my method to generate them in that case. The way that I have implemented this logic goes as follows.

private FormSection buildForm(final Optional<Form> formOptional) {
    Set<String> formNames = null;
    Set<Question> formTypes = null;
    Set<Question> formDetails = null;

    if (formOptional.isPresent()) {
        Form form = formOptional.get();
        formNames = formSection.names()
                .orElseGet(() -> getNamesFromDB());
        formTypes = formSection.formTypes()
                .orElseGet(() -> getFormTypesFromDB());
        formDetails = formSection.formDetails()
                .orElseGet(() -> getFormDetailsFromDB());
    } else {
        formNames = getNamesFromDB();
        formTypes = getFormTypesFromDB();
        formDetails = getFormDetailsFromDB();
    }
    return Form()
            .names(formNames)
            .formTypes(formTypes)
            .formDetails(formDetails)
            .build();
}

I feel like I'm redoing some checks and that this whole if-else statement could be optimized. I'm using Java 8 by the way.

`Optional` is (usually) not well suited for parameters. It’s for return values. Consider forcing your caller to determine whether they have a `Form` or not. One recommended design is having two overloaded methods, one without any parameter for when you don’t have a `Form`, and one with a `Form` parameter that must not be `null`. The two may share some logic behind the scenes, of course. — Ole V.V., Jan 16 '21 at 11:59
Generally, you should not do such obsolete `null` pre-initialization of variables. The variables are assigned in each branch. But in your case, you don’t need two branches at all, you can use `return Form() .names(formOptional.flatMap(Form::names).orElseGet(() -> getNamesFromDB())) .formTypes(formOptional.flatMap(Form::formTypes).orElseGet(() -> getFormTypesFromDB())) .formDetails(formOptional.flatMap(Form::formDetails) .orElseGet(() -> getFormDetailsFromDB())) .build();` — Holger, Jan 18 '21 at 13:13

score 3 · Answer 1 · answered Jan 16 '21 at 00:11

Make a form impl that is entirely blank. Then strongly reconsider your APIs; Optional is an appropriate thing to return when you're writing stream terminals. It's borderline okay to return them for methods (but not idiomatic). It's almost universally considered 'wrong' to use them anywhere else (such as fields, or as a parameter type); see for example this top answer (Brian Goetz was team lead for Lambdas at oracle) that tells you not to do this.

The preferred solution to avoid nulls is to have sensible defaults if possible. Not all object kinds have these (and don't force it), but given that you have a form that has 100% optional data, it is appropriate here: Is there a semantic difference between a 'null' form (or a non-present optional, if you prefer), and a present/non-null form where every single thing the form contains is null/non-present?

I don't think you intend for there to be a difference. In which case - great. Ditch optional forms. Adopt the default form. You can then just pass the 'default form' (the one where all 'fields' are non-present) to this method:

class Form {
    public static final Form BLANK_FORM = ....;
}

private FormSection buildForm(Form form) {
}

and adjust whatever code currently ends up with an Optional<Form> to instead shift to returning a blank form. For example, instead of:

Optional<Form> formOptional = Optional.ofNullable(x);

use:

Form form = x == null ? Form.BLANK_FORM : x;

Or if you have:

if (....) {
    return Optional.empty();
} else {
    // fetch all sorts of details.
    Form f = new Form(...);
    return Optional.of(f);
}

replace those return statements with respectively return Form.BLANK_FORM; and return f;. You get the idea :)

Thank you very much for this detailed answer. I think I understand what you're trying to say, but I do have one situation where I'm still lost with this proposed solution. So basically this "Form" class currently is an Immutable class that maps the json requests passed to the code to a Java class. So using a default form does make sense, but there can be an instance where the user wants to pass only a single property and let the rest take defaults. Would this solution still work for that? Another design problem with using defaults is that Immutable class would have to depend on the DB :( — AnOldSoul, Jan 16 '21 at 00:23
_Would this solution still work for that?_ Yes, it would. _Another design problem with using defaults is that Immutable class would have to depend on the DB_ No, it wouldn't need to. Change nothing, other than introduce a constant (public static final) field containing a form initialized with all-blank (Optional.empty()) values. That's all you need. — rzwitserloot, Jan 16 '21 at 01:16

areus · Answer 2 · 2021-01-18T14:08:26.390

You can use flatMap method from Optional to "chain" operations that return another Optional

formNames = form.flatMap(Form::names).orElseGet(() -> getNamesFromDB());

flatMap gets applied only if form is not empty, otherwise returns an empty optional. If it's present, is replaced by the optional returned by names() method in Form.

private FormSection buildForm(final Optional<Form> opForm) {

    Set<String> formNames = opForm.flatMap(Form::names).orElseGet(() -> getNamesFromDB());
    Set<Question> formTypes = opForm.flatMap(Form::formTypes).orElseGet(() -> getFormTypesFromDB());
    Set<Question> formDetails = opForm.flatMap(Form::formDetails).orElseGet(() -> getFormDetailsFromDB());

    return Form()
            .names(formNames)
            .formTypes(formTypes)
            .formDetails(formDetails)
            .build();
}

How to optimize the if-else statement due to Optional Object's properties also being optional?

2 Answers2