RFR: JDK-8264222: Use switch expression in jshell where possible [v2]
Tagir F.Valeev
tvaleev at openjdk.java.net
Sun Mar 28 02:36:58 UTC 2021
On Sat, 27 Mar 2021 15:08:13 GMT, Brian Goetz <briangoetz at openjdk.org> 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));
>> };
>>
>> 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>.
>>
@briangoetz @stuart-marks thank you for your comments. I don't like (2) because the first case is significantly longer than the other ones. As a result, there's too much of whitespace on the subsequent lines, and it's hard to follow visually from the case label to the resulting expression without the editor's help (e. g. highlighting the current line). (4) solves this problem, so for code reading, it looks the best.
There's another consideration though: the code maintenance cost. I'm not sure about other editors but AFAIK, IntelliJ IDEA doesn't help you to maintain the vertical alignment for switch cases (we have a similar option for variable declarations, probably we should support switch cases as well), so any edits of the surrounding code may require manual reformatting, and autoformatting accidentally applied to this code may screw the things up.
As for 5-8, there's also an option to align arrows vertically:
(9)
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));
};
`
In this case, the longest line has 114 characters, which looks within the surrounding code style.
We can also put labels one per line, line in 4. This makes lines even shorter:
(10)
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));
};
By the way, I found that there's a redundant "preview" warning suppression at line 230. If we remove it and inline the variable, there's another opportunity for an expression switch. I also changed to (4) and (10) and added a space after 'default' in `ArgTokenizer::nextToken`. See the updated PR.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3210
More information about the kulla-dev
mailing list