Functional Style if/else in java

Question

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?

Answer 1

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) {

Answer 2

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 :)