4
\$\begingroup\$

I am writing a Java method to extract a String property, called number, from a Person object.

A Person object has several properties that can be null and number belongs to house, which belongs to addresses, which belongs to metadata which finally belongs to our person.

As addresses is a list of Address objects, I want to extract the first non-null number. That is what I have so far:

public class HouseNumberResolver {
    public static String numberFrom (Person person) {
        return Optional.ofNullable(person)
                .map(Person::getMetadata)
                .map(Metadata::getAddresses)
                .map(HouseNumberResolver::getFirstHouseNumber)
                .orElse("");
    }

    private static String getFirstHouseNumber(List<Address> addresses) {
        for (Address address : addresses) {
            String number = Optional.ofNullable(address)
                    .map(Address::getHouse)
                    .map(House::getNumber)
                    .orElse(null);

            if (number != null) {
                return number;
            }
        }

        return null;
    }
}

However, I feel that this is quite ugly. Ideally, I wanted to have something like:

    private static String getFirstHouseNumber(List<Address> addresses) {
        for (Address address : addresses) {
            Optional.ofNullable(address)
                .map(Address::getHouse)
                .map(House::getNumber)
                .ifPresent(number -> return number)
                .orElse(null)
        }
    }

That would return the number there, but it seems like this is not possible.

Any recommendations are welcome.

\$\endgroup\$
2
  • \$\begingroup\$ Apart from your question: don't surprise your readers: your methods name is numberFrom but returns a string object (or do you deal with sub numbers line 1a?. But more important: do not return null unless it is a valid member of the result set, which I argue here. \$\endgroup\$ Commented Nov 12, 2017 at 20:51
  • \$\begingroup\$ You are right for the first part! As for the second part, I do not return null from the public method, but from the private (which will become an empty string return in the public one). \$\endgroup\$
    – pavlos163
    Commented Nov 12, 2017 at 22:45

1 Answer 1

3
\$\begingroup\$

The problem here is that your Optional would be required to either return your method or to continue with the next loop iteration. This cannot be expressed that way in Java.

One solution is to convert from external iteration (using the for loop) to internal iteration using a Stream. You can find a lot of documentation for working with streams (start with the Javadoc for Stream and the java.util.stream package), so I won't go into low level details here.

addresses.stream()
    .map(Address::getHouse)
    .map(House::getNumber)
    .filter(number -> number != null)
    .findFirst();

This executes the mappings, drops all numbers that are null and returns the first non-null number. It solves the return-vs-continue problem, but it won't work as expected because Stream.map(…) will happily put a null value in the resulting stream. You would get a NullPointerException at map(House::getNumber) if some Address has no House.

You can use your original Optional chain as mapping function and your method would look like this:

private static String getFirstHouseNumber(List<Address> addresses)
{
    return addresses.stream()
            .map(address -> Optional.ofNullable(address)
                    .map(Address::getHouse)
                    .map(House::getNumber))
            .filter(Optional::isPresent)
            .map(Optional::get)
            .findFirst()
            .orElse(null);
}

This looks more concise than your first method but it is not really that obvious what is going on. You can probably guess it but to say for sure you will probably need some moments.

Maybe one can come up with something a bit more readable but I'm afraid that given the current setup it won't get much better.


To get much more benefit (not just for this case but for the whole application) you may think about refactoring.

Note that the following may or may not be useful. As I don't know the rest of your application this are just some observations and hints from the perspective of this code snippet.

I see two main problems here:

  1. Extensive use of null
  2. Violation of law of Demeter

Is it really the case that all of your properties may be null here?

I guess each Person should have some Metadata (even if sparsely filled). You can always get a list of known addresses for a person. This list may be empty but it should not contain null values. An Address should always have a House on it if some Person lives there. So the only thing that may be null is the house number (e.g. if you know the person lives in city X but you don't know the exact street and house number).

Does your code at this point really have to know how your person is structured internally?

From the perspective of the code above I would expect to get a list of addresses directly from the person. I don't care about the metadata. Similarly I don't care about the house.

Maybe you check the house number for null just to verify if the address is complete? Maybe what you are actually looking for is the first valid and complete address of some person?

If all this assumptions apply, your code could look like this:

public Optional<Address> getValidAddress(Person person) {
    return person.getAddresses().stream()
            .filter(Address::isComplete)
            .findFirst();
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ The title says java 8, but if 9 or better is allowed, you can simplify ....filter(Optional::isPresent).map(Optional::get)... to ....flatMap(Optional::stream)... \$\endgroup\$
    – Marty Neal
    Commented May 6, 2019 at 22:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.