1

I have some C++ classes with geometric figures where I need to overload the comparison operators (such as >, <, =, etc..) and the output operator << in a way that every time I need to print or compare any instance of these classes, I refer to the value of their area (which I get using getArea()).

I tried to place those operator overloads in the parent class GeometricShape but I got all kinds of compiler errors which made no much sense to me.

If I place the operator overloads in each child class it works fine, but it's quite ugly and not ideal (a lot of code repetition).

How can I remove this code repetition and move the operators overloading to the parent class?

using namespace std;

#include <iostream>
#include <stdlib.h>
#include <math.h>

class GeometricShape
{
    public:

    double getArea();
};

class Poligon : public GeometricShape
{
    protected:

    double base;
    double height;

    public:

    Poligon(double base, double height) : base(base), height(height) {}
    Poligon(float base, float height) : Poligon(double(base), double(height)) {}
    Poligon(int base, int height) : Poligon(double(base), double(height)) {}

    double getBase()
    {
        return this->base;
    }

    double getHeight()
    {
        return this->height;
    }

    void setBase(double b)
    {
        this->base = b;
    }

    void setHeight(double a)
    {
        this->height = a;
    }
};


class Rectangle : public Poligon
{
    public:

    Rectangle(double base, double height) : Poligon(base, height) {}
    Rectangle(float base, float height) : Poligon(base, height) {}
    Rectangle(int base, int height) : Poligon(base, height) {}

    double getArea()
    {
        return base * height;
    }

    // HOW TO MOVE THIS CODE INTO GeometricShape and reuse it???
    friend ostream& operator<<(ostream& out, Rectangle &obj)
    {
        out << obj.getArea();
        return out;
    }
    friend bool operator==(Rectangle &obj1, Rectangle &obj2)
    {
        return obj1.getArea() == obj2.getArea();
    }
    friend bool operator>(Rectangle &obj1, Rectangle &obj2)
    {
        return obj1.getArea() > obj2.getArea();
    }
    friend bool operator<(Rectangle &obj1, Rectangle &obj2)
    {
        return obj1.getArea() < obj2.getArea();
    }
    friend bool operator <= (Rectangle &obj1, Rectangle &obj2)
    {
        return obj1.getArea() <= obj2.getArea();
    }
    friend bool operator >= (Rectangle &obj1, Rectangle &obj2)
    {
        return obj1.getArea() >= obj2.getArea();
    }
};

class Triangle : public Poligon
{
    public:

    Triangle(double base, double height) : Poligon(base, height) {}
    Triangle(float base, float height) : Poligon(base, height) {}
    Triangle(int base, int height) : Poligon(base, height) {}

    double getArea()
    {
        return (base * height)/2;
    }
    
    // HOW TO MOVE THIS CODE INTO GeometricShape and reuse it???
    friend ostream& operator<<(ostream& out, Triangle &obj)
    {
        out << obj.getArea();
        return out;
    }
    friend bool operator==(Triangle &obj1, Triangle &obj2)
    {
        return obj1.getArea() == obj2.getArea();
    }
    friend bool operator>(Triangle &obj1, Triangle &obj2)
    {
        return obj1.getArea() > obj2.getArea();
    }
    friend bool operator<(Triangle &obj1, Triangle &obj2)
    {
        return obj1.getArea() < obj2.getArea();
    }
    friend bool operator <= (Triangle &obj1, Triangle &obj2)
    {
        return obj1.getArea() <= obj2.getArea();
    }
    friend bool operator >= (Triangle &obj1, Triangle &obj2)
    {
        return obj1.getArea() >= obj2.getArea();
    }
};

class Circle : public GeometricShape
{
    double radius;

    public:

    Circle(double radius) : radius(radius) {}

    double getArea()
    {
        return (radius * radius) * M_PI;
    }

    double getRadius()
    {
        return this->radius;
    }

    void setRadius(double r)
    {
        this->radius = r;
    }

    // HOW TO MOVE THIS CODE INTO GeometricShape and reuse it???
    friend ostream& operator<<(ostream& out, Circle &obj)
    {
        out << obj.getArea();
        return out;
    }
    friend bool operator==(Circle &obj1, Circle &obj2)
    {
        return obj1.getArea() == obj2.getArea();
    }
    friend bool operator>(Circle &obj1, Circle &obj2)
    {
        return obj1.getArea() > obj2.getArea();
    }
    friend bool operator<(Circle &obj1, Circle &obj2)
    {
        return obj1.getArea() < obj2.getArea();
    }
    friend bool operator <= (Circle &obj1, Circle &obj2)
    {
        return obj1.getArea() <= obj2.getArea();
    }
    friend bool operator >= (Circle &obj1, Circle &obj2)
    {
        return obj1.getArea() >= obj2.getArea();
    }
};

Francesco Borzi
  • 56,083
  • 47
  • 179
  • 252
  • 2
    `getArea` should be virtual, and the operators should be based on (and friended to) `GeomatricShape`. The derivations of said-same (i.e. `Poligon`, `Rectangle`, etc.) then provide virtual overrides of `getArea`. I strongly suspect a [classic slicing problem](https://stackoverflow.com/questions/274626/what-is-object-slicing) is going to be in your future, btw, if you're planning on collecting these things. – WhozCraig Jun 22 '21 at 10:42
  • 2
    *"HOW TO MOVE THIS CODE INTO GeometricShape"* → don't. Move it out and make it a freestanding function. – spectras Jun 22 '21 at 10:44
  • 1
    Missing `const` BTW. – Jarod42 Jun 22 '21 at 10:46
  • 1
    this would be easier if you show the code that does not work together with the compiler error message. It doesn't make sense to you, but compiler error messages are holding lots of useful information and we could help you to understand it. – 463035818_is_not_an_ai Jun 22 '21 at 10:48
  • As a sidenote, understanding your use case would also help. You might not even need inheritance in the first place. – spectras Jun 22 '21 at 10:49
  • If you are using C++20 you can implement the space ship operator `<=>` instead of all those individual operators. Still I have doubts if basing equality on the area only is really suitable – triangles and circles are fundamentally different things... – Aconcagua Jun 22 '21 at 10:59

1 Answers1

3

There are quite a few things that need to be fixed with this code.

An external freestanding operator< should give no compilation errors:

bool operator<(GeometricShape& s1, GeometricShape& s2) {
    return s1.getArea() < s2.getArea();
}
  1. You should mark both the arguments to operator< as const. This would require marking all the instances of getArea const as well.
  2. getArea should be marked virtual in GeometricShape. All the subclasses should mark the method with override as well. Otherwise, getArea from GeometricShape will be called.
  3. In Geometric shape you should make the getArea() function pure virtual.
  4. To make sure that the objects of derived classes have the correct destructors called on them, you should mark the destructor in GeometricShape as virtual.

This would make your method signature look something like:

For GeometricShape:

virtual double getArea() const = 0;

For derived classes:

double getArea() const override;

EDIT: There were quite a lot of suggestions in the comments on how to improve this piece of code. I have tried my best to summarise them here.

Rajat Jain
  • 452
  • 1
  • 6
  • 16
  • 1
    2. It is sufficient to declare it as `virtual` in the base. 3. I dont understand what you mean by that at all. – 463035818_is_not_an_ai Jun 22 '21 at 10:51
  • 3
    the `operator<<` is a bit odd, `bool operator<<(GeometricShape& s1, GeometricShape& s2)` should be `std::ostream& operator<<(std::ostream& out, GeometricShape const& gs)` EDIT1: Or maybe you wanted an `operator<` EDIT2: still needs `const` on those inputs – ben10 Jun 22 '21 at 10:52
  • 1
    I think you confused `<` with `<<` for the example – 463035818_is_not_an_ai Jun 22 '21 at 10:53
  • @463035818_is_not_a_number you're right, but explicitly marking the methods as virtual is a good practice. I'll fix the operator<< overload. I thought OP wanted to override that. – Rajat Jain Jun 22 '21 at 10:56
  • 1
    @RajatJain it is good practice to use `override` on methods that override a base class method, and then adding `virtual` adds no additional information – 463035818_is_not_an_ai Jun 22 '21 at 10:57
  • @463035818_is_not_a_number agreed, `override` should be added too. – Rajat Jain Jun 22 '21 at 11:01
  • 3
    Also, there should be a virtual destructor in `GeometricShape`, `virtual ~GeometricShape() = default;` – Daniel Dearlove Jun 22 '21 at 11:02
  • Maybe one small one, for the sake of completion: make these operators non-member`friend`s of `GeometricShape` as OP had them. Helps reduce lookup and may improve build times. – ben10 Jun 22 '21 at 11:15
  • @ben10 without checking the content of your suggestion, the motivation is wrong. You do not alter your design based on hypothetical minor variations in build time. – spectras Jun 22 '21 at 13:16
  • @spectras I may be wrong but I'm here to learn. so, should the `operator<<` for `GeometricShape` participate in ADL for other types? to me it kind of makes sense to just make those operators `friend`s of that base class and let them them be defined inside of `GeometricShape`. would this affect the design? I may be missing something so I'm genuinely curious – ben10 Jun 22 '21 at 14:09
  • @ben10> what do you think? Is there a design difference between a friend and a non-friend function? Does it make a difference when maintaining the code and when checking the invariants of the class? – spectras Jun 22 '21 at 14:24
  • @spectras this is my opinion, and applies only in this case here, but, with the correct signature for an `operator<<`, i.e. `friend std::ostream& operator<<(std::ostream& out, GeometricShape const& gs) { /* code */ return out; }` - same for `< `(defined in the header of the `GeometricShape`), since the object is const, invariants are kept and the code localized for humans and for the compiler. I can see why `friend` seems bad (access to private) but I sure can't shake the feeling that it fits so well for operators like these. ever encounter bad coupling issues with these operators + friend? – ben10 Jun 22 '21 at 14:34
  • 1
    @ben10> generally speaking, gratuitous `friend` is kinda like missing `const`. But that was not my point. My point is this is a change in the design, and such a change must not be motivated by hypothetical variations in build time. I did not write your suggestion was necessarily wrong, but that the stated motivation was. – spectras Jun 22 '21 at 14:41