Using Optional to prevent NullPointers In TrainWrecks

Question

I was just wondering if the following is the wrong use case for Optional. It probably is, because it looks nasty.

(It is regarding calling "Legacy" code that returns nulls.)

  private static class Person {
    private Address address;
    private String name;
  }

  private static class Address {
    private Integer housenr;
    private String houseletter;
    private String street;
  }

  public String getAddress(Person person) {
    return Optional.ofNullable(person.getAddress())
      .map(x -> Optional.ofNullable(x.getHousenr()).map(y -> y + " ").orElse("") +
          Optional.ofNullable(x.getHouseletter()).map(y -> y + " ").orElse("") +
          Optional.ofNullable(x.getStreet()).orElse(""))
      .orElse("<unknown>");
  }

  // unit test if string representation of an address is properly generated
  assertThat(getAddress(charlesBabbage))
      .isEqualTo("4 l Regentstreet");

I should probably put the code in a method of the Person class and the Address class, so it doesn't bother me so much.

Or should I do it "the old way":

  public String getAddress(Person person) {
    if (person.getAddress() == null) {
      return "<unknown>";
    }
    StringBuilder builder = new StringBuilder();
    if (person.getAddress().getHousenr() != null) {
      builder.append(person.getAddress().getHousenr() + " ");
    }
    if (person.getAddress().getHouseletter() != null) {
      builder.append(person.getAddress().getHouseletter() + " ");
    }
    if (person.getAddress().getStreet() != null) {
      builder.append(person.getAddress().getStreet() + " ");
    }
    return builder.toString();
  }

Bear in mind that this is just an example. More fields can be added like affix., postoffice box, town/city, municipality, state, country (not to mention foreign addresses) exacerbating the problem.

Answer 1

In both your examples, you are repeating code that checks that elements are not null before adding them to a String. You can reduce this repetitive work by using Stream and joining:

public String getAddress(Person person) {
    return Optional.ofNullable(person.getAddress())
            .map(x -> Stream.of(x.getHousenr(), x.getHouseletter(), x.getStreet())
                        .filter(Objects::nonNull)
                        .map(Object::toString)
                        .collect(Collectors.joining(" "))
            )
            .orElse("<unknown>");
}

Or similarly without Optional:

public String getAddress(Person person) {
    Address address = person.getAddress();
    if (address == null) {
        return "<unknown>";
    }
    return Stream.of(address.getHousenr(), address.getHouseletter(), address.getStreet())
            .filter(Objects::nonNull)
            .map(Object::toString)
            .collect(Collectors.joining(" "))
}

Answer 2

I do not believe it’s the wrong use case at all. True, it is not that easy to read without thinking about formatting and white space, but the way you are using Optional seems fine to me.

For the sake of an assertion, it’s fine to have that (truly ugly) code. I don’t know your final intended use case, but assuming you just want to generate String representations of these objects, I would encapsulate the logic which checks for nulls within the corresponding classes, in a toString() method or similar.

edit: Nasty looking code does not make its use wrong. Though your coworkers might end up hating you