Ternary operator should not be nested

[pstreef]

draft for: #51

Recipe to replace nested ternary statements.

[timtebeek]

Thanks for starting yet another one! Fun to see how you're progressing. :)

It's getting a bit late for an in depth review, so I'll keep it at some general comments for now that might help you find similar recipes to copy patterns from.

I noticed you're having some excess braces / blocks inserted; while ideally these are not inserted, sometimes you can simplify your logic by inserting them unconditionally, and then cleaning up after yourself by explicitly invoking new RemoveUnneededBlock().getVisitor().visit...(...). We have a few such cases for unnecessary parenthesis, but this might suit you here as well. Mostly as a way to get the tests going first, and then clean things up after.

Good seeing the tests you have already. I think it's fine to start small for instance with return statements only, and expand from there. Any tests we don't cover yet you can then annotate with @ExpectedToFail & @Issue("") referencing a new backlog item for further improvements. Benefit of only looking at return for now is indeed that we don't need else blocks yet.

[pstreef]

The thought to use something to clean the braces had crossed my mind but I thought that would be considered cheating 🙈

Tests are green now but polishing is still required. Will leave this as a draft until and revisit it later this week (or possibly later today).

[timtebeek]

Maybe we could add a few more tests just to be sure where the choice options are not literal strings. So for instance:

1. method calls
2. or null,
3. or expressions "foo" + 123,
4. or options preceded by a comment or newline.

All such cases could require slightly different handling, which could negatively affect either the functioning of code, or preservation of whitespace/comments for readability.

[rpau]

Thanks @pstreef for your contribution!

I would like to add to the @timtebeek list, a new test for lambda expressions. It might happen that we should add a new block entry before a new if statement.

 s.stream().map(item -> item.startsWith("s") ? "ss".equals(item) ?"a": "b": "c").collect(Collectors.toSet());

should be translated to

s.stream().map(item -> { 
  if (item.startsWith("s") {
    if ("ss".equals(item)) {
       return "a";
    } else {
       return "b";
    }
    return "c";
 }
}).collect(Collectors.toSet());

[timtebeek]

a new test for lambda expressions. It might happen that we should add a new block entry before a new if statement.

 s.stream().map(item -> item.startsWith("s") ? "ss".equals(item) ?"a": "b": "c").collect(Collectors.toSet());

Interesting case as well! :) Might be good to add a test that verifies no changes are made in such cases yet; so not @ExpectedToFail, but rather guard against accidental changes that could result in incorrect code, as the use within a lambda implies a return. Unless we're able to cover this case easily as well of course.

[pstreef]

Thanks for these notes/additions. I will take them into account.

@rpau I'm doubting if that is really a rewrite everyone would be happy with. Some people prefer to keep their streams brace-less (which is also why you might see ternaries like these pop up) and in stead of adding this in line would expect a method call.

So this would be an opinionated approach, which I guess openrewrite is full of, but I'm still doubting which way would be "best"

[rpau]

@pstreef the problem extracting a method is that we can't infer a meaningful method name, and I agree that the strategy might be opinionated.

IMO, Skipping these cases for lambda expressions might create the impression that the recipe is not fully working. As a developer, I would like at least to identify those cases and then adapt the code.. extracting the method for instance, but if no changes are made, it is hard to identify if the code is satisfying the rule or not.

We can add a boolean parameter to support those cases or not, but as a developer, I believe that we want to identify it to create trust in our recipes.

[knutwannheden]

@pstreef Sorry about these late review comments. I didn't look carefully enough the first time around.

[pstreef]

@knutwannheden thx for the detailed review! I'm still fairly new to all this so we can expect there can be some mistakes for anything untested.

I'd love your input on this failing test: doReplaceMultiLevelTernariesWithComments
I'm missing a comment that is part of a "before" of a JLeftPadded. if I use getFalsePart it automatically extracts the element and the comment is dropped. How should I get this out?

[knutwannheden]

I can't think of any other cases to test. Once we release and deploy it to public.moderne.io I wouldn't be surprised if we discover a few corner cases more, we didn't think of, but that is normal 😄

[knutwannheden]

I'd love your input on this failing test: doReplaceMultiLevelTernariesWithComments
I'm missing a comment that is part of a "before" of a JLeftPadded. if I use getFalsePart it automatically extracts the element and the comment is dropped. How should I get this out?

I can take a look at this. But I think we could also integrate this PR as is and fix this problem later.

[timtebeek]

Impressive work! I've added a few last minor comments, but trust your judgement in resolving those before a merge.

As a last comment: Perhaps it'd be nice to clearly divide the tests over ~3 nested classes: ifs / simple switch / switch expressions.

[pstreef]

Regarding the nesting. I made it a bit better for the already nested SwitchExpressionSupported. There is no "simple" switch, so I split expression, if and no change

[timtebeek]

Maybe good to also annotated one or two representative tests with @DocumentExample; that way they ought to show up in the docs (soon).