REPL tool implementation code review

[Robert Field]

On 09/09/15 01:16, Paul Sandoz wrote:
> Hi Robert,
>
> The changesets look good.

Cool!

>
>
> On 9 Sep 2015, at 03:32, Robert Field <robert.field at oracle.com> wrote:
>>> jdk/internal/jshell/tool/JShellTool.java:981
>>>> private void cmdEdit(String arg) {
>>>>      Set<Snippet> keySet = argToKeySet(arg);
>>>>      if (keySet == null) {
>>>>          return;
>>>>      }
>>>>      Set<String> srcSet = new LinkedHashSet<>();
>>>>      for (Snippet key : keySet) {
>>>>          String src = key.source();
>>>>          switch (key.subKind()) {
>>>>              case VAR_VALUE_SUBKIND:
>>>>                  break;
>>>>              case ASSIGNMENT_SUBKIND:
>>>>              case OTHER_EXPRESSION_SUBKIND:
>>>>              case TEMP_VAR_EXPRESSION_SUBKIND:
>>>>                  if (!src.endsWith(";")) {
>>>>                      src = src + ";";
>>>>                  }
>>>>                  srcSet.add(src);
>>>>                  break;
>>>>              default:
>>>>                  srcSet.add(src);
>>>>                  break;
>>>>          }
>>>>      }
>>>>      StringBuilder sb = new StringBuilder();
>>>>      for (String s : srcSet) {
>>>>          sb.append(s);
>>>>          sb.append('\n');
>>>>      }
>>>>      String src = sb.toString();
>>> There appears to be a reliance on an undefined iteration order, perhaps argToKeySet should return LinkedHashSet?
>> It does use LinkedHashSet.
>>
> But the argToKeySet() method returns a Set, which may or may not have a defined encounter order, it’s not clear and would be more so if argToKeySet returned LinkedHashSet.

So, you are saying, for documentation purposes, I should declare it (now 
called argToSnippetSet) as returning LinkedHashSet, and the sets within 
the method (keySet now snippetSet and srcSet) as LinkedHashSet too? I 
thought the standard was to declare the interface and that one shouldn't 
declare them as their implementation type.

>
>
>>>
>>> jdk/internal/jshell/tool/ExternalEditor.java:134
>>>> private void saveFile() {
>>>>      List<String> lines;
>>>>      try {
>>>>          lines = Files.readAllLines(tmpfile);
>>>>      } catch (IOException ex) {
>>>>          errorHandler.accept("Failure read edit file: " + ex.getMessage());
>>>>          return ;
>>>>      }
>>>>      StringBuilder sb = new StringBuilder();
>>>>      for (String ln : lines) {
>>>>          sb.append(ln);
>>>>          sb.append('\n');
>>>>      }
>>>>      saveHandler.accept(sb.toString());
>>>> }
>>> Why don’t you just read as bytes then create a String from those bytes?
>> I want the normalization and termination.  Used Remi's suggestion, making it:
>>
>>     private void saveFile() {
>>         try {
>> saveHandler.accept(Files.lines(tmpfile).collect(Collectors.joining("\n", "", "\n")));
>>         } catch (IOException ex) {
>>             errorHandler.accept("Failure in read edit file: " + ex.getMessage());
>>         }
>>     }
>>
> Ok. I cannot recall if there was or you added a comment regarding normalization and termination, if not it might be useful to do so, in case someone comes along later on and inadvertently changes it.

OK, will do.

Thanks,
Robert

>
> Paul.