3
\$\begingroup\$

As Optional have only ifPresent and not have another function to be run in false case so I did the below class for that, please check it and give me your feedback:

public class OptionalConsumer<T> {
private Optional<T> optional;

private OptionalConsumer(Optional<T> optional) {
    this.optional = optional;
}

public static <T> OptionalConsumer<T> of(Optional<T> optional) {
    return new OptionalConsumer<>(optional);
}

public OptionalConsumer<T> ifPresent(Consumer<T> c) {
    optional.ifPresent(c);
    return this;
}

public OptionalConsumer<T> ifNotPresent(Runnable r) {
    if (!optional.isPresent())
        r.run();
    return this;
}

} Then :

Optional<Any> o = Optional.of(...);
OptionalConsumer.of(o).ifPresent(s ->System.out.println("isPresent "+s))
        .ifNotPresent(() -> System.out.println("! isPresent"));
\$\endgroup\$
2
2
\$\begingroup\$

Two things:

  1. Your class only provides a if-else statement, really. If it would also provide all the features in the existing java.util.Optional, it would have been slightly more useful.

  2. private Optional<T> optional; should definitely be marked with final

Okay then, three things:

  1. Your factory method takes an existing Optional<T>, but you don't provide a getter or anything for the Optional<T>, which means that you could also provide an ordinary OptionalConsumer<T> ofNullable(T value)
\$\endgroup\$
5
  • \$\begingroup\$ you are right in 2 points Optional have to be final and have to provide getter for it, but as you can get from the class name it is Consumer mainly so I do not need to provide Optional features, I wanted to execute consumer ifPresent and Runnable ifNotPresent, this is the target from OptionalConsumer, otherwise I was going to extends Optional. \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 11:31
  • \$\begingroup\$ @BassemRedaZohdy You can't extend Optional, as Optional has private constructors and the class is marked final. What I'm saying is that it seems like you want to extend the class, but what you are actually doing is reducing it, as you provide just one functionality that does not exist in the original, while removing the other functionalities. And the functionality that you do provide is just an if-else statement, nothing else. \$\endgroup\$ – Simon Forsberg Sep 23 '15 at 11:48
  • \$\begingroup\$ As I told before I'm not willing to extends the Optional, but only to execute Consumer in case ifPresent and Runnable in case of ifNotPresent, while I'm feeling that original Optional is missing ifNotPresent, if you are ok with if-else there is no problem, I'm not providing this to replace if-else at all. \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 11:58
  • \$\begingroup\$ @BassemRedaZohdy Then why exactly are you providing this? What is it that you want this code to accomplish for you, if not an if-else? \$\endgroup\$ – Simon Forsberg Sep 23 '15 at 13:19
  • \$\begingroup\$ Check original question on stackoverflow and my implementation about was the answer for it stackoverflow.com/questions/23773024/… \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 13:24
1
\$\begingroup\$

I think in general the functionality of ifNotPresent(…) is there as Optional.orElseGet(…). You can use this method of Optional to run something if it is empty.

The functionality you really added is the possibility to chain both methods to get something like an if (…) then (…) else (…) statement. In my opinion this is a bit against the mind behind the Optional pattern. It is not just a new way to write if statements. To write this you can still use:

if (myOptional.isPresent()) {
    // do something
} else {
    // do something else
}

You should design your code in a way that basically it always acts the same independent of the presence of a value. Sure it may only processes the content of the Optional where there is some (using .ifPresent(…)), retrieve the value if you don't have it yet but it can be calculated (using .orElseGet(…)), have a default (using .orElse(…)), or indicate an error because the value had to be there (using .orElseThrow(…)).

But you should not (have to) switch logic depending on the presence of the Optional. There are other design patterns to do this.

So in general my advice would be to rework your if/then/else to get the thing that is special in an .orElseGet(…) in a way that it prepares the value so that it can be processed together with the present-case using the returned value.

\$\endgroup\$
5
  • \$\begingroup\$ The target of this class not to deal with value in Optional, it is to consume or execute code in case of ifNotPresent, so I do not want to provide alternative data in case data is not present I need to execute Runnable in case of data is not present. \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 11:34
  • 1
    \$\begingroup\$ Why do you want to do it with a Lambda? The code will run immediatelly anyway so you can just place it directly in the else case of an if statement. In general one of your most important goals should be to write readable code. Someone will have to read the code later (it might be you) and you want to make his job as easy as possible. With the classical if statement (as shown in my original answer) I'd say this goal is reached much better as with all the boilerplate you need in OptionalConsumer with the object construction and lambda expressions. \$\endgroup\$ – Matthias Wimmer Sep 23 '15 at 11:45
  • \$\begingroup\$ Because I was looking for something equivalent to ifPresent provided with Optional itself, this is main target, I'm not doing that class to deal with all if-else case, I'm doing it to be used when you need to use ifPresent do this ifNotPresent do that, otherwise if you want do you if-else it is OK, I think it is better to check the initial question derived me to do this stackoverflow.com/questions/23773024/… \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 11:55
  • 2
    \$\begingroup\$ Thanks for the pointer. I note that you already got comments there, that you should just use the if-then-else statement. Go a step back and re-thing your problem. What you try to make functional style will never be functional. Your Runnable will never return anything, and if it would be fully functional it would also have no side effects. If something has no side effects and does not return anything it would be just useless. Therefore what you try to do is be definition imperative programming style and there is no reason not to use imperative programming constructs for it. \$\endgroup\$ – Matthias Wimmer Sep 23 '15 at 12:09
  • \$\begingroup\$ Thanks for your comments, as I told you I wanted to use something equivalent to ifPresent, provided by original Optional which is also no side effects and does not return anything it is only consuming. \$\endgroup\$ – Bassem Reda Zohdy Sep 23 '15 at 12:19
-1
\$\begingroup\$

ifNotPresent method seems to be a bit outside the principle of its fellow opposite ifPresent.

Why does it take a Runnable as argument? I'd expect a Consumer<T> here also. Although Runnable is now a functional interface, it is more often associated with multithreading than with functional style. And if the intention was to use its threading abilities, the run() call in the method body is incorrect. new Thread(r).start(); should have been used.

Another remark, much less important. Since this wrapper class is very short and has only one field, there is no much sence in hiding the one-arg constructor allowing only the builder of() method publicly available. Both may be left public.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I think it doesn't take a Consumer because there's nothing to consume, hence not present. \$\endgroup\$ – h.j.k. Sep 23 '15 at 6:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.