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

[Comment Position] what is wrong with inline comments? #297

Open
schibbi opened this issue Jan 24, 2023 · 12 comments
Open

[Comment Position] what is wrong with inline comments? #297

schibbi opened this issue Jan 24, 2023 · 12 comments
Labels
Clarity of Text The issue or pull request helps to improve the clarity of the text clean-abap

Comments

@schibbi
Copy link

schibbi commented Jan 24, 2023

Relevant sections of the style guide
https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#put-comments-before-the-statement-they-relate-to

Question
What exactly is meant with "invasive" related to short comments for short lines of code like
output = calculate_result( input ). " delegate pattern
?
Or asking more generally: what is the problem with such comments?

a) I find such inline-comments very clear.
b) I would consider them more compact and less invasive then adding a new line just for the comment above the statement
c) I prefer inline comments over comments in separate lines, because I have seen many cases where separate-line-comments got disconnected from their related statement because of "not so diligent" code changes, so that a (potentially helpful) comment got completely confusing, because the statement being several lines of code "away" from the comment now...

Any insights or comments? I also did not find anything specific to comment positions in the web (but lot's of stuff related to the content of comments).

@schibbi schibbi added Clarity of Text The issue or pull request helps to improve the clarity of the text clean-abap labels Jan 24, 2023
@fabianlupa
Copy link
Contributor

fabianlupa commented Jan 29, 2023

I think they are mostly fine as well. Some aspects come to mind that might favor the separate line(s):

  • If another actual parameter is used in calculate_result in your example the method call will have another line according to the styleguide. Then it's not that clear where the comment should go and if it comments the whole call or a single parameter. If it is added later you'll have to move the comment.
  • If the comment is a bit longer and the assignment also is longer the maximum line length (for example 120) might get hit. It's not easy to break up the comment to another line then as it will not be aligned with the first line. You then will likely need to adjust the code to have the comment before the line of code.
  • I assume the refactoring tools will either fail or just delete the comment (extract method etc.) if it is in the same line as the code.

@Jelena-P
Copy link

Jelena-P commented Feb 10, 2023

Came here to comment about the comments. :)

Not sure where this suggestion came from, it's first time I'm hearing a recommendation to use " instead of *. It's an interesting point that " comment indents with the command but then (a) is this really important? (b) the example shown should not require a comment to begin with. It's the whole point of Clean ABAP to avoid unnecessary comments and use clear names for variables and methods.

Also, the comment/uncomment keyboard shortcuts in both SAP GUI and Eclipse use * comments. I write comments only to explain something that is not clear from the code, so it takes a few lines. I write the comment, do nice formatting and then use keyboard shortcut to make it a comment. Which would add *. ¯_(ツ)_/¯

My recommendation would be to use these as intended: full lines of comments with *, inline comments with ". I rarely use inline comments these days, so suggesting to use " instead of * (as it's currently written) is a very weird one.

Edit: @schibbi I also agree with your point that if the comment pertains to one line, then inline comment is actually a better approach for the reasons you stated.

@fabianlupa
Copy link
Contributor

@Jelena-P So you write a multiline comment without any * or " and then press Ctrl+< after being done? I can't imagine that at all, the comment will be underlined red the whole way while writing it as it's a syntax error until you are done (in ADT)?! Also naturally on manual line break you will be at the wrong position and you'll have to correct afterwards with multi line selection (Ctrl+shift+a)?
I also cannot think of another language where comments aren't automatically aligned with the code blocks around them so to me " is the expected behavior, to your point (a).

@ConjuringCoffee
Copy link
Contributor

I've just broadly checked the Clean Code book by Robert C. Martin and I wasn't able to find any explicit mention of this rule. He seems to have a preference against inline comments though, seeing as the only example I was able to find was from a bad practice example.

The closest rule I was able to find was the one against comments behind closing brackets. I've seen that in ABAP as well, like this:

ENDIF. " IF sy-subrc = 0

The preference against inline comments is also in line with Martin's formatting rules which prefer shorter code lines.

As a side note, I like that it prevents versioning comments like this:

g_test = 1. "GHL080498

However, I do agree that there are cases in which inline comments make more sense. I think these cases are very rare though. I would be fine with removing the rule because the preference against them is still conveyed by other rules.

@Jelena-P
Copy link

@fabianlupa The key here is to use SE80 where nothing is highlighted in red. :) Also, sometimes I even write the comments some place else and copy-paste them. Obviously I try to do self-documenting code as much as possible but sometimes 2-3 lines worth of comments are really needed. I am aware I'm likely putting much more thought into the comment text than an average ABAPer. :)

@Jelena-P
Copy link

@ConjuringCoffee The "versioning comments" (which I hate with passion) are already addressed by another point.

After looking at the comments here, I would change the suggestion to point out the indentation feature of the inline comments but to provide no opinion otherwise. Both can be used whenever appropriate. There is nothing more annoying than 3 lines of comments preceded by an inline comment sign AND varying (!) number of spaces in front of it. But I'm not sure how to formulate a suggestion on this without sounding too obsessive.

@ConjuringCoffee
Copy link
Contributor

There is nothing more annoying than 3 lines of comments preceded by an inline comment sign AND varying (!) number of spaces in front of it. But I'm not sure how to formulate a suggestion on this without sounding too obsessive.

@Jelena-P Could you please make an example for such a case?

@Jelena-P
Copy link

@ConjuringCoffee It would look something like this:

image

@JozsefSzikszai
Copy link

image

Why would it look like this? Pretty Printer aligns the apostrophes.

@Jelena-P
Copy link

Jelena-P commented Feb 20, 2023

@JozsefSzikszai IDK, some people still don't use PP for some reason. I was literally looking at a comment like this when I went to check what does Clean ABAP say about this and then stumbled upon this post.

With PP, here is also what happens:

image

*mean

@fabianlupa
Copy link
Contributor

Someone must have entered those spaces after the " manually. I'd argue that person didn't try at all to align things correctly. * Wouldn't help that either.

@JozsefSzikszai
Copy link

@Jelena-P
Any clients I have been working in the past 10+ years, using PP (and with exact setup of PP) was part of the development guidelines. Not using PP, also means failing the Code Review.
If a developer uses random number of spaces in their comments (which for me is a sign of not paying attention to details), I assume there will be more issues in the code itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clarity of Text The issue or pull request helps to improve the clarity of the text clean-abap
Projects
None yet
Development

No branches or pull requests

5 participants