Code review: Are you using Optional.orElse wrong?

After using Java 8 for a few years now I’ve seen one particular misuse of the new APIs pop up a few times and it’s when using the orElse and orElseGet methods. The mistake is easy to make and there is a chance that it exists in your code base too!

TL;DR

It’s easy to make the mistake and use Optional.orElse when you should have used orElseGet. The result is that what you thought would have been executed lazily or not at all will be called immediately. So make sure you aren’t for example accessing the database in your orElse methods.

If you like reading code more than text, skip to the Try it out section. There is a regex down there as well so you can search your own code base.

The code above contains a nasty bug, can you spot it?

🐛 The bug

In our example we have a method for fetching a users inventory from the database. The contents of the inventory is not important so you are free to decide whatever fun stuff it contains.

The method declaration is:

public Optional<Inventory> findInventoryByUser(User user)

Which means that if we find an Inventory for the user the returned Optional will contain it. Otherwise the Optional will be empty.

Let’s start out with some code using this method

public Inventory getInventory(User user) {
Optional inventory = findInventoryByUser(User user)
    if (inventory.isPresent()) {
return inventory.get();
} else {
throw new NotFoundException();
}
}

Nice! That is a nice method that we can use to fetch inventories from. But it could be simplified a bit using some of the Optional APIs.

public Inventory getInventory(User user) {
return findInventoryByUser(User user)
.orElseThrow(() -> new NotFoundException());
}

Woah! Short and readable, like it! But we want to change this a bit. We don’t want to throw an exception if we don’t find the Inventory we just simply want to create one and insert it into the database.

public Inventory getInventory(User user) {
return findInventoryByUser(userUuid)
.orElse(createInventory(user));
}

Looks good right? And easy to read too! The only problem here is that no matter if we find the inventory or not we’ll create a new empty inventory and insert it into our database. So everytime someone calls this method we’ll create another inventory… OOPS!

🛠 The solution

This is the corrected code using the orElseGet method instead using the Supplier interface to only create the inventory if it isn’t found.

public Inventory getInventory(User user) {
return findInventoryByUser(userUuid)
.orElseGet(() -> createInventory(user));
}

To clarify the difference let’s look at the method declarations

public T orElse(T other)
public T orElseGet(Supplier<? extends T> other)

The orElse method takes the value other which it returns if the Optional returned from findInventoryByUser was empty.

The orElseGet method takes a Supplier which it invokes and returns the result from if the Optional returned from findInventoryByUser was empty. Meaning that the method createInventory will only be invoked if we couldn’t find an inventory.

Simple rule for orElse and orElseGet methods

The simple rule that I follow for these methods is that for orElse I always supply a constant or a variable and if I need to call a method or constructor I use orElseGet.

inventory.orElse(EMPTY_INVENTORY); // Constant for orElse
inventory.orElseGet(() -> createEmptyInventory()); // Function call for orElseGet

Search regex

Use this regex in for example Intellij to search for this potential bug. Don’t forget to turn Regex search on. Thanks Leon Hennings and Qinjin Wang for small updates 👍

orElse\([A-Za-z\s\.]*\(

💻 Try it out

Here is some code for you to try it out yourself.