7

I am trying to replace below code block with optional, just want to check will there be any bad implications of doing this like performance or anything as such?

Existing code:

UserObj userObj=new UserObj();
Object result = fetchDetails();
if (null != result) {
    userObj.setResult(result.toString());
}

With Java 8 Optional:

UserObj userObj=new UserObj();
Optional.ofNullable(fetchDetails()).ifPresent(var -> userObj.setResult(var.toString()));

Doing this change just to make code look concise as there are lot of null check blocks in my code.

ETO
  • 6,970
  • 1
  • 20
  • 37
springenthusiast
  • 403
  • 1
  • 8
  • 30
  • 6
    The correct way would be if `fetchDetails` would already return an `Optional`. This class is meant to be returned by methods that want to indicate *no result*. Otherwise the additional safety-mechanism doesn't catch (to prevent people from forgetting the null-check, they can not forget if they only get an `Optional` back. Because they can not access the content without triggering the check). – Zabuzard Jan 29 '19 at 08:25
  • 2
    I think this question should be asked in `Code Review` community. – wake-0 Jan 29 '19 at 08:27
  • 2
    Your old code looks dubious: `userObj.setResult(userObj.toString())`. Apparently, you actually wanted `userObj.setResult(result.toString())` – Holger Jan 29 '19 at 08:49
  • 1
    There is a slight overhead of using `Optional`, you and your users will probably never notice. Your code using `Optional` looks good. I agree with those saying that it will be even better if `fetchDetails` would return an `Optional` from the start. – Ole V.V. Jan 29 '19 at 09:08

2 Answers2

9

First of all I think you're misunderstanding the purpose of Optional. It is not just for replacing 

if(obj != null){ ... }

The main point of Optional is to provide a means for a function returning a value to indicate the absence of a return value. Please read this post for more details.

The proper use of Optional in your case would be returning optional ResultObj from fetchDetails method:

Optional<ResultObj> fetchDetails() {
   ...
}

Then you simply chain the methods on fetched Optional as you did before.

Update

In case you cannot modify fetchDetails there is still an option of wrapping it into your own method like the following:

Optional<ResultObj> fetchOptionalDetails() {
    return Optional.ofNullable(fetchDefails());
}

Creating a new method will add a tiny overhead, but the code will be much more readable:

fetchOptionalDetails().ifPresent(details -> /* do something */);
ETO
  • 6,970
  • 1
  • 20
  • 37
  • 5
    This is the correct answer. Replacing a lot of code, just to replace if-statements is not helpful. Optionals are there to signify potentially nullary return values. – TreffnonX Jan 29 '19 at 09:19
  • I was aware that Optional is designed to be used mainly as method return type but in my case fetchDetails method is provided by 3rd party jar which I don't have control, so wanted to check if writing the above way of coding is correct instead of if and null check – springenthusiast Jan 29 '19 at 10:04
  • 1
    @springenthusiast In this case your code is fine. Although I would slightly change it. Please check my update. – ETO Jan 29 '19 at 10:15
0

Returning Optional from fetchDetails() makes more sense here. then you can directly check ifPresent and then proceed

Buddy
  • 10,874
  • 5
  • 41
  • 58
swati verma
  • 1