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

Remove function names from comments (where not useful) #111

Closed
skliper opened this issue Nov 17, 2020 · 3 comments · Fixed by #185
Closed

Remove function names from comments (where not useful) #111

skliper opened this issue Nov 17, 2020 · 3 comments · Fixed by #185
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@skliper
Copy link
Contributor

skliper commented Nov 17, 2020

Is your feature request related to a problem? Please describe.
Function names in comments (end of function comment, function header comment) historically have been poorly maintained.

Example:

int32 SAMPLE_APP_Process(const SAMPLE_APP_Process_t *Msg)

} /* End of SAMPLE_APP_ProcessCC */

Describe the solution you'd like
Remove redundant information (these end of function comments and name from the header).

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper added enhancement New feature or request good first issue Good for newcomers labels Nov 17, 2020
@ShobhitSamuelSingh
Copy link

Hi, I want to work on this issue.

ShobhitSamuelSingh added a commit to ShobhitSamuelSingh/sample_app that referenced this issue Jan 19, 2021
@thnkslprpt
Copy link
Contributor

thnkslprpt commented Oct 4, 2022

In addition to the redundant 'end of function' comments, there are also countless /* end if */ and /* end while */-type comments inside functions.

I would say that the only comments like this that provide enough value to justify the increased clutter, are of family in the example below (from another app):
https://github.com/nasa/SC/blob/68e9c6574d3ae847f9d88ebd01368ecef98a48bb/fsw/src/sc_cmds.c#L231

That is, complex functions with several layers of nested if/else statements. In those cases, the comments actually do help significantly to maintain composure when moving through those nested statements.

But a simple /* end if */ after every if block probably adds more clutter than value.

In any case, even the components/apps that do have /* end if */-type comments don't apply it consistently Even in the same file - there are functions that do and functions that don't. In light of that, they should be removed simply for the sake of consistency.

Examples of what I would say are unnecessary /* end if */-type comments:
https://github.com/nasa/SC/blob/68e9c6574d3ae847f9d88ebd01368ecef98a48bb/fsw/src/sc_state.c#L82-L84
https://github.com/nasa/cFE/blob/749444164c799d5498a8f0621f8e16a946a57145/modules/sb/fsw/src/cfe_sb_priv.c#L145
 
Regarding the function names in header comments (above each function), I agree they can be removed as they add no value (the function name is literally written a few lines below each of these header comments), and like you said @skliper, they often get out of sync when function names are changed.

Removing them might be a better solution (and one-time action) rather than relying on them being constantly maintained into the distant future...
 
In summary, YES to removing:

  1. /* End of function */-type comments?
  2. Low-added-value /* end if */-type comments inside functions?
  3. Function name in function header comments?

@skliper
Copy link
Contributor Author

skliper commented Oct 4, 2022

Yes, Yes, Yes. With automated formatting applied to enforce consistent indentation I don't see much value in any of those comments.

thnkslprpt added a commit to thnkslprpt/sample_app that referenced this issue Oct 4, 2022
thnkslprpt added a commit to thnkslprpt/sample_app that referenced this issue Oct 4, 2022
thnkslprpt added a commit to thnkslprpt/sample_app that referenced this issue Oct 5, 2022
This was referenced Oct 5, 2022
dzbaker added a commit that referenced this issue Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants