Implement Graham Scan in C++ #261
Conversation
Gathros added the Implementation 
| | ||
| // M will be the point on the hull | ||
| auto M = 2u; | ||
| for (auto i = 0u; i < N; ++i) { |
There was a problem hiding this comment.
why are you using auto so much, it's hard to read.
There was a problem hiding this comment.
I follow C++ Core Guideline: ES.11: Use auto to avoid redundant repetition of type names
There was a problem hiding this comment.
This also obfuscates a bug: if the vector is large enough so that N > size_t(0u - 1) then this turns into an infinite loop.
There was a problem hiding this comment.
In both lines above the type of vars is clear: 2u is unsigned int, 0u is unsigned int. Core Guidelines states: Use auto to avoid redundancy. I want to use unsigned int and writing it instead of writing auto violates the guideline.
Reply to the comment about big vector bug: It is more work to make code work for cases if array size is bigger 4 billion elements and code does not look nice. I believe handling cases for arrays bigger 4 billion elements is special case and require special treatment.
There was a problem hiding this comment.
I realize this discussion happened a while ago, I am catching up now. Reading the guidelines (ES.11), there are a few notable exceptions to using auto:
Exception: Avoid auto for initializer lists and in cases where you know exactly which type you want and where an initializer might require conversion.
(I added emphasis)
So it would seem that there is no problem using unsigned int or size_tbecause those are the specific types we want here. We are also not using the types incessantly and are working on code at a fundamentally different scale than the guideline was created for.
In my opinion, unsigned int M = 2 is understandable no matter what background you have auto M = 2u is not.
| | ||
| // M will be the point on the hull | ||
| size_t M = 2; | ||
| for (size_t i = 0; i < N; ++i) { |
There was a problem hiding this comment.
I don't think the loop needs to start from 0 since you know points 0, 1, and 0 are co linear and the points 0,1, and 1 are too. So start from i = 2.
| } | ||
| // All points are collinear | ||
| if (i == N - 1) | ||
| break; |
There was a problem hiding this comment.
Add a space after this line, the no brackets if statement is confusing when the lines are this close together.
| Another stale pull request. |
4 participants
No description provided.