"Align logical expressions": Spaces between operator and condition

[ConjuringCoffee]

Hi Jörg-Michael, the rule "Align logical expressions" doesn't always produce results that I find pleasant to look at.

Here's an example of an IF consisting of two conditions connected with an AND:

IF sy-sysid = 'DEV'
   AND sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

The default ABAP Cleaner profile formats this to:

IF     sy-sysid = 'DEV'
   AND sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

I find the spaces between IF and sy-sysid to just look... weird? I understand why the spaces are added: Both conditions are on the same "level" because of the AND, so the first condition is indented accordingly. I think that can make sense for bigger conditions, but it looks weird for this example.

I'm not really sure how the problem could be addressed properly. Maybe you could provide an option to generally remove spaces between the operators (IF, ELSEIF, AND, OR) and the condition itself?

[jelliottp]

Completely agree with this, and it's the reason I have currently disabled this rule for us. It actually causes an error in abapOpenChecks for extra spaces as well. I am choosing to format it just like you have in the first example (or alternatively leave the AND condition on the first line if there are only 2 conditions overall).

[ConjuringCoffee]

Aligning the comparison operator = for this example also looks a bit weird:

IF sy-sysid     = 'DEV'
   AND sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

Maybe there should be an option to exclude smaller checks? I feel like only bigger IF blocks benefit from the spaces introduced by the ABAP Cleaner rule.

[jmgrassau]

Hi ConjuringCoffee and Josh,

yeah, I do agree that the layout doesn't look great here, and actually that's one of the reasons why I personally prefer

which leads to:

  IF  sy-sysid = 'DEV'
  AND sy-mandt = '001'.
    WRITE 'OK'.
  ENDIF.

  IF sy-sysid = 'DEV'
  OR sy-sysid = 'ABC'.
    WRITE 'OK'.
  ENDIF.

To me, this looks better than having three different indentations (the AND just being one position right of the WRITE), as in

IF sy-sysid = 'DEV'
   AND sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

IF sy-sysid = 'DEV'
   OR sy-sysid = 'ABC'.
  WRITE 'OK'.
ENDIF.

The worst case here would be AND or OR at line end, so you could mistake the second line for an assignment:

IF sy-sysid = 'DEV' AND
  sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

IF sy-sysid = 'DEV' OR
  sy-sysid = 'ABC'.
  WRITE 'OK'.
ENDIF.

But adding an option for this would be possible, of course… However, what exactly should the option be? Something like "Condense smaller expressions with up to [2] lines"?

Kind regards, Jörg-Michael

[fabianlupa]

The worst case here would be AND or OR at line end, so you could mistake the second line for an assignment

Hmm that's my usual go to and quite common I think? The next line would just be one more space to right usually to be at the same level as the first token after the IF so it's a bit less likely to mistake it for being inside the block.

Not sure what's best here. Could also enforce brackets for all conditions :o

[jelliottp]

I slightly misspoke on my first comment, this is actually my preferred solution at the moment. Others might not agree, but it aligns with what I see in some other languages such as java (where they do not necessarily intend to try and align everything):

IF sy-sysid = 'DEV'
    AND sy-mandt = '001'.
  WRITE 'OK'.
ENDIF.

In this case, a continuation of the same statement across multiple lines would be tabbed over 2 tabs. I tend to believe consistent indentation in these situations to be more important than aligning expressions. I somewhat understand the desire to align everything, but it can create odd indentations that you wouldn't do otherwise.

[ConjuringCoffee]

yeah, I do agree that the layout doesn't look great here, and actually that's one of the reasons why I personally prefer

Not even this approach solves all of the problems. Here's another problematic example.

Using your settings:

IF      p_type IS NOT INITIAL
AND NOT line_exists( lt_supported_types[ table_line = p_type ] ).

Using my settings:

IF         p_type IS NOT INITIAL
   AND NOT line_exists( lt_supported_types[ table_line = p_type ] ).

However, what exactly should the option be? Something like "Condense smaller expressions with up to [2] lines"?

Yes, that's a possibility. The other possibility I mentioned is to have an option to generally remove spaces between the operators (IF, ELSEIF, AND, OR) and the condition itself, what did you think about that?

[jmgrassau]

Hi ConjuringCoffee,

The other possibility I mentioned is to have an option to generally remove spaces between the operators (IF, ELSEIF, AND, OR) and the condition itself

To be sure that I don't misunderstand you, could you show with these examples (currently in ABAP cleaner formatting) what that would look like?

    IF    a = abap_false AND b  > 3
       OR a = abap_true  AND b <= 10.
      " do something
    ENDIF.

    IF     (    c IS NOT SUPPLIED
             OR b IS INITIAL )
       AND (    d IS SUPPLIED
             OR b IS NOT INITIAL ).
      " do something

    ELSEIF    line_exists( its_table[ 0 ] )
           OR (     lines( its_table ) > 2
                AND line_exists( its_table[ 1 ] ) ).
      " do something
    ENDIF.

Starting from line 2, would you have a fixed indent (e.g. +4) and then just remove all multiple spaces from within the lines? So you'd get this?:

    IF a = abap_false AND b  > 3
        OR a = abap_true AND b <= 10.
      " do something
    ENDIF.

    IF ( c IS NOT SUPPLIED
        OR b IS INITIAL )
        AND ( d IS SUPPLIED
        OR b IS NOT INITIAL ).
      " do something

    ELSEIF line_exists( its_table[ 0 ] )
        OR ( lines( its_table ) > 2
        AND line_exists( its_table[ 1 ] ) ).
      " do something
    ENDIF.

Or do you mean it in a different way?

Kind regards, Jörg-Michael

[ConjuringCoffee]

Hi Jörg-Michael, I'll discuss this with my colleagues and get back to you afterwards.

[ConjuringCoffee]

Hi Jörg-Michael, I've discussed it internally and we came to the conclusion that we can live with the current formatting style of the ABAP Cleaner.

The ABAP Cleaner style makes it very transparent how the conditions are aligned. I think this benefit outweighs that sometimes the additional spaces look a bit weird.

I'm not closing this issue because @jelliottp is also involved in this thread. Josh, maybe you'd like to provide the examples that Jörg-Michael has requested? 😉

[jelliottp]

I favor the last example with fixed indentation with all extra spaces removed. I understand if others' don't, but an option for that would be nice.

[ConjuringCoffee]

Unfortunately, the topic still hasn't fully calmed down internally over here. I'll be setting up some additional examples soon, but I'll need a bit of time. Thanks for your patience 😅

[ConjuringCoffee]

Wow, this is a really challenging topic! 😅 Here's how I would have formatted the example before the ABAP Cleaner existed:

    IF a = abap_false AND b  > 3
        OR a = abap_true AND b <= 10.
      " do something
    ENDIF.

    IF ( c IS NOT SUPPLIED
          OR b IS INITIAL )
        AND ( d IS SUPPLIED
          OR b IS NOT INITIAL ).
      " do something

    ELSEIF line_exists( its_table[ 0 ] )
        OR ( lines( its_table ) > 2
          AND line_exists( its_table[ 1 ] ) ).
      " do something
    ENDIF.

The basic principle: Indent by four spaces after the first line, then indent two spaces for every parenthesis.

But you know what? It might look a bit more familiar, but it really is less readable than the ABAP Cleaner result.

    IF    a = abap_false AND b  > 3
       OR a = abap_true  AND b <= 10.
      " do something
    ENDIF.

    IF     (    c IS NOT SUPPLIED
             OR b IS INITIAL )
       AND (    d IS SUPPLIED
             OR b IS NOT INITIAL ).
      " do something

    ELSEIF    line_exists( its_table[ 0 ] )
           OR (     lines( its_table ) > 2
                AND line_exists( its_table[ 1 ] ) ).
      " do something
    ENDIF.

I hope all of my colleagues can live with that formatting style. 😇