Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Remove unnecessary parenthesis #105

Closed
jan-jezek opened this issue Sep 13, 2023 · 8 comments
Closed

Feature request: Remove unnecessary parenthesis #105

jan-jezek opened this issue Sep 13, 2023 · 8 comments
Assignees

Comments

@jan-jezek
Copy link

jan-jezek commented Sep 13, 2023

Hello,

Would it be possible to create a rule, that removes unnecessary parenthesis as described in the abaplint rule: https://rules.abaplint.org/many_parentheses/

Example:

IF ( variable IS INITIAL ).
  variable = 42.
ENDIF.

Expected:

IF variable IS INITIAL.
  variable = 42.
ENDIF.
@jmgrassau
Copy link
Member

Hi Jan,

from "thumbs up" response, definitely our most popular proposal so far! :-)

I completely agree with your example, but I wonder whether in other cases, parentheses might actually be intended to improve readability, e.g. by grouping subexpressions that semantically belong together …

  IF lv_is_valid = abap_true AND ( lv_year = 2023 AND lv_month < 10 ).
  ENDIF.

… or because not everyone may be so familiar with operator precedence rules:

  IF ( lv_year = 2022 AND lv_month > 8 ) OR ( lv_year = 2023 AND lv_month < 10 ).
  ENDIF.

But that could of course be solved with options, e.g. "Remove parentheses around subexpressions…":

  • never
  • if all logical operators are the same
  • whenever operator precedence allows

Kind regards,
Jörg-Michael

@ConjuringCoffee
Copy link
Contributor

Good thinking. I think I'd choose "Never" for the option "Remove parentheses around subexpressions". I'd only be interested in removing the parentheses if the entire condition is wrapped in parentheses.

@fabianlupa
Copy link

IF ( lv_year = 2022 AND lv_month > 8 ) OR ( lv_year = 2023 AND lv_month < 10 ).
ENDIF.

Please don't remove those, I cannot for the life of me remember the precedence rules especially if the condition is broken up into multiple lines.... I tend to just always add the brackets if AND and OR are combined, necessary or not.

@jelliottp
Copy link

IF ( lv_year = 2022 AND lv_month > 8 ) OR ( lv_year = 2023 AND lv_month < 10 ).
ENDIF.

Please don't remove those, I cannot for the life of me remember the precedence rules especially if the condition is broken up into multiple lines.... I tend to just always add the brackets if AND and OR are combined, necessary or not.

I think in that case you could simply not enable this rule or select the "Never" option.

@jmgrassau jmgrassau self-assigned this Nov 1, 2023
@jmgrassau
Copy link
Member

Hi all,

so here's a preview of the new cleanup rule "Remove needless parentheses" for the next release: I'd suggest the following options, of which only the first one would be activated by default:

image

The first option, "Remove needless parentheses around the entire logical expression", does the following (covering 61% of the 2000+ statements with needless parentheses that I found in sample code):
image

The second option, "Remove needless parentheses around relational expressions" (11% of the cases) still looks rather tame and harmless to me:
image

Third option, "Remove needless parentheses from OR ( ... AND ... )" (26%) would be a bit more daring (see discussion above):
image

Fourth option, "Remove needless parentheses from AND ( ... AND ... ) / OR ( ... OR ... )" (1%):
image

The only cases of needless parentheses that I found in sample code NOT covered by these four options would be OR ( NOT ... ) as well as AND ( NOT ... ), but those were so rare that I didn't think an option would be needed. Also, theoretically, you could remove parentheses from NOT ( rel_expr ), e.g. NOT ( a = b ), but for that, I didn't find a single case.

Hope this makes sense, including the proposed default settings?

@fabianlupa:

I cannot for the life of me remember the precedence rules

Maybe try "OrANGe" as a mnemonic (from weakest to strongest precedence):

  • Or
  • And
  • Not
  • Ge (greater or equal, >=) as a representative of any relational expression (i.e. comparison or predicate)

EQUIV would be even weaker than "OR", but that's so weak and so rarely used, it's not even mentioned in the mnemonic ;-)

Kind regards,
Jörg-Michael

@jmgrassau
Copy link
Member

… so you could say, as long as you well-structured logical expression appears in OrANGe 🍊 order…

CHECK            a  = b
         AND NOT c GE d
      OR         a  = c
         AND NOT c GE b.

you're fine without any parentheses :-)

@fabianlupa
Copy link

Thanks for the programming lesson @jmgrassau , shouldn't be too difficult to remember, coincidentally orange is part of the name of my current employer... 🍊

jmgrassau added a commit to jmgrassau/abap-cleaner that referenced this issue Nov 6, 2023
@jmgrassau
Copy link
Member

Hi Jan,

thanks again for opening this issue! The new cleanup rule "Remove needless parentheses" is now available in version 1.11.0 which was just released!

Kind regards,
Jörg-Michael

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants