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

[Close Brackets at Line End] Is this always a good thing? #353

Open
pokrakam opened this issue Jun 13, 2024 · 9 comments
Open

[Close Brackets at Line End] Is this always a good thing? #353

pokrakam opened this issue Jun 13, 2024 · 9 comments
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap

Comments

@pokrakam
Copy link
Contributor

pokrakam commented Jun 13, 2024

Relevant sections of the style guide
Close Brackets at Line End

Description
I would like to challenge this rule. For starters, the default behaviour in ADT is the opposite. But there are numerous languages (JavaScript, C++ and Java) where closing on a new line is widely recommended. Personally I used to dislike it, but since learning more JavaScript I've gotten accustomed to it and also caved in and started to use the ADT defaults in ABAP. I was pleasantly surprised to find it increases readability and maintainability in ABAP for complex parameters with multiple brackets. It's fewer lines vs clarity, and I think sometimes clarity is better.
It makes sense in functional-styled languages as there can be several lines and nesting levels in between brackets, and functional ABAP is also evolving in this direction.

See for example SAP's own JS Guidelines and Google's Java Style Guide

I would suggest softening this rule, or recommending that the outermost brackets (and only those) can be on the last line.

Examples
The current examples in the ABAP Style Guide are simple, and it makes sense to have the brackets at line end if there are no additional brackets within the parameters.
But, especially with the shift to functional ABAP, more complex parameters become harder to read, especially with multiple method calls after another. I often find myself counting brackets, and having the outermost one separated JS-Style reduces the perceived bracket depth by one level.

I find this one harder to read:

myobj->do_stuff( 
  key   = item->key
  value = COND #( WHEN date > sy-datum
                  THEN get_estimate( )
                  ELSE get_current( ) )
  data  = CONV #( get_something( ) ) ).

Better (and ADT default):

myobj->do_stuff( 
  key   = item->key
  value = COND #( WHEN date > sy-datum
                  THEN get_estimate( )
                  ELSE get_current( ) )
  data  = CONV #( get_something( ) ) 
).

Or maybe the rule could be further qualified along the lines of "no more than two levels of nesting on one line".

This is not without precedence in ABAP either, we do see closing brackets on a new line in chained method calls (although I must admit I find this less visually appealing):

result = add( a 
  )->add( b
  )->add( c 
  )->add( d ).
@pokrakam pokrakam added Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap labels Jun 13, 2024
@pokrakam pokrakam changed the title [Close Brackets at Line End] Is this really necessary? [Close Brackets at Line End] Is this always a good thing? Jun 13, 2024
@Root3287
Copy link

I've been known to use the following. Just so it wouldn't feel so awkward with an end of a line.

result = foo(
  a = a
  b = b
)->foo( 
  a = c
  b = d
)->foo(
  a = e
  b = f
)->foo( g ).

@fabianlupa
Copy link
Contributor

fabianlupa commented Jun 15, 2024

I personally also like closing brackets indented in the next line better, at least in the more complex cases and then for consistency everywhere. In the following example I always guess the amount of closing brackets needing by adding additional ones until the syntax error disappears. Or actually rather I type it out with new lines and let ABAP cleaner do its thing... This is especially annoying when you want to add a new entry to a table constructor expression and need to find the correct place between the brackets. Thinking of the deeply nested structures needed for cmd_ei_api / vmd_ei_api in CVI.

DATA(customer) = VALUE ty_customer(
                           customer_id = '1234'
                           addresses   = VALUE #( ( street                = 'Musterstraße'
                                                    house_number          = '9'
                                                    city                  = 'Walldorf'
                                                    country               = 'DE' )
                                                  ( street                = 'Musterstraße'
                                                    house_number          = '5a'
                                                    city                  = 'Walldorf'
                                                    country               = 'DE'
                                                    is_primary            = abap_true
                                                    communication_mediums = VALUE #(
                                                        ( communication_key = 'EMAIL'
                                                          email_address     = 'some-address@sap.com' ) ) ) ) ).

Versus this approach where you can see corresponding brackets without editor highlighting just by indentation:

DATA(customer) = VALUE ty_customer(
  customer_id = '1234'
  addresses   = VALUE #(
    ( 
      street                = 'Musterstraße'
      house_number          = '9'
      city                  = 'Walldorf'
      country               = 'DE'
    )
    ( 
      street                = 'Musterstraße'
      house_number          = '5a'
      city                  = 'Walldorf'
      country               = 'DE'
      is_primary            = abap_true
      communication_mediums = VALUE #(
        (
          communication_key = 'EMAIL'
          email_address     = 'some-address@sap.com'
        )
      )
    )
  )
).

Edit: To be clear though, my preference isn't strong, if there is tooling in place like ABAP Cleaner to format it that way and like ADT with highlighting the corresponding brackets.

@pokrakam
Copy link
Contributor Author

@fabianlupa that's exactly what I mean! Eclipse does a half-job of highlighting it, but you still have to move your cursor around to find the right spot and there's still that annoying little delay.
I think the reason JS and others only recommend to only put closing brackets on their own is that those are the ones we puzzle over the vast majority of the time. I would compact your second example a little by writing:

DATA(customer) = VALUE ty_customer(
  customer_id = '1234'
  addresses   = VALUE #(
    ( street                = 'Musterstraße'
      house_number          = '9'
      city                  = 'Walldorf'
      country               = 'DE'
    )
    ( street                = 'Musterstraße'
      house_number          = '5a'
      city                  = 'Walldorf'
      country               = 'DE'
      is_primary            = abap_true
      communication_mediums = VALUE #(
        ( communication_key = 'EMAIL'
          email_address     = 'some-address@sap.com'
        )
      )
    )
  )
).

Thus, drop to the next line with indent after the opening bracket if the line(s) would get too long, else keep going. I think a maximum bracket nesting depth of two per line is easy enough to read without having to backtrack or start counting.

On a slightly different note, one thing I miss in ADT versus VScode is the vertical lines to show indents, which is also very helpful with complex bracketing:
image

@fabianlupa
Copy link
Contributor

I think the reason JS and others only recommend to only put closing brackets on their own is that those are the ones we puzzle over the vast majority of the time. I would compact your second example a little by writing:

Hmm, I would have to get used to that. The recommendations you linked don't ever have any code in the same line after the opening bracket, or do they? So the opening bracket isn't on its own line, but the block it opens doesn't start on the same line while in your adjusted example it does.

And just as a counter point: Other style guides do recommended opening brackets on new lines by the way, from the top of my head C# (https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions) and (partly) PHP (https://www.php-fig.org/psr/psr-12/).

@pokrakam
Copy link
Contributor Author

I might have it confused with something else. The beauty of standards is that there are so many to choose from 🙂
It does make sense in ABAP though, it's the line length of the expression that influences whether to continue on the same line after an opening bracket or not. I think it's OK to apply this to standalone nested constructs like your example.

@fabianlupa
Copy link
Contributor

I was intrigued to find more examples in other style guides ;)

Kotlin:

In long argument lists, put a line break after the opening parenthesis. Indent arguments by four spaces. Group multiple closely related arguments on the same line.

drawSquare(
    x = 10, y = 10,
    width = 100, height = 100,
    fill = true
)

https://kotlinlang.org/docs/coding-conventions.html#method-calls

Rust:

In this case, put each argument on its own block-indented line, break after the opening parenthesis and before the closing parenthesis, and use a trailing comma:

a_function_call(
    arg1,
    a_nested_call(a, b),
)

https://doc.rust-lang.org/nightly/style-guide/expressions.html#multi-line-calls

Does the current recommendation in this styleguide have any source / reference or is the only reasoning "it's needlessly longer"? Seems to be this commit which was before any external discussion took place I assume fda8a5f .

@pokrakam
Copy link
Contributor Author

The ABAP documentation is also a bit inconsistent, see the example code here.
I definitely don't like this close-brackets-in-the-middle style:

    FINAL(itab1) = VALUE itab1(
      ( col1 = 1 col2 = VALUE line1-col2( ( col1 = 111 col2 = 112 )
                                          ( col1 = 121 col2 = 122 ) ) )
      ( col1 = 2 col2 = VALUE line1-col2( ( col1 = 211 col2 = 212 )
                                          ( col1 = 221 col2 = 222 ) ) )
      ( col1 = 3 col2 = VALUE line1-col2( ( col1 = 311 col2 = 312 )
                                          ( col1 = 321 col2 = 322 ) ) )
                             ).

and then there's:

    FINAL(result) = REDUCE string(
      INIT text TYPE string
      FOR wa1 IN itab1
      FOR wa2 IN wa1-col2
      NEXT text =
             |{ text } { wa1-col1 }, { wa2-col1 }, { wa2-col2 }\n| ).

which I find much better as:

    FINAL(result) = REDUCE string(
      INIT text TYPE string
      FOR wa1 IN itab1
      FOR wa2 IN wa1-col2
      NEXT text = |{ text } { wa1-col1 }, { wa2-col1 }, { wa2-col2 }\n|
    ).

@nununo
Copy link

nununo commented Jun 19, 2024

A good reason for opting for having the bracket in its own line is related with version control. If you have this:

exec(
  a = 1
  b = 2 ).

And you add parameter c you will have two changed lines:

exec(
  a = 1
  b = 2       " this line changed
  c = 3 ).    " this line added

While, if you have the bracket in its own line:

exec(
  a = 1
  b = 2
).

When adding the parameter, you only change one line:

exec(
  a = 1
  b = 2
  c = 3   " this line added
).

@Jules1337dev
Copy link

Is there any news on this topic? I had the same request, see the last mentioned issue here. Different formatting options should at least be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap
Projects
None yet
Development

No branches or pull requests

5 participants