-
Notifications
You must be signed in to change notification settings - Fork 66
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
Line detector documentation #16
Line detector documentation #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting partial review comments
bool notifyOccupancy(int frameNo, bool occupancy); | ||
|
||
//TODO(isaach): Should be a property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 If you do have a TODO with this form, consider matching the username form you use on GitHub
//TODO(isaach): Should be a property. | |
//TODO(iharwell): Should be a property. |
/// <summary> | ||
/// The first point of the line. | ||
/// </summary> | ||
public Point p1 { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ While the fields were not marked readonly
, it's not clear that was intentional. Is it possible to make these properties readonly?
public Point p1 { get; set; } | |
public Point p1 { get; } |
/// <summary> | ||
/// The overlap threshold used to determine occupancy. | ||
/// </summary> | ||
public double overlapFractionThreshold { get; set; } = DEFAULT_OCCUPANCY_THRESHOLD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Consider removing this default value assignment, and updating the constructor to chain like this:
: this(a, b, c, d, DEFAULT_OCCUPANCY_THRESHOLD)
Minor changes to the documentation comments to ensure everything works as expected. Minor refactoring removed to place in a separate branch.
Nothing too involved here. I went through and added XML documentation to nearly everything in the LineDetector project. This should make it a bit easier to work on moving forward.
There's a little bit left to do, though. In particular, some classes don't have class-level documentation yet.
There are also a couple of TODOs in the comments regarding some methods that should probably be replaced with properties, some interface members that could use some reorganizing, etc.