2

This question is similar to Reading multiple variables from an object wrapped in Option[] but in the context of Java instead of Scala.

Suppose I have a method that returns an Optional<Address>. I need to extract multiple fields from the wrapped address, say getCity(), getCountry(), and getZIP(). If the address is not present, some default values should be used. Then the logic should continue with the resulting values.

The question is what is the best and most idiomatic way of doing this. I can think of the following three:

1) if-else

City city;
Country country;
ZIP zip;
Optional<Address> optAddr = getAddress();
if (optAddr.isPresent()) {
  Address addr = optAddr.get();
  city    = addr.getCity();
  country = addr.getCountry();
  zip     = addr.getZIP();
}
else {
  city    = null;
  country = Country.getUSA();
  zip     = ZIP.DEFAULT;
}
// ... use city, country, and zip

Pros:

  • No extra objects created - most efficient variant.
  • Default-cases are grouped together.

Cons:

  • Verbose.

2) Multiple chains

Optional<Address> optAddr = getAddress();
City city       = optAddr.map(Address::getCity()
                         .orElse(null);
Country country = optAddr.map(Address::getCountry)
                         .orElseGet(Country::getUSA);
ZIP zip         = optAddr.map(Address::getZIP)
                         .orElse(ZIP.DEFAULT);
// ... use city, country, and zip

Note that here the logic is slightly different since default value apply not only when the address is missing, but also when the corresponding address field is null.

But that's not an issue in my case.

Pros:

  • Concise.
  • Variables are immediately initialized to a value.
  • Closer to how it is done with a single field.

Cons:

  • Default values are spread apart.
  • Chained calls to map potentially create intermediate objects (but in an idiomatic way).
  • Maybe unidiomatic use of multiple chains on a single Optional?

3a) Wrap default values in an object

private Address createDefaultAddress() {
  Address addr = new Address();
  addr.setCity(null);
  addr.setCountry(Country.getUSA());
  addr.setZIP(ZIP.DEFAULT);
  return addr;
}

Address addr = getAddress().orElseGet(this::createDefaultAddress);
City city       = addr.getCity();
Country country = addr.getCountry();
ZIP zip         = addr.getZIP();
// ... use city, country, and zip

Pros:

  • Clarity.

Cons:

  • Fields are packed into an Address just for extracting them immediately afterwards.

3b) Wrap default values in a constant

As suggested by @HariMenon and @flakes, the an Address with default values can be stored in a constant and reused for each call to reduce the overhead.

One could also initialize that 'constant' lazily:

private Address defaultAddress;

private Address getDefaultAddress() {
  if (defaultAddress == null) {
    defaultAddress = new Address();
    defaultAddress.setCity(null);
    defaultAddress.setCountry(Country.getUSA());
    defaultAddress.setZIP(ZIP.DEFAULT);
  }
  return defaultAddress;
}

Address addr = getAddress().orElseGet(this::getDefaultAddress);
// ... same as 3.1

Bonus question:

Suppose I have full control of the getAddress() method, and I know that all usages would be similar to this example, but the default values vary. Should I change it to return null instead of Optional to better accomodate the if-else solution?

mperktold
  • 436
  • 4
  • 14
  • 2nd one is giving you the best deal. Your individual values can be null in the 3rd case even if address is not null. So effectively you will end up without defaults for individual fields. – muasif80 Mar 02 '19 at 07:18
  • @muasif80 that's right. In my specific use case, that's not an issue, but it's definitely something to have in mind when applying the pattern to another use case. – mperktold Mar 02 '19 at 09:43

2 Answers2

2

This is somewhat subjective, so there isn't going to be a right answer. But here's my 2 cents and why I think so. Option 1 is definitely not the idiomatic way of doing this with Java 8.

Between 2 and 3, what you would choose would depend on whether you want to use a default Address, or would you want a separate default city, country and zip. If Address is not null, but city is null, do you still want to use a default city? Option 3 would make that ugly. Otherwise, I prefer option 3. Even if you are wrapping it into an object, that object can be a constant named DEFAULT_ADDRESS, and then it becomes very clear exactly what it is, and is therefore more readable.

Option 2 makes it read as if you have a separate default country, city and zip, which does not seem to be the case. But like I said, if that is the case, you should use that.

Edit for 3b and bonus: If you are thinking about perf impact of the wrapper, you are way overthinking it. Creating wrappers like these is extremely cheap in Java and modern JIT compilers will optimize this very well in most cases. Readability over perf unless the code is proved by measurement to be a perf bottleneck. For Optional vs null, just pick one coding style and stick with it for the project.

If you are returning Optionals, then make sure that method never returns null (a method that can return both Optional and null is an abomination). Many people are of the opinion that you should always use Optionals. Personally, I disagree slightly. If it is a new codebase and we are using Optionals everywhere, then it is great. Stick with it and don't use nulls and null checks anywhere. If it is an old codebase which already uses nulls at several places, just use nulls and document that the method can return null in certain cases. Combining Optionals and nulls creates a terrible mess, and I don't think Optional gives sufficient benefit to outweigh that cost. Optionals are great in languages that supported them from the beginning. But they were added as an afterthought in Java, and Java still has nulls, so the benefits are actually not that great IMO. This is again very subjective though, and what I would use will vary on a case-by-case basis.

Hari Menon
  • 33,649
  • 14
  • 85
  • 108
  • I agree. Please see also my edit regarding lazily initializing the default address (option 3b) and returning null instead of Optional from getAddress() (bonus question). What do you say about these? – mperktold Mar 02 '19 at 10:37
  • I have kind of a long winded response to that. Updated my answer. – Hari Menon Mar 02 '19 at 20:56
  • It‘s indeed an old codebase, so you point is highly relevant. Thanks! – mperktold Mar 03 '19 at 06:55
0

I agree with the points made by @HariMenon. If I were you, I would opt for option three. However, instead of creating a new Address each time, just create the values once and store the Address in a private field. If an appropriate ctor does not exist, you could use double brace initialization to simplify the construction slightly. Just make sure not to mutate the default Address variable.

private static final Address DEFAULT_ADDRESS = new Address() {{
    setCity(null);
    setCountry(Country.getUSA());
    setZIP(ZIP.DEFAULT);
}};
...
Address addr = getAddress().orElse(DEFAULT_ADDRESS);
flakes
  • 21,558
  • 8
  • 41
  • 88
  • I agree that creating a constant is better. However, I'm not a big fan of double brace initialization, as it unnecessarily creates a subclass with an outer this pointer. – mperktold Mar 02 '19 at 09:23
  • @mperktold I tend to agree. I wouldn't want to expose this class to the rest of the code base, but if the object is generated once, and is a constant, and is isolated to the file, then there is very little downside to using it. The take-away I offer is that you can just initialize the variable once, either as double brace init or a static helper method. – flakes Mar 02 '19 at 09:45