1

I was trying the following code:

Optional.ofNullable(listEvidence).ifPresent(
            eviList -> {
                List<String> repoKeysList = new ArrayList<String>();
                for (Evidence evidence : eviList) {
                    repoKeysList.add(evidence.getRepositoryKey());
                }   
                log.debug("UserContentBean: processRepoSyncCleanup repoKeysList - "+repoKeysList);
                evidenceDetailDAO.updateIsSelectedFlag(repoKeysList, 0,configurationService.getNodeName());
            }
            ).orElseThrow();

but I am getting the following error:

Cannot invoke orElseThrow() on the primitive type void

It seems to me that orElseThrow() is misplaced in the code. but updateIsSelectedFlag() throws an exception which needs to be handled.

Also, this piece of code is a part of, say, method1(). I tried adding throws Exception to method1(), but it yet asks me to handle the exception raised by updateIsSelectedFlag(). I couldn't understand the reason for the same.

Makoto
  • 104,088
  • 27
  • 192
  • 230
NehalM
  • 43
  • 1
  • 8
  • 2
    This code has a smell about it. You're using `Optional.ifPresent`, but you're *building the `Optional` in situ*. It seems to me like you should just be using normal `if` statements here. – Makoto May 12 '17 at 15:03
  • 3
    Yurk... Instead of doing that, simply do `Objects.requireNonNull(listEvidence, "listEvidence");` and use directly the `listEvidence` without `Optional`... – NoDataFound May 12 '17 at 15:05

5 Answers5

4

This seems like a poor approach to the situation. In general, speaking on the use of Optional, you're expecting that usually as a return value, not a value that you do operations on. In effect, you're still doing a null check; it's just unnecessarily obfuscated.

Instead, perform the operations you intend to do with the if statement.

if (null != listEvidence) {
    List<String> repoKeysList = new ArrayList<String>();
    for (Evidence evidence : listEvidence) {
        repoKeysList.add(evidence.getRepositoryKey());
    }
    log.debug("UserContentBean: processRepoSyncCleanup repoKeysList - " + repoKeysList);
    evidenceDetailDAO.updateIsSelectedFlag(repoKeysList, 0, configurationService.getNodeName());
}

For more context into the conversation around Optional, this Stack Overflow question goes into some detail as to what use cases Optional is meant for.

Community
  • 1
  • 1
Makoto
  • 104,088
  • 27
  • 192
  • 230
  • @JeremyGrand: Looks to me like the original code is content with throwing the exception. I see no reason not to replicate that here. – Makoto May 12 '17 at 15:14
  • oh yeah, I just went crazy for a bit, their issue was that the Consumer did not allow for a checked exception to be thrown. The if statement fixes that. – Jeremy Grand May 12 '17 at 15:16
3

Optional.ifPresent returns a void type, and not the Optional itself.

What you're trying to do should be done with a if {listEvidence != null} else {} block.

Jeremy Grand
  • 2,300
  • 12
  • 18
0

You are misusing the Optional class.

First problem is that inside ifPresent() you are using a lambda expression that contains code that throws a checked exception. Lambdas cannot throw checked excpetions (i.e those that do not extend RuntimeException).

Second problem is that you are misusing the orElseThrow() method of Optional. It is supposed to be used to either get a maybe nullable object or throw an exception if it is actually null like so:

MyObject notNull = Optional.ofNullable(maybeNull).orElseThrow(new MyException());

For your use case you can either go with the old school if-else or if you want to play with Optional for some reason the way to go about it would be:

Optional.ofNullable(listEvidence).ifPresent((eviList -> {
  // your logic
  try {
    evidenceDetailDAO.updateIsSelectedFlag(repoKeysList, 0,configurationService.getNodeName());
  } catch (Exception e) {
    throw new RuntimeException(e);
  }
});

This way you convert the checked exception into an unchecked one inside the lambda and you can handle it outside the Optional by adding another try-catch for RuntimeException.

PentaKon
  • 4,139
  • 5
  • 43
  • 80
0

As pointed out by Jeremy, Optional.ifPresent returns void. In addition, you miss an argument to public <X extends Throwable> T orElseThrow(Supplier<? extends X> exceptionSupplier) throws X

Note that orElseThrow will never handle the exception thrown by updateIsSelectedFlag(). The purpose of orElseThrow is to "get the value inside the optional if it present and if its is not present throw an exception".

updateIsSelectedFlag() must throw unchecked exceptions if you want to use it in the Consumer<T> that is the argument of ifPresent. Have a look at the Consumer interface, it does not support checked exceptions. If updateIsSelectedFlag() cannot throw unchecked exceptions, you have to catch the exceptions and throw a new unchecked exception (such as a RuntimeException) in the Consumer.

You could achieve something similar to what you wrote by using Optional.map instead of Optional.ifPResent and by fixing the rest of the code (managing your exception, adding the argument to orElseThrow). But as suggested by many, you should better rewrite the code. For now, you are not writing what you intend to do.

Tip : since you have the good idea of using Optional, avoid to test for a null value. Prefer
listEvidence = Optional.ofNullable(something);
if (listEvidence.isPresent())
to
if (null != listEvidence)
If you avoid null values, your code will never throw NullPointerException.

Community
  • 1
  • 1
Emmanuel Guiton
  • 1,315
  • 13
  • 24
0

Like everybody else has said, in your case it's better just to use if (listEvidence != null).

However, if you do need to use an optional like this in another case, you can store it in a variable instead of chaining everything together.

Say listEvidence is of type MyClass.

Optional<MyClass> myOptional = Optional.ofNullable(listEvidence);

myOptional.ifPresent(
        eviList -> {
            List<String> repoKeysList = new ArrayList<String>();
            for (Evidence evidence : eviList) {
                repoKeysList.add(evidence.getRepositoryKey());
            }   
            log.debug("UserContentBean: processRepoSyncCleanup repoKeysList - "+repoKeysList);
            evidenceDetailDAO.updateIsSelectedFlag(repoKeysList, 0,configurationService.getNodeName());
        });

myOptional.orElseThrow(() -> new RuntimeException());
swanhella
  • 441
  • 6
  • 14