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

Request code comments for calibration_service.cpp #21

Open
phvass opened this issue Apr 11, 2014 · 5 comments
Open

Request code comments for calibration_service.cpp #21

phvass opened this issue Apr 11, 2014 · 5 comments
Assignees

Comments

@phvass
Copy link

phvass commented Apr 11, 2014

Please provide in-line code comments to the thread calibration_service.cpp

@gavanderhoorn
Copy link
Member

Could you elaborate a bit? Is there a pull request to be reviewed?

@shaun-edwards
Copy link
Member

The code in calibration_service.cpp is quite long. The proper place for comments for node level functionality is the ros wiki. We are also reworking the calibration code to be more self-contained and documented at a library level (i.e. in doxygen). This would result in a very thin wrapper node, which should require very little commenting. As a rule documentation should be done in the header file and on the wiki. Redundant documentation should be avoided because it will inevitably go out of sync. As far as comments in source files, it is far better to write code that reads well (i.e. self documents without comments). Comments in source files should be very limited.

@shaun-edwards shaun-edwards self-assigned this Apr 11, 2014
@phvass
Copy link
Author

phvass commented Apr 11, 2014

OK. Rescinded.

@phvass phvass closed this as completed Apr 11, 2014
@shaun-edwards
Copy link
Member

@phvass . I'm going to keep it open because it's a good point. Often times the fix for the issue is not was initially proposed. The reason for the issue, that calibration_service is hard to follow, is still valid and should be addressed. We will just do so in a different way.

Thanks for submitting, and please feel free to keep doing so.

@shaun-edwards shaun-edwards reopened this Apr 11, 2014
@drchrislewis
Copy link
Member

the calibration_service.cpp code looks like a thin ros wrapper to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants