0

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;
    }
};
ttppbb
  • 65
  • 8
  • 2
    1) Why are you using `Container`, and not simply `std::vector`? Or better yet `std::vector>`? 2) if you're going to store raw pointers in a container, your base class `Geometry` better have a virtual destructor, otherwise calling `delete geom;` leads to undefined behavior. – PaulMcKenzie Sep 14 '19 at 03:32
  • @PaulMcKenzie yes my base class does have a virtual destructor , i simplied this code and kept the relevent details only for the question. –  Sep 14 '19 at 03:40
  • 1
    You decided to eliminate one of the most important part of the code, a part of the code that could influence what answers you get and/or throw persons down the wrong path of trying to answer your question. – PaulMcKenzie Sep 14 '19 at 03:43
  • 1
    Closely related/dupe: https://stackoverflow.com/questions/5148706/copying-a-polymorphic-object-in-c – R Sahu Sep 14 '19 at 03:45
  • @PaulMcKenzie i have updated the code with virtual destructor for geometry. –  Sep 14 '19 at 03:47
  • 1
    *My question is that in Container class the constructor with arguments "`Container(std::string str, Geometry* geometry)`" is it the correct way to code it.* "correct" is a vague word, there can be different standards to determine whether something is "correct" – L. F. Sep 14 '19 at 04:11
  • @L.F i mean can i have a better method than this as it is quite raw if i add a new Subclass to the geometry than i have again change the code in container class. –  Sep 14 '19 at 04:13
  • @Lightness Races in Orbit ??? –  Sep 14 '19 at 16:06
  • 1
    @Sam The time of year when this question is posted some 20, 30 times per week :) – Lightness Races in Orbit Sep 14 '19 at 17:42

1 Answers1

2

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

Peter
  • 35,646
  • 4
  • 32
  • 74
  • Error C2440 'return': cannot convert from 'Sphere *' to 'std::unique_ptr>' –  Sep 14 '19 at 05:14
  • 1
    Yeah, okay. Modified to fix. Forgot to do an explicit conversion. – Peter Sep 14 '19 at 05:19
  • i am facing the same problem here also Container(Geometry *p) : geom(p->Clone()) {}; –  Sep 14 '19 at 05:37
  • even if i try to cast it geom = (Geometry*)geometry->Clone(); , still the error persists. –  Sep 14 '19 at 05:37
  • Or should i return Geometry* from the clone function. –  Sep 14 '19 at 05:40
  • 1
    I can't reproduce that one. All variants of the `Clone()` function I've shown return `std::unique_ptr`, and I haven't named any variable `geometry`. Don't cast to a raw pointer. – Peter Sep 14 '19 at 06:08
  • C2440 'type cast': cannot convert from 'std::unique_ptr>' to 'Geometry *' –  Sep 14 '19 at 06:09
  • on this line geom = geometry->Clone(); –  Sep 14 '19 at 06:10
  • You presumably have given `geom` a type `Geometry *`. If you look in my answer, it has a type `std::unique_ptr`. – Peter Sep 14 '19 at 06:11
  • i am talking avout this line Container(Geometry *p) : geom(p->Clone()) {}; –  Sep 14 '19 at 06:14
  • C2440 'type cast': cannot convert from 'std::unique_ptr>' to 'Geometry *' –  Sep 14 '19 at 06:16
  • If you look at the definition I gave for the `Container`, the ONLY usage of a raw pointer is for the type of argument to a constructor (`Container(Geometry *p)`). There is no other raw pointer used in that class at all - everything else uses `unique_ptr`. If you are trying to use raw pointers anywhere else, you will break the logic of what I gave you .... as, it seems, your compiler is telling you. – Peter Sep 14 '19 at 06:19
  • if i want to use as Geometry* instead of unique_ptr can i do that as i have work on existing code and it has Geometry* –  Sep 14 '19 at 06:21
  • 1
    You can, but then you need to remember to explicitly manage the lifetime of objects e.g. `delete` objects when no longer needed in the destructor of `Container` (which then cannot be a `default`). I would recommend AGAINST that, as it's easy to make an error in that, not clean up as you intend when exceptions are thrown, etc. (Before C++11, I'd recommend `std::auto_ptr` instead of `unique_ptr` - and still not use raw pointers). – Peter Sep 14 '19 at 06:43
  • 1
    Don't use `std::auto_ptr`. Ever. At all. – Lightness Races in Orbit Sep 14 '19 at 14:46