JShell vs Optional.get()

[Stuart Marks]

(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