user11498510
user11498510

Reputation:

How should I write the constructor for my class

I have a container class to which i can send a Geometry object as constructor argument.

Geometry is a polymorphic class as interface for other geometric types like Sphere and Rectangle.

My question is that in Container class the constructor with arguments "Container(std::string str, Geometry* geometry)" can i code this in more flexible manner.

Whenever i will add a new SubClass to Geometry than i would need to write another IF condtion in Container class Constructor.

include "pch.h"
#include <iostream>
#include<fstream>
#include "Container.h"
#include "Geometry.h"
#include "Sphere.h"
#include "Rectangle.h"
#include "Container.h"

int main()
{
    const char* fileName = "saved.txt"; 
    Sphere sph;
    Rectangle rect;
    Container contSphere("ABC", &sph);
    Container contRectangle("DEF", &rect);
    Sphere* s = (Sphere*)contSphere.getGeomtry();
    s->PrintGeom();
    Rectangle* r = (Rectangle*)contRectangle.getGeomtry();
    r->PrintGeom(); 
    do
    {
        std::cout << '\n' << "Press a key to continue...";
    } while (std::cin.get() != '\n');
}

///////////////////////////////////////////////////////////////////////////////////////////////

#pragma once
#include <string>
class Geometry
{
private:
    std::string stdstringGeom;
    std::string stdstrType;

public:

    Geometry() : stdstringGeom("GeometyrString"), stdstrType("Geometry") {}
    virtual ~Geometry() {}


    virtual std::string getType()
    {
        return stdstrType;
    }

     virtual void  PrintGeom()
    {
        std::cout << "geometry virtual function";
    }

};

/////////////////////////////////////////////////////////////////////////////////

#pragma once

#include "Geometry.h"
class Sphere : public Geometry
{
private:
    std::string stdstrSphere;
    std::string stdstrType;


public:
    Sphere() : Geometry() ,  stdstrSphere( "DefaultSphere") , stdstrType("Sphere") {}
    Sphere( std::string str) : Geometry() ,  stdstrSphere(str) , stdstrType("Sphere"){}
    void PrintGeom()
    {
        std::cout << "Sphere Virtual Function" << std::endl;
    }

    std::string getType()
    {
        return stdstrType;
    }

};

///////////////// Defination for Constructor class////////////////////

#include "Geometry.h"
#include "Sphere.h"
#include "Rectangle.h"

class Container
{
private:
    std::string stdstrCont;
    Geometry* geom;

public:
    Container() : stdstrCont("NoName") { geom = new Geometry; }
    Container(std::string str, Geometry* geometry) : stdstrCont(str)
    {
    // I am doing this to avoid slicing and i want to do a deep copy.   
     if (geometry->getType() == "Sphere")
        {
            Sphere* sph = (Sphere*)geometry;
            geom = new Sphere(*sph);
        }
        else if (geometry->getType() == "Rectangle")
        {
            Rectangle* rec = (Rectangle*)geometry;
            geom = new Rectangle(*rec);

        }
    }

    ~Container()
    {
        if (geom != nullptr)
            delete geom;
    }

    Geometry* getGeomtry()
    {
        return geom;
    }

    void PrintContainer()
    {
        std::cout << stdstrCont;
    }
};

Upvotes: 0

Views: 140

Answers (1)

Peter
Peter

Reputation: 36597

Your design is all backward, and a consequence is that you are making the Container responsible for working out the type of all Geometry objects passed to it, in order to copy them. That will make your Container a maintenance nightmare - if someone creates another class derived from Geometry, they can forget to modify Container accordingly.

You've also omitted code that is relevant to your question (like virtual destructors) and included code that is irrelevant to the question (declaration of std::string members, and initialisation of them in constructors, and other virtual functions).

Instead, it would be better to make the class Geometry, and its derived classes, responsible for copying themselves.

At heart would be the Geometry class itself

 #include <memory>     // for std::unique_ptr
 class Geometry
 {
    public:
        Geometry() {};
        virtual ~Geometry() = default;

         virtual std::unique_ptr<Geometry> Clone() const = 0;
 };

(I've omitted the std::string members for convenience). The derived classes then override the Clone() function, viz;

 class Sphere: public Geometry
 {
    public:
        Sphere() : Geometry() {};
         ~Sphere() = default;

         std::unique_ptr<Geometry> Clone() const {return std::unique_ptr<Geometry>(new Sphere(*this));}; 
 };

 // similarly for other shapes

The Clone() function is pure virtual in Geometry, so the derived classes cannot be instantiated unless you remember to override it.

The responsibility for cloning rests with Geometry and its derived classes - and the compiler will helpfully diagnose an error if a class is derived from Geometry that does not override the Clone() function.

Then, all a Container needs to do is have a member std::unique_ptr<Geometry> or (if you intend to have a set of them) a std::vector<std::unique_ptr<Geometry> >. For the case where the Container only needs a single Geometry, the definition of Container might be

 class Container
 {
       public:
            Container() : geom() {};
            Container(Geometry *p) : geom(p->Clone()) {};
            Container(const Container &c) : geom(c.geom->Clone()) {};
            Container &operator=(const Container &c)
            {
                  geom = c.geom->Clone();   // this will release the existing c.geom
                  return *this;
            };
            ~Container() = default;
       private:

             std::unique_ptr<Geometry> geom;
 };

The reason I've used std::unique_ptr<Geometry> in the above (instead of Geometry * as in your code) is that std::unique_ptr<> avoids the need to explicitly decide when to destroy a Geometry object.

The no-argument constructor of Container initialises to containing a null Geometry (i.e. a null pointer).

The constructor of Container that accepts a Geometry * clones the passed object, and does not assume ownership of it. This means the caller is responsible for the lifetime of the object it passes. This is consistent with your main(), which constructs objects of automatic storage duration.

Note that I'm following the "rule of three" - if a non-default version of a copy constructor, copy assignment, or destructor is defined, then the other two should also be defined. The operator=() in the above works in a manner consistent with the copy constructor (i.e. it clones objects in the Container, rather than causing Geometry objects to be shared between Containers). The destructor is explicitly defined as the default, since the Container is not explicitly managing lifetime of its members (the destructor of std::unique_ptr does the work).

Upvotes: 2

Related Questions