C++ returning MyClass* vs MyClass

Question

I have a template class I am creating and I need a method that returns an object of that class and assigns it. I have a method that takes in 2 objects, creates a new one, and returns it, as well as an overload of the assignment operator to copy members correctly.

I have tried to do this 2 ways (both methods are static).

Method 1: Works, but isn't this bad practice?

template <class T>
MyClass<T>* MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
    MyClass<T> *newObject= new MyClass<T>(a.member, b.member2);

    //...do stuff to the object

    return newObject;
}

Method 2: Doesn't work, but isn't this a better line of thought?

template <class T>
MyClass<T> MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
    MyClass<T> newObject(a.member, b.member2);

    //...do stuff to the object

    return newObject;
}

My fear with method 1 is that I am creating a new object in the heap and not ever destroying it. I am assigning all of its members to another instance of MyClass, but what happens to the first instance if I perform the operation a second time? Does the first "new" object I created on the stack just sit there?

The problem I have with method 2 is that the object is going out of scope before my assignment operation overload is called. I looked into copy constructors and move constructors. Unfortunately, I do not see those being called in this situation (probably a major lack of experience here).

For completion, here is the remaining portions of relevant code.

template <class T>
void MyClass<T>::operator = (const MyClass& b)
{
    if (this == &b) { return; }

    DeleteData();
    //just calls the same member removal operation as the destructor
    //to clear everything out to receive new data

    //...simple member copying stuff
}

template<class T>
inline void MyClass<T>::DeleteData()
{
    //call delete on members which need to be deleted manually
}

template <class T>
MyClass<T>::~MyClass()
{
    DeleteData();
}

template <class T>
MyClass<T>::MyClass(const T member, const T member2)
{
    //member initialization operations
}

EXAMPLE USAGE:

MyClass<T> myObj1(member1, member 2);

MyClass<T> myObj2(member1, member 2);

MyClass<T> myObj3 = *(MyClass<T>::MyFunction(myObj1, myObj2));

TL;DR Question:

When using new inside a function to return a class* instead of a class is the created object destroyed when it no longer has any references?

If not, what is the best practice to return an object that is created in a method?

If so, is this method a bad practice?

I apologize if this has been answered before, I am new enough to C++ that my previous researched has not yielded me a satisfactory answer, likely due to me not knowing what I don't know.

IMPLEMENTED SOLUTION: (I also added a copy constructor. Without it, this solution still worked for the scope of the question, but there were some items beyond this question's scope that were fixed by one. Hopefully someone in the future might find this info useful)

//this is a static function of MyClass
template <class T>
std::unique_ptr<MyClass<T>> MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
    std::unique_ptr<MyClass<T>> newObj = std::make_unique<MyClass<T>>(a.member, b.member2);

    //...Be the factory...

    return newObj ;
}

int main()
{
    MyClass<T> myObj1(member1, member 2);

    MyClass<T> myObj2(member1, member 2);

    MyClass<T> myObj3 = *(MyClass<T>::MyFunction(myObj1, myObj2));
}

Answer 1

When using new inside a function to return a class* instead of a class is the created object destroyed when it no longer has any references?

It is not. Anything passed to new must be explicitly deleted, or passed to a container that assumes ownership of it (e.g. unique_ptr).

If so, is this method a bad practice?

It depends, but typically, yes, returning and dealing with raw pointers in general is discouraged as it is prone to memory leaks. The code you have above indeed leaks (as you described).

If not, what is the best practice to return an object that is created in a method?

It depends. If you don't mind copying, or if your object has move semantics defined, you can return a concrete object like in method 2 and hope the compiler performs return value optimization.

If you do mind copying, then consider returning a smart pointer of some sort:

template <class T>
std::unique_ptr<MyClass<T>> MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
  auto newObject = std::unique_ptr<MyClass<T> {new MyClass<T>(a.member, b.member2)};
  //...do stuff to the object
  return newObject;
}

Answer 2

How about this:

template <class T>
std::shared_ptr<MyClass<T>> MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
    auto newObject = std::make_shared<MyClass<T>>(MyClass<T>(a.member, b.member2));

    //...do stuff to the object

    return newObject;
}

This is the right way to do it in modern C++, if you ever think about returning pointers. The shared pointer will clean everything for you, and you'll not be passing copies. Assuming you don't want to use a move constructor.

EDIT:

Since people criticized shared_ptr a lot in the comments, because it's killing a fly with a bazooka (and I agree), I offer also the same with unique_ptr. Just be aware that unique_ptr is a unique object that must be moved, not copied.

template <class T>
std::unique_ptr<MyClass<T>> MyClass<T>::MyFunction(const MyClass& a, const MyClass& b)
{
    auto newObject = std::unique_ptr<MyClass<T>>(new MyClass<T>(a.member, b.member2));

    //...do stuff to the object

    return newObject;
}

PS: Notice that I didn't use make_unique (as opposed to make_shared). This is because make_unique is C++14, not C++11.

Answer 3

It is quite common, in C++ code, to pass pointers around everywhere. Generally speaking, it is best practice to have the creator of a pointer also destroy it. But it is also acceptable to pass ownership of the pointer to someone else have them take responsibility of maintaining and destroying the pointer.

This is especially true in your case where it appears your method is nothing more than a factory method. It is perfectly okay to think of a factory method as being the same as the constructor. And the one who took responsibility for creating - and has responsibility for destroying - the pointer.

If you are less concerned about best practices and just want it to be memory safe, you can use a std::shared_pointer in #include <memory>. These act kinda like garbage collectors. You use them exactly as you would a normal pointer. But as soon as the last reference to it goes out of scope, it deletes the underlying pointer for you.