RFR: JDK-8264222: Use switch expression in jshell where possible [v2]

Stuart Marks smarks at openjdk.java.net
Sat Mar 27 05:13:26 UTC 2021 

On Sat, 27 Mar 2021 01:15:55 GMT, Brian Goetz <briangoetz at openjdk.org> wrote:

>> I have some formatting questions. I also have some weak opinions on formatting, but not strong enough to make recommendations yet. (I see you wrote "Better formatting ideas are welcome" so thanks for being open to this.) I'm interested in feedback on how the code ends up _reading_, particularly from the people who will be maintaining this code. But comments from others are welcome too.
>> 
>> The switch (c) in `ArgTokenizer::nextToken` is pretty nicely formatted, since each case has a single constant, they're all short, and the values are also all constants that are textually short. The only thing I'd say here is to add a bit of spacing to the default case so that the arrows line up.
>> 
>> In `Eval::prettyExpr` switch (typeName), the switch is a bit irregular since one of the case arms has multiple constants. This makes it a bit harder to find one if you're looking for it in particular. For example, suppose you want to find out how "int" is handled. In the old code you can scan down looking at each `case` and then look at the constant right next to it. In the new code (as modified) the "int" is farther away from the `case` keyword and is a bit lost among the values, so it's harder to see. Let me see how the variations look.
>> 
>> (1) Here's the modified version as proposed in the PR:
>> 
>>     String sinit = switch (typeName) {
>>         case "byte", "short", "int" -> "0";
>>         case "long" -> "0L";
>>         case "float" -> "0.0f";
>>         case "double" -> "0.0d";
>>         case "boolean" -> "false";
>>         case "char" -> "'\\u0000'";
>>         default -> "null";
>>     };
>> 
>> (2) Variation with the arrows on the same line, but lined up:
>> 
>>     String sinit = switch (typeName) {
>>         case "byte", "short", "int" -> "0";
>>         case "long"                 -> "0L";
>>         case "float"                -> "0.0f";
>>         case "double"               -> "0.0d";
>>         case "boolean"              -> "false";
>>         case "char"                 -> "'\\u0000'";
>>         default                     -> "null";
>>     };
>> 
>> (3) Variation with arrows on the next line:
>> 
>>     String sinit = switch (typeName) {
>>         case "byte", "short", "int"
>>             -> "0";
>>         case "long"
>>             -> "0L";
>>         case "float"
>>             -> "0.0f";
>>         case "double"
>>             -> "0.0d";
>>         case "boolean"
>>             -> "false";
>>         case "char"
>>             -> "'\\u0000'";
>>         default
>>             -> "null";
>>     };
>> 
>> (4) Variation with arrows on the same line, aligned, with case constants one per line:
>> 
>>     String sinit = switch (typeName) {
>>         case "byte",
>>              "short",
>>              "int"     -> "0";
>>         case "long"    -> "0L";
>>         case "float"   -> "0.0f";
>>         case "double"  -> "0.0d";
>>         case "boolean" -> "false";
>>         case "char"    -> "'\\u0000'";
>>         default        -> "null";
>>     };
>> 
>> Personally I like variations (2) and (4), since they not only line the arrows up, the column of arrows creates a visual divider between the case constants and the values. Variation (3) is quite visually noisy to my eye, though still probably an improvement over the original code (a switch statement, not expression). Are there other variations? What do people think?
>
> I'll answer from two perspectives: what I personally like, vs what I 
> would recommend in a style guide.
> 
>  From a personal perspective, I like #2 the best, and #4 after that.  It 
> is really easy to see what's going on.  But, cases where you can 
> actually line up like this are rare, since most switches involve more 
> than just simple constants on the RHS.
> 
> On the other hand, I would never ask anyone to code like that; it seems 
> too obsessive to ask people to do.  What I'd suggest from a style guide 
> perspective is #1.
> 
> For this particular example, #3 is the worst on both points.
> 
> But, this particular example is specific; both the predicates and 
> consequences are syntactically short.  If they were longer, #3 would be 
> more attractive.
> 
> So as a general recommendation #1 if it fits, #3 if #1 doens't fit, and 
> #2/#4 if the formatting works out and you're feeling generous.
> 
> 
> On 3/26/2021 8:40 PM, Stuart Marks wrote:
>>
>> I have some formatting questions. I also have some weak opinions on 
>> formatting, but not strong enough to make recommendations yet. (I see 
>> you wrote "Better formatting ideas are welcome" so thanks for being 
>> open to this.) I'm interested in feedback on how the code ends up 
>> /reading/, particularly from the people who will be maintaining this 
>> code. But comments from others are welcome too.
>>
>> The switch (c) in |ArgTokenizer::nextToken| is pretty nicely 
>> formatted, since each case has a single constant, they're all short, 
>> and the values are also all constants that are textually short. The 
>> only thing I'd say here is to add a bit of spacing to the default case 
>> so that the arrows line up.
>>
>> In |Eval::prettyExpr| switch (typeName), the switch is a bit irregular 
>> since one of the case arms has multiple constants. This makes it a bit 
>> harder to find one if you're looking for it in particular. For 
>> example, suppose you want to find out how "int" is handled. In the old 
>> code you can scan down looking at each |case| and then look at the 
>> constant right next to it. In the new code (as modified) the "int" is 
>> farther away from the |case| keyword and is a bit lost among the 
>> values, so it's harder to see. Let me see how the variations look.
>>
>> Here's the modified version as proposed in the PR:
>>
>> |String sinit = switch (typeName) { case "byte", "short", "int" -> 
>> "0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" -> 
>> "0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default 
>> -> "null"; }; |
>>
>> Variation with the arrows on the same line, but lined up:
>>
>> |String sinit = switch (typeName) { case "byte", "short", "int" -> 
>> "0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" -> 
>> "0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default 
>> -> "null"; }; |
>>
>> Variation with arrows on the next line:
>>
>> |String sinit = switch (typeName) { case "byte", "short", "int" -> 
>> "0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" -> 
>> "0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default 
>> -> "null"; }; |
>>
>> Variation with arrows on the same line, aligned, with case constants 
>> one per line:
>>
>> |String sinit = switch (typeName) { case "byte", "short", "int" -> 
>> "0"; case "long" -> "0L"; case "float" -> "0.0f"; case "double" -> 
>> "0.0d"; case "boolean" -> "false"; case "char" -> "'\\u0000'"; default 
>> -> "null"; }; |
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub 
>> <https://github.com/openjdk/jdk/pull/3210#issuecomment-808605528>, or 
>> unsubscribe 
>> <https://github.com/notifications/unsubscribe-auth/AABJ4RHKGGO7PSHNFZSZD6DTFUSOZANCNFSM4Z2NQ5BQ>.
>>

Right, with longer case constants or values, the formatting dynamics of the entire expression change. Here's another example from the changeset, from `Eval::kindOfTree`.

(5) Here's the version as proposed in the PR:

    OuterWrap outer = switch (probableKind) {
        case IMPORT -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);
        case EXPRESSION -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
        case VAR, TYPE_DECL, METHOD -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
        default -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
    };

(6) Line breaks before the arrow:

    OuterWrap outer = switch (probableKind) {
        case IMPORT
            -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);
        case EXPRESSION
            -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));
        case VAR, TYPE_DECL, METHOD 
            -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));
        default
            -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
    };

(7) An extra blank line between each case:

    OuterWrap outer = switch (probableKind) {
        case IMPORT
            -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);

        case EXPRESSION
            -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));

        case VAR, TYPE_DECL, METHOD 
            -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));

        default
            -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
    };

(8) As above, but with the case constants one to a line:

    OuterWrap outer = switch (probableKind) {
        case IMPORT
            -> state.outerMap.wrapImport(Wrap.simpleWrap(compileSource), snip);

        case EXPRESSION
            -> state.outerMap.wrapInTrialClass(Wrap.methodReturnWrap(compileSource));

        case VAR,
             TYPE_DECL,
             METHOD
            -> state.outerMap.wrapInTrialClass(Wrap.classMemberWrap(compileSource));

        default
            -> state.outerMap.wrapInTrialClass(Wrap.methodWrap(compileSource));
    };

-------------

PR: https://git.openjdk.java.net/jdk/pull/3210

More information about the kulla-dev mailing list