RFR: JDK-8264222: Use switch expression in jshell where possible [v2]
Brian Goetz
briangoetz at openjdk.java.net
Sat Mar 27 15:11:26 UTC 2021
On Sat, 27 Mar 2021 05:10:59 GMT, Stuart Marks <smarks at openjdk.org> wrote:
>> 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));
> };
>
> To my eye, (5) is too dense. The lines are too long. This is indented two levels (one for method in class, the second for code in method) and the longest line is 116 characters long. (Personally I tend to try to keep lines close to 100 or so at most.) The density of the lines makes it difficult to pick out the case constants from the values, as the arrows are buried in the middle. It's impossible to align the arrows, since that would make the lines even longer.
>
> (6) puts line breaks before each arrow, which is kind of like (3) from the previous round of examples. This is an improvement, I think, and it's tolerable, whereas in the previous round (3) was quite bad. It's still quite dense, however.
>
> For (7) I simply added line breaks between each case. I think this actually improves things a lot. It still takes up less vertical space than the original switch statement, but the additional white space makes it easier to read, and to pick out the case constants from values, I think. I note that in a switch statement, since `break;` often appears on its own line, it does have the useful effect of separating the case arms with an almost-blank line. Since we don't need `break` here, we have to add a blank line explicitly.
>
> (8) is a variation of (7) with the case constants on individual lines. The advantage here is that all the constants line up with each other, but frankly I don't think this buys much. It also creates a weird disjointed indent, where the second and subsequent case constants are indented by five because of C-A-S-E-space, but the arrow is at the standard indent of four. You could line up the arrows at an indent level of five, but bleargh, this doesn't really help anything.
>
> =====
>
> From my first round of examples, I said I liked (2) and (4), but revisiting them, I've decided I like (4) less. I suppose I could tolerate (1) but the constants and values being all smashed together makes it difficult to separate them. I'd recommend (2) more strongly because having the arrows all line up provides a clear visual separation between the constants and the values. Of course, the recommendation for (2) stands only if the constants and values can all fit on a single line.
>
> For the second round of examples, I think adding the blank lines makes a big difference. It means that the switch expression takes up almost as much vertical space as the switch statement. It can save a bit because with the statement, the multiple case labels were each on their own line, whereas for the expression I'm ok with putting the constants on the same line. Even though almost as much vertical space is consumed, I think the readability is quite good, and it still gains the benefits of switch expressions like exhaustiveness and lack of fall-thru.
(5) is fine if it fits within reasonable line limits, and is what we
should recommend as the starting point.
(6) is the natural way to unwrap (5) when the lines get long.
(Personally I'm largely indifferent to 6/7/8; they are all acceptable,
and I think the distinction is mostly based on how dense and nonuniform
the consequences are.)
If the consequences are "similar" to each other, or they are simple,
then 6 is a winner. If they start to diverge in subtle ways, adding
more whitespace may make it easier for readers to understand what is
going on, which might pull us towards 7/8.
Like with the previous choices, I might be inclined to do 7 for myself,
but I wouldn't want to require others to code that way. I think its in
the category of "use more whitespace if you think it makes the code more
readable."
So:
- 5 if it fits
- 6 if you need a little more space
- 7 or 8 if (6) results in something too dense to read
One thing I think you've omitted, but which we should cover, is whether
the -> goes on the line with the case, or the consequence:
case FOO ->
fooAction()
vs
case FOO
-> fooAction()
I think this is similar to advice for joining two long boolean
expressions with &&, or chains of method invocations; it is more obvious
that the second line is a continuation of the first when the operator is
at the left margin rather than the right. So I prefer the second to the
first (as all your examples used.)
On 3/27/2021 1:11 AM, Stuart Marks wrote:
>
> 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)); }; |
>
> —
> 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-808656221>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABJ4REFPSB5USLVAJZ2AADTFVSG7ANCNFSM4Z2NQ5BQ>.
>
-------------
PR: https://git.openjdk.java.net/jdk/pull/3210
More information about the kulla-dev
mailing list