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

Connor wool/update localization code documentation #404

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

connor-wool
Copy link
Contributor

@connor-wool connor-wool commented Mar 24, 2018

Adds clarifying comments to code in the localization system. I had to go through and read a bunch of ROS documentation to figure out what's going on, and why certain things are included. Hopefully, by adding these comments, nobody has to do that ever again ;_;


This change is Reviewable

@connor-wool
Copy link
Contributor Author

Obligatory #404: Pull Request Not Found

@skallaher
Copy link
Member

In general, your comments are far too in depth. There is no need to document every individual line of code and how ROS calls work. You have documented this as if it were a ROS tutorial which is unnecessary.


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/localization/localization.cpp, line 9 at r1 (raw file):

{
    /* Initializes a new ROS node for the current process, and gives it the 
    name "localization". This name must be unique across the whole ROS system */

This kind of documentation is unnecessary. Tutorial-like documentation on ROS is better suited in examples so that these files are not bloated. Assume that anyone reading this has a basic understanding of ROS. For many of the following comments, simply note what each section of code is doing, not exactly how it works.


src/localization/localization.cpp, line 37 at r1 (raw file):

    RobosubSensors is designed to handle and sanitize input from outside topics,
    and the LocalizationSystem class manages the particle filter and kalman
    filter classes */

You are defining what a C++ constructor is here. This is better worded as:

Initialize our sensors and localization systems for the Kalman and particle filter.

Be brief with comments in source files. You can elaborate further if absolutely necessary on the wiki.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

2 participants