Which pointer assignment is better (C++)

Question

foo.h

#include "class1.h"

class foo{

    private:
        class1* class1ObjectPointer;
    
    public:
        foo();
        virtual ~foo();
    
        void foomethod();

}

foo.cpp (VERSION 1)

#include "foo.h"

foo::foo()
{
    this->class1ObjectPointer = new class1();
}


foo::~foo()
{
    if( this->class1ObjectPointer != NULL ){

        delete class1ObjectPointer;
        this->class1ObjectPointer = NULL;
    }
}


foo::foomethod(){

    *(this->class1ObjectPointer) = some_method(); 
    //suppose some method returns an object of class1

}

foo.cpp (VERSION 2)

#include "foo.h"

foo::foo()
{
    this->class1ObjectPointer = NULL;
}


foo::~foo()
{
    if( this->class1ObjectPointer != NULL ){

        delete class1ObjectPointer;
        this->class1ObjectPointer = NULL;
    }
}


foo::foomethod(){

    class1 object;
    object = some_method(); 
    //suppose some method returns an object of class1
    this->class1ObjectPointer = new class1(object); // copy constructor
}

Which pointer assignment is better in the following cases:

1. objects of class1 always have fixed size (e.g. a class with fixed variables in it)
2. objects of class1 may have members with variable size (e.g. a class that has a matrix in it which can potentially have different sizes)

---

Would you suggest any other and better way of doing what I did in these snippets?

Answer 1

the instance of class has constant size regardless of any assignments. If you assign a member-variable to some pointer, the size of the instance will not change. Just some junk in memory (or NULL value if you had initialized it first) will be replaced by the address of some other allocated object.

Answer 2

It's probably better (more efficient, and easier to avoid memory leaks) to reassign the object per your first example than to allocate a new object - as demonstrated by the memory leak in your second example.

It looks very much like you don't need a pointer at all; just embed an object in your class:

class foo {
private:
    class1 class1Object;

public:
    // implicit constructors, assignment and destructor are fine
    void foomethod() {class1Object = some_method();}
};

Your second situation makes no sense - objects of a particular class are all the same size.

If you really do need to store a pointer (perhaps because you need polymorphism), then the simplest way to handle it is with a smart pointer:

class foo {
private:
    std::unique_ptr<class1> class1ObjectPointer;

public:
    foo() : class1ObjectPointer(new class1) {}
    // implicit copying and destructor are fine

    foomethod() {*class1ObjectPointer = some_method();}
    // or, if not assignable, class1ObjectPointer.reset(new class1(*some_method()));
};

If you really want to manage it yourself, then you'll need to override the default copy constructor and copy-assignment operator per the Rule of Three, make sure you delete the old object when reassigning the pointer, and be careful about exception safety. And there's no need to either check for NULL before deleting, or to assign NULL in the destructor.

Answer 3

1. the if ptr != NULL check is redundant in front of delete since it does the check internally.

2. Which of these variants is better depends on the case. Both have their uses.

3. Don’t use pointers here at all. If you have to, use std::unique_ptr (requires C++11) or boost::shared_ptr.

4. In any case, members in constructors should be initialised, not assigned. That is, write it as follows:

   foo::foo() : class1ObjectPointer(new class1) { }
   

   This uses an initialisation list.