Having nicer body for ifPresentOrElse

[Björn Björnsen]

Hi everyone,
I am going to refactor the code below, to use .ifpresentOrElse()
I am wondering what is the best way to do that.
Here is the code (Naming is not important )

private MyArtifact method() {
    Optional<MyDataType> data = db.getDatafor(XXX)
          MyDetail detail = createDetail(data.get().getDetail());
    return MyArtifact.newBuilder()
            .withDetail(detail)
            .build();
  }

Here is the solution 1:

private MyArtifact method() {
    Optional<MyDataType> data = db.getDatafor(XXX)
final MyDetail[] detail = new MyDetail[1];
 data.ifPresentOrElse( 
                   info -> detail[0] = createDetail(info.getDetail()) , 
                    () -> {THROW AN_EXCEPTION(""); } // handle it as it should.  ); 
     return MyArtifact.newBuilder()
            .withDetail(detail[0]) 
            .build(); }

solution 2 is using AtomicRefrence:

private MyArtifact method() {
    Optional<MyDataType> data = db.getDatafor(XXX)
          AtomicRefrence<MyDetail> detail =null:
        data.ifPresentOrElse( info -> detail = createDetail(info.getDetail()) , 
                () -> {THROW AN_EXCEPTION(""); } // handle it as it should.  ); 
              return MyArtifact.newBuilder()
                   .withDetail(detail) 
                  .build(); 

 }

I am wondering if there is any solution, nicer that AtomicRefrence of having an array-size 1 ?
p.s. not sure why, but even having MyDetail final wont help, compiler error

Thanks.

[Stephan van Hulst]

return db.getDatafor(XXX)
  .map(MyDataType::getDetail)
  .map(this::createDetail)
  .map(MyArtifact.newBuilder::withDetail)
  .map(MyArtifact.Builder::build)
  .orElseThrow(SomeException::new);

[Stephan van Hulst]

I prefer this one though:

var data   = db.getDatafor(XXX).orElseThrow(SomeException::new);
var detail = createDetail(data.getDetail());

return MyArtifact.newBuilder()
  .withDetail(detail)
  .build();

I'm not sure why you think using ifPresentOrElse() is an improvement.

[Björn Björnsen]

Stephan van Hulst wrote:

return db.getDatafor(XXX)
  .map(MyDataType::getDetail)
  .map(this::createDetail)
  .map(MyArtifact.newBuilder::withDetail)
  .map(MyArtifact.Builder::build)
  .orElseThrow(SomeException::new);

Wow,
Nice one,
Completely forgor about method chainig of optional values :-D

[Björn Björnsen]

Stephan van Hulst wrote:I prefer this one though:

var data   = db.getDatafor(XXX).orElseThrow(SomeException::new);
var detail = createDetail(data.getDetail());

return MyArtifact.newBuilder()
  .withDetail(detail)
  .build();

I'm not sure why you think using ifPresentOrElse() is an improvement.

To make sure not getting NPE on data.get.
Also keep sonar happy 😊

But thanks, the second solution also looks nice.

[Stephan van Hulst]

Sonar is a tool that supports the developer. It's not your job to make Sonar happy, it's Sonar's job to make you happy.

[Carey Brown]

Stephan, trying to follow your example. Does the "orElseThrow" pair up with the first ".map", the last, or any?

[Mike Simmons]

In Stephan's original code, the "orElseThrow()" really applies to all the map() calls, as well as the original db.getDatafor(XXX).  That is, if the original value was null, or if  any of the mapped functions returns null, then the Optional becomes an empty Optional, and all subsequent map() calls have no further effect, and the orElseThrow() will cause an exception to be thrown.  So an exception can be thrown due to a null from any of the mapped funtions.

In Stephan's later preferred code, the "orElseThrow()" applies only to the original db.getDataFor(XXX) call.  I assume he prefers that because it's clearer - that's the place we expect that an empty Optional might occur, and his code handles that clearly... and after that, there's really no need for Optional and map() any more.  So the second version is better at communicating intent.  

The only possible downside is, this assumes that none of the later methods used (like createDetail()) will return a null.  If they did, well, the second version would throw a NullPointerException, while the first version with multiple map() calls would work just fine. That might be a good idea, if you can't trust those later methods not to return null.  But it they are trustworthy, then the multiple map() calls are an unnecessary obfuscation.

[Campbell Ritchie]

Please remind us all why orElseThrow(XYZException::new) is better than orElseThrow(new XYZException()). I remember reading about that in Ken Kousen's book, and I think I know the answer.

[Stephan van Hulst]

Simply put, why create an object until you're definitely going to need it?

[Stephan van Hulst]

Another good reason might be that Optional doesn't contain a method orElseThrow(Throwable).

[Campbell Ritchie]

Stephan van Hulst wrote:. . . why create an object until you're definitely going to need it?

That is what I remember from previous reading.

[Rob Spoor]

Campbell Ritchie wrote:Please remind us all why orElseThrow(XYZException::new) is better than orElseThrow(new XYZException()). I remember reading about that in Ken Kousen's book, and I think I know the answer.

orElseThrow comes in two overloads: one without arguments that throws a NoSuchElementException, and one with a supplier. In other words, orElseThrow(new XYZException()) doesn't even compile.

[Campbell Ritchie]

Stephan did mention that, Rob, but I found your explanation easier to understand.

[Mike Simmons]

Campbell Ritchie wrote:Please remind us all why orElseThrow(XYZException::new) is better than orElseThrow(new XYZException()). I remember reading about that in Ken Kousen's book, and I think I know the answer.

I think Ken's book probably addresses a different question, since the second code isn't even an option in the real world.  I can think of two variants that might have been worth discussing:

1. Why isn't orElseThrow(Throwable) provided as an option in the API?

2. Is there a reason to prefer orElseThrow(XYZException::new) instead of orElseThrow(() -> new XYZException())?

The first one was addressed by Stephan: it would require us to create the exception (and populate its stack trace, a somewhat time-consuming operation) whether or not anything actually went wrong.  That could potentially degrade performance for no good reason.  We don't want to create an exception unless we actually need it.

The second one is probably what Ken was talking about in his book.  In situations where we can use either a lambda expression or a method handle, is there any reason to prefer one or the other?  A lambda expression is more flexible, of course - you have more freedom to write arbitrary code into the expression.  Within some limits, of course.  I think the main benefit of using a method is performance... though I'm not sure how big a difference it really makes.  When you use a lambda expression, the compiler generates methods and/or nested class to contain the implementation.  Whereas if you use a method reference, it has the ability to calls the already-existing method more directly.  I'm not sure if there are any other benefits, though.