JShell vs Optional.get()

[Robert Field]

Thanks much, Stuart!

I've created:  https://bugs.openjdk.java.net/browse/JDK-8155581

to address this issue.

-Robert

On 04/27/16 18:37, Stuart Marks wrote:
> (new thread spawned from "Fwd: RFR(m): 8140281 deprecate Optional.get()")
>
>> Thumbs up to JShellTool code.
>
> Thanks. The original changeset seems to have been ... ah ... 
> sidetracked, so I probably won't be pushing the change from get() to 
> getWhenPresent() anytime soon.
>
> Meanwhile, I'll explain in a bit more detail about some issues I see 
> in the particular bit of JShell code that uses Optional.get(). I had 
> said:
>
>>> I think this code can be cleaned up and replaced with a call to
>>> String.join(). I'd be happy to help with this as part of a separate
>>> changeset.
>>
>> Always happy to get code improvements.
>
> Thanks for being receptive to comments.
>
> From the earlier webrev,
>
> http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/
>
> if you look in 
> src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java, 
> there is the following code:
>
> 1198             int length = 0;
> 1199             int first = replayableHistory.size();
> 1200             while(length < Preferences.MAX_VALUE_LENGTH && 
> --first >= 0) {
> 1201                 length += replayableHistory.get(first).length() + 
> sepLen;
> 1202             }
> 1203             String hist = replayableHistory
> 1204                     .subList(first + 1, replayableHistory.size())
> 1205                     .stream()
> 1206                     .reduce( (a, b) -> a + RECORD_SEPARATOR + b)
> 1207                     .get();
>
> The problem here is that get() will throw an exception if reduce() 
> returns an empty Optional, which it can, based on my reading of the 
> code. Changing this to the [proposed] getWhenPresent() makes this a 
> bit more explicit, but doesn't fix the problem.
>
> The usual fix is to use one of the Optional methods that handles the 
> empty case. Instead of get(), for example, one might call 
> orElse("default") to supply a default string value.
>
> But looking further, I note that the reduce() lambda is a (flawed) 
> idiom for joining strings with a separator. In Java 8 we introduced a 
> String.join() method which does this, avoiding the need for a stream 
> entirely. It also handles the empty case correctly.
>
>     String hist = String.join(RECORD_SEPARATOR,
>         replayableHistory.subList(first + 1, replayableHistory.size()));
>
> s'marks
>