0
\$\begingroup\$

I would like to ask the opinion for experts out there. The following code snippet was written by a colleague of mine who is very fond of functional style in Java.

public Book findBookByIsbnOrId(String isbn, String id) {
   return Optional.ofNullable(isbn).map(i -> {
      return bookDao.findByIsbn(i);
   })
   .orElseGet(() -> {
      return bookDao.findById(id);
   });
}

This code has been modified to be used at Stack Exchange. Essentially, finding the book has preference (ISBN) which when not present should be search via (ID).

I would argue that this code although functionally fulfils the given requirement is less readable than

if (StringUtils.isNotBlank(isbn)) {
   return bookDao.findByIsbn(isbn);
} else {
   return bookDao.findById(id);
}

I would like to ask the opinion of others whether they think this is just over-engineering?

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Welcome to Code Review! Can you please confirm that this is code you own or maintain? For licensing, moral, and procedural reasons, we cannot review code written by other programmers (unless its maintenance has been transferred to you). \$\endgroup\$ Nov 22, 2022 at 17:30
  • 2
    \$\begingroup\$ Yes, I do maintain this code. And the code has been modified from the original example to fictitous scenario \$\endgroup\$ Nov 22, 2022 at 17:34
  • \$\begingroup\$ I would also like to know why did the question got downvoted when all I ask is for a review (at review stackexchange). I want to know the pros/cons of specific use of functional style of coding. Its not meant to be used for bashing. The whole point is, perhaps I am missing something and would love to learn it otherwise. \$\endgroup\$ Nov 22, 2022 at 18:02
  • 3
    \$\begingroup\$ Couple of reasons come to mind: 1) You didn't write the code. 2) The code has been modified before posting, we got bad experiences with this (answers not being applicable). 3) The title is very generic. 4) It's a snippet without much context. While you may think that this can still be reviewed and answers have indeed arrived, it's not a great fit within the scope of the site (as per the help center). Downvotes indicate perceived low-value and I think those points could've played a part in it. \$\endgroup\$
    – Mast
    Nov 24, 2022 at 16:26

2 Answers 2

4
\$\begingroup\$

The two versions with equivalent functionality and cleaned would be:

public Book findBookByIsbnOrId(String isbn, String id) {
   return Optional.ofNullable(isbn)
       .map(bookDao::findByIsbn)
       .orElseGet(() -> bookDao.findById(id));
}

public Book findBookByIsbnOrId(String isbn, String id) {
   return isbn != null
       ? bookDao.findByIsbn(isbn)
       : bookDao.findById(id));
}

Though using Stream is often better and allows more powerful expressions, here the nullable ISBN is dealt with an Optional and that is circumstantial here.

So your second version is much neater. However the first version might be a reaction to a nullable parameter - which is bad style. So he turned it into an Optional<String> on principle. The API should have been different, the most similar - though probably still not good style - would have been:

public Book findBookByIsbnOrId(Optional<String> isbn, String id) {
   return isbn
       .map(bookDao::findByIsbn)
       .orElseGet(() -> bookDao.findById(id));
}

Conclusion: your version suffices, but nice would be

public Book findBookByIsbnOrId(@Nullable String isbn, String id) {
\$\endgroup\$
1
  • \$\begingroup\$ Although answer from @K.H. is also as helpful, I chose this submission as it gives an insight into the developers intent. Thanks :) \$\endgroup\$ Nov 23, 2022 at 7:51
1
\$\begingroup\$

I agree, that the second one is more readable. It is obviously just less amount of lines and characters.

Things can change if it was more complicated example.

Also the first one is a lot nicer in other languages, where return and named parameter is not required in lambdas and optionals are first class citizen. Then you can do the same with a lot less. Java just isn't very good at saving characters :)

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for your review. Do you happen to point me to some examples (don't have to be Java specific) which are complicated enough to use this style of code. Also what do you mean by Optionals being not first class citizens in Java? \$\endgroup\$ Nov 22, 2022 at 19:04
  • \$\begingroup\$ I don't do much Java nowadays so don't have any live sample. I mean optional support built in the language. Then you have String? instead of Optional<String>, elvis operator and other things that make those operations easier to work with. \$\endgroup\$
    – K.H.
    Nov 22, 2022 at 19:57
  • \$\begingroup\$ Languages like Typescript, Swift and Kotlin are examples, where optionals and functional programming works well. \$\endgroup\$
    – K.H.
    Nov 22, 2022 at 20:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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