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

[Brian Goetz]

On Sat, 27 Mar 2021 00:39:59 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> Marked as reviewed by briangoetz (Reviewer).
>
> 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>.
>

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

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