Dfyjkt

I was updating the dependencies to Spring 5 in my project and was bombarded with compilation errors where the method definition of findOne() has been replaced by findById() which now returns an Optional (correct me if I am wrong).

While refactoring, I came across multiple approaches that I can choose to adopt, and I would therefore like some input on which one is to be preferred.

1st approach:

ExpectedPackage ep = expectedPackageRepository.findById(1).orElse(null);
if(ep != null)
 ep.setDateModified(new Date());
 expectedPackageRepository.saveAndFlush(ep);

2nd approach:

Optional<ExpectedPackage> ep = expectedPackageRepository.findById(1);
if(ep.isPresent())
 ep.get().setDateModified(new Date());
 expectedPackageRepository.saveAndFlush(ep.get());

Or is there a third and better approach that I have missed? I went through several questions and a couple of articles, but I did not find a clear answer.

You can also do:

expectedPackageRepository.findById(1).ifPresent(
 ep -> 
 ep.setDateModified(new Date());
 expectedPackageRepository.saveAndFlush(ep);
 
);

Ideally, you would also extract the part between brackets () to a separate method. Then, you could write like this:

 expectedPackageRepository.findById(1).ifPresent(this::doSomethingWithEp);

Where:

void doSomethingWithEp(ExpectedPackage ep) 
 ep.setDateModified(new Date());
 expectedPackageRepository.saveAndFlush(ep);

You can read the documentation of ifPresent here: https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#ifPresent-java.util.function.Consumer-

As it states, it will perform the specified action if the value is present and do nothing otherwise.

The other answer is basically some refactoring of your second approach, which has nothing wrong per-se, it's just a matter of style. Of course chaining and extraction to a separate method will make this a lot more readable and clear, no doubt (+1 from me), especially since the correct usage of ifPresent.

I'd just add here that get, well, was seen as somehow a design error ( or may be a bad method name, probably that came from guava mindset ). Using get even if it documented to throw an Exception when that value is missing is somehow weird ( if you think getters here, you would not expect a getter to throw an Exception). And you would not expect that get needs to be called after isPresent, at least not on the very first interactions with Optional. Thus get was proposed to be deprecated ( and hopefully removed ), thus java-10 adds a better addition orElseThrow() - this makes sense right after you read it, cause the throwing part is in the name of the method, so no surprises.

Also, someone should tell you about that usage of new Date() that when used with Optional from java-8 just looks weird, there are much better time/date related classes already.

I am also not very sure why you are updating a modified date manually, when there are spring annotations for that like PreUpdate/PrePersist.

Yes, there are other approaches.

If you absolutely expect there always to be a value, then use Optional::orElseThrow to throw an Exception if a null appears.

If you expect a null to possibly arrive, and have an alternative instance available as a fall-back option, use Optional::orElse.

If the fall-back instance is not on hand, but you have a function to call to provide a fall-back instance, use Optional::orElseGet.

If you don’t care about receiving a null, and want to do nothing when a null arrives, use Optional::ifPresent. Pass the block of code to be run if a value arrives.

If you only care if a value arrives that meets some requirement, use Optional::filter. Pass a Predicate defining your requirement. For example, we care only if an Optional< String > contains text and that text has the word purple in it: myOptional.filter( s -> s.contains( "purple" ) ).ifPresent( this::print ) ;. If null received, our desired operation (a call to print in this example) never happens. If a value was received but failed to meet our predicate, our desired operation never happens.

Doing if( myOptional.isPresent() ) SomeClass x = myOptional.get() ; … is valid, and safe. But this is not the original intent of Optional as it is basically the same as doing an old-fashioned null-check if ( null == x ) … . The other methods on Optional provide a more clear and elegant way to express your intentions towards a possible null arriving.

you can also do:

Optional<ExpectedPackage> updatedPackage = expectedPackageRepository.findById(1).map(ep -> 
 ep.setDateModified(new Date());
 return expectedPackageRepository.saveAndFlush(ep);
);