Dynamic memory management for a class hierarchy of geometric shapes

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 
#include 
#include 
#define _USE_MATH_DEFINES
#include 
#include 

#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 area() > b->area()) {
        std::cout  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;
}

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> shapes;

and

std::unique_ptr 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 (sideA);                 
         } break; // Put the break out side the case scope block
         case 1: {
             int sideA = rand() % 100;
             int sideB = rand() % 100;
             std::cout (sideA, sideB);
         } break;
         // ...
     }
     // No need for the default case
     return std::unique_ptr();
}

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 then1)

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 
#include 
#include 
#include 
#include 

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).

1As mentioned by @Tamoghna Chowdhury before.

StackExchange Code Review Q#158982, answer score: 13