0
\$\begingroup\$

I have a requirement of a templated clone() method which Base classes can implement through an ICloneable interface and all the derived classes will automatically get a pure virtual clone<T>() method to override. I know this is confusing, so below is my code which probably explains it better.

// ICloneable interface
template <class T>
class ICloneable
{
public:
    // smart pointer based polymorphic clone method 
    // of template parameter T
    virtual std::unique_ptr<T> clone() const = 0;
};

// Any class deriving from ICar would need to 
// implement std::unique_ptr<ICar> clone()
// and std::string name()

class ICar : public ICloneable<ICar>
{
public:
    virtual std::string name() = 0;
    virtual ~ICar() {}
};

// Derived car object implementing ICar interface
class NamedCar : public ICar
{
private:
    std::string _name;
public:
    NamedCar() {_name = "This car does not have a name";}
    NamedCar(std::string name) { _name = name; }

    virtual ~NamedCar() {}
    // override
    virtual std::string name() { return _name; }

    // override
    virtual std::unique_ptr<ICar> clone() const
    {
        return std::unique_ptr<ICar>(new NamedCar(*this));
    }
};

class MercCar : public ICar
{
public:
    MercCar() {}
    virtual ~MercCar() {}
    // override
    virtual std::string name() { return "Merc"; }
    // override
    virtual std::unique_ptr<ICar> clone() const 
    {
        return std::unique_ptr<ICar>(new MercCar(*this));
    }
};


// Any class deriving from IBicycle would need to 
// implement std::unique_ptr<IBicycle> clone()
// and void describe()

class IBicycle : public ICloneable<IBicycle>
{
public:
    virtual ~IBicycle() {}
    virtual void describe() =  0;
};

class MyBicycle : public IBicycle
{
public:
    MyBicycle() {}

    virtual void describe() 
    {
        std::cout << "This is my bicycle" << std::endl;
    }

    virtual std::unique_ptr<IBicycle> clone() const
    {
        return std::unique_ptr<IBicycle>(new MyBicycle(*this));
    }
    virtual ~MyBicycle() {} 
};

template <typename T>
class MyGarage
{
private:
    T _data;
    std::unique_ptr<ICar> _car_ptr;
    std::unique_ptr<IBicycle> _bicycle_ptr;
public:
    MyGarage(T& data, const std::unique_ptr<ICar>& cp, std::unique_ptr<IBicycle>& bp) : _data(data)
    {
        _car_ptr = cp->clone();
        _bicycle_ptr = bp->clone();
    }

    MyGarage(const MyGarage& source) : data(source._data)
    {
        _car_ptr = source._car_ptr->clone();
        _bicycle_ptr = source._bicycle_ptr->clone();
    }

    MyGarage& operator=(const MyGarage& source)
    {
        if (this != &source)
        {
            _data = source.data;
            _car_ptr = source._car_ptr->clone();
            _bicycle_ptr = source._bicycle_ptr->clone();
        }
    return *this;
    }

    std::unique_ptr<ICar> car_ptr()
    {
        return _car_ptr->clone();
    }

    std::unique_ptr<IBicycle> bicycle_ptr()
    {
        return _bicycle_ptr->clone();
    }

    void car_ptr(const std::unique_ptr<ICar>& cp)
    {
        _car_ptr = cp->clone();
    }

    void bicycle_ptr(const std::unique_ptr<IBicycle> bp)
    {
        _bicycle_ptr = bp->clone();
    }

    T data()
    {
        return _data;
    }

    void data(const T& data)
    {
        _data = data;
    }
};

int main()
{
    // Check whether the clone method works for IBicycle and ICar
    std::unique_ptr<IBicycle> p_bike;

    MyBicycle mybike;
    p_bike = mybike.clone();
    p_bike->describe();

    std::unique_ptr<ICar> p_car;
    NamedCar n_car("Ford");

    NamedCar kia_car("Kia");
    MercCar my_car;
    p_car = n_car.clone();
    std::cout << p_car->name() << std::endl;
    p_car = kia_car.clone();
    std::cout << p_car->name() << std::endl;
    p_car = my_car.clone();
    std::cout << p_car->name() << std::endl;
    p_car = n_car.clone();
    std::cout << p_car->name() << std::endl;
    NamedCar unnamed_car;
    p_car = unnamed_car.clone();
    std::cout << p_car->name() << std::endl;
    int x = 123;

    // Check the functionality of MyGarage class
    MyGarage<int> garage(x, p_car, p_bike);

    std::cout << garage.car_ptr()->name() << std::endl;

    garage.bicycle_ptr()->describe();

    garage.car_ptr(n_car.clone());
    std::cout << garage.car_ptr()->name() << std::endl;
    garage.car_ptr(my_car.clone());
    std::cout << garage.car_ptr()->name() << std::endl;
    int newx = x + 982;
    std::cout << garage.data() << std::endl;
    garage.data(newx);
    std::cout << garage.data() << std::endl;
}

The output is:

This is my bicycle
Ford
Kia
Merc
Ford
This car does not have a name
This car does not have a name
This is my bicycle
Ford
Merc
123
1105

which I expected.

However, I wanted somebody's feedback on this i.e. whether this is a good design for such a problem, especially the MyGarage class - where I have to expose a setter for the internal unique_ptr, which I am not sure is a good idea. Also, is there a possibility of this design unknowingly leaking memory?

\$\endgroup\$
  • 1
    \$\begingroup\$ Clone is bad design pattern in C++ (it works better in languages like Java). In C++ we have better ways of doing things (can't tell you what they are because you don't describe the problem you are trying to solve (you describe a solution)). But reading your code you have the classic implementation of clone(). As such it is fine. But I would bet there is a much better way of doing it if only you told us the real problem. \$\endgroup\$ – Martin York Oct 10 '14 at 17:03
  • \$\begingroup\$ Can you explain why cloning is a bad design in C++? \$\endgroup\$ – Kapil Sharma Oct 10 '14 at 17:58
  • \$\begingroup\$ Its bad because you don't actually need it (in 99.9% of the use cases). It is why we have copy constructors and references. Your use case may require it (but I doubt it) but until you describe the problem rather than the solution Its impossible to tell what the best solution is. \$\endgroup\$ – Martin York Oct 10 '14 at 19:35
1
\$\begingroup\$

Use the Copy and Swap Idiom for assignment operators. This is suseptable to failure if one of the clone() operations fails and throws an exception (you will be left in a non consistent state and thus have not provided the strong exception guarantee)

    MyGarage& operator=(const MyGarage& source)
    {
        if (this != &source)
        {
            _data = source.data;
            _car_ptr = source._car_ptr->clone();
            _bicycle_ptr = source._bicycle_ptr->clone();
        }
    return *this;
    }

So: If I fetch the car out of the garage.

    std::unique_ptr<ICar> car_ptr()
    {
        return _car_ptr->clone();
    }

I don't get my car I get a clone of my car. So while I am about and about driving the country side there is still a car in my garage that my wife can use. That's a nice trick. Not sure this is really the semantics you want (it could be but hard to tell without knowing the problem).

Same issue as car_ptr() but why would anybody clone a bike when there is always a car available.

    std::unique_ptr<IBicycle> bicycle_ptr()
    {
        return _bicycle_ptr->clone();
    }

Yep the whole point of unique_ptr is to transfer ownership.

    void car_ptr(const std::unique_ptr<ICar>& cp)
    {
        _car_ptr = cp->clone();
    }

This is not transferring ownership. It retains ownership but clones a new car into the garage.

While we are here those names are truly terrible. The fact that you have _ptr in the name. I would expect something like.

    std::unique_ptr<ICar>  getCar() // retrieves car from the gurage.
    {                               // The return type of unique ptr means
                                    // there is an ownership transfer thus it
                                    // will actually remove the car from the
                                    // gurage. So it will have no car after the get
        return std::move(_car_ptr);
    }

    void putCar(std::unqiue_ptr& car)  // Passing a unique_ptr to a function
    {                                  // As an indication you are trying to give
                                       // the object ownership. In C++11 we also
                                       // have move semantics so it should probably
                                       // be passed by r-value ref but this works
                                       // because it is not a const reference.
        _car_ptr = std::move(car);
    }

Here you are return a copy of the data.
If the type T is big that can be a pain.

    T data()
    {
        return _data;
    }

Normally when you provide access to members (which is usually a bad idea as it break encapsulation) you return it by reference. You can do it two ways by const reference or normal reference (or usually both).

    T& data()
    {
        return _data;
    }
    T const& data() const
    {
        return _data;
    }

If the user wants a copy. Then all they have to do is assign it to a local variable and you will get a copy. But if they want to manipulate the object in the garage without a copy then you can also do that (so best of both worlds).

\$\endgroup\$
  • \$\begingroup\$ I understand the points that you made here. Actually, my actual problem is slightly complex. I basically have "service proxy" objects and I am trying to wrap them in a ProxyInfo class. Each service proxy has a metadata and presence checker (something that pings the service continuously) associated with it. So, just replace Garage with a ProxyInfo class, ICar with IMetadata, and IPresenceVerifier with IBicycle and you have the problem that I am trying to solve. I don't want to move the metadata objects but just give a copy of it to the client (same for IPresenceVerifier). \$\endgroup\$ – Kapil Sharma Oct 10 '14 at 17:55
  • \$\begingroup\$ I know the example I posted might not have been the best. Please let me know in case you want more info. Agree with your implementation of data() function. \$\endgroup\$ – Kapil Sharma Oct 10 '14 at 17:57
  • \$\begingroup\$ I see nothing in that description where you require a clone. I have still no idea why you think you need to copy the object (note objects in C++ are not like objects in Java (not even close we have much better fine grain resource management)). I think you need to start from scratch with a new question. In that question describe the problem (not how you have implemented it (as you keep describing implementations)). Then show us the code you have used to solve the problem. Then we can comment on how good a method it is for solving the problem. \$\endgroup\$ – Martin York Oct 10 '14 at 19:41
  • \$\begingroup\$ Also this is worth a read: stackoverflow.com/q/228783/14065 \$\endgroup\$ – Martin York Oct 10 '14 at 19:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.