Weiler-Atherton polygon-clipping algorithm in C++

I have searched throughout the internet and found no OO approach to implement Weiler-Atherton algorithm, so I implemented the following.

Please, help me to make the implementation more efficient and concise.

```
#include
#include
#include
//#include
#include

using namespace std;

// Define Infinite (Using INT_MAX caused overflow problems
#define INF 10000.00l
//#define M_PI 3.14
#define SHOW(X) std::cout 0)? 1: 2; // clock or counterclock wise
}
bool IsValid()
{
if(x!=INVALID && y!=INVALID)
{
return true;
}
else return false;
}
bool operator==(const Point2d & point)
{
return (x == point.x)&&(y == point.y);
}
bool operator!=(const Point2d & point)
{
return (x != point.x)&&(y != point.y);
}
void Show()
{
std::cout
class Collection
{
private:
std::vector collection;
void Copy(Collection const& pc)
{
std::vector col = pc.GetList();

this->Add(col);
}
public:
Collection(){}
Collection(Collection const& pc)
{
Copy(pc);
}
Collection & operator=(Collection const & pc)
{
Copy(pc);
return *this;
}
void Add(double x, double y)
{
collection.push_back(Point2d(x,y));
}
void Add(T const& point)
{
collection.push_back(point);
}
void Add(std::vector const& points)
{
for(size_t i=0 ; iAdd(points[i]);
}
}
int size()
{
return collection.size();
}
T & operator[](int index)
{
return collection[index];
}
T operator[](int index) const//reader
{
return collection[index];
}
bool IsExistent(T & item)
{
if (std::find(collection.begin(), collection.end(), item) != collection.end()) return true;
else false;
}
int GetIndex(T & item)
{
return find(collection.begin(), collection.end(), item) - collection.begin();
}

I will focus on the C++ parts, not necesarily the algorithm.

using namespace std;

Don't! In header files this should never appear, and in C++ files (if you must use it) use it only in local namespaces. When you add it at the top of the file like you did, it imports every symbol in std:: locally (and prevents your code - or the code of any future mantainer's) to avoid certain names.

The defines for INF and INVALID should be declared using const or constexpr and the defines for SHOW, degreesToRadians and radiansToDegrees should be declared as functions.

Consider using C++-style casts (static_cast and reinterpret_cast are what you need). The C-style casts are unsafe, they introduce hard-to-find points in code that are inflexible to change and they are impossible to parse unless you parse/compile the entire code.

Consider writing GetOrientation like this:

enum class Orientation { clockwise, counterclockwise };
static Orientation GetOrientation(Point2d const&p, Point2d const&q, Point2d const&r);

You have a comment on the return value of GetOrientation specifying what the values (1 and 2) mean. Using the enum class Orientation as a return type removes the need of the comment and keeps the code mode clear in intent (than the function returning int).

IsValid returns a boolean, so the code could be written like this:

bool IsValid()
{
    return x!=INVALID && y!=INVALID;
}

You can write the Point2D::operator== as:

bool operator==(const Point2d & point) const
//                                     ^^^^^ <- notice the const
{
    return std::tie(x, y) == std::tie(point.x, point.y);
//  and tie the values in a tuple, performing single comparison
}

You can write operator!= to return !(*this == point);. This has two advantages:

-
if you want to change what it means for points to be equal and different, you only change one operator (this is much better for future maintenance)

-
you make it clear to whoever is reading the code that the two operations are opposite.

Collection should be called PointsCollection (or similar) and not be defined as a template.

You define it as a template, but it is specifically implemented for Point2D instances in various places.

Even better, you can discard the class completely and use a std::vector The underlying std::vector already implements all the functionality you added here.

Consider splitting your code into multiple files (instead of using #pragma region for separating code areas).

Also consider splitting your classes into declaration and definition.

Your pattern of copy construction and assignment operator using a private Copy function should probably be re-written using the copy&swap idiom (it is less code to write/maintain, exception safe and minimalistic).

StackExchange Code Review Q#99246, answer score: 7