Added Jarvis March for C++ #283
Conversation
june128
added
the
Implementation
|
Hey @Gorzoid and welcome and thanks for contributing! We currently have a lot of open C++ pull requests, so I think we're a little short on avilable C++ reviewers. Your PR might take a little longer than usual to get reviewed because of this. I hope you don't mind. |
|
That's grand, yeah I can see there are a lot C++ PR's. |
| next_point_it = std::max_element( | ||
| start, | ||
| end, | ||
| [p1](const Point& p2, const Point& p3){ |
There was a problem hiding this comment.
This is actually incorrect, [p1] will generate better code since the Point struct is fairly small (and so you can pass it in registers). Since you don't need to mutate the captured variable, I would stick with the by value version
| }; | ||
|
|
||
| template<typename ForwardIter> | ||
| std::vector<Point> jarvis_march(ForwardIter start, ForwardIter end) |
There was a problem hiding this comment.
Take as input const std::vector<Point> &
| }; | ||
|
|
||
| template<typename ForwardIter> | ||
| std::vector<Point> jarvis_march(ForwardIter start, ForwardIter end) |
There was a problem hiding this comment.
It crashes in case of empty set. Should return an empty vector.
There was a problem hiding this comment.
I think it is possible to make it work without making empty vector a special case. For instance use while loop instead of do...while. Can you play around and try to refactor code?
There was a problem hiding this comment.
If I take out the do...while I need to add extra code to add the first element. Anyway the main problem is the min_element to find the left most point. If empty it will return points.end(), I can't think of any way to resolve that would result in shorter code.
There's a few things I was unsure of while making this, for input I used template ForwardIter, which allows it to be used with multiple datastructures. I wasn't sure, however whether to return a vector, or use an OutputIter argument. The vector option is simpler but kind of ruins the point of user template iterators as input. Due to this algorithm not having a predetermined output size (up to the size of the points), I feel using an OutputIter might lead to buffer overflows if used incorrectly.
Seeing as this is meant to be informative rather than a standardized algorithm, I feel like I should just keep the vector return. Using a vector as the input would hardly change the code and I still kinda like using range based algorithms.