Overloaded << operator returning wrong value (C++)

Question

I'm a beginner when it comes to C++ and have recently ran in to a very frustrating problem with my small program where I'm practicing operator overloading and templates.

I've created a template-class called SortedVector that can store instances of various types.

using namespace std;

template <class T, int size> class SortedVector {
public:
    SortedVector();
    bool add(const T& v);
    T& median();
    void sortArray();
    void removeLarge(const T& v);
    void print(ostream &os);
    void compexch(T& x, T& y);
    void sortArray(T* data, int s);

private:
    T arr[size];
    int arraySize;
};

template <class T, int size> SortedVector<T, size>::SortedVector() {


    arraySize = 0;

    for (int i = 0; i < size; i++) {
        arr[i] = T();
    }
}

template <class T, int size> bool SortedVector<T, size>::add(const T& v) {

    if (arraySize > size - 1) {
        cout << "Array is full!" << endl;
        return false;
    } else {

        arr[arraySize] = v;
        arraySize++;

        sortArray(arr, arraySize);
    }

    return true;
}

template <class T, int size> void SortedVector<T, size>::sortArray(T* data, int s) {
    for (int i = 0; i < s - 1; i++) {
        for (int j = i + 1; j < s; j++) {
            compexch(data[i], data[j]);
        }
    }
}

template <class T, int size > T & SortedVector<T, size>::median() {

}

template <class T, int size> void SortedVector<T, size>::removeLarge(const T & v) {

}

template <class T, int size> void SortedVector<T, size>::print(ostream & os) {


    for (int i = 0; i < arraySize; i++) {
        cout << arr[i] << endl;
    }
}

template <class T, int size> inline void SortedVector<T, size>::compexch(T& x, T& y) {
    if (y < x) {
        T temp = x;
        x = y;
        y = temp;
    }
}

It can store ints succesfully and it can also store Polygons (a custom made class created in a earlier assignment).

Polygon.h:

class Polygon {
public:

    Polygon(Vertex vertexArray[], int size);
    Polygon() : vertices(0), arraySize(0) {}
    ~Polygon() {delete[] vertices;}
    void add(Vertex v);
    float area();
    int minx();
    int maxx();
    int miny();
    int maxy();
    int numVertices() const {return arraySize;}
    friend ostream &operator << (ostream &output, const Polygon& polygon);
    friend bool operator > (Polygon polygon1, Polygon polygon2);
    friend bool operator < (Polygon polygon1, Polygon polygon2);

private:

    int arraySize;
    Vertex * vertices;

};

Polygon.cpp declaration:

using namespace std;

void Polygon::add(Vertex v) {
    arraySize++;
    Vertex * tempVertexes = new Vertex[arraySize];
    for (int i = 0; i < arraySize; i++) {
        if (i == arraySize - 1) {
            tempVertexes[i] = v;
        } else {
            tempVertexes[i] = vertices[i];
        }
    }
    delete [] vertices;
    vertices = tempVertexes;

}

Polygon::Polygon(Vertex vertexArray[], int size) {
    arraySize = size;
    vertices = new Vertex[size];

    for (int i = 0; i < size; i++) {
        vertices[i] = vertexArray[i];
    }
}

float Polygon::area() {
    float area = 0.0f;

    for (int i = 0; i < arraySize - 1; ++i) {
        area += (vertices[i].getXposition() * vertices[i + 1].getYposition()) - (vertices[i + 1].getXposition() * vertices[i].getYposition());
    }

    area += (vertices[0].getYposition() * vertices[arraySize - 1].getXposition()) - (vertices[arraySize - 1].getYposition() * vertices[0].getXposition());

    area = abs(area) *0.5;

    return area;
}



ostream& operator<<(ostream &output, const Polygon& polygon) { //Kolla denna!


    output << "{";
    for (int i = 0; i < polygon.numVertices(); i++) {
        output << "(" << polygon.vertices[i].getXposition() << "," << polygon.vertices[i].getYposition() << ")";

    }
    output << "}";

    return output;

}

bool operator>(Polygon polygon1, Polygon polygon2) {
    if (polygon1.area() > polygon2.area()) {
        return true;
    } else {
        return false;
    }
}

bool operator<(Polygon polygon1, Polygon polygon2) {
    if (polygon1.area() < polygon2.area()) {
        return true;
    } else {
        return false;
    }
}


template <class T> inline void compexch(T& x, T& y) {
    if (y < x) {
        T temp = x;
        x = y;
        y = temp;
    }
}

The code for the Vertex class:

class Vertex {
public:
    Vertex() : y(0), x(0) {}
    Vertex(int xPosition, int yPosition) : x(xPosition), y(yPosition) {}
    ~Vertex() {}
    int getXposition() const {return x;}
    int getYposition() const {return y;}

private:
    int x;
    int y;


};

The problem however is that the overloaded <<-operator seems print out the wrong values from the main-method:

int main() {

    SortedVector<Polygon, 10> polygons;
    SortedVector<int, 6> ints;

    ints.add(3);
    ints.add(1);
    ints.add(6);

    Vertex varr[10];
    varr[0] = Vertex(0, 0);
    varr[1] = Vertex(10, 0);
    varr[2] = Vertex(5, 2);
    varr[3] = Vertex(5, 5);
    polygons.add(Polygon(varr, 4));
    cout << "varr area:" << (Polygon(varr, 4)).area() << endl;

    varr[0] = Vertex(0, 0);
    varr[1] = Vertex(25, 8);
    varr[2] = Vertex(10, 23);
    polygons.add(Polygon(varr, 3));
    cout << "var area (1):" << (Polygon(varr, 3)).area() << endl;
    varr[0] = Vertex(0, 0);
    varr[1] = Vertex(5, 0);
    varr[2] = Vertex(5, 3);
    varr[3] = Vertex(4, 8);
    varr[4] = Vertex(2, 10);
    polygons.add(Polygon(varr, 5));
    cout << "var area (2):" << (Polygon(varr, 5)).area() << endl;
    polygons.print(cout);
    ints.print(cout);

    cout << "MEDIAN: " << ints.median() << endl;
    cout << "MEDIAN: " << polygons.median() << endl;

    return 0;

}

The code that is printed is:

var area (1):247.5
var area (2):33.5
{(6029504,0)(5,0)(5,3)}
{(6029504,0)(5,0)(5,3)(4,8)}
{(6029504,0)(5,0)(5,3)(4,8)(2,10)}
1
3
6
MEDIAN: 1
MEDIAN: {(6029504,0)(5,0)(5,3)}

Firstly, the method prints out the same polygon but with varying sizes. Secondly, it points out the wrong getXPosition() for the first object in the array. Everything else (that is implemented, like the ints and the area) is correct tho. Why is this? Am I missing something important here or am I just completely of with my program?

If theres any more code needed I am happy to provide it.

Regards

Answer 1

Given the code you posted, the issues are clear as to what's wrong.

You're passing Polygon's by value here:

friend bool operator > (Polygon polygon1, Polygon polygon2);
friend bool operator < (Polygon polygon1, Polygon polygon2);

and you're copying and assigning values here in: compexch:

if (y < x) {
    T temp = x;  // copy constructor
    x = y;  // assignment
    y = temp;  // assigment
}

This means that copies will be made, and your Polygon class cannot be copied safely. You will have memory leaks and bugs when calling either of these functions.

You should implement the appropriate copy constructor and assignment operator, whose signatures are:

Polygon(const Polygon& rhs);  // copy constructor
Polygon& operator=(const Polygon& rhs); // assignment operator

Both of these functions should be implemented. Please see the Rule of 3 for this information.

However, for operator < and operator >, you should pass references, not values to these functions:

friend bool operator > (Polygon& polygon1, Polygon& polygon2);
friend bool operator < (Polygon& polygon1, Polygon& polygon2);

Then the copy constructor and assignment operator are not brought into play, since the parameter type is a reference.

---

Let's try to implement the copy / assignment functions anyway, for completeness:

For example, the copy constructor can be implemented like this:

Polygon::Polygon(const Polygon& rhs) : vertices(new int[rhs.arraySize]), 
                                       arraySize(rhs.arraySize)
{
   for (int i = 0; i < arraySize; ++i)
      vertices[i] = rhs.vertices[i];
}

Then for the assignment operator, using the copy / swap idiom:

Polygon& operator=(const Polygon& rhs)
{
    Polygon temp(rhs);
    std::swap(temp.arraySize, arraySize);
    std::swap(temp.vertices, vertices);
    return *this;
}

Once you've implemented these function, plus the destructor that calls delete[], you should no longer have an issue with copying the objects.

---

Other issues:

In addition, you really should only overload < and ==, initially with their "full" implementation, and write the other relational operators with respect to these two operators.

Right now, you're making the classic mistake of writing one operator (operator >), and then trying to turn the logic "inside-out" when implementing operator <. What if the logic for operator > were more complex, and it took yeoman's work to figure out what is the "opposite of <"?

If you implemented ==, then operator > just becomes:

return !(polygon1 < polygon2) && !(polygon == polygon2);  // <-- this can be further improved by implementing operator !=