Galactic code review #29
Conversation
src/full_coverage_path_planner.cpp
Outdated
| bool do_publish = false; | ||
| float orientation = eDirNone; | ||
| float orientation = dir::none; | ||
| RCLCPP_INFO(rclcpp::get_logger("FullCoveragePathPlanner"), "Received goalpoints with length: %lu", goalpoints.size()); | ||
| if (goalpoints.size() > 1) |
There was a problem hiding this comment.
Can you elaborate more on your previous comment?
There was a problem hiding this comment.
Sure!
Early-outs make the code more readable by handling the exceptions in a short block at the top.
function do_awesome()
{
if (everything_okay_as_expected)
{
Do
lots
of
indented stuff
}
else
{
warn about it
}
}
Can be replaced by:
function do_awesome()
{
if (!everything_okay_as_expected)
{
warn about it
return;
}
Do
lots
of
stuff
}
There was a problem hiding this comment.
And here it looks like goalpoints.size() <= 1 is exceptional.
There was a problem hiding this comment.
Got it... in our case the function does not return right after the else condition, so we can not early-out. I can switch the order to at least show the especial case first.
There was a problem hiding this comment.
Aaah, I thought it did!
Then you may leave it like this as well. Up to you.
There was a problem hiding this comment.
On a similar note. I think the node dies if goalpoints.size() == 0. Since that case isn't handled.
There was a problem hiding this comment.
Yep.. will add that one
|
ping |
|
|
||
| namespace full_coverage_path_planner | ||
| { | ||
|
|
||
| enum dir |
|
These improvements all look good, so approving. But this PR is of course only a review of the improvements, not a full review. So that issue stays open. I'll pick that up later this week. |
Made most changes suggested in PR25 #25