-2

I have the following methods:

Optional<Book> findBook(UUID bookId);

void doSomething(Book book, SomeOtherSutff someOtherStuff);

Since it's not recommended to have methods that have an Optional as a parameter I'm forced to do this:

doSomething(optionalBook.orElse(null), someOtherStuff)

The method parseBook contains a null check.

This seems wrong but I'm not sure what else can I do.

Example updated.

Thanks

kylie.zoltan
  • 377
  • 3
  • 15
  • why is it not recommended to have methods with `Optional` parameters? – thmasker Jan 30 '23 at 11:02
  • Yes, it's wrong. Use [`Optional.map`](https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#map-java.util.function.Function-) instead. – Konrad Rudolph Jan 30 '23 at 11:02
  • What does `parseBook` return if you pass it `null`? – tgdavies Jan 30 '23 at 11:04
  • 2
    @thmasker https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments – kylie.zoltan Jan 30 '23 at 11:05
  • Do you need to call `parseBook` even if your optional is empty? – khelwood Jan 30 '23 at 11:06
  • 1
    By the way, just because its convention to not use `Optional` for optional parameters, does not necessarily mean that you absolutely have to bend your head around it. There can be valid applications to go against general convention. (This here is not one of them, but still.) – Zabuzard Jan 30 '23 at 11:09
  • 1
    @kylie.zoltan why do you need to call `parseBook` even if you don't have any `book`? That doesn't make sense and, if actually needed, it may be a better part of the code to make improvements. If there is a "code smell" it's better to fix the issue and not just add a puff of "air freshener" – Andrei Jan 30 '23 at 11:22
  • @Andrei It's just an example. – kylie.zoltan Jan 30 '23 at 11:24
  • 3
    @kylie.zoltan if the example doesn't make sense but you actual use case does then you need to provide a better example. Or describe your situation better, without examples. Otherwise you are limiting the types of answers you will receive. – Andrei Jan 30 '23 at 11:31
  • 1
    There are different opinions about where to use `Optional`. I think Nikolai Parlog [makes a good argument](https://nipafx.dev/stephen-colebourne-java-optional-strict-approach/) why it is a good argument to use `Optional` everywhere, also in parameters. Your example also demonstrates this. I really think you should have `Optional` as the parameters type. – Lii Jan 30 '23 at 11:52

2 Answers2

1

One of the reasons for not using an Optional<T> as an input parameter is that you will likely have to construct it on-the-fly just to pass it to that method.

Even if now you have an Optional<Book> ready to use, from findBook, it may be that future changes to the code will mean the Optional is unwrapped and you end up with a naked Book you'll have to wrap. That would be annoying, at least.

The solution with unwrapping the Optional when calling the doSomething method is, I think, the best quick fix available.

Ideally, you want to avoid situations like this from occuring, as they cause confusion and could lead to bugs.

Depending on the semantics, the complexity of your doSomething and the importance of the Book parameter you could:

  1. Create a doSomething(Book book, SomeOtherSutff someOtherStuff) and a doSomething(SomeOtherSutff someOtherStuff). This is useful if doSomething can be called standalone, without a Book, from other parts of the code.

  2. Split doSomething in three: before doing Book things, doing Book things and "the rest of the processing". This can have various benefits, especially when processing many items. It may be that instead of doing (A, B, C), (A, B, C), ..., (A, B, C) that it's better to do (A, A, ..., A), (B, B, ..., B), (C, C, ...C) or (A, A, ..., A), (B, C), (B, C), ..., (B, C)

  3. Do nothing. It doesn't seem like more than a minor annoyance and making changes will cost time and risk introducing bugs.

Andrei
  • 4,880
  • 23
  • 30
  • 1
    I wouldn't add a check to see if the `Optional` parameter is not `null`. If it is `null` then it is an unrecoverable error. At most I can throw an exception other than a `NPE`, but is that really worth it? Instead, the parameter can be marked as `@NotNull Optional` and the tooling can warn the developers if they mistakenly pass `null` – Andrei Jan 30 '23 at 12:18
0

With following code you'll only call parseBook when Optional<Book> isn't empty

BookInfo info = optionalBook
   .map(book -> parseBook(book))
   .orElse(null);
Nikolai Shevchenko
  • 7,083
  • 8
  • 33
  • 42