Dynamic memory management for a class hierarchy of geometric shapes

Question

The task was to write function that compare areas of two random generated geometric shapes (circle, square, rectangle) using base class with virtual function.

Am I doing it right in terms of memory management?
Will my figures generated inside generateRandomShape() by new Square() etc. last in memory outside this function untill I manually delete them?

#include <iostream>
#include <cstdlib>
#include <ctime>
#define _USE_MATH_DEFINES
#include <cmath>
#include <vector>

#define M_PI       3.14159265358979323846

class Shape {
public:
    virtual float area() = 0;
};

class Square : public Shape {
protected:
    float sideA;

public:
    Square(float sideA) : sideA(sideA) {}
    float area() { return sideA * sideA; }
};

class Circle : public Shape {
protected:
    float radius;

public:
    Circle(float radius) : radius(radius) {}
    float area() { return M_PI * radius * radius; }
};

class Rectangle : public Shape {
protected:
    float sideA, sideB;

public:
    Rectangle(float sideA, float sideB) : sideA(sideA), sideB(sideB) {}
    float area() { return sideA * sideB; }
};

void compareAreasOffigures(Shape* a, Shape* b){
    if (a->area() == b->area()) {
        std::cout << "Areas of two shapes are equal." << std::endl;
    }
    else if (a->area() > b->area()) {
        std::cout << "Area of first Shape is bigger." << std::endl;
    }
    else {
        std::cout << "Area of second Shape is bigger." << std::endl;
    }
}

Shape * generateRandomShape() {
    int Shape;
    Shape = rand() % 3;

    switch (Shape)  {

        case 0: {
            int sideA = rand() % 100;
            std::cout << "Square was generated, side: " << sideA << std::endl;
            return new Square(sideA);
            break;
        }

        case 1: {
            int sideA = rand() % 100;
            int sideB = rand() % 100;
            std::cout << "Rectangle was generated, side A: " << sideA << " side B: " << sideB << std::endl;
            return new Rectangle(sideA, sideB);
            break;
        }
        case 2: {
            float radius = rand() % 100;
            std::cout << "Circle was generated, radius: " << radius << std::endl;
            return new Circle(radius);
            break;
        }
        default: return NULL;
    }
}

int main(void){

    srand(time(0));

    std::vector<Shape*> shapes;

    shapes.push_back(generateRandomShape());
    shapes.push_back(generateRandomShape());

    compareAreasOffigures(shapes[0], shapes[1]);

    for (auto v : shapes) { delete v; } //cleanup
    shapes.clear();

    return EXIT_SUCCESS;
}

Answer 1

Dynamic Memory Management

Am I doing it right in terms of memory menagment?

You should rather use smart pointers to store the Shape instances in the vector:

 std::vector<std::unique_ptr<Shape>> shapes;

and

 std::unique_ptr<Shape> generateRandomShape() {
     int Shape; // Take care! Some compilers don't like it if variables 
                // arenamed the same as defined types
     Shape = rand() % 3;

         switch (Shape)  {
         case 0: {
             int sideA = rand() % 100;
             std::cout << "Square was generated, side: " << sideA << std::endl;
             return std::make_unique<Square>(sideA);                 
         } break; // Put the break out side the case scope block
         case 1: {
             int sideA = rand() % 100;
             int sideB = rand() % 100;
             std::cout << "Rectangle was generated, side A: " << sideA << " side B: " << sideB << std::endl;
             return std::make_unique<Rectangle>(sideA, sideB);
         } break;
         // ...
     }
     // No need for the default case
     return std::unique_ptr<Shape>();
}

thus you can completetly omit

 for (auto v : shapes) { delete v; } //cleanup

Will my figures generated inside generateRandomShape() by new Square() etc. last in memory outside this function untill I manually delete them?

Yes, but handling new and delete on your own turns out to be error prone and semantically unclear.

Use virtual destructors for dynamic polymorphic inheritance

For formal correctness you should declare virtual destructors for your abstract base class and the derived classes

class Shape {
public:
    float area() = 0;
    virtual ~Shape() = default; // <<<<<<<<<<<<<<<<<<<<
};

This isn't done automatically for compiler generated destructors (see an explanation here).

Use const class members whenever appropriate

Use const whenever appropriate. The area() calculation doesn't change any of the internal states of a Shape instance, so it should be declared as a const member function:

class Shape {
public:
    float area() const = 0;
              // ^^^^^ 
};

This improves your classes to be used in wider contexts, e.g. with temporary class instances.

Also you could rewrite your function signature

void compareAreasOffigures(Shape* a, Shape* b);

like that then¹⁾

void compareAreasOffigures(const Shape* a, const Shape* b);

or even better

void compareAreasOffigures(const Shape& a, const Shape& b);

---

Note: Do that as soon as possible in your class designs, adding later is much harder than removing that constraint in a later refactoring or override.

Prefer const variable definitions over using preprocessor macros

Instead of

 #define M_PI       3.14159265358979323846

rather use

const double M_PI = 3.14159265358979323846;

or

constexpr double M_PI = 3.14159265358979323846;

for sake of type safety and clarity.

Don't use srand(), rand() with modern c++

Use a better random number generator than

srand(time(0));

// ...

rand();

There are various random generators available with the current standard in the numerics library.

Don't use == to compare floating point values

Don't use equality comparison for floating point types like

if (a->area() == b->area())

it's unlikely these will be exactly equal.

Rather test against std::numeric_limits::epsilon, there's a comprehensive example in the linked documentation.

Your purpose of using _USE_MATH_DEFINES is unclear and inconsistent

Your use of _USE_MATH_DEFINES and defining M_PI yourself is inconsistent.

The purpose of #define _USE_MATH_DEFINES is to use the predefined constants from cmath or math.h, but you define M_PI yourself anyways.

As mentioned here it should be defined before any of the #include statements to guarantee it works

#define _USE_MATH_DEFINES
#include <iostream>
#include <cstdlib>
#include <ctime>
#include <cmath>
#include <vector>

since you cannot know if any of the other header files already includes cmath or math.h.

The highlight of your solution

After so many points of critique, I want to emphasize one bonus point of your solution:

You weren't trapped by the Square is not a Rectangle confusion and provided unrelated classes for these (same for Circle is not an Ellipse).

Beginners often use inheritance like

class Rectangle {
protected:
    float sideA, sideB;
};

class Square : public Rectangle {
};

and run into trouble to keep the class member variables consistent (here's a more detailed resource).

---

1_{As mentioned by @Tamoghna Chowdhury before.}

Answer 2

@πάντα ῥεῖ's answer covers everything except something I think should have been obvious - a square is-a rectangle geometrically. Hence to go by OO design, Square should be a subclass of Rectangle if both are immutable (to avoid inconsistency among member variables), which they are in this case.

Square does not even need any of its own fields or an implementation of the area() method that way - via proper initialization in the constructor, the base class Rectangle's relevant fields and methods can handle it.

class Square : public Rectangle {
public:
    Square(float side) : Rectangle(side, side) {}
};

Note: @πάντα ῥεῖ disagreed with this, quoting this article. I'll bring the last part of the article to your attention to show that this approach is not incorrect for immutable classes:

As long as the Rectangle class is immutable, we can define subclasses that are limited to a particular subset of rectangles, such as squares. This is one more reason to prefer immutability. To the extent possible, define the public interface in terms of what an object is and what it does rather than what you can do to it. However that’s not always possible, and in those cases you need to be extremely careful around inheritance. Otherwise constraints can be violated when you least expect, thus introducing subtle and potentially dangerous bugs in your code.

_{Emphasis mine}

As far as I can see, combined with @πάντα ῥεῖ's suggestion for const member variables, your Shape subclasses are all effectively immutable, and hence my advice follows through. See the explicit absence any properties, setters or public fields - that implies your classes are immutable; and hence, a square is-a rectangle.

Now, if in the future you need to make your Shape classes mutable, please do follow @πάντα ῥεῖ's advice and make Square an implementation of the highest common interface, Shape, instead of a subclass of Rectangle. However, as long as they are immutable, this approach is both valid mathematically and OOP-ly (satisfies the Liskov Substitution Principle and the Single Responsibility Principle).

Other tiny nitpicks:

1. Minor typo: compareAreasOffigures should be compareAreasOfFigures to be consistent camelCase.

2. compareAreasOffigures(Shape* a, Shape* b) could be compareAreasOffigures(const Shape& a, const Shape& b), using const references instead of pointers. Gets you some syntax sugar for free, too.

   The whole method then becomes:

   void compareAreasOfFigures(const Shape& a, const Shape& b){
       // 5 digits of precision should be enough, go up to 7 if required
       if (almostEqual(a.area(), b.area(), 5)) {
           std::cout << "Areas of two shapes are equal." << std::endl;
       }
       else if (a.area() > b.area()) {
           std::cout << "Area of first Shape is bigger." << std::endl;
       }
       else {
           std::cout << "Area of second Shape is bigger." << std::endl;
       }
   }
   

   You'll need this bunch of imports and a template function:

   #include <limits>
   #include <type_traits>
   #include <algorithm>
   
   template<class T>
   typename std::enable_if<!std::numeric_limits<T>::is_integer, bool>::type
       almostEqual(T x, T y, int ulp)
   {
       // the machine epsilon has to be scaled to the magnitude of the values used
       // and multiplied by the desired precision in ULPs (units in the last place)
       return std::abs(x-y) < std::numeric_limits<T>::epsilon() * std::abs(x+y) * ulp
       // unless the result is subnormal
              || std::abs(x-y) < std::numeric_limits<T>::min();
   }
   

   _{Code taken from http://en.cppreference.com/w/cpp/types/numeric_limits/epsilon as was suggested by @πάντα ῥεῖ}