[incubator-kie-drools-5906] [new-parser] unification in accumulate

[tkobayas]

Issue:

• [new-parser] unification in accumulate #5906

[tkobayas]

@yurloc @mariofusco @gitgabrio Please review, thanks!

[gitgabrio]

HI @tkobayas Thanks for the PR.
Sorry to be picky, but those if else if lines maybe could be simplified;
what about something like:

 String label = ctx.label() != null ? ctx.label().drlIdentifier().getText() : null;
 boolean unify = ctx.unify() != null;
String bind = unify ? ctx.unif().drlIdentifier().getText() : label;

Would this work ?

[tkobayas]

Hi @gitgabrio

ctx.label() and ctx.unif() could be both null. So if we use ternary operator, it would be

boolean unify = ctx.unif() != null;
String bind = unify ? ctx.unif().drlIdentifier().getText() : ctx.label() != null ? ctx.label().drlIdentifier().getText() : null;

Do you like this better?

[mariofusco]

Personally I'm not strongly opinionated on this syntax details and both versions look equally good to me.

[gitgabrio]

Hi @tkobayas: fine 👍

[gitgabrio]

Hi @mariofusco
My point is about readibility: of course both versions are equals, but the former (three lines) is easier to understand at first glance; the latter (two lines) requires a little bit more effort (IMO), for someone that has to read it for the first time.
On a broader POV, if the code is made by "easier to read" lines, it is less tiring to read and work on (of course, I'm not talking of one specific line).
So, my comment, as usual, is target more to improve the readibility of the code as whole, and that's achievable only by lot of very small improvements: hope this would make sense 😄
Anyway, I leave that to @tkobayas

[gitgabrio]

Thanks @yurloc !
But, if
"ctx.label() and ctx.unify() can't be both non-null. "
then, all the versions discussed until now (even the original one in the PR) skip such check, silently swalloping up the fact that both are not null:

        String bind = null;
        boolean unify = false;
        if (ctx.label() != null) {
            bind = ctx.label().drlIdentifier().getText();
        } else if (ctx.unif() != null) {
            bind = ctx.unif().drlIdentifier().getText();
            unify = true;
        }

If at least one of them has to be null, then a different approach is required... guys, I didn't thought to get so far 😄

[yurloc]

@gitgabrio The fact that they can both be null and only one of them can be non-null follows from the structure of the grammar rule:

accumulateFunction : (label|unif)? drlIdentifier conditionalExpressions ;

So if you're suggesting we should write a defensive code and add something like

if (ctx.label() != null && ctx.unif() != null) throw new IllegalStateException();

then I hope this post explains that it would be redundant. Testing such a condition is testing the ANTLR parser itself, which is unnecessary.

P.S. In other words, the fact that label and unif can't be both non-null is guaranteed by the grammar, so no need to check that on our side.

[tkobayas]

Ahh, sorry, I wrote my previous post "ternary operators with 2 lines" before I noticed that @gitgabrio had edited his first post (which missed ctx.label() null-check before the edit). Please forget my "2 lines" post.

Now the comparison is "if-based original version" vs "ternary operators with 3 lines by Gabriele". From "easier to read" POV, I prefer "if-based original version". But I'm also not strongly opinionated on syntax details. If @gitgabrio strongly likes the ternary operators, I'm fine to change. Let me know.

[gitgabrio]

@tkobayas I'm really sorry this has become more complicated then expected, so I'll leave the choose to you, no prblm! 👍

[tkobayas]

Got it, thanks, @gitgabrio !