GrandOrgue / grandorgue

GrandOrgue software
Other
166 stars 40 forks source link

Renamed some methods added the const qualifier and added some comments #1751

Closed oleg68 closed 9 months ago

oleg68 commented 9 months ago

This is the first PR related to #1724

This PR makes the code a little bit clear

  1. Addes the const qualifier to lots of GetXXX() methods
  2. Added the override qualifier to lots of methods
  3. Renamed some methods
  4. Added some comments
  5. Moved some short implementations from .cpp to .h
  6. GOAudioSection::Setup(): extracted the same code from the both branches of if/else

No GO behavior should be changed.

larspalo commented 9 months ago

@oleg68 Yes, your last versions are much better!

larspalo commented 9 months ago

@oleg68 If you're not intending to expand upon the parameter comments, then at least add a TODO to remove the comments that are only temporary work additions when the final commit that resolves the issue is due. But better would be to really add the explanations to the comments that would perhaps be useful in the future!

oleg68 commented 9 months ago

@larspalo

The description of parameters should be at the method specification, not at the method call.

Some other programming langualges (ex. Oracle pl/sql, Kotlin etc) allow not only positional notation but also naming notation of passing parameters. It makes the calling code more readable.

Unfortunally c++ does not support naming notation. Putting the parameter names to the comments is the only possible workaround.

So I insist on parameter names in the comments. I hope it will help other future developers.

larspalo commented 9 months ago

So I insist on parameter names in the comments. I hope it will help other future developers.

And I insist that they are better than they are currently.

rousseldenis commented 9 months ago

The description of parameters should be at the method specification, not at the method call.

Agree on that

oleg68 commented 9 months ago

So I insist on parameter names in the comments. I hope it will help other future developers.

And I insist that they are better than they are currently.

OK. I added a TODO of removing the parameter name comments in the future after reducing the number of parameters of DoCrossfade

larspalo commented 9 months ago

@oleg68 Ok, fair enough. Except that the comments are prime examples of poor comment practice, I never saw the need of them to begin with as when hovering over the function call in Eclipse IDE I could see the parameters directly as a reference. Don't know if it works the same in Netbeans though...

oleg68 commented 9 months ago

Don't know if it works the same in Netbeans though

Netbeans does not show the parameter names.

We'll just have to remember this later.

I have already started working on it, but it is not so simple.

larspalo commented 9 months ago

@oleg68 Sure, more or less every IDE behaves differently. This is what I see:

Skärmbild från 2023-12-26 21-23-26

I have already started working on it, but it is not so simple.

I understand that. Thanks for your efforts!