Your main mistake is that you're still thinking in more procedural terms. This is not meant as a criticism of you as a person, it's merely an observation. Thinking in more functional terms comes with time and practice, and therefore the methods isPresent and get look like the most obvious correct things to call for you. Your secondary minor mistake is creating your Optional inside your method. The Optional is meant to help document that something may or may not return a value. You might get nothing.
This has led you do write perfectly legible code that seems perfectly reasonable, but you were seduced by the vile twin temptresses that are get and isPresent.
Of course the question quickly becomes "why are isPresent and get even there?"
A thing that many people here miss is that isPresent() is that it's not something that is made for new code written by people fully on board with how gosh-darn useful lambdas are, and who like the functional thing.
It does however, give us a few (two) good, great, glamourous(?) benefits:
- It eases transition of legacy code to use the new features.
- It eases the learning curves of Optional.
The first one is rather simple.
Imagine you have an API that looked like this:
public interface SnickersCounter {
/**
* Provides a proper count of how many snickers have been consumed in total.
*/
public SnickersCount howManySnickersHaveBeenEaten();
/**
* returns the last snickers eaten.<br>
* If no snickers have been eaten null is returned for contrived reasons.
*/
public Snickers lastConsumedSnickers();
}
And you had a legacy class using this as such (fill in the blanks):
Snickers lastSnickers = snickersCounter.lastConsumedSnickers();
if(null == lastSnickers) {
throw new NoSuchSnickersException();
}
else {
consumer.giveDiabetes(lastSnickers);
}
A contrived example to be sure. But bear with me here.
Java 8 has now launched and we're scrambling to get aboard.
So one of the things we do is that we want to replace our old interface with something that returns Optional.
Why?
Because as someone else has already graciously mentioned:
This takes the guesswork out of whether or not something can be null
This has already been pointed out by others. But now we have a problem. Imagine we have (excuse me while I hit alt+F7 on an innocent method), 46 places where this method is called in well tested legacy code that does an excellent job otherwise. Now you have to update all of these.
THIS is where isPresent shines.
Because now:
Snickers lastSnickers = snickersCounter.lastConsumedSnickers();
if(null == lastSnickers) {
throw new NoSuchSnickersException();
}
else {
consumer.giveDiabetes(lastSnickers);
}
becomes:
Optional<Snickers> lastSnickers = snickersCounter.lastConsumedSnickers();
if(!lastSnickers.isPresent()) {
throw new NoSuchSnickersException();
}
else {
consumer.giveDiabetes(lastSnickers.get());
}
And this is a simple change you can give the new junior: He can do something useful, and he'll get to explore the codebase at the same time. win-win. After all, something akin to this pattern is pretty widespread. And now you don't have to rewrite the code to use lambdas or anything.
(In this particular case it would be trivial, but I leave thinking up examples where it would be difficult as an exercise to the reader.)
Notice that this means that the way you did it is essentially a way to deal with legacy code without doing costly rewrites. So what about new code?
Well, in your case, where you just want to print something out, you'd simply do:
snickersCounter.lastConsumedSnickers().ifPresent(System.out::println);
Which is pretty simple, and perfectly clear.
The point that's slowly bubbling up to the surface then, is that there exist use cases for get() and isPresent(). They're there for letting you modify existing code mechanically to use the newer types without thinking too much about it.
What you're doing is therefore misguided in the following ways:
- You're calling a method that may return null. The correct idea would be that the method returns null.
- You're using the legacy bandaid methods to deal with this optional, instead of using the tasty new methods that contain lambda fanciness.
If you do want to use Optional as a simple null-safety check, what you should have done is simply this:
new Optional.ofNullable(employeeServive.getEmployee())
.map(Employee::getId)
.ifPresent(System.out::println);
Of course, the good looking version of this looks like:
employeeService.getEmployee()
.map(Employee::getId)
.ifPresent(System.out::println);
By the way, while it is by no means required, I do recommend using a new line per operation, so that it's easier to read.
Easy to read and understand beats conciseness any day of the week.
This is of course a very simple example where it's easy to understand everything that we're trying to do. It's not always this simple in real life. But notice how in this example, what we're expressing is our intentions. We want to GET the employee, GET his ID, and if possible, print it. This is the second big win with Optional. It allows us to create clearer code. I also do think that doing things like making a method that does a bunch of stuff so that you can feed it to a map is in general a good idea.
