Jarvis's March Convex Hull

I'm more concerned about the coding best practices and how it's written here than the actual algorithm and math. I'm concerned about stack constraints, passing arguments,..etc. If there is anything critically wrong in the way the code is written - I would really appreciate comments!

#include 
#include 
#include 

using namespace std;

struct point{
    float x, y;
    point(){
        x=0; y=0;
    }
    point(float aX, float aY){
        x = aX; y = aY;
    }
};

float computePolarAngle(point convex_pt, point candidate_pt){
    //move origin to convex_pt
    float candidate_x = candidate_pt.x - convex_pt.x;
    float candidate_y = candidate_pt.y - convex_pt.y;
    float angle = (180/M_PI)*atan2(candidate_y,candidate_x);

    //incase the angle goes into the negative we always want to find the counterclockwise 
    //angle from x-axis centered at the selected point
    if(angle& points, vector& convex_points, vector& convex_points_indices){

    //checks
    if(points.size()  deep copy
    vector remaining_points = points;

    //gift wrapping part loop while we have not reached the starting convex point again
    point convex_pt(lowestPt.x, lowestPt.y);
    bool flag_complete = false;
    float eps = 1e-10;

    while(!flag_complete){
        //compute polar angles of all points
        float minIndex = -1;
        float minAngle = 360;
        for(int index=0; index  points;
    points.push_back(point(1,1));
    points.push_back(point(1.1,-1));
    points.push_back(point(-1, 1));
    points.push_back(point(-1.1,-1.1));
    points.push_back(point(0.75,0.5));
    points.push_back(point(-0.5,0.3));
    points.push_back(point(0.25,-0.8));
    points.push_back(point(0.1,-0.9));

    vector convex_points; 
    vector convex_points_indices;

    convexHull(points, convex_points, convex_points_indices);
}

General C++ advice

There's a few general suggestions here that are for the most part independent of your algorithms.

using namespace std

using namespace std;

This is usually a code smell, generally speaking the only time you would really want to do this is when you are making some small throwaway program for testing a concept or making an example (or similar).

In that case the reduction in typing time actually has a positive ROI. But you don't want to do it in any production code because this pollutes the global namespace which is something you should try to avoid, any benefit from time saved typing is immediately wiped out the first time your program breaks because a name conflict.

See this Stack Overflow question for more info on that.

If the main purpose for doing this is to cut down typing std:: then you can
selectively bring in just the names you need by doing:

using std::cout;
using std::cin;

an so on. This cuts down on typing without the downside of polluting the global namespace.

Prefer initialization list

In this code you are using assignment to initialize members:

struct point{
    float x, y;
    point(){
        x=0; y=0;
    }
    point(float aX, float aY){
        x = aX; y = aY;
    }
};

However using the member initializer list
is usually a better way of initializing variables with the constructor.

struct point{
    float x, y;
    point():
        x(0),
        y(0)
    {
    }
    point(float aX, float aY):
        x(aX),
        y(aY)
    {
    }
};

Doing it this way ensures that members get correctly initialized and this format can also
provide the compiler opportunities to optimize the code generated. For the inbuilt default types int, char, etc. there is no performance difference but for user defined types there can be.

See the C++ FAQ entry for more on this topic.

Prefer pass by reference to const

You have a function:

float computePolarAngle(point convex_pt, point candidate_pt){
    //function doesn't change convex_pt
}

It's preferable to instead pass these parameters by reference to const like so:

float computePolarAngle(point const& convex_pt, point coinst& candidate_pt){
    //function doesn't change convex_pt
}

There's 2 main reasons to do this:

• You no longer need to make a unnecessary copy of the parameters for your function

• You more clearly state your intent with the code, any change to bar will now

throw an error at compile time. This can prevent undesirable things from happening.
We all make mistakes, this just helps us catch one class of mistake sooner.

Typedefs

In many places in your code you have:

vector

You might want to make a typedef for this type:

typedef std::vector point_container_t;

Then use that typedef throughout your code. This makes it easier to make changes
later on if you decide to change the data types you use. This isn't always a clear cut decision, choose based on the ROI for doing so in your project.

Initializer list

With C++11 we can condense this code:

vector points;
points.push_back(point(1,1));
points.push_back(point(1.1,-1));
points.push_back(point(-1, 1));
points.push_back(point(-1.1,-1.1));
points.push_back(point(0.75,0.5));
points.push_back(point(-0.5,0.3));
points.push_back(point(0.25,-0.8));
points.push_back(point(0.1,-0.9));

to:

vector points {
    point(1,1),
    point(1.1,-1),
    point(-1, 1),
    point(-1.1,-1.1),
    point(0.75,0.5),
    point(-0.5,0.3),
    point(0.25,-0.8),
    point(0.1,-0.9)
};

Once again, C++11 removes a bunch of boilerplate.

Documentation

You don't have any, you might want to consider adding some.

I'm a big fan of doxygen with C++, have a look into that.

Convex hull function

There's a few issues with this function so lets break them down.:

Function length

One of the main problems with this function is that for what it does it's way too long in terms of lines of code. You really should be breaking out some of this code into smaller functions. Specifically, there's a few times where you are not following the don't repeat yourself principle.

For example, things like this:

point lowestPt(points.at(0).x, points.at(0).y);
int lowestIndex = -1;
for(int index=0; index < int(points.size()) ; index++){
    if(points.at(index).y < lowestPt.y)
    {
        lowestPt.x = points.at(index).x;
        lowestPt.y = points.at(index).y;
        lowestIndex = index;
    }
}

should really be in their own functions:

int compute_lowest_index(vector const& points){
    //calculate index
    return index;
}

This will help you out when you write unit tests for your code. Making as many of these small functions that are easily testable will let you greatly improve the confidence in the correctness of your code when paired with a good suite of unit tests.

Bail out early with bad input

You have this check for invalid input:

```
if(points.size() <= 2){
cout<<"points should consist

StackExchange Code Review Q#73583, answer score: 13